Skip to content

Conversation

bparks13
Copy link
Member

This PR now saves the zoom state when writing stimulus parameters, and reapplies the zoom state so that any changes can be immediately viewed without needing to zoom in again.

Example GIF
save-zoom-state

Example GIF: Timebase Changes
timebase-change

Note that this may conflict with #459, since that PR refactors this class to be more generalized. Care needs to be taken that whichever one gets merged first is brought back into the other one so that the behavior remains correctly fixed.

Fixes #349

@bparks13 bparks13 added this to the 0.6.1 milestone Sep 11, 2025
@bparks13 bparks13 requested a review from jonnew September 11, 2025 17:24
@bparks13 bparks13 self-assigned this Sep 11, 2025
@jonnew
Copy link
Member

jonnew commented Sep 16, 2025

When I scroll aggressively, sometimes, all the waveforms disappear and I cant find them. This both should not happen and there should be a Reset Zoom button that brings everything back to default:

image

@jonnew
Copy link
Member

jonnew commented Sep 16, 2025

its seems like there is some arbitrary max zoom setting that does not allow me to see pulse shapes in cerntain aspect ratios where the aspect ratio is defined as pulse width/stim sequence length:

image image

@jonnew
Copy link
Member

jonnew commented Sep 16, 2025

Scale bars X/Y starting points need to account for line width so they dont look hacked

image

Copy link
Member

@jonnew jonnew left a comment

Choose a reason for hiding this comment

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

I would start on this one and think about variable lifetimes a bit more.

@bparks13 bparks13 force-pushed the issue-349 branch 2 times, most recently from 551758b to 5810ddb Compare September 16, 2025 19:15
@bparks13
Copy link
Member Author

GIFs of behavior:

Save Zoom State [Note: no change since the last update in behavior, but implementation has been updated]

rhs2116-zoom-state

Reset Zoom

After removing the zoom-in boundary checking in 631c78f, it is possible for users to zoom in excessively and lose all reference in the axis. The Reset Zoom button can get them out of this state now.

reset-zoom

@bparks13 bparks13 requested a review from jonnew September 16, 2025 19:24
@jonnew
Copy link
Member

jonnew commented Sep 29, 2025

I just put 1 in all the boxes and applied to all channels. Says its valid and plots nothing:

image

I realized this was because the zoom was not reset, even when going from no stimulus to a defined stimulus. It seems to me that in this case you probably want to invoke reset zoom.

Another thing I noticed was that changing a value with the electrode selected does not make the change automatically, the user needs to press apply. I think maybe the apply button is not needed and that changes should be applied when they are made.

EDIT: I took care of the automatic update stuff, but the initial zoom behavior still needs to be addressed,

- Instead of requiring user to press Apply button, leaving the control
  or pressing enter causes and stimulus update.
Copy link
Member

@jonnew jonnew left a comment

Choose a reason for hiding this comment

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

See my comment

@bparks13
Copy link
Member Author

@jonnew With the new changes, if the user has not selected a contact then they could be receive the same warning multiple times if they are setting stimulus parameters without selecting a contact:

image

This warning fires whenever they leave the current UI element, so if they are tabbing through the controls and aren't changing parameters, the same warning pops up.

Additionally, this commit also disrupts the tab order, with the new UI element now occurring after the buttons instead of before.

I understand the intent behind these changes, and I think that we should probably get rid of the pop-up window so that users can freely modify parameters without selecting a contact. This way, they can get the benefit of immediate updates if they have a contact selected, or they can deselect all contacts and set the parameters they want before choosing a contact and pressing Apply.

I will address these changes, and also fix the zoom state when first drawing a stimulus.

- Reset zoom state when drawing a stimulus after none was presented; the update to save the zoom state inadvertently caused the pane to remain blank in this scenario
- Remove dialog warning that no contacts are selected after the addition of automatic updates
- Correct the tab order of UI elements
@bparks13 bparks13 requested a review from jonnew September 30, 2025 14:16
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.

Rhs2116 - save zoom state when writing new stimuli parameters
2 participants