[PATCH] Fix EVT_STC_PAINTED recursion issue (Was: Re: Repaint lockup in ctlSQLBox::OnPositionStc on windows) - Mailing list pgadmin-hackers

Hi all,
04.10.2015 7:32, J.F. Oster wrote:
> Hello Nikolai,
>
> Appreciate your research!
> I suggest you to send a *.patch (git diff) attachmeht to the list
> for the team members and others interested to test it in their
> environments.

Well, technically I can, but it will be ugly, see below.

Problem is, the change of STC_UPDATEUI to STC_PAINTED in pgadmin was not
intended to break pgadmin on Windows, but rather to fix it on Mac. This
change was obviously incorrect, but simply reverting it would probably
cause regressions for Mac users. As I personally don't use Mac, I can
not develop and test a proper fix for it. Therefore, all what I can
suggest myself is this:

--- ctlSQLBox.cpp.orig    Fri Sep 25 13:20:24 2015
+++ ctlSQLBox.cpp    Sun Oct 04 15:47:17 2015
@@ -40,7 +40,11 @@
      EVT_MENU(MNU_FIND, ctlSQLBox::OnSearchReplace)
      EVT_MENU(MNU_AUTOCOMPLETE, ctlSQLBox::OnAutoComplete)
      EVT_KILL_FOCUS(ctlSQLBox::OnKillFocus)
+#ifdef __WXMAC__
      EVT_STC_PAINTED(-1,  ctlSQLBox::OnPositionStc)
+#else
+    EVT_STC_UPDATEUI(-1,  ctlSQLBox::OnPositionStc)
+#endif
      EVT_STC_MARGINCLICK(-1, ctlSQLBox::OnMarginClick)
      EVT_END_PROCESS(-1,  ctlSQLBox::OnEndProcess)
  END_EVENT_TABLE()


Thank you,
Nikolai

> Sunday, October 4, 2015, 1:36:45 AM, you wrote:
>
> NZ>  Hi all,
>
> NZ>  ok, it is broken since this commit:
>
> NZ>  http://git.postgresql.org/gitweb/?p=pgadmin3.git;a=patch;h=b0ecbbca7f77c0f07cff67bba3d2bca28954a1e2
>
> NZ>  -   EVT_STC_UPDATEUI(-1,  ctlSQLBox::OnPositionStc)
> NZ>  +   EVT_STC_PAINTED(-1,  ctlSQLBox::OnPositionStc)
>
> NZ>  It appears that in previous version of scintilla, there existed a
> NZ>  SCN_POSCHANGED event, which then got obsolete and replaced by
> NZ>  SCN_UPDATEUI, and probably that is why the name 'OnPositionStc' was
> NZ>  chosen for the handler. So linking OnPositionStc() to STC_UPDATEUI was
> NZ>  correct and in accordance with scintilla's manual.
> NZ>  However the change from EVT_STC_UPDATEUI to EVT_STC_PAINTED was
> NZ>  incorrect. These events are not quite equivalent. EVT_STC_PAINTED has a
> NZ>  somewhat different purpose.
>
> NZ>  I've checked, reverting back to EVT_STC_UPDATEUI indeed fixed CPU load
> NZ>  and lockup issues on windows. See also
> NZ>  http://www.scintilla.org/ScintillaDoc.html#SCN_PAINTED
> NZ>  http://www.scintilla.org/ScintillaDoc.html#SCN_UPDATEUI
> NZ>  http://www.scintilla.org/ScintillaHistory.html
> NZ>  http://web.mit.edu/jhawk/mnt/spo/sandbox/punya/anjuta-1.2.3/doc/ScintillaDoc.html
> NZ>    (-- this is some older document with a more detailed explanation of
> NZ>  SCN_UPDATEUI)
>
> NZ>  Some relevant excerptions:
>
> NZ>  "SCN_PAINTED: is to update some _other_ widgets based on a change."
>
> NZ>  "SCN_POSCHANGED: notification is deprecated as it was causing confusion.
> NZ>  Use SCN_UPDATEUI instead."
>
> NZ>  "SCN_UPDATEUI, SCN_CHECKBRACE:
> NZ>  Either the text or styling [...] common use is to check whether the
> NZ>  caret is next to a brace and set highlights on this brace and its
> NZ>  corresponding matching brace."
>
> NZ>  So for me this all sounds clear sufficiently. I'll stick with
> NZ>  EVT_STC_UPDATEUI and I can rebuild it myself if I have to. Whether fix
> NZ>  git respectively or not is up to maintainers now.
>
>
> NZ>  Thank you,
> NZ>  Nikolai
>
> NZ>  03.10.2015 21:56, I wrote:
>>> Hi all,
>>>
>>> Ok, I've verified that ctlSQLBox::OnPositionStc() is broken.
>>>
>>> If I comment the code for highlighting in it (that is, almost all of its
>>> body), the problem goes away.
>>>
>>> I've found quite some problems observed with it and attempted to fix
>>> previously, like e.g. the one from 2011:
>>> ---------------
>>> + // Ensure we don't recurse through any paint handlers
>>> + Freeze();
>>> UpdateLineNumber();
>>> + Thaw();
>>>
>>> // Clear all highlighting
>>> ---------------
>>> That was most probably only a partial fix, and it was subsequently
>>> reverted.
>>>
>>> I'd like to also note that 'OnPositionStc' name is pretty much
>>> misleading. If it was named 'OnPaintedStc' it would be more consistent
>>> and more hinting where the problem is. I'm aware that renaming alone
>>> don't usually fix bugs, but it might help humans better catch ones.
>>>
>>> Ok, anyway, I've got no real fix for now, but I'm going on.
>>>
>>>
>>> Thank you,
>>> Nikolai
>>>
>>>
> ....
>