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

Expose axes limits in viewer options #304

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

kecnry
Copy link
Contributor

@kecnry kecnry commented Apr 4, 2022

Description

This pull request adds axes limits to the viewer options for profile and image viewers. In theory this could be extended to other viewers, although handling aspect ratio within image viewers will require additional logic (currently the aspect ratio is enforced at every keypress, which overwrites the value typed by the user, making the axes limit editing effectively useless while equal aspect ratio is enabled).

@codecov
Copy link

codecov bot commented Apr 4, 2022

Codecov Report

Patch coverage: 75.60% and project coverage change: -0.09% ⚠️

Comparison is base (60f58c3) 86.84% compared to head (ffdf175) 86.75%.
Report is 4 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #304      +/-   ##
==========================================
- Coverage   86.84%   86.75%   -0.09%     
==========================================
  Files          89       90       +1     
  Lines        5077     5120      +43     
==========================================
+ Hits         4409     4442      +33     
- Misses        668      678      +10     
Files Changed Coverage Δ
glue_jupyter/widgets/axes_limits.py 67.74% <67.74%> (ø)
glue_jupyter/common/state_widgets/viewer_image.py 97.87% <100.00%> (+0.19%) ⬆️
...lue_jupyter/common/state_widgets/viewer_profile.py 100.00% <100.00%> (ø)
glue_jupyter/widgets/__init__.py 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@mariobuikhuizen mariobuikhuizen left a comment

Choose a reason for hiding this comment

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

Looks good!

I see a few issues in a vue template:

glue_jupyter/widgets/axes_limits.vue Outdated Show resolved Hide resolved
glue_jupyter/widgets/axes_limits.vue Outdated Show resolved Hide resolved
@kecnry kecnry force-pushed the profile-viewer-options-limits branch from 6f1f778 to bc67c56 Compare April 4, 2022 16:15
@mariobuikhuizen
Copy link
Collaborator

The solution for the problem with margins inside the jdaviz sidebar is to use <v-row no-gutters>

@mariobuikhuizen
Copy link
Collaborator

And add some margin between the glue-float-fields with class="mr-1" and class="ml-1"

@kecnry kecnry force-pushed the profile-viewer-options-limits branch from bc67c56 to cae81df Compare April 4, 2022 17:59
Copy link
Member

@astrofrog astrofrog left a comment

Choose a reason for hiding this comment

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

This is a great start, thanks! It would indeed be nice for the limits to be updated only once the user has finished entering values, otherwise typing e.g. 1000 means the limits will change four times (1, then 10, then 100, then 1000). Maybe @mariobuikhuizen and @maartenbreddels have ideas how to deal with this? I think this would solve the aspect ratio issue you mention for the image viewer. Other than that, I'm happy with this, and we can add it to other viewers in a separate PR.

@astrofrog astrofrog changed the title expose axes limits in profile viewer options Expose axes limits in profile viewer options Apr 7, 2022
@astrofrog
Copy link
Member

@kecnry I've pushed some commits here to add the 'Apply limits' button. Do you think this seems ok?

Comment on lines +30 to +38
with delay_callback(self.glue_state, 'x_min', 'x_max', 'y_min', 'y_max'):
if self.x_min is not None:
self.glue_state.x_min = self.x_min
if self.x_max is not None:
self.glue_state.x_max = self.x_max
if self.y_min is not None:
self.glue_state.y_min = self.y_min
if self.y_max is not None:
self.glue_state.y_max = self.y_max
Copy link
Contributor Author

Choose a reason for hiding this comment

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

when used in an image viewer (now enabled), how is a change in all limits, with equal aspect enabled, handled with this delayed callback? From testing, it seems that the user-set x-limits are preserved and the y-limits are somehow adjusted to enforce the equal aspect?

I'm honestly not sure what the expected behavior should be in this case... maybe the smallest bounding box in x and y that contains the user-provided values?

@kecnry kecnry changed the title Expose axes limits in profile viewer options Expose axes limits in viewer options Sep 12, 2023
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.

3 participants