Skip to content

Use buttons in toolbar #1797

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

Merged
merged 4 commits into from
Apr 10, 2025
Merged

Use buttons in toolbar #1797

merged 4 commits into from
Apr 10, 2025

Conversation

bnmajor
Copy link
Collaborator

@bnmajor bnmajor commented Mar 3, 2025

This branch also needs an okay from UDRI folks since this change would affect all user workflows (although hopefully as an improvement for all).
Feedback and remaining changes:

  • Make sure the frame can still be input manually
  • Investigate omega to make sure that the toolbar is not being re-sized/shifting around (Note: There is no fix in this PR but I was unable to reproduce re-sizing/shifting)

@bnmajor bnmajor force-pushed the use-buttons-in-toolbar branch from 50846ba to d60738f Compare March 3, 2025 13:53
@bnmajor bnmajor requested a review from psavery March 3, 2025 13:53
@bnmajor
Copy link
Collaborator Author

bnmajor commented Mar 25, 2025

@psavery There is some additional weirdness that I have noticed while testing this branch. If you click on a point on the slider the value appears to jump around rather that jumping right to that position. In this example it jumps to the last frame and the the value that was clicked on the slider:
Screencast from 03-25-2025 01:23:02 PM.webm
In this example it jumps up in increments of 10 rather than jumping right to the position:
Screencast from 03-25-2025 01:29:29 PM.webm
This does seem to happen in master as well. While I am able to confirm that additional action signals are being sent (causing this behavior) I cannot understand why or if there is a way to correct the behavior.

In the image series toolbar replace the spinbox for multi-frame images with
back/forward buttons and an input text box. Functionality should remain the
same, the goal is simply to provide larger buttons that are easier to use.

Signed-off-by: Brianna Major <brianna.major@kitware.com>
@bnmajor bnmajor force-pushed the use-buttons-in-toolbar branch from 710eebf to ec6bceb Compare March 25, 2025 18:31
self.back_button.setEnabled(self.slider.minimum() != val)
self.forward_button.setEnabled(self.slider.maximum() != val)

def change_frame(self, value):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
def change_frame(self, value):
def shift_frame(self, value):

Can you rename this so it's clear that we are shifting by the value, rather than changing to the value?

Comment on lines 83 to 84
self.frame.textChanged.connect(
lambda i: self.slider.setSliderPosition(int(i)))
Copy link
Collaborator

@psavery psavery Apr 9, 2025

Choose a reason for hiding this comment

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

Suggested change
self.frame.textChanged.connect(
lambda i: self.slider.setSliderPosition(int(i)))
self.frame.editingFinished.connect(self.on_frame_edited)

I think we should only trigger an update after the user has finished editing. Otherwise, if they type in 11, for example, it triggers an update with every keystroke (so first loads frame 1 and then loads frame 11).

But editingFinished also does not supply the new text as an argument, so you'd also have to check it with self.frame.text().

Also, we need to do some validation for the text that the user enters. I can type in invalid text like f and it produces an exception. Instead, we should do something like this:

    try:
        val = int(text)
    except ValueError:
        # Not a valid integer. Restore the previous value.
        val = prev_val

    # Clip the value to the min/max range
    val = max(min_val, val)
    val = min(max_val, val)

Notice also we should clip the value to the min/max range. If the user sets it to something outside of the range, force it to go back to be within the range.

@psavery
Copy link
Collaborator

psavery commented Apr 9, 2025

We need to increase the width of the line edit based upon the max number of digits in the range. Notice that if the max value is 4 digits, when we get to the 4 digit numbers, part of the number is cut off. The below number is 1351.
image

For FIDDLE, it can be just wide enough to hold 1 digit, since that's all it will ever have.

Maybe you can do something like this: num_digits = len(str(max_frame_index)).

bnmajor added 3 commits April 10, 2025 12:18
This better reflects what this method does.

Signed-off-by: Brianna Major <brianna.major@kitware.com>
- Confirm it is an integer
- Clip to range of available frames

Signed-off-by: Brianna Major <brianna.major@kitware.com>
Signed-off-by: Brianna Major <brianna.major@kitware.com>
@psavery psavery merged commit dd5140f into HEXRD:master Apr 10, 2025
9 checks passed
@psavery psavery deleted the use-buttons-in-toolbar branch April 10, 2025 20:14
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.

2 participants