Imviz click to center in pan/zoom

[pllim]

Description

This pull request is to enable "click to center" when in pan/zoom mode (either linked or unlinked)

This is driven by interactive actions, and by precedence, I didn't write any non-interactive unit tests for it. The underlying center_on() method is already tested in test_astrowidgets_api.py.

Checklist for package maintainer(s)

This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.

• Are two approvals required? Branch protection rule does not check for the second approval. If a second approval is not necessary, please apply the trivial label.
• Do the proposed changes actually accomplish desired goals? Also manually run the affected example notebooks, if necessary.
• Do the proposed changes follow the STScI Style Guides?
• Are tests added/updated as required? If so, do they follow the STScI Style Guides?
• Are docs added/updated as required? If so, do they follow the STScI Style Guides?
• Did the CI pass? If not, are the failures related?
• Is a change log needed? If yes, is it added to CHANGES.rst?
• Is a milestone set?
• After merge, any internal documentations need updating (e.g., JIRA, Innerspace)? 🐱

[pllim]

Not sure why auto labeling failed but should not block this PR.

[codecov]

Codecov Report

Merging #1319 (d14f598) into main (61c6102) will decrease coverage by 0.13%.
The diff coverage is 34.78%.

@@            Coverage Diff             @@
##             main    #1319      +/-   ##
==========================================
- Coverage   81.22%   81.09%   -0.14%     
==========================================
  Files          91       91              
  Lines        7570     7591      +21     
==========================================
+ Hits         6149     6156       +7     
- Misses       1421     1435      +14     

Impacted Files	Coverage Δ
jdaviz/configs/imviz/plugins/viewers.py	80.47% <ø> (ø)
jdaviz/configs/imviz/plugins/tools.py	42.74% <34.78%> (-1.92%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 61c6102...d14f598. Read the comment docs.

[kecnry]

Can this logic not be in the tools themselves? Do we want this to be imviz-specific behavior and not in all image viewer (or all viewers)?

[pllim]

Can this logic not be in the tools themselves?

It was only requested for Imviz. 🤷

If you want this to be native in Pan/Zoom tool, then it has to go upstream in glue-jupyter and also I don't know how it can grab the event X/Y inside the tool, so I will need to consult @astrofrog .

[kecnry]

We can put it in a subclass of the panzoom tools (in core/tools.py). I just think its cleaner to keep all the tool-related logic in one place.

I don't know how it can grab the event X/Y inside the tool

We'll have to see if the existing selector (likely BqplotPanZoomMode.interact) fires anything if not dragging a valid selection.

[pllim]

The PanZoom stuff is all the way in bqplot. I can't quite make sense of it but I don't see any "click" mentioned.

https://github.com/bqplot/bqplot/blob/0.12.x/bqplot/interacts.py#L140

https://github.com/bqplot/bqplot/blob/0.12.x/js/src/PanZoom.ts

https://github.com/bqplot/bqplot/blob/0.12.x/js/src/PanZoomModel.ts

[kecnry]

Then we can still add a callback on the viewer in the same way that we do for the BlinkOnce tool. That should essentially be the same block of code as here, except moved into a subclass of BqplotPanZoomMode. We can then easily decide which viewers get that capability and which don't, just be choosing which version of the tool to include in the toolbar.

[pllim]

Okay, I think refactored it as you suggested, @kecnry . Is this what you meant?

[kecnry]

Yes, exactly! And seems to work perfectly! This is currently still imviz-specific, but I think that's fine for now (and it lives in imviz tools, so that makes sense... if we ever want to generalize it for other viewers, we can easily move it into the core tools). Besides one line in the docs, this isn't really discoverable, but I think that's ok too.

Would be nice to have better test coverage, but testing the actual on_click logic probably wouldn't be easy (although I guess we could pass data manually to on_click to mimic the location... but then its still not fully testing the interactive action). If we've usually let these things go through without full coverage, I'm fine with that for now and we can revisit it when we implement better UI/UX testing.

[javerbukh]

Looks good!

[pllim]

Thanks for the approvals!