Menu
▾
▴
[Tcl-bugs] [ tktoolkit-Patches-2838812 ] Multi-purpose patch: fixes bug#1391053, adds wm attrs.
|
From: SourceForge.net <no...@so...> - 2010-01-04 21:39:53
|
Patches item #2838812, was opened at 2009-08-17 12:42 Message generated for change (Comment added) made by a_kovalenko You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=312997&aid=2838812&group_id=12997 Please note that this message will contain a full copy of the comment thread, including the initial issue submission, for this request, not just the latest update. Category: 68. Win Window Operations Group: None Status: Open Resolution: None Priority: 7 Private: No Submitted By: Anton Kovalenko (a_kovalenko) Assigned to: Pat Thoyts (patthoyts) Summary: Multi-purpose patch: fixes bug#1391053, adds wm attrs. Initial Comment: Bug#1391053: spurious events generated on Windows after completion of tk_getOpenFile, tk_chooseColor and similar modal dialogs. This patch, redone against CVS head, provides a real solution for this long-standing problem, instead of the current pseudo-solution that hurts but not helps (see win/tkWinDialog.c:EatSpuriousMessageBugFix). Steps to reproduce: (1) bind . <1> "puts Click"; bind . <3> tk_getOpenFile; maximize main window; right-click, then select a file. When the file dialog is closed on click-that-selects-a-file, you'll see the Click reaching the main window. Similar thing for closed Tk non-main toplevel windows by double-clicking on system menu icon: if another tk window happens to be under the click, it receives an event. Tk assumes that if a mouse button was released and is now pressed, then it must generate an event (Tk_UpdatePointer). This patch adds a special flag for Tk_UpdatePointer to take exception from this assumption. This modification proven to be useful for many other things during 4 years I maintain it for my projects. There is another obscure manifestation of a similar problem, related to non-client clicks (like WM_NCLBUTTONDOWN): tk really shouldn't generate ButtonDown on these events. This patch adds WM_NC* handling to the toplevel window procedure, using special flag for Tk_UpdatePointer to prevent these spurious events too, and another flag to prevent cursor shape changes for non-client areas). Unfortunately there are another things in this patch (separating them is not easy): support for wm attr -contexthelp (uses virtual <<Help>> event, no need to worry about event mask bits exhaustion); support for wm attr -shadow (recreates wrapper window with CS_DROPSHADOW). To complicate things even more, this patch includes tkUnixRFont update for -underline and -overstrike support in Xft backed. This part can be separated :) I would like to see something done in this direction: while we can live without improvements, though they're discussed and accepted long ago, the bugs addressed by this patch are really annoying. E.g. a user does Menu->File->Open, and his file-selecting click goes directly to toolbar->New file :( Should I simply commit this whole thing myself? If it's unacceptable, can anyone provide the directions for refactoring it? Maybe I should wait 4 more years.. ---------------------------------------------------------------------- >Comment By: Anton Kovalenko (a_kovalenko) Date: 2010-01-05 00:39 Message: First, many thanks to patthoyts for reviewing those patches. Sorry, but I'm going to defend my variant of Eat Spurious Message Bug Fix against the one currently present in tk 8.5/ 8.6 heads. Point one: did you try to hold the button for more than 250 msecs while testing current tk heads? It leaks more than a ButtonRelease in this case: it leaks an entire click, i.e. ButtonPress/ButtonRelease pair. In the context of my alternative solution, ButtonRelease is not "leaked" -- it's just _what happens_ when the button is pressed over a dialog window, then the dialog disappears, and then the button is released. I've yet to see any GUI misbehavior resulting from this event (that is not spurious, but realistic; it's the _click_ (press+release) that was spurious). If you prefer the way of handling things provided by the current solution, it's a must to prevent at least WM_PAINTs from being eaten or delayer. This aspect of the old solution is just plain ugly. The application _hangs_ for 250 ms., it's very noticeable, especially on fast computers where Tk normally works fast. I hate to say that, but the whole Tk's machinery of mouse event translations is broken by design. Tk assumes that the difference in button states should be translated into press/release events. I believe there was a reason to avoid the obvious alternative, where WM_*BUTTONDOWN would be translated into a press and WM_*BUTTONUP into a release (similar thing is possible in X11, with appropriate X events instead of WM_* messages). I don't know what were the reasons to prefer button state difference translation, instead of window system events translation. But this kind state translation _does_ hurt: only the window system is able to route button state changes appropriately, 'cause it knows where the _change_ happened. 0001-ak-Track-pointer-button-state.patch is just an attempt to provide a workaround for this state-translation system: the way to notify Tk that it should not generate any events for the change between the last button state and the current one. No real solution is possible without rewriting Tk event layer so it will translate window system events instead of tracking button state changes. Both in the File dialog and context help cases, _each_ possible solution will either (1) leak a button click, (2) leak a button release, (3) hang a little and then leak a click if the button is still down, (4) hang a little and then leak a button release if the button is still down. Original bug is (1), currently used Tk solution is (3), my proposed solution is (2). It's easy to turn my proposed solution into (4); the ugliness of the result is the only thing discouraging me from doing it. I'm afraid we are trapped in (1,2,3,4) variants, unless we redesign Tk event handling as described above. ---------------------------------------------------------------------- Comment By: Pat Thoyts (patthoyts) Date: 2010-01-04 02:30 Message: I've done a bit of testing on this using the git patch series below. The context help seems to work mostly ok but leaks a ButtonRelease event to the target widget. The current 8.5.8 using EatSpuriousMessages does not leak any ButtonRelease events when double-clicking a file icon in the tk_getOpenFile dialog over a button widget. However when we use this patch series which replaces the EatSpuriousMessages we see ButtonRelease events leaking from this again. I have attached my test script which shows this up. While the leaked buttonRelease events are not producing a click on the buttons, this is a regression from the current state of affairs. ---------------------------------------------------------------------- Comment By: Pat Thoyts (patthoyts) Date: 2010-01-02 19:43 Message: I have added an attempt to break this into functional pieces using Git. I applied this patch to my working HEAD tree (called master in git) then used 'git gui' to stage just the hunks or lines for each area to create commits that just do one thing. The unix Xft underline/strikeout issue has already been resolved in 8.6 so I discarded that along with some spurious edits (unused variables, white-space only). The first patch is still doing two things - its changing the windows EatSpuriousMessages and making changes to the event mask stuff. This should be broken into two. In the contexthelp part, I'm a bit uncertain about the use of a static to hold the help cursor id. I dont know if that will be ok with threads or using a safeTk interp. In the dropshadow patch, there is no explanation why the window class style is being changed from CS_VREDRAW|CS_HREDRAW to no-flags. This needs explaining. I'm not sure if the hunk in WmProc is supposed to be part of this patch or the first patch either. ---------------------------------------------------------------------- Comment By: Pat Thoyts (patthoyts) Date: 2010-01-02 16:44 Message: You should not mix functionality in a single patch. This patch needs to be broken into individual pieces that fix one thing at a time. That way, if/when later we try to identify when some new issue was created we can isolate the specific change. It is also much harder to review such multi-function patches. I know from previous testing that the contexthelp patch had problems. Dropshadow is a different thing too. You need to use something like git or quilt or learn to apply and revert patches. before I started to use git I used to maintain a number of patches and apply them (patch -p0 < feature.patch), work on them, regenerate the patch (cvs diff > feature.patch) and revert it (patch -p0 -R < feature.patch) to switch to some other task. Another option is to use multiple Tk sandbox checkouts to isolate the work. These days I use git checkout -b feature-patch; hack; git commit and various combinations of rebase/cherry-pick/merge to tidy up the patch if necessary. ---------------------------------------------------------------------- Comment By: Anton Kovalenko (a_kovalenko) Date: 2009-08-17 13:28 Message: P.S. Maybe NOBUTTONEVENTS_MASK added by this patch should be made public (not tkInt.h). We need some way for extensions to implement modal dialogs on Windows correctly. E.g. TDBC datasource selection dialog should call Tcl_SetServiceMode before/after dialog invocation, and it will need a solution similar to EatSpuriousMessageBugFix(). At least, this patch makes correct implementation possible at all. ---------------------------------------------------------------------- Comment By: Donal K. Fellows (dkf) Date: 2009-08-17 13:15 Message: Megapatches cause indigestion. Raising prio so that it's more likely to be reviewed soon. (No time to look now...) ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=312997&aid=2838812&group_id=12997 |