Re: [PATCH] Fix EVT_STC_PAINTED recursion issue (Was: Re: Repaint lockup in ctlSQLBox::OnPositionStc on windows)

Thanks - patch applied.

On Sun, Oct 4, 2015 at 2:52 PM, Nikolai Zhubr <n-a-zhubr(at)yandex(dot)ru> wrote:
> 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
>>>>
>>>>
>> ....
>>
>
>
>
> --
> Sent via pgadmin-hackers mailing list (pgadmin-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgadmin-hackers

--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company