The 'getCursorPosition' method is not synced when mouse or arrow keys change the cursor position of editor.

[danijelz]

The 'getCursorPosition' is relying on server values stored in AceEditor.cursorPosition and those are not updated after mouse or keyboard events change the cursor position. The possible solutions:

1. Enhance the 'sync' method so it would also update 'cursorPosition' and 'selection' and clients would be responsible for synchronisation before usage.
2. Implemented automatic synchronisation of server values after mouse and keyboard events.
3. Implemented automatic synchronisation of server values after mouse events and track key events and update 'cursorPosition' accordingly.

As far as I'm concerned the proposal 1. would be a necessity, even if other other solution is also implemented, since data is allready being transfered and adding couple of fields wouldn't do much harm. The additional values should behave same as 'editor-blur' event values.

It also seems like the update of cursor position is not updated properly after 'editor-blur' event since it always retuns same values in array. More specificaly it returns [column,column].

[F0rce]

this commit has been reverted!

i'm still working on those changes

[F0rce]

Hello @danijelz,

i've been trying to resolve your issue pretty long last night.

But i'll have to explain a bit, how the ace editor syncs it value.

In earlier version of the ace editor (where it was still on PolymerElement) there was an issue, that put the cursor at the end of the editor, when typing faster then usual. This was because vaadin was not possible to correctly handle all the events the ace editor sent, in real time. When typing faster, than the handling was done it randomly put the cursor at the end. When this issue was discoverd I started working on the old Ace Editor https://github.com/Sergio70/AceEditorV14. I changed the whole frontend to only synchronize the editors value and selection when the editor is blurred (mouse/keyboard loses focus) and stopped my maintaining about 2,5 weeks ago to focus on the this newer version with the better Webcomponent Framework.

I think a workaround for your problem, would be to add a Vaadin shortcutListener():

        Shortcuts.addShortcutListener(<AceEditor>, event -> {
            <AceEditor>.addSyncCompletedListener(evt -> {
                // current cursor position
            });
            <AceEditor>.sync();

        }, Key.ARROW_RIGHT).listenOn(<AceEditor>);

you can obviously also do that for the Key.ARROW_LEFT

Yesterday evening when trying to resolve your issue, I found the same behaviour when syncing selection & cursor in 2 seperate events. So I think it's mandatory to keep the current logic (blur) of syncing the values to reduce client and server communication.

JFYI: I already implemented the requested changes to the autoCompletion (locally) and will push them shortly. I just want to hear your opinion on this.

Have a great day, David

[danijelz]

Hi, @F0rce,

I'm not sure the workaround will solve the problem or maybe my understanding of your proposal is poor. As I see it the 'getCursorPosition' method is returning wrong values in some circumstances and that renders it unusable. At least I can't see the purpose of it if it doesn't return the correct values. The same is with 'getSelection' method which behaves fine untill the user starts moving around with the keyboard.

After inspecting the server side of code I can see that maintainig the cursor state state is quite an issue. So my proposal for fast solution is that clients should be aware of the quirks and force synchronization of those values before use. It can be done by adding one time 'SyncCompletedListener' which would trigger the required functionality with fresh state from client, e.g.:

    aceEditor.addSyncCompletedListener(this::executeQueryAfterSync);
    ...
    private void executeQueryAfterSync(AceForceSyncEvent event) {
        event.unregisterListener();
        // use synced values from event
    }

For this to work the 'AceForceSyncEvent' should carry the accurate info about cursor position, which is an easy addon, but it is inadequate solution in my opinion. It's hard to communicate this idiosyncratic behaviour to clients so I would either rename the 'getCursorPosition' method so it would be eloquent aboud its mode of operation, something like 'getLastSyncedCursorPosition' or even remove it.

Other possible solution would be to keep the current method and update state according to the user actions.

Thanks again for your responsiveness...

[F0rce]

Thanks for your super detailed answer.

I'll take a look at keyboard input and find a way to solve it.

After inspecting the server side of code I can see that maintainig the cursor state state is quite an issue.

I don't really get that. I get that the synchronisation is not happening when using the keyboard, but the editor value, cursorPosition(column & row), selection (rowStart, from, rowEnd, to) and the selectedText are refreshed before sending it to the frontend. I'll just have to find a way to keep it synced / up to date.

I'll keep you updated, David

[danijelz]

cursorPosition(column & row), selection (rowStart, from, rowEnd, to) and the selectedText are refreshed before sending it to the frontend.

I forgot to mention that I never change the value programmatically - on server side - so I don't know how accurate the state is after those changes.

[danijelz]

Just for info, after switching to version 1.1.0 and cacheing cursor position manualy within 'SyncCompletedListener' the values are reliable.

[F0rce]

thats why I included the sync event. Extra for those kind of situation. I'll validate some more tests, but I think its the best way to keep client & server communication by default as low as possible.

thanks you for your input

[danijelz]

I think its the best way to keep client & server communication by default as low as possible.

I agree with that but I also think the API should be clear about that so 'getCursorPosition' method should be renamed such that this would be communicated. I would also suggest that AceForceSyncEvent should become internal api which would only update server state and upon which public listeners would be notified. By that the internal state would be updated prior to every client and so clients would have no need for custam state keeping. Something like:

public AceEditor() {
    ...
    // AceForceSyncDomEvent should be part of internal api with same fields as AceForceSyncEvent
    addListener(AceForceSyncDomEvent.class, this::onForceSyncDomEvent);
}

private void onForceSyncDomEvent(AceForceSyncDomEvent event) {
    this.selectedText = event.getSelectedText();
    this.selection = new int[] { event.getSelectionRowStart(), event.getSelectionFrom(), 
        event.getSelectionRowEnd(), event.getSelectionTo() } ;
    this.cursorPosition = new int[] { event.getCursorRow(), event.getCursorColumn() };

    //internal state updated so all clients are safe to call getCursorPosition()
    fireEvent(new AceForceSyncEvent(selectedText, ...));
}

//Some sugar for clients waiting for syncing
public void runAfterSync(Runnable action) {
    requireNonNull(action);
    addListener(AceForceSyncDomEvent.class, event -> runAfterSync(event, action));
    sync();
}

private void runAfterSync(AceForceSyncEvent event, Runnable action) {
    event.unregisterListener();
    action.run();
}

[F0rce]

I'll validate your code and probably implement it.

Holy smokes, thank you very much 👍🏻

[F0rce]

when I correctly understand everything. the addSyncCompletedListener() is with this logic not needed anymore.

[danijelz]

when I correctly understand everything. the addSyncCompletedListener() is with this logic not needed anymore.

It is not but you can leave the API for backward compatibility. By adding internal/package private AceForceSyncDomEvent you ensure that only 'AceForceSyncDomEvent' listeners are called from client and those are managed by you.

After updating internal state of server which happens in onForceSyncDomEvent(AceForceSyncDomEvent) method you can convert 'AceForceSyncDomEvent' to 'AceForceSyncEvent' and use it to call fireEvent(...) for legacy liteners. You should remove '@DomEvent' and '@EventData' annotations in 'AceForceSyncEvent' if you choose this path.

Hope I didn't complicate too much...

[F0rce]

Hello @danijelz, as I already stated in the other issue, I've not been able to focus on the edior.

Update: I just had to add a Objects. in front of it and it works. Thanks Stackoverflow :)

Today I was working on your pretty solution about the sync and I tried to implement your version.

When I added the runAfterSync(Runnable action) Method, the requireNonNull(Runnable action) Method it is non existent. Is that something you created yourself and would like to share?

Thanks in advance, David

[danijelz]

I made static import. The full signature is Objects#requireNonNull(Object).

https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/util/Objects.html

[F0rce]

Thanks for the fast answer,

could you be so kind and verify these changes I made (aswell as #2 & #6 so I could close those issues)? Before I upload everything to the directory.

If not, I'll try as good as possible by myself.

Thanks, David

[danijelz]

Sorry but I'm on eviction until Monday and I only have a phone with me. I can check it on Monday though.

[F0rce]

I'm working on the other issues aswell, so would be perfect. But only if you really have the motivation and time for it.

Thanks in advance, David

[danijelz]

@F0rce I'm able to test it now. How do you expect me to do it? Should I clone the code and test custom version? If so, I would need some guidelines with lit-ace.

[F0rce]

If you clone this repo, you can test everything. The updated lit-ace version is also implemented here, so you don't have to worry about it. On the side of lit-ace, it is a Lit-Element Wrapper around c9's ace editor. But the whole magic mostly happens on java side.

If you clone this repo, just mvn jetty:run to install all the dependencies and npm packages. And use the view located in /test/java/de/f0rce/View.java to change some things or try some things out.

Thanks in advance, David

[danijelz]

The code looks OK but you didn't add "shoot and forget" methods for one time listeners:

public void forceSync(Runnable action) {
    requireNonNull(action);
    addListener(AceForceSyncDomEvent.class, event -> afterSync(event, action));
    sync();
}

private void afterSync(AceForceSyncDomEvent event, Runnable action) {
    event.unregisterListener();
    action.run();
}

public void forceSync(Consumer<AceForceSyncEvent> action) {
    requireNonNull(action);
    addListener(AceForceSyncDomEvent.class, event -> afterSync(event, action));
    sync();
}

private void afterSync(AceForceSyncDomEvent event, Consumer<AceForceSyncEvent> action) {
    event.unregisterListener();
    action.accept(new AceForceSyncEvent(event.getValue(), ...));
}

But since those can be added by us users on calling site I'm closing the issue.

Thanks again for fast responses.