fix: don't append Shift modifier text twice to accelerators #15400
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced Oct 25, 2018
nornagon
requested changes
Oct 26, 2018
| @@ -19,17 +19,17 @@ index 7e55ef366ac8320f730cdcb268453b1fa2710887..c3fb98b426cd7c12f66eaaf358f4ff18 | |||
| @@ -21,9 +22,7 @@ | |||
| #include <windows.h> | |||
| #endif | |||
|
|
|||
There was a problem hiding this comment.
those spaces are important, leave them in please! this patch should be exactly as exported by git-export-patches.
There was a problem hiding this comment.
I knew there was something up with those spaces, gonna fix asap
brenca
force-pushed
the
brenca/fix-shift-accelerators
branch
from
65559f0
to
55dabaf
Compare
ckerr
approved these changes
Oct 30, 2018
|
@nornagon thoughts on the revised PR? |
nornagon
approved these changes
Oct 30, 2018
brenca
force-pushed
the
brenca/fix-shift-accelerators
branch
from
55dabaf
to
569e264
Compare
|
@nornagon ask, and thou shall receive 🙇 |
deepak1556
reviewed
Oct 30, 2018
atom/browser/api/atom_api_menu.cc
Outdated
| @@ -155,6 +155,13 @@ base::string16 Menu::GetSublabelAt(int index) const { | |||
| return model_->GetSublabelAt(index); | |||
| } | |||
|
|
|||
| base::string16 Menu::GetAcceleratorTextAt(int index) const { | |||
| auto* accelerator = new ui::Accelerator(); | |||
There was a problem hiding this comment.
Who owns this accelerator ? I don't see any explicit ownership claim in the code path, maybe I am missing something here.
There was a problem hiding this comment.
oooh, good catch! this should probably be on the stack.
Suggested change
| auto* accelerator = new ui::Accelerator(); | |
| ui::Accelerator accelerator; | |
| model_->GetAcceleratorAtWithParams(index, true, &accelerator); |
There was a problem hiding this comment.
Done! Thanks for catching this
brenca
force-pushed
the
brenca/fix-shift-accelerators
branch
from
976b4d1
to
880b9bb
Compare
brenca
force-pushed
the
brenca/fix-shift-accelerators
branch
from
3a6c073
to
2b0e4be
Compare
deepak1556
approved these changes
Oct 31, 2018
|
Release Notes Persisted
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of Change
Fixes #15392. For some reason our patch applied a
Shiftmodifier to a whole bunch of accelerator characters (likeBackspace, butTabandEnterwere also affected), my best guess is that those used to be broken withShiftbut now are working properly inchromium.I'll backport this manually, trop couldn't deal with
4-0-x.Checklist
npm testpassesRelease Notes
Notes: fixed some accelerators having
Shiftappended to them twice