ace
ace copied to clipboard
AceEditor.getValue() is not up-to-date after addAceChangedListener() listener notified
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
- Register listener
- Display
e.getValue()
and compare tothis.ace.getValue()
- 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
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
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.
https://github.com/F0rce/ace/blob/3f24eb83f2c9a4da34c7ea286cffa68f08fc048c/src/main/java/de/f0rce/ace/AceEditor.java#L75-L99
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 ?
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 updategetValue()
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:
- Invoking
getValue()
returns current value X - An
AceChangedListener
is notified that the new value is Y - Invoking
getValue()
still returns the old value X, but then... - 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.
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<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.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
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<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.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.