#1509 Horizontal mouse wheel support

Yuri Strot Wed 27 Apr 2011

SWT has horizontal mouse wheel support since 3.6: https://bugs.eclipse.org/bugs/show_bug.cgi?id=223637. There are SWT.MouseVerticalWheel, SWT.MouseHorizontalWheel and SWT.MouseWheel = MouseVerticalWheel. In JavaScript we can also use event.axis or event.wheelDeltaX to listen horizontal scrolling. Does it make sense to replace EventId.mouseWheel with EventId.mouseVerticalWheel and EventId.mouseHorizontalWheel?

brian Wed 27 Apr 2011

This is a good question. We should probably fix the API one way or another. By convention we've been using letter h or v instead of the long spelled out words (hbar/vbar or HAlign/VAlign). Although I sort of think X and Y might make more sense.

I see couple of options:

  1. Deprecate EventId.mouseWheel and add EventId.mouseWheelY and mouseWheelX
  2. Keep EventId.mouseWheel for vertical and add mouseWheelX
  3. Keep one EventId.mouseWheel and add new meta-data to Event such as Orientation

Yuri Strot Thu 28 Apr 2011

  • 3) Don't really like the idea to have unnecessary field for most of events
  • 2) Looks too confusing IMHO
  • 1) Probably the best option

Note that in browser when you scroll diagonally you'll get one event with x and y deltas. So in case of additional field it's better to have Size? delta. However this doesn't look useful for other events.

andy Thu 28 Apr 2011

I would treat this the same as mouseMoved - so a single mouseWheel callback. Is it plausible to reuse Event.pos to contain the "delta"? Tho you give up the the actual mouse pos then, which I think would come back to bite us.

brian Thu 28 Apr 2011

Yeah if the lower level toolkit reports mouse wheel x and y changes in one event, then FWT should too. And if FWT adopts that approach we easily cover cases where X and Y are reported independently without breaking anybody's code.

What I suggest is a breaking change to the onMouseWheel event:

  • count: stop reporting this field which previously was Y scroll delta
  • data: start to report this as a Point with both X and Y deltas

Sound like a game plan?

Yuri Strot Fri 29 Apr 2011

I think data should works. However it's currently marked as "user data" and in case of wheel event we restrict user to collect another data. Also data is Obj? and we need to point out the right field/type in fandoc and cast it everytime...

Why we have single event type for all kind of events? If I add mouse move handler, why I need "number of characters", "index for list based events" and popup menu? Another issue with such approach is necessity to mark all fields nullable. Even if I listen mouse move I should use not safe Point?

May be it's better to use hierarchical events and just provide following:

class MouseWheelEvent : MouseEvent
{
  Point delta
  ...
}

Why not?

DanielFath Fri 29 Apr 2011

Umm... is there a MouseEvent? Maybe Mouse(Wheel)Event should be it's seperate event with special data etc.

PS. TBH I don't get why all events got subsumed under one big Event?

brian Fri 29 Apr 2011

I think adding Point delta field to Event is fine, so let's do that.

Why we have single event type for all kind of events?

Because I personally find 20 different event classes just a bunch of bloat in the API and it is often hard to remember what class corresponds to what event. So I really like the way SWT had a simple, single Event class (at least originally I guess).

brian Fri 29 Apr 2011

Promoted to ticket #1509 and assigned to brian

DanielFath Fri 29 Apr 2011

Dunno, that seemed contrary to good OOP practices. I mean what should I do if I want to create my own event (like a multi-touch event)? Manually change the fwt to add that one event or wait for you guys to implement it? Plus all the data in one class is a bit redundant if I'm not mistaken (there isn't an instance of multiple events occuring at one time, right?).

Yuri Strot Fri 29 Apr 2011

I think there is no need to remember all event types while it's associated with handler:

widget.onMouseMove.add |e| { echo("moved to ${e.pos}") }
widget.onMouseWheel.add |e| { echo("scrolled to ${e.delta}") }
widget.onSelect.add |e| { echo("selection changed to ${e.offset}") }

I needn't care about event type here. But in case of structural events IDE can do a good job for me to provide actual code completion.

andy Fri 29 Apr 2011

To weigh in here - I have probably written more FWT code than anyone else by a pretty good margin - and this has never been an issue. I did one time extend Event since data wasn't flexible enough - but that was a very sophisticated use case. Overall I think the simplification of a single Event type and a single EventListener type is much more useful in practice than a proper Type hierarchy.

DanielFath Fri 29 Apr 2011

I see, its hard to evade years of Java conditioning. But ystrot has a point. How much is current system IDE friendly? Is there a more elegant way to not make too many Event classes but still keep it simple?

Also why did SWT switch from a single Event to a multitude of smaller classes?

Yuri Strot Fri 29 Apr 2011

Oh, EventListener... OK, without generics event type hierarchy doesn't look real.

Anyway I think this is not a breaking change which really make sense. The right way for event model would be reactive programming. But for now the new delta field should work.

Just one more question: Why Point, not Size? As I understand Point is like position while Size is something like dimension which is closer for wheel delta. We haven't complete arithmetic for gfx geometry yet, but I think it makes sense at some point. And in this case the difference between Point/Size will be appreciable: Point +/- Size = Point makes sense. So can use: textWidget.topRightPos += wheelEvent.delta.

brian Fri 29 Apr 2011

Just one more question: Why Point, not Size?

I think Point makes more sense since its a translation on the x-y coordinate space. Or at least it makes more sense than using width and height to indicate changes.

qualidafial Fri 29 Apr 2011

Just want to reiterate a point made earlier:

In SWT, event type is just an int so I can invent my own if needed.

In FWT it's an enum that I can't change. Would be nice if we substituted this with some const mixin so that anybody could define their own event types.

brian Sat 30 Apr 2011

In SWT, event type is just an int so I can invent my own if needed.

Good point, maybe we can just add a new value to the enum like custom?

andy Sat 30 Apr 2011

Good point, maybe we can just add a new value to the enum like custom

I've run across this one or twice - you still want to define what the event is without having to extend Event - so I think you'd have to make id an Int (and switch EventId over to const static fields) to make it worthwhile.

brian Mon 2 May 2011

Giving up the enum seems a bit drastic since its so nice to have a printable enum.

I would suggest these changes:

  1. add custom to EventId
  2. add customId to Event

That isn't perfect b/c it treats custom ids as third class citizens. But seems a good compromise to keep clean enum for built-in, commonly used event ids.

Yuri Strot Mon 2 May 2011

Sorry guys, but I didn't catch why you need custom events. If user created custom component outside of fwt it can use own events and own listeners. And I think this is a good idea because Event already overflowed. Even if you need to handle fwt event and provide custom one (for example key press -> text change) and want to reuse collected information, it's better to use composition:

class MyEvent
{
  Event e
  ...
}

P.S. Looks like there are a lot of discussions outside of original mouse wheel topic. I think it makes sense to open few separate topics.

qualidafial Wed 4 May 2011

Andy, I'm not advocating we use Ints for event IDs like SWT. I would rather introduce some const mixin for event IDs, and retrofit EventId to implement it. That way we don't even need a custom event type in FWT.

andy Fri 27 May 2011

Ticket resolved in 1.0.59

Both horizontal and vertical mouse wheel support are now available for both SWT and JavaScript. The wheel delta for each direction is available from the new Event.delta field. Much thanks to ystrot for putting together these patches.

Login or Signup to reply.