MouseTracker original events in handlers #215

msalsbery · 2013-09-06T00:21:58Z

Implemented "Expose original event in handlers" (#23) for MouseTracker

Added OpenSeadragon.getElementOffset() method. Element-relative mouse
coordinates should be correct even if the element and/or page is
scrolled (#131)

Implemented "Expose original event in handlers" (#23) for MouseTracker Added OpenSeadragon.getElementOffset() method. Element-relative mouse coordinates should be correct even if the element and/or page is scrolled (#131)

houseofyin · 2013-09-06T01:12:56Z

Would it make sense to extend the event tests to verify the new features?

houseofyin · 2013-09-06T01:15:49Z

src/button.js

                inTo( _this, $.ButtonState.HOVER );
            }
        },

-        focusHandler: function( tracker, position, buttonDownElement, buttonDownAny ) {
-            this.enterHandler( tracker, position, buttonDownElement, buttonDownAny );
+        // TODO: This didn't match handler signature in MouseTracker. Investigate!


For this and the other similar cases, I suspect the mismatch wasn't noticed because the extra parameters had not been used. However, that's just a hunch on my part.

They were used by passing them to different handlers however. I removed the commented code and it should work like it did, but it needs to be fixed in my opinion.

Comment & Formatting fixes

More formatting fixes

msalsbery · 2013-09-06T18:15:54Z

New commits - cleaned up distracting whitespace changes, cleaned up comments mentioned by houseofyin.

More formatting fixes

Bug fixes. Event handler methods shouldn't be called directly in theory...

Pass original touch events to handlers.

iangilman · 2013-09-07T00:07:21Z

src/mousetracker.js

@@ -127,7 +138,7 @@
         *      Are we currently tracking mouse events.
         * @property {Boolean} capturing
         *      Are we curruently capturing mouse events.
-         * @property {Boolean} buttonDown
+         * @property {Boolean} buttonDownElement


This name makes it seem like it's an element rather than a flag. That's a tricky one to come up with a good name for, though. Maybe stick with buttonDown?

Or maybe standardize on insideElementPress?

iangilman · 2013-09-07T00:57:34Z

Wow, this is quite the patch! As you see I had a bunch of comments and questions, but I think in general it's going in the right direction. I agree that this patch needs tests, especially since it's such a major change.

On the whitespace front, sounds like you should tell your IDE to mellow out! I use Sublime Text for what it's worth, and I'm very happy with it.

Thanks @houseofyin for pitching in on the review! And of course thanks @msalsbery for rolling up your sleeves and dig into this!

msalsbery · 2013-09-07T14:59:00Z

The white space changes are all unintentional. I'm not itching to make any changes there :) I turned on auto spacing for non empty parenthesis to match existing code style, but my IDE's removal of white space and auto indenting blocks causes the most problems since I don't notice it happening then have to make a thousand manual changes...frustrating. Definitely needs to chill .)

I agree with all the comments and I will fix them all. Just wanted to note that I left all names and logic intact this first pass intentionally to lessen confusion except for the mousedowninelement flag name that was inconsistent and the mousemove mousemovewindow mousemoveie stuff that was necessary to keep existing consistency when I added a mousemove handler that didn't exist before. I personally use naming like onMouseMoveCaptured and onMouseMove to differentiate between captured and non captured handlers, but I tried to stick with existing convention here. Seems based on your comments we are on the same page and I can be a bit more confident in making changes that make sense.

The shift shiftKey duplication was I believe because the native event uses one name and another was being used. I know I had a good reason at the time :) I will reevaluate that one and fix it.

Touch events are all converted to mouse events which is why you weren't seeing where we extract the touch event. It's there though, just happens right at the touch event handler and was never used past that point. Now the original touch event moves through to the MouseTracker subscriber handlers.

Documentation: I'm still not up on how that gets generated from the code comments and I don't know where to look to see the generated documentation so I left it as is with a rough shell of my changes... Help! :)

houseofyin · 2013-09-13T01:57:03Z

I think the code changes look good. There still look to be some unneeded while space changes, but I'll defer to @iangilman as to whether that is a problem from a code management perspective.

iangilman · 2013-09-13T22:21:24Z

@houseofyin, thanks for looking it over!

I'm okay with the current state of whitespace changes in this patch, but I do look forward to fewer whitespace changes in future patches. ;-)

@msalsbery, your next steps all sound good. What do you think about adding the tests and breaking change documentation to this patch and doing the rest in follow-up patches?

msalsbery · 2013-09-13T22:39:29Z

your next steps all sound good. What do you think about adding the tests and breaking change documentation to this patch and doing the rest in follow-up patches?

Yes definitely separate patch(es).

msalsbery · 2013-09-13T22:42:10Z

What's the convention on spaces in non-empty () and []? I personally have never used leading and trailing spaces there and don't find it reads any better than I'm used to. Just my opinion.

houseofyin · 2013-09-13T22:55:18Z

Should there be a code formatter integrated with grunt to enforce whatever style is chosen?

msalsbery · 2013-09-16T16:32:01Z

In changelog.txt, what version number should the change be documented under?

iangilman · 2013-09-16T17:22:42Z

The codebase currently uses "inner white space" for non-empty () and [], thus:

if ( foo ) {
    var a = [ "boo", "baz" ];
}

I personally don't care for that, and this is the only project I've worked on that has had this style, so I don't think it's all that common. I believe @thatcher is the one who started using that style and he's been stepping back from contributing lately (though he may come back!).

So, as long as we have that style I feel we should remain consistent to it. If we all agree we want to change it, we should do it for the entire project all at once.

@houseofyin we do currently have a few code style rules in our linter, but nothing terribly extreme. The problem with doing it at the linter level is that it's easy to forget about and then your patch doesn't pass the tests or whatever. A stronger approach would be to add some sort of rule in Grunt that actually modifies the source code, but that sounds like a dangerous path to start down. Another approach would be to include whatever style enforcement options that are possible in the .sublime-project file for users of Sublime Text, and likewise for whatever other IDEs people use.

iangilman · 2013-09-16T17:24:42Z

@msalsbery we'll likely release 0.9.131 before landing this patch. Document the changes under a new 1.0.0 section at the top.

iangilman · 2013-09-16T22:30:39Z

There are a number of spots in this patch where the inner white space has been removed from square brackets... perhaps you should revert that aspect of the change?

Also, I'm curious which browsers and devices you've been able to test on.

msalsbery · 2013-09-16T22:42:13Z

I test on IE 10, Chrome, Safari Windows, and Firefox...all fairly current or current versions. Will be testing on iOS Safari iPhone/iPad in next few days. That's my available test platforms at the moment.

I'll go through and fix whitespace.

iangilman · 2013-09-17T16:25:22Z

Sounds like a great set of test platforms. I've got an Android here as well, so I can try there.

MouseTracker Unit Tests

msalsbery · 2013-09-20T16:21:20Z

New commit - unit tests for MouseTracker.

Sorry for the delay...got sidetracked implementing a Durandal version upgrade with lots of breaking changes. Fun :)

changelog.txt update

msalsbery · 2013-09-20T16:38:31Z

New commit - changelog.txt update

Whitespace fixes on un-empty [] brackets

msalsbery · 2013-09-20T16:59:12Z

New commit - whitespace fixes on un-empty []

changelog.txt fixed issue reference numbers (#23) (#215)

msalsbery · 2013-09-20T17:06:05Z

New commit - changelog.txt issue number fix (#215 to #23)

msalsbery · 2013-09-20T17:24:35Z

New commit - hopefully the last changelog.txt fix

iangilman · 2013-09-23T16:52:36Z

Looks great! I think that's it, right? Please merge in the latest master (looks like there are conflicts) and then I'll merge this pull request.

I'm going to publish what we currently have before merging. Then we should do any other planned breaking changes (such as #200) before we publish this. @msalsbery can you take care of #200?

msalsbery · 2013-09-23T19:18:20Z

Yes I'll take care of all the changes mentioned in #200

msalsbery · 2013-09-24T14:34:02Z

I'm not showing any conflicts with the Master branch...

"This branch is 15 commits ahead and 0 commits behind master"

iangilman · 2013-09-24T17:26:52Z

Your master or openseadragon/openseadragon's master? Have you pulled the latest into your fork?

iangilman · 2013-09-24T17:36:57Z

Anyway, I went ahead and merged (after fixing the conflict... it was just the changelog file). Congratulations on landing an excellent patch!

msalsbery · 2013-09-24T19:37:33Z

Cool thanks! Next time there's a conflict I'll try to find where to merge it back in to mine. Thanks for doing that.

MouseTracker original events in handlers

b3fa8f1

Implemented "Expose original event in handlers" (#23) for MouseTracker Added OpenSeadragon.getElementOffset() method. Element-relative mouse coordinates should be correct even if the element and/or page is scrolled (#131)

houseofyin reviewed Sep 6, 2013
View reviewed changes

msalsbery added 2 commits September 6, 2013 10:43

MouseTracker Original Events in handlers

56f5b9f

Comment & Formatting fixes

MouseTracker Original Events in Handlers

5f6b89b

More formatting fixes

msalsbery added 3 commits September 6, 2013 11:20

MouseTracker Original Events in Handlers

8229a9a

More formatting fixes

MouseTracker Original Events in Handlers

2e1f71a

Bug fixes. Event handler methods shouldn't be called directly in theory...

MouseTracker Original Events in Handlers

d60224b

Pass original touch events to handlers.

iangilman reviewed Sep 7, 2013
View reviewed changes

MouseTracker Original Events in Handlers

4adeacd

MouseTracker Unit Tests

MouseTracker Original Events in Handlers

47a911b

changelog.txt update

MouseTracker Original Events in Handlers

c274815

Whitespace fixes on un-empty [] brackets

MouseTracker Original EVents in Handlers

93159aa

changelog.txt fixed issue reference numbers (#23) (#215)

changelog.txt update for fixed issue #131

5335048

iangilman merged commit 5335048 into openseadragon:master Sep 24, 2013

msalsbery mentioned this pull request Oct 8, 2013

Expose original event in handlers #23

Closed

msalsbery mentioned this pull request Dec 2, 2013

Mouse location is confused by elements that scroll #131

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MouseTracker original events in handlers #215

MouseTracker original events in handlers #215

msalsbery commented Sep 6, 2013

houseofyin commented Sep 6, 2013

houseofyin Sep 6, 2013

msalsbery Sep 6, 2013

msalsbery commented Sep 6, 2013

iangilman Sep 7, 2013

iangilman Sep 7, 2013

iangilman commented Sep 7, 2013

msalsbery commented Sep 7, 2013

houseofyin commented Sep 13, 2013

iangilman commented Sep 13, 2013

msalsbery commented Sep 13, 2013

msalsbery commented Sep 13, 2013

houseofyin commented Sep 13, 2013

msalsbery commented Sep 16, 2013

iangilman commented Sep 16, 2013

iangilman commented Sep 16, 2013

iangilman commented Sep 16, 2013

msalsbery commented Sep 16, 2013

iangilman commented Sep 17, 2013

msalsbery commented Sep 20, 2013

msalsbery commented Sep 20, 2013

msalsbery commented Sep 20, 2013

msalsbery commented Sep 20, 2013

msalsbery commented Sep 20, 2013

iangilman commented Sep 23, 2013

msalsbery commented Sep 23, 2013

msalsbery commented Sep 24, 2013

iangilman commented Sep 24, 2013

iangilman commented Sep 24, 2013

msalsbery commented Sep 24, 2013

MouseTracker original events in handlers #215

MouseTracker original events in handlers #215

Conversation

msalsbery commented Sep 6, 2013

houseofyin commented Sep 6, 2013

houseofyin Sep 6, 2013

Choose a reason for hiding this comment

msalsbery Sep 6, 2013

Choose a reason for hiding this comment

msalsbery commented Sep 6, 2013

iangilman Sep 7, 2013

Choose a reason for hiding this comment

iangilman Sep 7, 2013

Choose a reason for hiding this comment

iangilman commented Sep 7, 2013

msalsbery commented Sep 7, 2013

houseofyin commented Sep 13, 2013

iangilman commented Sep 13, 2013

msalsbery commented Sep 13, 2013

msalsbery commented Sep 13, 2013

houseofyin commented Sep 13, 2013

msalsbery commented Sep 16, 2013

iangilman commented Sep 16, 2013

iangilman commented Sep 16, 2013

iangilman commented Sep 16, 2013

msalsbery commented Sep 16, 2013

iangilman commented Sep 17, 2013

msalsbery commented Sep 20, 2013

msalsbery commented Sep 20, 2013

msalsbery commented Sep 20, 2013

msalsbery commented Sep 20, 2013

msalsbery commented Sep 20, 2013

iangilman commented Sep 23, 2013

msalsbery commented Sep 23, 2013

msalsbery commented Sep 24, 2013

iangilman commented Sep 24, 2013

iangilman commented Sep 24, 2013

msalsbery commented Sep 24, 2013