ALyman's picture

Fix MouseDevice.Move

Project:The Open Toolkit library
Component:Code
Category:task
Priority:normal
Assigned:Unassigned
Status:closed
Description

OpenTK.Input.MouseDevice.Move was commented out in revision 1303, because the .XDelta and .YDelta properties were not working properly. The best solution mentioned in this forum thread, was to remove the delta support.

I've attached a patch to /trunk,r1519 that does a few things (I figured that while I was in there mucking about...):

- Re-enables the MouseDevice.Move event, using a new MouseMoveEventArgs class that holds an (X, Y) pair and also a (XDelta, YDelta) pair. XDelta and YDelta are calculated from the last known mouse position.
- Creates a new MouseDevice.WheelChanged event, which uses a new MouseWheelEventArgs class that holds an (X, Y) pair for the current mouse position, a Value and Delta properties. Delta is calculated from the last known wheel value.
- Cleans up MouseDevice.ButtonDown and MouseDevice.ButtonUp to use the same event handler delegate type, which uses a new MouseButtonEventArgs class, that holds an (X, Y) pair and a Button property.
- Cleans up a bit of the doc comments from MouseDevice (mostly just things that were throwing warnings at compile time)

Not that the ButtonDown and ButtonUp changes are not actually a bug fix, just a change to maintain consistency with the other new code.

I have tested these on Windows XP and OpenSUSE, and found no problems.

AttachmentSize
OpenTK.Input_.MouseDevice.patch12.14 KB

Comments

Comment viewing options

Select your preferred way to display the comments and click "Save settings" to activate your changes.
ALyman's picture

#1

Status:open» in progress (review)
the Fiddler's picture

#2

Status:in progress (review)» in progress

Thanks for the code, I believe you've nailed the best solution: deltas only make sense in the context of a mouse movement.

Two suggestions on the code:

  • Please mark existing functionality with the [Obsolete] attribute, instead of removing it outright, to give existing apps a chance to migrate.
  • Part of the work on 0.9.2 was to avoid temporary objects that can , like Mouse*EventArgs. The best solution, I think, is to change them to structs - not conceptually clean, but it's simple and works. I kinda wonder how the WinForms team tackled this issue (some kind of pooling, I'd guess).
ALyman's picture

#3

Sorry, I'm fairly used to the lifestyle of following /trunk on OSS I have a dependency on, and updating my own software once every couple weeks. Totally forget that other developers might not do that. :P

Attached is a new patch that supports no-compilation backwards compatibility (however, if you recompile, the .MouseDown and .MouseUp events will break, but it's an easy fix), while still providing the interface the original patch did:

  • Un-removed .XDelta, .YDelta, .WheelDelta, with ObsoleteAttribute's applied in warning-only mode, with proper messages.
  • Un-removed the MouseButtonDownEvent and MouseButtonUpEvent delegates.
  • Added .add_ButtonDown(MouseButtonDownEvent) and .remove_ButtonDown(MouseButtonDownEvent), along with a second delegate member, oldButtonDown, with the code to call it.
  • Added .add_ButtonUp(MouseButtonUpEvent) and .remove_ButtonUp(MouseButtonUpEvent), along with a second delegate member, oldButtonUp, with the code to call it.
  • All of the event-related compatibility methods have the ObsoleteAttribute applied, and the add_/remove_* methods also have an EditorBrowsableAttribute to hide them from intellisense.

Everything for compatibility is surrounded by an #if/#endif (with a corresponding top-of-file #define) on the symbol "COMPAT_REV1519", so it should be easy to remove them once you feel it's safe to do so.

As for the EventArgs-based reference types, the main reason I went with those is the same reason that the BCL recommends them -- it allows event subscribers that don't care about the actual event that calls it to function. For instance:

void Test() {
    mouseDevice.Move += new MouseMoveEventHandler(handleMouseEvent);
    mouseDevice.ButtonDown += new MouseButtonEventHandler(handleMouseEvent);
    mouseDevice.ButtonUp += new MouseButtonEventHandler(handleMouseEvent);
    mouseDevice.WheelChanged += new MouseWheelEventHandler(handleMouseEvent);
}
void handleMouseEvent(object sender, MouseEventArgs e) {
    // this gets called when /any/ mouse event happens.
}

I realized a couple things that I had intended to change in the first patch, but failed to do so. Mainly just a few clean-ups, based on recommended practice, but also one thing I realized was missing when I wrote the above code snippet:

  • The event handler delegates now have "Handler" on the end of their name. This makes it clear while reading through 3rd-party source that they are actually event handlers and not events themselves.
  • The event handler delegates take an object as sender. The reference to MouseDevice is no-longer necessary, since all the data pertaining to the event is contained within one of the EventArgs. Also, this would allow other things to raise these event types and allow user code to use the same handler methods.
  • Added a MouseButtonEventArgs.Pressed property, so it's possible to tell the difference between the MouseDown and MouseUp events from inside an event handler.
AttachmentSize
MouseDevice.patch18.57 KB
ALyman's picture

#4

Status:in progress» in progress (review)

How have I forgotten to change the status twice already in one issue?

ALyman's picture

#5

Has anyone looked at the patch since the last update?

the Fiddler's picture

#6

Yes, it's in the pipeline along with the joystick stuff, I just didn't want to add a breaking change this close to release.

The add_Event / remove_Event trick is pretty ingenious, never seen that before!

I'm reading up on best practices for events, now. (Ultimately, all events in OpenTK will have to be changed to follow the same pattern.)

Questions:

  1. EventArgs: I'm not saying it's a good idea to break BCL conventions, but I'm still worried about the performance impact. It *is* possible to write a single subscriber for multiple events:
    void handleMouseEvent<T>(object sender, T e) {
        // this gets called when /any/ mouse event happens.
    }

    I wonder if it's possible to implement Mouse*EventArgs in a way that doesn't allocate memory and doesn't break BCL conventions.

  2. I'm not sure I understand the reason for using object sender instead of a concrete type. Shouldn't the MouseDevice be solely responsible for raising Mouse* events? (Wouldn't anything else would break encapsulation?) And if these events are always raised by MouseDevices, why not use a concrete type?
the Fiddler's picture

#7

Status:in progress (review)» fixed

The patch, with slight modifications, is now committed. Thanks!

the Fiddler's picture

#8

Version:0.9.x-dev»
Status:fixed» closed

Closing bugs fixed in 0.9.3.