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

feat(gui): allow user to set custom shortcuts #1980

Merged
merged 33 commits into from
Aug 3, 2023

Conversation

Mino260806
Copy link
Contributor

@Mino260806 Mino260806 commented Aug 2, 2023

Supports keyboard keys and mouse buttons

I kept the default shortcuts for each action

Toolbar / Menu shortcuts

image

Code area

image

I created some new classes, please let me know if I should move any to another package

Also, I would like to know if there are other shortcuts that need to be customized. Currently, I added Menu / Toolbar actions and CodeArea.

A small question

forwardAction.putValue(Action.ACCELERATOR_KEY, getKeyStroke(KeyEvent.VK_RIGHT, KeyEvent.ALT_DOWN_MASK, SystemInfo.IS_MAC));

What is the significance of SystemInfo.IS_MAC argument here ? There's no way to keep it to make it customizable like other shortcuts

Related issues

#1721
#1718
#1609
#1479
#1085

@skylot
Copy link
Owner

skylot commented Aug 2, 2023

Wow, this is big PR, it will take some time for me to check it.

What is the significance of SystemInfo.IS_MAC argument here ?

Looks like there is some difference in registering modifiers in macOs, so this keystoke was set to run action on key release, check PR #1301 for details. To keep this behavior, we should set onKeyRelease to true for all actions with modifiers on macOs.
Also, macOs instead Ctrl often uses Command buttons in shortcuts (like Ctrl+F), check issue #165 and fix in #616. So we should set correct modifier (as initial value) for these actions using jadx.gui.utils.UiUtils.ctrlButton() method.

Also, I would like to know if there are other shortcuts that need to be customized

After quick search for getKeyStroke( in project, I think, it worth adding 'script run (F8)' in 'ScriptContentPanel' and auto complete key (ctrl+space) in 'ScriptCodeArea'

And some issues I notice:

  • for some reason, any F<number> keys cannot be set, because field continue to wait input.
  • I think we shouldn't accept left mouse button at all (just ignore it). I agreed to the prompt (it doesn't show the key that will be set) and was not able to do anything after that 🤣
  • changes in JNodeAction class, lead to ignoring node under mouse and always use node under cursor, but these can be different locations
  • it is nice to show confirmation about using duplicate shortcut, and show name of other action with same key in confirm (so it is possible to go and change it to something different)

@Mino260806
Copy link
Contributor Author

To keep this behavior, we should set onKeyRelease to true for all actions with modifiers on macOs

Okay, I was confused because it was only used in the line I linked

Also, macOs instead Ctrl often uses Command buttons in shortcuts (like Ctrl+F), check issue #165 and fix in #616. So we should set correct modifier (as initial value) for these actions using jadx.gui.utils.UiUtils.ctrlButton() method.

Yeah I'm already using it (see ActionModel enum)

for some reason, any F keys cannot be set, because field continue to wait input

It's normal. You probably have Fn lock enabled, trying hold Fn while pressing the F<number>

changes in JNodeAction class, lead to ignoring node under mouse and always use node under cursor, but these can be different locations

I don't understand this one. How can I reproduce this issue ?

@Mino260806
Copy link
Contributor Author

To keep this behavior, we should set onKeyRelease to true for all actions with modifiers on macOs

I'm unsure if this is what you meant db182ee

@skylot
Copy link
Owner

skylot commented Aug 2, 2023

Hm, I know why you use KEY_TYPED event: to catch several keys pressed at the same time, like ctrl+R. Now this is not working 😢
So, we should keep focus until non modifier key is pressed.

@Mino260806
Copy link
Contributor Author

Hm, I know why you use KEY_TYPED event: to catch several keys pressed at the same time, like ctrl+R. Now this is not working So, we should keep focus until non modifier key is pressed.

Or I'll just use KEY_RELEASED instead of KEY_TYPED to remove focus

@skylot
Copy link
Owner

skylot commented Aug 2, 2023

Ok, everything looks good now 👍

@skylot skylot changed the title feat(gui): enable user to set custom shortcuts feat(gui): allow user to set custom shortcuts Aug 2, 2023
@Mino260806
Copy link
Contributor Author

There is still some stuff I need to fix, please don't merge until I confirm

@skylot skylot marked this pull request as draft August 2, 2023 20:27
@skylot
Copy link
Owner

skylot commented Aug 2, 2023

There is still some stuff I need to fix, please don't merge until I confirm

Next time, mark PR as draft if it is not ready for merge 🙂

Also, I just notice that we can't have both keys and mouse shortcuts for navigation (#1718 case), only one not both 😭

@Mino260806
Copy link
Contributor Author

I struggled to make the dummy actions get called without being added to any component.
So I had to create this small class as a workaround

@Mino260806
Copy link
Contributor Author

Can you test the shortcuts in script panel ? I don't know how to access it

Other than that I think the PR is ready to be merged now

@Mino260806 Mino260806 marked this pull request as ready for review August 2, 2023 22:54
@skylot
Copy link
Owner

skylot commented Aug 2, 2023

Well, it works fine after resetting settings 🤣
But without reset, new actions are missing and cause NPE

java.lang.IllegalArgumentException: trigger key cannot be null
	at org.fife.ui.autocomplete.AutoCompletion.setTriggerKey(AutoCompletion.java:1174)
	at jadx.gui.plugins.script.ScriptCodeArea.addAutoComplete(ScriptCodeArea.java:37)
	at jadx.gui.plugins.script.ScriptCodeArea.<init>(ScriptCodeArea.java:29)
	at jadx.gui.plugins.script.ScriptContentPanel.<init>(ScriptContentPanel.java:59)
	at jadx.gui.treemodel.JInputScript.getContentPanel(JInputScript.java:41)
	at jadx.gui.ui.TabbedPane.getContentPanel(TabbedPane.java:404)
	at jadx.gui.ui.TabbedPane.showNode(TabbedPane.java:272)
	at jadx.gui.ui.MainWindow.nodeClickAction(MainWindow.java:804)
	at jadx.gui.ui.MainWindow$6.mousePressed(MainWindow.java:1168)

So, during load, default shortcuts values should be used if action missing in loaded settings.

To check scripts actions in left tree go to Inputs->Scripts right click and in popup select New script and save it, next open it to see a script's panel above editor.

@Mino260806
Copy link
Contributor Author

But without reset, new actions are missing and cause NPE

Issue noted, I'll fix it tomorrow

@Mino260806
Copy link
Contributor Author

Error: eckstyle] [ERROR] /home/runner/work/jadx/jadx/jadx-gui/src/main/java/jadx/gui/settings/data/ShortcutsMap.java:8:35: Usage of type 'HashMap' is not allowed. [IllegalType]

Why extending HashMap is not allowed ?

@Mino260806
Copy link
Contributor Author

Ready for merge

@skylot
Copy link
Owner

skylot commented Aug 3, 2023

Ok, I think it is fine to merge. Hope we don't get many regressions 😃

@Mino260806
Copy link
Contributor Author

Ok, I think it is fine to merge. Hope we don't get many regressions

It's fine. You can ping me in an issue related to this PR and I'll fix it

@skylot skylot merged commit 68b84ea into skylot:master Aug 3, 2023
5 checks passed
@skylot
Copy link
Owner

skylot commented Aug 3, 2023

@Mino260806 sure 🤣
And thank you for your help and hard work 👍

@Liyw979
Copy link

Liyw979 commented Nov 27, 2023

Where can I find the custom shortcuts panel?

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

3 participants