fixes multiple problems with split / preview editors

[Trias]

this is my refactoring for fixing problems with multiple open editors (split editor, preview editors...) (for example mentioned in #142 and #231)

unfortunately using a many-to-one (implicit)-mapping from editor to editorManager turned out to be the wrong abstraction, as you can have multiple editors open for the same file in intellij. i solved this by using a set of EditorEventManagers per uri and a one to one relationship from editors to EditorEventManager. This lead to problems in separating documentEvents, which are still somewhat intertwined with EditorEvents.

it's still very rough, as i tried to maintain as much as possible API-compatibility. It would be great if you can comment on how much this is needed. Also general comments are welcome.

I will test change this internally to find potential bugs and after that i hope you are interested in merging

[swissiety]

have you tried to use the Editor object itself instead of the uri?

[Trias]

Yes, i do use the editor instance as a key in EditorEventManagerBase#editorToManager.

Unfortunately we cannot use the editor as a key alone. LSP identifies a file with its uri and it is possible to send requests without a document being open in an editor (for example rename, createFile,...). Therefore i had to retain the uri connection. I added a warning to these methods.

We can refactor this by separating editor and document/file more cleanly, but this requires a lot of additional work and would possibly result in a different architecture. Contributions are of course welcome!

[swissiety]

good point! I like that idea of using Document.

[Trias]

note to self/ contributors: EditorFactory.getEventMulticaster().addDocumentListener() can be used for events on all open documents, which is probably a better path: https://jetbrains.org/intellij/sdk/docs/basics/architectural_overview/documents.html

[NipunaRanasinghe]

@Trias Thanks a lot for your time working on this! I just had a look and your approach seems to be promising. Did you have time for test this locally and did you encounter any regression issues after these changes?

Also regarding the API changes you were concerned about, lets try to keep the API compatibility as much as possible, but I'm +1 to introduce breaking changes if that's necessary, as we are still on pre 1.0

[Trias]

The restart function was broken, which i fixed. Apart from that ist seems to work. Ready for review ;)

[Trias]

there is another bug in the restart function, a bit more subtle this time... just to give you an update. i think i'll have a look into on friday

[swissiety]

there is another bug in the restart function, a bit more subtle this time... just to give you an update. i think i'll have a look into on friday

maybe you have a look in my fork - i restructured that start/restart already and fixed some problems - additionally the wrong widget reference is removed on a restart of the connection

[NipunaRanasinghe]

there is another bug in the restart function, a bit more subtle this time... just to give you an update. i think i'll have a look into on friday

@Trias sure I'll wait for it.. Also there seem to be a conflict due to a recent merge. I'd appreciate if you could resolve that too 🙂

[Trias]

see! there is the fix! ;)

I'm not really sure how to resolve the merge conflict. Maybe @swissiety can provide insight...

In the new DocumentEventManager I use DocumentUtils.offsetToLSPPos. Before @swissiety 's change this could be done with only the document reference, now apparently it's depending on the editor and its settings.

Is this really how IntelliJ handles this? iE depending on the editor tab size may be different and apparently needs to be recalculated? Is there a way to ge the settings from an document object alone? Maybe you could elaborate on how to reproduce the issue you were fixing... as i never had a problem in this code section.

[swissiety]

IntelliJs code for converting offset<->linenumber,characterOffset is done in an Editor object (I tried to get rid of it, too).
Additionally the Thing is that IntelliJ allows different tab settings for the editor object (dropdown at the bottom right corner) which is why using the editor object is more accurate than just using the default tab size from the common settings.

[Trias]

so if I'm in an empty file and i type a tab (and 4 space = 1 tab), for intellij i'm at position 0,1; but to the lsp server i am at position 0,3?

so depending on the editor open, different indentations may be used? sounds like a desaster waiting to happen...

is this documented somewhere or did you just figured this out?

[swissiety]

its the otherway around.

• LSP counts code units so a tab is a single char
• Intellij replaces tabs with spaces in its Editor (e.g. see change of char in line on the bottom if you type a tab)

This tab thing was implemented already i just adapted it to the inverse function the analogous way. You could write a testcase to clarify.

[Trias]

ok. weird thing by intellij. does this happen on the default settings? i always had the idea that a tab is automatically replaced with spaces and send as such to the Language server and the file system.

test cases would certainly help.

I'll think about it but i guess our best shot is to find some editor object associated with the document and let it do the calculations...

Thank you for your support, very much appreciated!

[swissiety]

ok. weird thing by intellij. does this happen on the default settings? i always had the idea that a tab is automatically replaced with spaces and send as such to the Language server and the file system.

On saving its converted to tabs again(?) i guess this happens in the same process like the replacement of \r\n <-> \n (?)

[NipunaRanasinghe]

Changes look good to me! @Trias really appreciate you huge effort on this.. I've been waiting for this before triggering the next release and it seems like we are all set to go 🙂