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.

Sign up for GitHub

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

Jump to bottom

Fix: disable mnemonic accelerator keys in menu's path labels #392

Closed
wants to merge 3 commits into from
Closed

Fix: disable mnemonic accelerator keys in menu's path labels #392

wants to merge 3 commits into from
+47 −34

Conversation

mehw
Copy link

@mehw mehw commented Apr 6, 2019 •
edited
Loading

Solvespace version: 3.0~46528bfc
OS: Gentoo GNU/Linux

Already signed Solvespace's Contributor License Agreement.

Bug

File > Open Recent and New Group > Link Recent replace/remove strings in file paths and activate mnemonic accelerator keys.

This means that on GNU/Linux the path /tmp/_foo_&_bar_.slvs is showed as /tmp/foo_bar.slvs.

settings.json

{
  "RecentFile_0":"\/tmp\/_foo_&_bar_.slvs"
}

before

Proposed solution

Add mnemonics argument to Platform::Menu::AddItem to treat as raw text label's mnemonic accelerator keys when it is false (src/platform/gui.h).

New pure virtual function Platform::Menu::AddItemRaw which treats as raw text label's mnemonic accelerator keys (src/platform/gui.h).

AddItemRaw doesn't replace/remove label's text (i.e. guigtk.cpp '&' -> '_'), nor it activates mnemonic accelerator keys.

after

Caveats

Solution tested only on Gentoo GNU/Linux.

The code in src/platform/guiwin.cpp and src/platform/guimac.mm isn't tested and may require amendments.

Does NegateMnemonics escape mnemonic accelerator keys correctly (src/platform/guiwin.cpp)?

Not sure if AddItemRaw in guiwin.cpp shall allocate a buffer before assigning it to mii.dwTypeData. Also, does SetMenuItemInfoW operate as expected updating the menu item's label?

Can someone review the code, please?

@whitequark
Copy link
Contributor

whitequark commented Apr 6, 2019 •
edited
Loading

I would prefer if you added an argument to AddItem, bool mnemonics = true, instead. This would lead to less code duplication. E.g. on macOS there are no mnemonics in the first place.

@mehw
Fix: disable mnemonic accelerator keys in menu's path labels

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
GPG key ID: 4AEE18F83AFDEB23
Expired
Verified
Learn about vigilant mode
69c6cf4 
New pure virtual function Platform::Menu::AddItemRaw which treats as
raw text label's mnemonic accelerator keys (src/platform/gui.h).

"File > Open Recent" and "New Group > Link Recent" now list paths in
their integrity: AddItemRaw doesn't replace/remove label's text (i.e.
guigtk.cpp '&' -> '_'), nor it activates mnemonic accelerator keys.
@mehw
Copy link
Author

mehw commented Apr 6, 2019

Ok, I'll provide an alternative way as new commit. Then, if you approve it, let's squash the commits.

@mehw
Fix: toggle off mnemonic accelerator keys for menu's path labels

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
GPG key ID: 4AEE18F83AFDEB23
Expired
Verified
Learn about vigilant mode
48871a4 
Add mnemonics argument to Platform::Menu::AddItem (src/platform/gui.h)
to treat as raw text label's mnemonic accelerator keys when false.

"File > Open Recent" and "New Group > Link Recent" now list paths in
their integrity.  With AddItem's mnemonics argument set to false, no
label's text is replaced/removed (i.e. guigtk.cpp '&' -> '_'), nor
mnemonic accelerator keys are activated.
@mehw mehw force-pushed the fix_mnemonic_recent_file_list branch from f2f638d to 48871a4 Compare April 6, 2019 05:38
whitequark
whitequark suggested changes Apr 12, 2019
View reviewed changes
@@ -171,7 +171,8 @@ class Menu {
virtual ~Menu() {}

virtual std::shared_ptr<MenuItem> AddItem(
const std::string &label, std::function<void()> onTrigger = std::function<void()>()) = 0;
const std::string &label, bool mnemonics = true,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think unless you put the mnemonics argument after onTrigger the default value for it is quite pointless.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks!

@mehw
Fix: implement whitequark's suggestions as per solvespace#392
89c3c92 
Add /*mnemonics=*/ in call sites.

Remove mnemonics default value in src/platform/gui.h.
@mehw
Copy link
Author

mehw commented Apr 13, 2019

Pull amended as per @whitequark request.

First post edited to reflect the current behavior.

@whitequark
Copy link
Contributor

You did the opposite of what I asked.

@whitequark
Copy link
Contributor

Thanks for the bug report and the patch. Fixed in ce0b130.

@whitequark whitequark closed this Apr 13, 2019
@mehw
Copy link
Author

mehw commented Apr 13, 2019

You did the opposite of what I asked.

I see... I misinterpreted your suggestion...

I think unless you put the mnemonics argument after onTrigger the default value for it is quite pointless.

I'm sorry, what I got was "do not use a default value for mnemonics"... So, I saw no reason to put mnemonics after onTrigger...

Thanks for the bug report and the patch. Fixed in ce0b130.

You're welcome. Thanks for fixing the master branch!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@whitequark whitequark

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@mehw @whitequark