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

Jump to run, auto-scroll and run-verticalHeader #73

Merged
merged 6 commits into from
Sep 8, 2023
Merged

Conversation

matheuscteo
Copy link
Member

This PR resolves #52 , resolves #57 and resolves #55.

  • A example of the jump-to-run feature can be found bellow:

Peek 2023-07-20 14-12

  • Autoscroll when new runs are added can be disabled in a tableMenu action:

Peek 2023-07-20 14-21

  • The frozen column with the run numbers is implemented as vertical header. I had to add a cornerButton to the table view to get a "label" for such header. This means that if one click the top-left word 'Run', the whole table is selected. I'd consider this a feature.

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.
@matheuscteo matheuscteo requested a review from JamesWrigley July 20, 2023 12:30
Comment on lines 440 to 449
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)
Copy link
Member

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'? 🤔

Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member Author

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

@cydanil
Copy link
Member

cydanil commented Sep 4, 2023

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.

@matheuscteo
Copy link
Member Author

matheuscteo commented Sep 4, 2023

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.

Good point, @cydanil . I just addressed this in 469e5b5. Thanks!

As suggested by @takluyver the QWidgetAction was replaced by the
standard QAction with use of the setCheckable prop.
@takluyver
Copy link
Member

Thanks, LGTM.

Copy link
Member

@JamesWrigley JamesWrigley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@matheuscteo matheuscteo merged commit 470aab7 into master Sep 8, 2023
@matheuscteo matheuscteo deleted the jump-to-run branch September 8, 2023 12:09
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.

Jump-to-run Autoscroll when new runs are added Fix the Run column so that it's always on the left
4 participants