F0rce/ace

AceEditor.getValue() is not up-to-date after addAceChangedListener() listener notified

archiecobbs opened this issue · 6 comments

archiecobbs commented

Describe the bug

I have a Vaadin CustomField that wraps an AceEditor - very straightforward.

In order to keep the CustomField's value up-to-date, I register a listener.

It looks something like this:

public class JavascriptField extends CustomField<String> {

    private final AceEditor ace = new AceEditor();

    public JavascriptField() {
        this.ace.setMode(AceMode.javascript);
        this.ace.setTheme(AceTheme.chrome);
        this.ace.setShowPrintMargin(false);
        this.ace.addAceChangedListener(e -> this.updateValue());
        this.add(ace);
    }

// CustomField

    @Override
    protected String generateModelValue() {
        return this.ace.getValue();
    }

    @Override
    protected void setPresentationValue(String newValue) {
        this.ace.setValue(newValue);
    }

    ...

The bug is that when a listener notification comes, the event value e.getValue() is up-to-date with whatever was just entered into the GUI. However, then updateValue() is invoked (shown above), and (as documented by CustomField) it invokes generateModelValue() which invokes this.ace.getValue() (shown above) but then the value returned by this.ace.getValue() is out of date.

So the end result is that the CustomField always has a stale value.

All Vaadin notifications that I've encountered in the past are "up to date" in the sense that if you query the state directly from the event handler, that state is consistent, i.e., up-to-date, with respect to the change you're being notified about.

However AceEditor doesn't seem to provide this guarantee, which seems like a bug.

To Reproduce

  1. Register listener
  2. Display e.getValue() and compare to this.ace.getValue()
  3. Notice they are not the same

Expected behavior

When a listener is notified, this.ace.getValue() should always equal e.getValue().

Desktop (please complete the following information):

  • OS: Mac OS 12.5
  • Browser Firefox 103.0.1
  • Add-on version 3.3.3
F0rce commented

Good Evening @archiecobbs,

First of all thank you for this detailed issue. But this is intended and not a bug.

The AceChangedEvent was introduced in #21 as a way to save the value while the user is typing. This is in most of the cases discouraged as a lot of data (and I mean a lot) is transferred between client and server (which will cause wierd racing conditions if the user types a lot or the value of the editor is super long).

The AceChangedEvent does not update the value internally, exactly as you said.

I implemented the Blur event, as Vaadin did in all text input components. As soon as the user clicks out of the editor (on a button or something else) the current value is exchanged between client and server. This approach is a lot faster and the convenient way of doing so.

So instead of using the AceChangedEvent you should use the AceBlurEvent added via .addBlurListener().

If you need further assistance, don't hesitate to ask.

Have a great weekend,
David

archiecobbs commented

Hi David,

OK thanks for the explanation - that makes sense.

Not sure if what you've explained is properly documented though... you may want to consider adding a note to the Javadoc for addAceChangedListener() that the (a) getValue() will not be up-to-date and (b) addBlurListener() is a more efficient way to stay updated.

Also, while I understand that addBlurListener() is more efficient and therefore a more appropriate choice for my use case, is there any reason for getValue() to be out of date? After all, you have the new value in the listener event, so you certainly could update getValue() without generating any more network traffic.

F0rce commented

ace/src/main/java/de/f0rce/ace/AceEditor.java

Lines 75 to 99 in 3f24eb8

public AceEditor() {
super.addListener(AceBlurChanged.class, this::updateEditor);
super.addListener(AceForceSyncDomEvent.class, this::onForceSyncDomEvent);
this.setHeight("300px");
this.setWidth("100%");
}
public AceEditor(String height, String width) {
super.addListener(AceBlurChanged.class, this::updateEditor);
super.addListener(AceForceSyncDomEvent.class, this::onForceSyncDomEvent);
this.setHeight(height);
this.setWidth(width);
}
public AceEditor(AceTheme theme, AceMode mode, String height, String width) {
super.addListener(AceBlurChanged.class, this::updateEditor);
super.addListener(AceForceSyncDomEvent.class, this::onForceSyncDomEvent);
this.setTheme(theme);
this.setMode(mode);
this.setHeight(height);
this.setWidth(width);
}

In the constructor of the Ace Editor the Blur Listener (and the so called Sync Listener) are added to the editor. All the other events can be used by you for specific use cases and do not update the value internally. The reason is to keep (as I said) the communication between client and server to a minimum.

Out of curiosity, why wouldn't you use getValue() from the event listener (as it is "updated"), instead of getValue() from the AceEditor ?

archiecobbs commented

Hi David,

The reason is to keep (as I said) the communication between client and server to a minimum.

That's totally understandable.

I'm still wondering about the answer to this question though:

Is there any reason for getValue() to be out of date? After all, you have the new value in the listener event, so you certainly could update getValue() without generating any more network traffic.

In other words, if there is an AceChangedListener registered already, and then an update from the client is received, why not go ahead and update the current value of the widget? It shouldn't cost anything to do so.

Put another way, the following sequence doesn't make sense to me:

  1. Invoking getValue() returns current value X
  2. An AceChangedListener is notified that the new value is Y
  3. Invoking getValue() still returns the old value X, but then...
  4. A little later invoking getValue() returns the new value Y

This is just needlessly inconsistent. By "needlessly" I mean it can be fixed without adding any network traffic.

Out of curiosity, why wouldn't you use getValue() from the event listener (as it is "updated"), instead of getValue() from the AceEditor ?

It's because of the design of CustomField. Then listener being notified triggers a call to this.updateValue() which does not take any parameters. To pass along the value from the listener would require a hack using a ThreadLocal or whatever.

archiecobbs commented
urkl commented

On Fri, Sep 16, 2022 at 8:20 AM urkl @.*> wrote: It's because of the design of CustomField. Then listener being notified triggers a call to this.updateValue() which does not take any parameters. To pass along the value from the listener would require a hack using a ThreadLocal or whatever. @archiecobbs https://github.com/archiecobbs did you resolved issues with custom field?
No, now I'm just updating the field on loss of focus. Here is my CustomField implementation: / * Syntax highlighting JS editor. */ @SuppressWarnings("serial") public class JavascriptField extends CustomField { private final AceEditor ace = new AceEditor(); public JavascriptField() { this.ace.setMode(AceMode.javascript); this.ace.setTheme(AceTheme.chrome); this.ace.setShowPrintMargin(false); this.ace.addBlurListener(e -> this.updateValue()); this.add(ace); } @OverRide public void setHeight(String height) { this.ace.setHeight(height); super.setHeight(height); } @OverRide public void setHeight(float height, Unit unit) { this.ace.setHeight(height, unit); super.setHeight(height, unit); } // CustomField @OverRide protected String generateModelValue() { return this.ace.getValue(); } @OverRide protected void setPresentationValue(String newValue) { this.ace.setValue(newValue); } }
…
-Archie
-- Archie L. Cobbs

Archie thank you for this.