search

eclipse / nebula

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

[Rich Text] Modifylistener called a lot #433

Closed azoitl closed 2 years ago

azoitl commented 2 years ago

I'm doing my first experiments with the RichTextEditor and I noticed that the modifylistener is called nearly constantly. Even when I just move the mouse across the editor. Is this intended? Is there a way how I only get notified when the content of the editor is changed?

fipro78 commented 2 years ago

Actually the ModifyListener should only be triggered if the content changes. In which environment do you notice the incorrect behavior?

azoitl commented 2 years ago

I'm on a Debian/Linux machine, with Gnome 41.1, and Mouse follows focus. I integrated it in a new prototype editor page of a multipage form editor for the Eclipse 4diac project. I'm also happy to share my code if needed.

lcaron commented 2 years ago

We had some issues on windows, and I fixed it. Maybe this commit will give you a hint: https://github.com/eclipse/nebula/commit/062f4b3f764d041a5d5cbdb20b0e929e79d7c109#diff-a9b50441f0e9c093caac466dcb4cc54e0589791300aefe5a66af34b6b8a2be3d

Feel free to ask

fipro78 commented 2 years ago

Hi Laurent,

I just noticed that you included a 9 year old plugin to fix an issue that should not exist in the current ckeditor 4 version. Well don't want to blame you for this as I haven't found another solution. But did you raise a CQ before you added an additional third party code? Just to ensure this is ok from a legal point of view

fipro78 commented 2 years ago

Just to inform you here, I will have a look if updating the ckeditor version would fix the ModifyListener issues. But it will take some days.

fipro78 commented 2 years ago

First investigation result: The issue that the ModifyListener is called constantly exists also on Windows and it is actually caused by the onChange plugin that was added via Bug 572068 @lcaron pointed out above. We need to remove that plugin again for several reasons now:

  1. I think there is no IP approval for that plugin
  2. The plugin is causing regressions in the behavior
  3. The plugin is actually not developed anymore and looking into the pages it is outdated and should not be used with ckeditor 4 anymore.

I am working on a solution for the next release.

wimjongman commented 2 years ago

Thanks, Dirk!

lcaron commented 2 years ago

I'm very sorry for this regression (and sorry dirk, I can not remember if I raised a CQ).

I've made a rollback for issue 57058, so the "change" plugin is no longer included.

@fipro78 Can you please have a look at https://github.com/eclipse/nebula/pull/435 ?

fipro78 commented 2 years ago

No need to apologize for trying to fix an issue. :)

I answered on the PR. Hopefully I get a clue for fixing the strange modification issue.

wimjongman commented 2 years ago

No need to apologize for trying to fix an issue. :)

My thoughts exactly :-D

lcaron commented 2 years ago

Hopefully I get a clue for fixing the strange modification issue.

Did you get an answer yet or are you hoping for one ?

I played with CKEditor and the original bug is very very very strange. I tried only on Windows and I hope that this issue is not due to (the infamous) IE11.

fipro78 commented 2 years ago

Still hoping for an answer

https://github.com/ckeditor/ckeditor4/issues/5006

It is really strange. It seems to be only related to pressing DEL or BACKSPACE after setting the text but not pressing any other key before. If you press ESC or type anything to change the content otherwise like adding a character, the deletion also causes modifications.

fipro78 commented 2 years ago

Hi Laurent,

I have created a PR https://github.com/eclipse/nebula/pull/436

It looks good so far from my testings. Would be great if you could also verify that things are now working as intended.

lcaron commented 2 years ago

Hi Dirk

Well done. I'm gonna try tonight and I'll give you my feedback.

lcaron commented 2 years ago

Hi Dirk I played with the RichTextEditor widget and it's working flawlessly, so you can merge this PR (and close this bug :) ) Congratulations !

fipro78 commented 2 years ago

Merged the PR. With the revert commit from Laurent the constant calling of the ModifyListener is fixed and my PR fixes the issue that the ModifyListener is not called on initial opening and deleting characters. So the ModifyListener should now work as intended.