Message-ID: <56112F2E.5040107@yandex.ru>
Date: Sun, 04 Oct 2015 16:52:46 +0300
From: Nikolai Zhubr <n-a-zhubr@yandex.ru>
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; ru;
 rv:1.9.1.5) Gecko/20091204 Thunderbird/3.0
MIME-Version: 1.0
To: pgadmin-hackers <pgadmin-hackers@postgresql.org>
Subject: [PATCH] Fix EVT_STC_PAINTED recursion issue (Was: Re:
 Repaint lockup in ctlSQLBox::OnPositionStc on windows)
References: <r8vwwstv0bdjlhprr0tg4nva.1443292908241@email.android.com>
 <5606F2C8.9010001@yandex.ru> <56070F5A.3070102@yandex.ru>
 <1674144739.20150927082019@mail.ru> <5607CF32.7030707@yandex.ru>
 <5609BC61.8030503@yandex.ru> <560ADB34.6050700@yandex.ru>
 <561024FB.9000805@yandex.ru> <5610587D.5020605@yandex.ru>
 <1435296136.20151004073210@mail.ru>
In-Reply-To: <1435296136.20151004073210@mail.ru>
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk
Sender: pgadmin-hackers-owner@postgresql.org

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