CPButton fires actions and draws alternateImage when disabled : patch

[Thomas Balthazar]

Commit 1 : http://github.com/suitmymind/cappuccino/commit/522a44a60149fa24649c3cec82c3c7deceb14147
Commit 2 : http://github.com/suitmymind/cappuccino/commit/6f17a709b606dbbcd411c9da59870728987c2705
Ticket : http://cappuccino.lighthouseapp.com/projects/16499/tickets/188-cpbutton-fires-actions-and-draws-alternateimage-when-disabled

Hello,

Here : http://github.com/suitmymind/cappuccino/commit/522a44a60149fa24649c3cec82c3c7deceb14147
I test if the CPControl is Enabled before sending the action.

And here : http://github.com/suitmymind/cappuccino/commit/6f17a709b606dbbcd411c9da59870728987c2705
I prevent the button from being highlighted if disabled

Feel free to comment,
Thanks.

Thomas.

[Thomas Balthazar]

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/256c154305b307ed731ffd07bbe0f54d0f1109a7

Best,
Thomas.

[Thomas Balthazar]

Hello,

I've modified this patch following the advices of Francisco found here :
http://github.com/suitmymind/cappuccino/commit/256c154305b307ed731ffd07bbe0f54d0f1109a7

Here is the modification :
http://github.com/suitmymind/cappuccino/commit/fca4d7e4bc42d4dcf8a89c00c3fe66c48c0bd91a

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.

[Francisco Tolmasky]

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.

On Jan 9, 12:45 pm, Thomas Balthazar <gro...@suitmymind.com> wrote:
> Hello,
>

> 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...

[Thomas Balthazar]

Hello Francisco,

Thanks for the in depth explanation.
I'm working on it.

I'll keep you posted here.

Best,
Thomas Balthazar.

[Thomas Balthazar]

Hi Francisco,

I've started reworking the patch following your advices.
I'm not sure I'm on the right way.
If you find the time, could you please comment my code and give me
rough directions of what are the next steps?

Here it is :
http://github.com/suitmymind/cappuccino/commit/c1cd944f7e8fdddfc0ba746c38d02e8998d178f5#L1R385

Thanks in advance for any help/comment from everybody.
This patch is quite exciting to work on.

Cheers,
Thomas.

On 11 Jan 2009, at 02:56, Francisco Tolmasky 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
>>>> ...

[Thomas Balthazar]

I've re-worked the patch :
http://github.com/suitmymind/cappuccino/commit/eef6eaa65829b0fccb11b0b1c1ab11c3671051bd

It works, but I'd like to have your opinion on the solution I came with.

The log messages are still there in the code, so you can view what
happens in the following examples.

An example with an enabled button here :
http://suitmymind.com/tmp/cappuccino/ticket-188-enabled/index.html#debug

And with a disabled button here :
http://suitmymind.com/tmp/cappuccino/ticket-188-disabled/index.html#debug

I'll be glad to have some feedback on this case.
Thomas.

P.S. : the ticket link :
http://cappuccino.lighthouseapp.com/projects/16499/tickets/188

>>>>> :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...

[Francisco Tolmasky]

I'm going to test this out tomorrow (my current location refuses to
talk to github), but it looks pretty promising from a first glance. I
think we can just plain old get rid of the mouseDown: and
mouseDragged: from CPButton right?

On Jan 18, 10:32 am, "Thomas Balthazar" <gro...@suitmymind.com> wrote:
> I've re-worked the patch :http://github.com/suitmymind/cappuccino/commit/eef6eaa65829b0fccb11b0...

>
> It works, but I'd like to have your opinion on the solution I came with.
>
> The log messages are still there in the code, so you can view what
> happens in the following examples.
>
> An example with an enabled button here :http://suitmymind.com/tmp/cappuccino/ticket-188-enabled/index.html#debug
>

> And with a disabled button here :http://suitmymind.com/tmp/cappuccino/ticket-188-disabled/index.html#d...

>
> I'll be glad to have some feedback on this case.
> Thomas.
>
> P.S. : the ticket link :http://cappuccino.lighthouseapp.com/projects/16499/tickets/188
>
> On Wed, Jan 14, 2009 at 7:54 PM, Thomas Balthazar <gro...@suitmymind.com> wrote:
> > Hi Francisco,
>
> > I've started reworking the patch following your advices.
> > I'm not sure I'm on the right way.
> > If you find the time, could you please comment my code and give me rough
> > directions of what are the next steps?
>
> > Here it is :

> >http://github.com/suitmymind/cappuccino/commit/c1cd944f7e8fdddfc0ba74...

[Thomas Balthazar]

> I think we can just plain old get rid of the mouseDown: and
> mouseDragged: from CPButton right?

Yes, those methods are empty.
Since I'll maybe have to rework part of the patch, I'll remove them,
and all the debug stuff, after I get your feedback on this point.

Thanks!
Thomas.

[Francisco Tolmasky]

I just went over the patch and it looks pretty good, its basically
there. I went ahead and made the minor changes (and took out the logs
and stuff) and committed it to the master branch. The only minor
problem was a weird interaction with modal windows, so I went ahead
and fixed that too. Thanks a lot for this, we've been needing this
change for a *long* time.

On Jan 18, 1:01 pm, "Thomas Balthazar" <gro...@suitmymind.com> wrote:
> > I think we can just plain old get rid of the mouseDown: and
> > mouseDragged: from CPButton right?
>
> Yes, those methods are empty.
> Since I'll maybe have to rework part of the patch, I'll remove them,
> and all the debug stuff, after I get your feedback on this point.
>
> Thanks!
> Thomas.
>

[Thomas Balthazar]

Glad to help!
Thanks a lot for the advices.
I learned a lot on this case.