fredsa / playn

Cross platform game library for N≥4 platforms
0 stars 1 forks source link

Add onMouseOut and onMouseOver #174

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
Purpose of code changes on this branch:

This is the basic implementation of one approach for onMouseOver/onMouseOut 
discussed on the mailing list:
1) Add onMouseOver and onMouseOut methods to a new LayerListener interface.
2) Do not add Mouse.setListener(LayerListener), as onMouseOut doesn't make a 
ton of sense.
3) Use a single event for all three of onMouseOver/onMouseOut/onMouseMove

By adding separate methods for addListener(Mouse.Listener) and 
addListener(Mouse.LayerListener), we no longer break existing users.

I don't like that this introduces a performance hit on users not using Mouse. 
We can add a flag on MouseImpl to track whether we have mouse listeners at all, 
and set this flag in AbstractLayer::addListener and Mouse::setListener. This 
lets us skip the extra hitTests and doesn't require further flag maintenance, 
at the cost of spaghetti code. Thoughts?

I think this ends up being a cleaner API than the Utils class approach.

When reviewing my code changes, please focus on:

http://code.google.com/r/pdr-playn/source/detail?r=1ca5ac68a68196a9bbfbe87aadef8
1e0c8415294#

After the review, I'll merge this branch into:
/trunk

Original issue reported on code.google.com by pdr@chromium.org on 23 May 2012 at 3:14

GoogleCodeExporter commented 9 years ago
Ping for a review on the latest patch.

I think we settled on a pretty nice solution that's ready to land.

Original comment by pdr@chromium.org on 30 May 2012 at 12:32

GoogleCodeExporter commented 9 years ago
OK, small derailing, but based on this issue: 
http://code.google.com/p/playn/issues/detail?id=180 and based on the changes I 
made to Touch when adding Layer.addListener(Touch.LayerListener), I think we 
should rework this so that LayerListener and Listener are totally separate 
interfaces. (This is what I did for Touch of necessity.)

This will allow us to avoid the confusion of methods that aren't called in the 
same circumstances, or at all. The LayerListener would have this definition:

{{{
  interface LayerListener {
    void onMouseDown(ButtonEvent event);
    void onMouseUp(ButtonEvent event);
    void onMouseDrag(MotionEvent event);
    void onMouseOver(MotionEvent event);
    void onMouseOut(MotionEvent event);
  }
}}}

Notably: onMouseMove becomes onMouseDrag to indicate that motion events are 
only dispatched to layers when the user has first clicked into a layer and made 
it active, and onMouseWheelScroll is omitted, because that event is never 
dispatched to a layer. (As I mentioned in the comments of issue 180, maybe we 
want to change that, but I'm not yet convinced.)

Original comment by m...@samskivert.com on 30 May 2012 at 4:01

GoogleCodeExporter commented 9 years ago
Splitting Listener and LayerListener was a great idea and I think it's much 
clearer this way, especially for the onMouseDrag/onMouseMove and onMouseScroll 
cases.

I think there are usecases for onMouseMove on layer, but it's tangential to 
this change. One example might be a colorpicker layer where moving the mouse 
could pop up a little preview of the color before making the modal change of 
selecting a color.

I did make one other minor change: I changed the variable name on 
Layer.addListener(Mouse.LayerListener mouseLayerListener). Eclipse's 
autocomplete makes it difficult to tell the difference between 
Mouse.LayerListener and Touch.LayerListener and I think this will help point 
users in the right direction. I'm plenty happy to take this out if I'm 
overlooking something.

Final version:
http://code.google.com/r/pdr-playn/source/detail?r=ba338c1d2b2c049c91304576ca362
a9d90e80dce

Original comment by pdr@google.com on 31 May 2012 at 1:41

GoogleCodeExporter commented 9 years ago
Woohoo, and we landed: 
http://code.google.com/p/playn/source/detail?r=ba338c1d2b2c

Original comment by pdr@google.com on 31 May 2012 at 1:45

GoogleCodeExporter commented 9 years ago
I'm surprised that Eclipse doesn't tell you the type names of the method 
arguments but does tell you the argument names.

In any case, I don't think it's useful to call the mouse and touch listener 
arguments touchLayerListener and mouseLayerListener. Your purpose in naming 
them verbosely is to communicate their difference from one another and from 
pointerListener, so just call them mouseListener, touchListener and 
pointerListener. Once the user has made their choice, they can then type "new 
<tab>" and Eclipse will fill in the right type (or give them the choice between 
Foo.LayerListener and Foo.LayerAdapter) so the job of the argument name is done 
at that point and need not be further verbosed so as to communicate the 
argument type as well as its name.

Original comment by m...@samskivert.com on 31 May 2012 at 4:52