Hey Thomas,
Sorry for the delay on the review for this, but I finally had some
time to look into it more deeply. So the main issues with this patch
are the fact that CPControl should be handling most of this (as stated
earlier), and that the logic should take place entirely "in"
mouseDown:. What I mean by this is that mouseDown: should "trap" all
events until the next mouseUp:, and manage the entire flow. In
pseudocode:
- mouseDown:
while (getNextEvent)
if (currentEvent is mousedragged) highlight or unhighlight
else if (currentEvent is mouseUp) fire action, return
The reason for this is non-obvious but an unfortunate constraint that
internal Cappuccino code has to adhere to that user code does not.
Using mouseDown,mouseDragged,mouseUp combinations is not "wrong" and
works, but leads to subtle bugs appearing when you subclass. For
example, If I were to make my own CPControl or CPButton subclass, and
decided to overload the mouseDown: method, I could potentially break
the entire class, because mouseDown is intrinsically tied to
mouseDragged and mouseUp. Because of this, I would be forced to
overload *all* the methods to ensure to not have mouseDragged called
in some broken half-state. By placing all the logic in mouseDown, we
remove this constraint on our subclasses. Looking through CPControl
(and NSControl.m in Cocotron), it seems the appropriate thing to do is
have the following methods defined in CPControl (which are defined in
NSCell in Cocoa but we don't have cells)
- (BOOL)startTrackingAt:(CGPoint)aPoint
- (BOOL)continueTracking:(CGPoint)lastPoint at:(CGPoint)aPoint
- (void)stopTracking:(CGPoint)lastPoint at:(CGPoint)aPoint mouseIsUp:
(BOOL)mouseIsUp
- (void)trackMouse:(CPEvent)anEvent untilMouseUp:(BOOL)
shouldContinueUntilMouseUp
and the psuedo code would look something like this:
-mouseDown:anEvent
if (not enabled) return; // nothing to do.
[self trackMouse:anEvent untilMouseUp:YES];
-trackMouse:anEvent untilMouseUp:shouldContinueUntilMouseUp
if (event is mousedown) startTackingAt:event.locationInWIndow
else if (event is mousedragged && event location withing frame)
continueTracking:at:....
else if (event is out of frame) { stopTracking... if (event is mouseup
|| !shouldContinueUntilMouseUp) return;}
register to receive next event
A good place to see this sort of logic is in CPSplitView.j. If you
look at CPSplitView's mouseDown: method you'll see that it does this
fancy event-loop stuff. Again, this is just a rough outline of what
needs to happen, you've unfortunately (or fortunately ;) ) chosen one
of the "harder" problems in CPControl/CPButton, but it will be good to
finally have it in there. Another good reference is NSControl and
NSCell in the Cocotron project, as well as the doc pages on these two
at
developer.apple.com. And don't forget we're always in the IRC
channel to help out.
> I've modified this patch following the advices of Francisco found here :
http://github.com/suitmymind/cappuccino/commit/256c154305b307ed731ffd...
>
> Here is the modification :
http://github.com/suitmymind/cappuccino/commit/fca4d7e4bc42d4dcf8a89c...
>
> But I don't really understand how the modification to CPButton.j could
> be done in CPControl.j since it deals with _isHighlighted which
> belongs to CPButton. I'm surely missing something.
>
> Any help would be welcome.
>
> Best,
> Thomas Balthazar.
>
> On 05 Jan 2009, at 09:19, Thomas Balthazar wrote:
>
> > Hello,
>
> > The 2 commits used tab char instead of 4 spaces.
> > Heres is the new commit (1 commit instead of 2) :
> >
http://github.com/suitmymind/cappuccino/commit/256c154305b307ed731ffd...
>
> > Best,
> > Thomas.
>
> > On Sun, Jan 4, 2009 at 8:48 PM, Thomas Balthazar <
gro...@suitmymind.com
> > > wrote:
> >> Commit 1 :
http://github.com/suitmymind/cappuccino/commit/522a44a60149fa24649c3c...
> >> Commit 2 :
http://github.com/suitmymind/cappuccino/commit/6f17a709b606dbbcd411c9...
> >> Ticket :
http://cappuccino.lighthouseapp.com/projects/16499/tickets/188-cpbutt...
>
> >> Hello,
>
> >> Here :
http://github.com/suitmymind/cappuccino/commit/522a44a60149fa24649c3c...
> >> I test if the CPControl is Enabled before sending the action.
>
> >> And here :
http://github.com/suitmymind/cappuccino/commit/6f17a709b606dbbcd411c9...