search

eclipse / nebula

Nebula Project
https://eclipse.org/nebula
Eclipse Public License 2.0
85 stars 98 forks source link

RichText: SWTException when calling getText (too) early #457

Closed cpfeiffer closed 6 months ago

cpfeiffer commented 2 years ago

I've modified the rich text editor sample to show a crash when calling getText() before ckeditor has loaded:

See the added getText() call in https://github.com/GEBIT/nebula/commit/092e69d16e6eedbbbc62cbf976c0d992f2e836a9 (just launch the example to get the crash). I will create another ticket for the other change in that commit.

Exception in thread "main" org.eclipse.swt.SWTException: "getText" is undefined
    at org.eclipse.swt.browser.WebBrowser$EvaluateFunction.function(WebBrowser.java:195)
    at org.eclipse.swt.browser.WebSite.Invoke(WebSite.java:809)
    at org.eclipse.swt.browser.WebSite$7.method6(WebSite.java:182)
    at org.eclipse.swt.internal.ole.win32.COMObject.callback6(COMObject.java:120)
    at org.eclipse.swt.internal.ole.win32.COM.VtblCall(Native Method)
    at org.eclipse.swt.internal.ole.win32.IDispatch.Invoke(IDispatch.java:65)
    at org.eclipse.swt.ole.win32.OleAutomation.invoke(OleAutomation.java:575)
    at org.eclipse.swt.ole.win32.OleAutomation.invoke(OleAutomation.java:538)
    at org.eclipse.swt.browser.IE.execute(IE.java:1202)
    at org.eclipse.swt.browser.WebBrowser.nonBlockingExecute(WebBrowser.java:400)
    at org.eclipse.swt.browser.WebBrowser.evaluate(WebBrowser.java:450)
    at org.eclipse.swt.browser.WebBrowser.evaluate(WebBrowser.java:406)
    at org.eclipse.swt.browser.Browser.evaluate(Browser.java:666)
    at org.eclipse.swt.browser.Browser.evaluate(Browser.java:615)
    at org.eclipse.nebula.widgets.richtext.RichTextEditor.getText(RichTextEditor.java:404)
    at org.eclipse.nebula.widgets.richtext.example.RichTextEditorExample.createControls(RichTextEditorExample.java:103)
    at org.eclipse.nebula.widgets.richtext.example.RichTextEditorExample.main(RichTextEditorExample.java:55)
fipro78 commented 2 years ago

Will you provide a patch that could avoid this? Actually you could extend the check in getText() similar to setText() to inspect the editorLoaded flag. And if not true, simply return null. But actually I am unsure about the use case. The modification doesn't make any sense. Why would you try to get the text from a control that you have just created?

BTW, I think there are even more methods in RichTextEditor that would need such a guard if one tries to call the method before the initialization is done.

fipro78 commented 2 years ago

Oh IIRC I decided actively to not add the editorLoaded check, so a developer finds the error in his/her code at development/testing time. See, with the check, the getText() method would return null, as it could not do anything else. So you would wonder why the value is null, although a value is set. And it would be a timing issue (I remember that I myself was searching for such an issue for quite a while).

Although I understand that from a SWT perspective this feels wrong, for such a scenario you need to be aware that you are working on a Browser underneath. And for this you need a ProgressListener that gives you the indication that the page is now ready. The internal Browser can be accessed via the RichTextEditorConfiguration for example.

So to be honest, I don't think that it would make sense to fix this, as it would hide the underlying issue then, that you try to access something at a time where it is not yet accessible. And that is a developer issue, not a user issue that could happen by accident.

cpfeiffer commented 2 years ago

Thanks for the quick answers! It could return initialValue when the editor is not loaded, yet. Otherwise, it might be helpful to add a method isEditorLoaded() so that clients can check for this condition. Or is there a different way to check this? (That flag might also not be 100% accurate as is, because quite some more initialization happens after it is set to true.)

fipro78 commented 2 years ago

Well it adds some BrowserFunctions related to resizing in embedded mode. And the setters can only be called after editorLoaded is set to true. So not sure how this could be changed.

Again, it would be an option to handle that scenario. But actually I am not sure about the use case here. Why is there a need to call getText() in a state where the user interface builds up?

IIRC I had a similar discussion in the past, not sure about the context anymore. But it turned out that it really is a special case, and it could be solved by adding a ProgressListener on the internal Browser via the RichTextEditorConfiguration. Not sure if this would also work here, but I don't get the point why the content of a control needs to be evaluated at creation time. The value that is set should be known even without asking the Control. Or what am I missing here?

fipro78 commented 2 years ago

BTW, since there is some concurrency going on here, returning the initial value could also lead to some strange behavior if the value is asked while the ProgressListener is running. Means, editorLoaded is set to true, but setText() is not executed yet.

cpfeiffer commented 2 years ago

I only tried to call getText() while debugging #458, so it's not a real issue for me. Just unexpected, and (so I thought) avoidable.

I would expect the ProgressListener to be called in the main thread so there should not be further concurrency issues (when getText() also needs to be called in the main thread).

lcaron commented 10 months ago

@cpfeiffer Any news about this issue ?

lcaron commented 6 months ago

This issue has no activity since months. I decide to close it, but it could be reopened.