Mantis Bugtracker
  

Viewing Issue Simple Details Jump to Notes ] View Advanced ] Issue History ] Print ]
ID Category Severity Reproducibility Date Submitted Last Update
0001037 [Squeak] Morphic major always 04-02-05 23:59 08-26-05 18:32
Reporter wiz View Status public  
Assigned To
Priority normal Resolution open  
Status closed   Product Version 3.8
Summary 0001037: [bug] [testers] Click state handler causes mouse up to be processed twice because of (unitended?) recursion.
Description MouseClickState>>handleEvent:from: is nonrecursive except at one point where it is processing the first mouseup. There it calls handleEvent: with a copy of the mouseup event after firing the click selector.

I've check with a diagnostic and this is definitely a recursive call. The same mouseup comes thru a second time (the click state has changed. And the recursive event glides thru without causing anymore changes.)
The recursive call releases it back to handleEvent with the true flag. It is processed to complection.
When handleEvent exits the remaining unrecursed event is released back to handleEvent with the original false flag. Furthur processing is not done and handleEvent releases back to its original caller.

I do not believe this is what was actually intended. But I do not understand what the actual intention was either.

The probable consequence of the recursion is that some event listeners are processing mouse up twice instead of once. System Windows are probably affected by this.
Additional Information This is a long standing bug. I originally wrote the original author of the code jcg but received no responces (this was long ago) the letter was:

Dear jcg

Hi. I have been trying to learn squeak and recently have been reading the code in
MouseClickState event handling. The original version of this has your initials on it and was written/added in Sept 2001, The routine that calls it for action is HandMorph>>handleEvent; and that was written six months earlier.

MouseClickState>>handleEvent:from: is nonrecursive except at one point where it is processing the first mouseup. There it calls handleEvent: with a copy of the mouseup event after firing the click selector.

I've check with a diagnostic and this is definitely a recursive call. The same mouseup comes thru a second time (the click state has changed. And the recursive event glides thru without causing anymore changes.)
The recursive call releases it back to handleEvent with the true flag. It is processed to complection.
When handleEvent exits the remaining unrecursed event is released back to handleEvent with the original false flag. Furthur processing is not done and handleEvent releases back to its original caller.

I do not believe this is what was actually intended. But I do not understand what the actual intention was either.

Changing the code so that instead of
self click.
aHand handleEvent: copiedEvt .
^false

we have just:

self click.
^true

would seem to do the trick as far as the post processing results are concerned.
So if there is a difference then it would have to be in what happens before clickstate is checked a second time.



It would also from the rest of the method seem to be the most logical way to have coded it in the first place. So obvious that the fact that it was not is curious.

Was there something click was supposed to change such as mouse focus that would make the recurssion useful?

If click does not change mouse focus then you have the same old listeners to mouse up being fired twice. Which would explain a few things.

So I am trying to track down the history and the folklore on ClickState to see what I'm missing.

Any help would be appreciated.

-----------------

I had also posted other complaints about the handler which Ned Konz addressed.

I've been meaning to raise this issue on mantis and I've finally gotten a general click state tester to facilate fully checking the handler.

IMHO the handler needs to be re-examined and redone. It has had bugs in it and is written too poorly to be easily proved correct or maintained.

My contribution here are the analysis tools I built while tracking problems with the routine.


Major or minor?

I've chosen major assuming that the poor state of the code is a clue that it is causing problems down the line that people have complained of but not tracked back to this routine. I don't have a smoking gun yet. Still squeak will work better, maybe much better once this is properly fixed.


Attached Files  CSExersizer-wiz.5.cs [^] (7,987 bytes) 04-02-05 23:59
 ClickTestTools-wiz.2.cs.gz [^] (5,082 bytes) 04-03-05 00:01
 CSRecurseFix-wiz.1.cs [^] (3,808 bytes) 04-03-05 00:03
 ClickProbeTools-wiz.1.cs.gz [^] (4,568 bytes) 05-01-05 06:54
 ClickStateFocusBug.gif [^] (18,650 bytes) 05-01-05 06:54
 CSExersizer-wiz.8.cs [^] (11,106 bytes) 08-22-05 09:53

- Relationships

SYSTEM WARNING: Creating default object from empty value

related to 0001567closed  [BUG][FIX] PLM click on two different mouse buttons interpreted as double click 
related to 0007039new  Click events are only honored after moving the mouse 
child of 0006975new  [RFE] Better event distictions between drag, click and mouse up. 

- Notes
(0001338 - 925 - 1132 - 1132 - 1132 - 1132 - 1132)
wiz
04-03-05 00:14
edited on: 04-19-05 05:43

 CSExersizer-wiz.5.cs
        This is a generalized exerciser similar to double click example. It adds a dblClicktime out action ( cycling thru different border widths) and has a way to activate/deactivate the 4 possible actions.
Most recently worked on in 3.8 in early April 2005


 ClickTestTools-wiz.2.cs.gz
        This is an analysis tool that allows collecting a history of clickstates and the mouse events that pass thru them. I haven't work on this since July 2004 but I don't believe any of the things this touches has been changed since then. So it should still be relevant.


 CSRecurseFix-wiz.1.cs
         This is also circa August 2004. If removes the recursion but shouldn't be considerer a "fix" it is just the alternative for testing purposes. If there was a reason for the recursion that I could not figure out then this will possibly cause mouseups not to be handled where they should.

 
(0001387 - 51 - 51 - 51 - 188 - 188 - 188)
wiz
04-19-05 07:10

See also related tweak issues 0001039 and 0001077
 
(0001457 - 1973 - 2136 - 2136 - 2136 - 2136 - 2136)
wiz
05-01-05 07:15
edited on: 08-22-05 09:36

ClickProbeTools-wiz.1.cs is the fourth iteration of test tools. It now tracks both the event and the mouse focus. The probe tools are now orthogonal to the Exersizer tool.

ProbeTools looks and the event histories.
Exersizers create instances that can be used to exersize different event logics.

MouseFocus is important because of a recusion related bug. The mouse focus in DoubleClickExample is cleared to nil after the recursive mouse up. This means that the second mouse down and the final double click mouse up are dispatched with a mouse focus of nil. This probably will explain some other weird stuff.

I haven't tried to determine yet if the mouse focus problem is the fault of DoubleClickExample or the clickstate handler. My moneys on the latter 'cause that would be a cooler bug.

ClickStateFocusBug.gif is an example of the bug happening. History is a collection of the event followed by the mousefocus for each event processed by clickstate.

21Aug2005: After more looking I found that the focus is reset when the Mouseup event is "sentTo:" aMorph. The only way to avoid it is to not send it thru to be processed or to send it true with the wasHandled flag set. Which means it won't really process either.

So the question is what is the focused on morph suppose to see during each of the variations of a double click event?

It seems to me the fate of each event depends on the total state of
ActiveEvent
it's own was handled flag.
click states idea of whether to pass the event thru to be processed.
mouseFocus

and at this point I'm not sure if there aren't other globally available stuff that has to be taken into account (like the preferences on genie gestures.)

Squeak is much more confident in its ability to juggle these things than it ought to be. If you don't believe me take a look at DoubleClickExampleMorphs>drag: comment. That wishful state of thinking wasn't even true in squeak3.0 (I went back and checked).

 
(0001458 - 595 - 619 - 619 - 619 - 619 - 619)
wiz
05-01-05 07:24

As a side note. There is also probably another bug in the clickstate code.

The drag threshold is computed from the difference between the local event and the first click event. Since the local event has been transformed and the first click event has not they are not garanteed to be in the same reference frame. This is an apples to oranges comparison error.

In practice the bug is not apparent because the local transform is usually the identity transform. For the sake of the treshold calculation the position of the untransformed current event should be compared to the first event.
 
(0002492 - 540 - 588 - 588 - 718 - 718 - 718)
wiz
08-22-05 09:53
edited on: 08-23-05 21:59

See also 0001567 PLM click on two different mouse buttons interpreted as double click.

The proposed fix is to the #handleEvent:from: method which we have be dealing with here.

Part of what I'm actively working on has been a new CSExersizer. This one has buttons right on the morph itself and a more esthetic behavior for #startDrag.

I've uploaded the most recent version: CSExersizer-wiz.8.cs .It allows interactively testing with full control over which selectors are active. It looks best when given a 45 degree twist clockwise.

 
(0002503 - 991 - 1068 - 1068 - 1068 - 1068 - 1068)
wiz
08-23-05 22:15

More study has lead me to some tenative answers to my own questions and further clarity on what is going on.

Mouse listeners are not processing mouse events just collecting info.

Events will get "eaten' by MCS (MouseClickState) unless MCS answers true. In which case they are processed by the handling code below.

Processing a mousedown would by default interupt and reset clickstate (blue button activity possibly excepted). So they must be eaten.

Processing a mouseUp will unset the mouseFocus so this should not happen until all else is done. In practice it does happen but as it does not reset the clickstate the doubleclick , or timeout or drag will still be detected.

So there seems to have been an intention to delay the mouseup until all was done and then handle it. I don't think the present code succeeds in this however.

I'm still curious to find the authoriative story of how things are supposed to work. This means finding both the story and the authority.
 
(0002525 - 866 - 956 - 956 - 956 - 956 - 956)
wiz
08-25-05 10:54

I did some more historical research with some surprising results.
DoubleClickExample actually worked as described in its drag method. It worked in Squeak 2.7 but was broken by 2.8.
The list of authors is large:
2.7 di
2.8 mir
3.2 RAA
3.4 jcg
3.6 or 3.7 nk
and the bugs seem to creep in as the code changes.

Also nk (Ned Konz) in his connecter code seems to have bypassed this handling and implemented mouse handling via his fsm (finite state machine) code.

I suspect the specs for what the clients expected from the doubleclick code changed as well. And I will be very surprised if the expectation amoung clients (even excepting DoubleClickExample who'se expectation hasn't been met since 2.7) is consistent.


There has also been a trend of more and more layers of indirection being added with each rewrite. How fast does this handling need to be?
 
(0002536 - 182 - 197 - 197 - 457 - 457 - 457)
wiz
08-26-05 09:19

I've posted an extensive fix to mantis 0001567. And will continue to post my work there. So that now supercedes this bug report.

So someone should close this --> forwarded to 0001567.
 
(0002541 - 71 - 71 - 71 - 201 - 201 - 201)
KenCausey
08-26-05 18:32

Closed at reporters request. See 0001567 for continuation of this issue.
 

- Issue History
Date Modified Username Field Change
04-02-05 23:59 wiz New Issue
04-02-05 23:59 wiz File Added: CSExersizer-wiz.5.cs
04-03-05 00:01 wiz File Added: ClickTestTools-wiz.2.cs.gz
04-03-05 00:03 wiz File Added: CSRecurseFix-wiz.1.cs
04-03-05 00:14 wiz Note Added: 0001338
04-19-05 05:43 wiz Note Edited: 0001338
04-19-05 07:10 wiz Note Added: 0001387
05-01-05 06:54 wiz File Added: ClickProbeTools-wiz.1.cs.gz
05-01-05 06:54 wiz File Added: ClickStateFocusBug.gif
05-01-05 07:15 wiz Note Added: 0001457
05-01-05 07:24 wiz Note Added: 0001458
08-22-05 09:36 wiz Note Edited: 0001457
08-22-05 09:53 wiz File Added: CSExersizer-wiz.8.cs
08-22-05 09:53 wiz Note Added: 0002492
08-22-05 10:02 wiz Note Edited: 0002492
08-23-05 21:59 wiz Note Edited: 0002492
08-23-05 22:15 wiz Note Added: 0002503
08-25-05 10:54 wiz Note Added: 0002525
08-25-05 10:59 wiz Note Added: 0002526
08-25-05 11:05 wiz Note Deleted: 0002526
08-26-05 09:19 wiz Note Added: 0002536
08-26-05 18:26 KenCausey Relationship added related to 0001567
08-26-05 18:32 KenCausey Status new => closed
08-26-05 18:32 KenCausey Note Added: 0002541
05-09-08 02:05 wiz Relationship added child of 0006975
03-17-09 05:00 wiz Relationship added related to 0007039


Mantis 1.0.8[^]
Copyright © 2000 - 2007 Mantis Group
110 total queries executed.
53 unique queries executed.
Powered by Mantis Bugtracker