Re: [PATCH] Fix EVT_STC_PAINTED recursion issue (Was: Re: Repaint lockup in ctlSQLBox::OnPositionStc on windows) - Mailing list pgadmin-hackers
From | Dave Page |
---|---|
Subject | Re: [PATCH] Fix EVT_STC_PAINTED recursion issue (Was: Re: Repaint lockup in ctlSQLBox::OnPositionStc on windows) |
Date | |
Msg-id | CA+OCxoxSuUh5MvCU18ZgBikdD_C1j+aTSCXrJnEAs7WHOZT+Pw@mail.gmail.com Whole thread Raw |
In response to | [PATCH] Fix EVT_STC_PAINTED recursion issue (Was: Re: Repaint lockup in ctlSQLBox::OnPositionStc on windows) (Nikolai Zhubr <n-a-zhubr@yandex.ru>) |
List | pgadmin-hackers |
Thanks - patch applied. On Sun, Oct 4, 2015 at 2:52 PM, Nikolai Zhubr <n-a-zhubr@yandex.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@postgresql.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
pgadmin-hackers by date: