-
Notifications
You must be signed in to change notification settings - Fork 27
Enhance Help Menu for Better Functionality Discoverability #314
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
base: main
Are you sure you want to change the base?
Conversation
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.
Could you post some screenshots of the new UI? That would facilitate review
All PRs should also ideally add some sort of test that hits the new lines. For your PR I guess that would be faking a click in the GUI on the menu and ensuring a new modal dialog window is visible
mne_qt_browser/_pg_figure.py
Outdated
+ scroll_area.verticalScrollBar().width() | ||
) | ||
self.update() | ||
# Replaced this class with a function declared in MNEQtBrowser |
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.
Should be able to delete all this, no need to leave commented. It will live on in our git history
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 have added screenshots of both the Keyboard Shortcuts and Mouse Controls dropdown menus.
Currently, the menu opens a dropdown when hovered, but it does not show a separate pop-up (modal) when clicked. Let me know if this behavior meets your requirements or if you need a modal dialog instead.
If no further changes are needed, I will remove the commented-out code as suggested.
Otherwise, I will make all necessary changes and push them together.
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.
Hey, I just removed the comments, you can check it out
@mohitkumhar I think what @hoechenberger had in mind was a top-level menu like that in #126. It looks like you added a menu that is accessed via a toolbar button instead |
Hey @larsoner , |
@mohitkumhar Thanks for your work on this! What I actually had in mind was adding a full menu bar, though. Something like what you'd see in other applications like e.g. MS Word: ![]() Since menu items can be linked to keyboard shortcuts (which will be displayed in the menu itself), these would be kind of self-documenting. |
Sure @hoechenberger I understand it, let me create menu and share sample |
You can take #126 as a starting point |
Hey @hoechenberger , Kindly check it out! demo.mp4 |
Hey , kindly check the commit I made |
("Mark/unmark bad channel ", "Left-click channel"), | ||
("Mark/unmark bad segment ", "Left-click data"), | ||
("Toggle Annotation Tool / Visibility ", "A"), | ||
("Toggle Butterfly ", "B"), |
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.
Looks like a good start. A few ideas:
- From a UI perspective, a clickable menu item should initiate an action. Some of these can't, such as "Create Annotation". These should at least be "disabled" (i.e., still visible but not clickable). Not 100% sure we want these in a menu but... maybe okay to leave them.
- Some of them when clicked should initiate the action, such as the "Toggle DC Correction" -- clicking it should actually toggle DC correction.
Also this code isn't quite DRY... every first option has a bunch of extra spaces at the end. (1) I don't think these should be needed, and (2) if they absolutely are, they could be added to add_menu_action
rather than being in every one of these entries.
def add_menu_action(menu, description, shortcut): | ||
widget = QWidget() | ||
layout = QHBoxLayout(widget) | ||
layout.setContentsMargins(10, 2, 10, 2) |
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.
... this could probably be tweaked rather than using spaces below to fix the spacing.
|
||
for menu, items in shortcut_data.items(): | ||
for description, shortcut in items: | ||
add_menu_action(menu, description, shortcut) |
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.
You should use the same shortcuts as defined for the browser window, e.g.,
help_menu.addAction(
'Show keyboard shortcuts',
browser._toggle_help_fig,
shortcut=browser.mne.keyboard_shortcuts['?']['qt_key']
)
See #126
Reference issue
Example: Fixes #84
What does this fix?
This PR improves the Help menu by replacing the old HelpDialog with a simpler and more accessible menu. Now, users can find keyboard and mouse shortcuts directly in a dropdown menu instead of a separate dialog.
Key changes
Additional information
Please review the changes and let me know if there are any improvements or issues that need to be addressed.
Here are the screenshots of the UI