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

Make executing synthesized pinch zoom more similar to zoom #8223

Merged
merged 1 commit into from Nov 3, 2015

Conversation

@mrobinson
Copy link
Member

mrobinson commented Oct 27, 2015

Synthesized pinch zoom was removed in #8121 so that this combination of
mouse wheel and key press can be handled by the page. I mistakenly
re-implemented it #8215. In order to preserve synthesized pinch zoom,
which is very useful for testing on desktop computers, have it work
similarly to page zoom except with the ALT key pressed.

Review on Reviewable

@mrobinson
Copy link
Member Author

mrobinson commented Oct 27, 2015

@jdm
Copy link
Member

jdm commented Oct 27, 2015

This seems like a good solution to me, but I'm having a hard time understanding how the masking works, since it doesn't seem to actually check for the presence of Alt.

@mrobinson mrobinson force-pushed the mrobinson:pinch-zoom-2 branch from 798bde3 to 4955037 Oct 27, 2015
@mrobinson
Copy link
Member Author

mrobinson commented Oct 27, 2015

@jdm The masking was trickier than I expected, but I think the latest version properly checks for Alt.

@jdm
Copy link
Member

jdm commented Oct 27, 2015

@bors-servo: r+
Thanks!

@bors-servo
Copy link
Contributor

bors-servo commented Oct 27, 2015

📌 Commit 4955037 has been approved by jdm

@bors-servo
Copy link
Contributor

bors-servo commented Oct 27, 2015

Testing commit 4955037 with merge a65680a...

bors-servo added a commit that referenced this pull request Oct 27, 2015
Make executing synthesized pinch zoom more similar to zoom

Synthesized pinch zoom was removed in #8121 so that this combination of
mouse wheel and key press can be handled by the page. I mistakenly
re-implemented it #8215. In order to preserve synthesized pinch zoom,
which is very useful for testing on desktop computers, have it work
similarly to page zoom except with the ALT key pressed.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8223)
<!-- Reviewable:end -->
}
(CMD_OR_CONTROL, Key::Minus) => {
self.event_queue.borrow_mut().push(WindowEvent::Zoom(1.0 / 1.1));
}
(_control_and_alt, Key::Minus) => {

This comment has been minimized.

Copy link
@mbrubeck

mbrubeck Oct 27, 2015

Contributor

This is incorrect; it's equivalent to a wildcard. It binds to a new variable named _control_and_alt rather than comparing to the existing one.

@mbrubeck
Copy link
Contributor

mbrubeck commented Oct 27, 2015

@bors-servo
Copy link
Contributor

bors-servo commented Oct 27, 2015

Synthesized pinch zoom was removed in #8121 so that this combination of
mouse wheel and key press can be handled by the page. I mistakenly
re-implemented it #8215. In order to preserve synthesized pinch zoom,
which is very useful for testing on desktop computers, have it work
similarly to page zoom except with the ALT key pressed.
@mrobinson mrobinson force-pushed the mrobinson:pinch-zoom-2 branch from 4955037 to f71f7ba Oct 27, 2015
@mrobinson
Copy link
Member Author

mrobinson commented Oct 27, 2015

I think the latest version fixes the issue. Thanks again, @mbrubeck, for catching that.

@pcwalton
Copy link
Contributor

pcwalton commented Nov 2, 2015

@bors-servo
Copy link
Contributor

bors-servo commented Nov 2, 2015

📌 Commit f71f7ba has been approved by pcwalton

@bors-servo
Copy link
Contributor

bors-servo commented Nov 3, 2015

Testing commit f71f7ba with merge 8dd11cc...

bors-servo added a commit that referenced this pull request Nov 3, 2015
Make executing synthesized pinch zoom more similar to zoom

Synthesized pinch zoom was removed in #8121 so that this combination of
mouse wheel and key press can be handled by the page. I mistakenly
re-implemented it #8215. In order to preserve synthesized pinch zoom,
which is very useful for testing on desktop computers, have it work
similarly to page zoom except with the ALT key pressed.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8223)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Nov 3, 2015

The build was interrupted to prioritize another pull request.

@bors-servo
Copy link
Contributor

bors-servo commented Nov 3, 2015

Testing commit f71f7ba with merge 549f166...

@bors-servo
Copy link
Contributor

bors-servo commented Nov 3, 2015

💔 Test failed - linux-rel

@jdm
Copy link
Member

jdm commented Nov 3, 2015

@bors-servo
Copy link
Contributor

bors-servo commented Nov 3, 2015

Testing commit f71f7ba with merge 526a3c8...

bors-servo added a commit that referenced this pull request Nov 3, 2015
Make executing synthesized pinch zoom more similar to zoom

Synthesized pinch zoom was removed in #8121 so that this combination of
mouse wheel and key press can be handled by the page. I mistakenly
re-implemented it #8215. In order to preserve synthesized pinch zoom,
which is very useful for testing on desktop computers, have it work
similarly to page zoom except with the ALT key pressed.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8223)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Nov 3, 2015

@bors-servo bors-servo merged commit f71f7ba into servo:master Nov 3, 2015
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@mrobinson mrobinson deleted the mrobinson:pinch-zoom-2 branch Nov 3, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.