Skip to content
This repository has been archived by the owner on Sep 2, 2021. It is now read-only.

Linux native menus lost shortcuts #637

Merged
merged 4 commits into from Apr 17, 2018
Merged

Conversation

pelatx
Copy link
Contributor

@pelatx pelatx commented Apr 11, 2018

This PR makes the shortcuts to be shown again in the Linux native menus. Which were lost in my previous PR relative to the switchable menu entries.

A few shortcuts are still not shown. And honestly, at the moment I could not find the reason. Maybe someone can point out what I've missed.

The latter is more the reason for opening this PR than a merge in this state.

Thank you.

@saurabh95
Copy link
Collaborator

saurabh95 commented Apr 11, 2018

Hey, @pelatx Could you please point to menu items for which shortcuts are not visible. It would be helpful for us in process of finding the reason for it.
Also, I would recommend that you install emmet extension and check for menu items under emmet menu. I used that extension only for my testing since it has a lot of menu items (checked + non-checked) when I worked on adding checked menu items.

@pelatx
Copy link
Contributor Author

pelatx commented Apr 12, 2018

Sorry for the delay.

I've been doing a little more research and I've seen where the problem is coming from.

The menu entries that configure the shortcut when added to the menu ( in AddMenuItem()) are those that work as expected.
The problem occurs when this is not done in this way and the shortcut is then configured by calling SetMenuItemShortcut().

I have tried to solve it by setting the key in the model (model.setKey(tag, key)) within SetMenuItemShortcut(). And then recover it in SetMenuItemState() to pass it to ParseShortcut() within.

It does not work for now. I will keep trying.

PS: I do not know if I explained it well. 😅

@saurabh95
Copy link
Collaborator

Thanks @pelatx, this will surely help us in finding the probable solution.

Your solution seems correct to me, not sure why it is not working. Did you check if all the keys are stored when using SetMenuItemShortcut , I mean could you please check by logging all the keys present in the model when it reaches SetMenuItemShortcut and verify if all the desired keys are present or not? This will give us an idea if we are updating the correct map.

PS: This is just a hunch, I have not tried this myself.

@pelatx
Copy link
Contributor Author

pelatx commented Apr 13, 2018

Right hunch, @saurabh95.

I do not know how I could look so many times at the SetMenuItemShortcut function without seeing the '&' missing in NativeMenuModel& model = ...

@saurabh95
Copy link
Collaborator

I also didn't notice that, glad you found it
The changeset looks good to me

@saurabh95 saurabh95 self-requested a review April 14, 2018 06:20
@saurabh95
Copy link
Collaborator

@nethip Could you please review the changes?
It is LGTM from my side.

@saurabh95 saurabh95 requested a review from nethip April 16, 2018 06:30
@nethip
Copy link
Contributor

nethip commented Apr 17, 2018

Thanks for the fix @pelatx! LGTM. Is it OK If I squash the commits and merge?

@pelatx
Copy link
Contributor Author

pelatx commented Apr 17, 2018

It is OK @nethip.

@nethip
Copy link
Contributor

nethip commented Apr 17, 2018

Thanks @pelatx and thanks @saurabh95 for reviewing this as well.

@nethip nethip merged commit d22ab4c into adobe:master Apr 17, 2018
@pelatx pelatx deleted the linux-lost-shortcuts branch April 17, 2018 14:34
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants