Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SelectionManager] Use event.detail instead for counting the mousedown event #780

Merged
merged 2 commits into from Jul 13, 2017
Merged

[SelectionManager] Use event.detail instead for counting the mousedown event #780

merged 2 commits into from Jul 13, 2017

Conversation

mofux
Copy link
Contributor

@mofux mofux commented Jul 11, 2017

This PR introduces the usage of MouseEvent.detail to get the number of consecutive clicks.

See:
https://www.w3schools.com/jsref/event_detail.asp

This does not only reduce the complexity of the code base, but also respects the OS double click speed setting of the user.

@coveralls
Copy link

coveralls commented Jul 11, 2017

Coverage Status

Coverage increased (+0.1%) to 69.551% when pulling 9bd5282 on mofux:mousedown_counter into 86c5aa0 on sourcelair:master.

@Tyriar
Copy link
Member

Tyriar commented Jul 12, 2017

This is pretty cool, I'm a little concerned about browser support though as I don't trust w3schools and mdn says ? for most browsers https://developer.mozilla.org/en-US/docs/Web/API/UIEvent/detail

@rebornix you wouldn't happen to know why monaco doesn't use UIEvent.detail do you?

@Tyriar Tyriar self-assigned this Jul 12, 2017
@Tyriar Tyriar self-requested a review July 12, 2017 18:22
this._onDoubleClick(event);
} else if (this._clickCount === 3) {
} else if (event.detail === 3) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be >= to maintain current behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you are right, more than three clicks should yield the same behaviour as three clicks.

@mofux
Copy link
Contributor Author

mofux commented Jul 12, 2017

@Tyriar As far as I can tell, UIEvent.detail is well supported. I have tested it on Safari, Chrome, Firefox and IE10 a while back, all working. It is not very well documented, and I believe most people don't even know it exists. Actually, I have implemented a very similar solution to yours a few months ago, and then I've seen that cloud9 can handle the same thing, but with respecting the double click setting of the os, so I was wondering how they do it and found out that they were using evt.detail 😉

@@ -385,7 +385,7 @@ export class SelectionManager extends EventEmitter {
this._onSingleClick(event);
} else if (event.detail === 2) {
this._onDoubleClick(event);
} else if (event.detail === 3) {
} else if (event.detail >= 3) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually looking closer at the code it was correct before 😅, we shouldn't need to re-perform triple click as we don't drop the selection between clicks 3 and 4.

@Tyriar Tyriar added this to the 2.9.0 milestone Jul 12, 2017
@Tyriar
Copy link
Member

Tyriar commented Jul 12, 2017

Just revert that change and ready to go, pending the tests. I love deleting code 🎆

@coveralls
Copy link

Coverage Status

Coverage increased (+0.08%) to 69.523% when pulling 853e456 on mofux:mousedown_counter into 86c5aa0 on sourcelair:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.08%) to 69.523% when pulling 853e456 on mofux:mousedown_counter into 86c5aa0 on sourcelair:master.

@coveralls
Copy link

coveralls commented Jul 12, 2017

Coverage Status

Coverage increased (+0.1%) to 69.523% when pulling a2f517f on mofux:mousedown_counter into 01453e9 on sourcelair:master.

@mofux
Copy link
Contributor Author

mofux commented Jul 12, 2017

Ready for take-off 🚀

@Tyriar
Copy link
Member

Tyriar commented Jul 12, 2017

@parisk is it possible to make the mac lint/test Travis jobs non-block to merge? There is quite a queue for Mac builds apparently.

@coveralls
Copy link

coveralls commented Jul 12, 2017

Coverage Status

Coverage increased (+0.1%) to 69.523% when pulling d3fe3a1 on mofux:mousedown_counter into 01453e9 on sourcelair:master.

@Tyriar Tyriar merged commit 25fc01c into xtermjs:master Jul 13, 2017
@parisk
Copy link
Contributor

parisk commented Jul 13, 2017

@Tyriar, yes of course.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants