-
Notifications
You must be signed in to change notification settings - Fork 2
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
Jump to run, auto-scroll and run-verticalHeader #73
Conversation
At the top right corner of the GUI a magnifing glass icon is added. Upon clinking on it, a QLineEdit widget is poped. Upon entering a valid run number and pressing the return key the tableview scrolls to the corresponding row. When new rows are added, automatic scrolling is also supported.
damnit/gui/main_window.py
Outdated
self.autoscroll_checkbox = QtWidgets.QCheckBox('Autoscroll', self) | ||
self.autoscroll_checkbox.setStyleSheet("QCheckBox" | ||
"{" | ||
"padding-left : 10px;" | ||
"padding-bottom: 7px" | ||
"}") | ||
self.autoscroll_checkbox.setChecked(True) | ||
tableMenu = menu_bar.addMenu("Table") | ||
action_autoscroll = QtWidgets.QWidgetAction(menu_bar) | ||
action_autoscroll.setDefaultWidget(self.autoscroll_checkbox) |
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 don't think you need to fiddle around with a checkbox and a stylesheet - Qt already has a concept of a 'checkable' action. Make a regular QAction, then call action_autoscroll.setCheckable(True)
and Qt will show it with a toggle-able checkmark.
https://doc.qt.io/qt-5/qaction.html#checkable-prop
Also, I wonder if 'autoscroll' is clear to users? Maybe something like 'Scroll to newly added runs'? 🤔
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 agree with the label of the widged and changed to your suggestion. Yet, the checkable prop would not provide a very nice user experience, for upon clicking the action the context menu closes with this approach. I'd rather keep the QCheckBox for this reason.
An example of this issue can be found here.
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'd prefer the menu to close when I check the box, because that's how menus usually work. I've just checked in Firefox, Pycharm, Libreoffice and Inkscape - they all have checkboxes in menus, and in all of them, clicking the checkbox closes the menu.
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.
Thanks for the checking other UIs, if that's the case I agree. Changes were addressed in c149db4
Can you highlight/select the run that has been jumped to? It would make it clearer that the action happened, particularly in the second example (jumping to run 100) where it's a the bottom of the list. |
As suggested by @takluyver the QWidgetAction was replaced by the standard QAction with use of the setCheckable prop.
Thanks, LGTM. |
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.
LGTM!
This PR resolves #52 , resolves #57 and resolves #55.