-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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
File chooser parent hotkey #7130
base: master
Are you sure you want to change the base?
File chooser parent hotkey #7130
Conversation
Oops. Git noob mistake. Looking how to fix... |
1e06046
to
593cbd8
Compare
@@ -133,6 +133,11 @@ public void keyReleased(KeyEvent e) { | |||
if (e.getKeyCode() == KeyEvent.VK_ENTER) { | |||
e.consume(); | |||
handleEnterKey(); | |||
} else if (e.getKeyCode() == KeyEvent.VK_UP) { | |||
if (e.getModifiersEx() == KeyEvent.META_DOWN_MASK) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a utility for checking this in a system independent fashion:
DockingUtils.CONTROL_KEY_MODIFIER_MASK
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've noticed that if I start Ghidra and go straight to import that the key doesn't work, but if I click anywhere in the dialog or do almost anything with it first then it does work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that goes back to where the key listener is installed. The way it is written here, the focus has to be in the list of files for it to work. The correct solution would require a change in how and where the key listener is added. This starts to become black diamond skiing for Swing changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way it is written here, the focus has to be in the list of files for it to work. The correct solution would require a change in how and where the key listener is added.
Yes I looked around and then first put the listener somewhere else where it worked sometimes and then noticed key events were mostly getting heard in the DirectoryList...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a more subtle Swing mechanism for adding actions to containers. These actions can have key bindings mapped to them. In this scenario, the event would propagate from different focused widgets up to the main dialog container, who would then handle the event. We also have Ghidra actions that can be installed, which are a bit easier to use than Java's.
I plan on taking a look into this in order to find the best solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! I was going to add hotkeys for forward and back, which would be no harder, and then down, which would be a little harder since it depends on a directory being currently selected.
I don't know much Java and even less Swing.
I'm not sure what the rules are across platforms for hotkeys/shortcuts. This uses meta+up to catch command+up which is the idiomatic keypress to go to parent on Mac.
I'm not sure if the key might need to be caught in multiple places in case different pieces of the dialog are focussed. Also not sure whether if that's the case there is a place for it to go where the hierarchy of widgets of the dialog would make it just work if it doesn't already.
Because of those concerns this is just a draft pull request for your feedback.