remove BasicJupyterToolbar instance to avoid conflicts #1679

kecnry · 2022-09-27T16:55:46Z

Description

This pull request removes the BasicJupyterToolbar instance otherwise initialized by glue-jupyter so that it does not conflict with jdaviz's custom NestedJupyterToolbar.

Fixes #1665

Change log entry

Is a change log needed? If yes, is it added to CHANGES.rst? If you want to avoid merge conflicts,
list the proposed change log here for review and add to CHANGES.rst before merge. If no, maintainer
should add a no-changelog-entry-needed label.

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 milestone set?
After merge, any internal documentations need updating (e.g., JIRA, Innerspace)?

codecov · 2022-09-27T17:20:28Z

Codecov Report

Base: 87.26% // Head: 87.29% // Increases project coverage by +0.03% 🎉

Coverage data is based on head (503d1c1) compared to base (d001e2b).
Patch coverage: 94.44% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1679      +/-   ##
==========================================
+ Coverage   87.26%   87.29%   +0.03%     
==========================================
  Files          95       95              
  Lines       10091    10076      -15     
==========================================
- Hits         8806     8796      -10     
+ Misses       1285     1280       -5

Impacted Files	Coverage Δ
jdaviz/app.py	`94.02% <ø> (-0.03%)`	⬇️
jdaviz/configs/default/plugins/viewers.py	`95.93% <83.33%> (+0.10%)`	⬆️
jdaviz/components/toolbar_nested.py	`75.43% <100.00%> (+1.75%)`	⬆️
jdaviz/configs/cubeviz/plugins/viewers.py	`91.30% <100.00%> (-0.31%)`	⬇️
jdaviz/configs/imviz/plugins/viewers.py	`82.85% <100.00%> (-0.29%)`	⬇️
jdaviz/configs/mosviz/plugins/viewers.py	`79.41% <100.00%> (-0.21%)`	⬇️
jdaviz/configs/specviz/plugins/viewers.py	`84.10% <100.00%> (-0.13%)`	⬇️
jdaviz/core/tools.py	`81.15% <100.00%> (+0.19%)`	⬆️
... and 1 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

rosteen

I don't know why, but while this seems to fix one issue with the single-pixel subset tool (reselecting an existing subset on click), it adds new unexpected behavior: when clicking and dragging with the single-pixel tool selected, it will not create a circular subset. Not sure if that should be fixed here or separately, since while it's apparently caused by this it's sort of unrelated and the fix is probably in the tool code.

kecnry · 2022-09-28T17:23:06Z

it adds new unexpected behavior: when clicking and dragging with the single-pixel tool selected, it will not create a circular subset.

I'm not quite following this. But I can reproduce that the ROIClickAndDrag is not defaulting when all tools are off (and therefore any previously enabled subset tools are still active). Is this the same thing you're seeing? I definitely think that needs to be fixed here. It may be that the old toolbar was providing the default behavior and we need to re-implement that in the nested toolbar now that that toolbar no longer exists 🤔

rosteen · 2022-09-28T18:10:12Z

Whoops, I meant it will create a circular subset when you have the single-pixel tool selected if you click and drag with it. However, now that I went to record a screen capture of the behavior, I can't reproduce it. I'll mess with it a little more and see if I can do it again...

kecnry · 2022-09-28T18:12:55Z

Ok, I think that's the same thing that I was able to reproduce (should happen whenever you have the circle tool enabled and then disable or enable another non-subset tool). I'll move this to draft until I can sort that out. Thanks for the catch!

rosteen · 2022-09-28T18:14:07Z

Ah yep there it is, have to click the single-pixel subset tool without disabling the circle-select tool first. Thanks for looking into it!

Co-authored-by: Kyle Conroy <[email protected]>

pllim · 2022-09-28T20:41:20Z

jdaviz/configs/default/plugins/viewers.py

+        self.toolbar = NestedJupyterToolbar(self, self.tools_nested, default_tool_priority)
+
+    @property
+    def tools(self):


Coverage says this is not tested. Is this really used?

we don't use it anywhere, but glue-jupyter defaults viewer.tools to a list of strings for what BasicJupyterToolbar would use. Since this PR now prevents BasicJupyterToolbar from being created and replaces it with NestedJupyterToolbar, I figured we shouldn't carry around the irrelevant list and so just replace it with the equivalent flat list of tool names in our toolbar.

With the upstream fix, do we still need this?

Yes, we still have our own toolbar implementation.

pllim · 2022-09-29T21:20:04Z

#1647 has been merged. Please rebase, thanks!

jdaviz/core/tools.py

kecnry · 2022-10-03T13:36:41Z

I think this should work now, but we may want to carefully consider the implementation and if there are any "cleaner" options before accepting/merging.

This also opens up the possibility for us to redefine when the subset selection is reset to "Create New". The current behavior (adopted from glue) is to reset whenever a subset selection tool is activated. This can be a bit unintuitive in some cases though, especially when switching between a subset selection tool and a zoom tool with the intention to continue revising the same selection. A similar annoyance is reported as #1653. (Work towards changing this behavior should probably be in a follow-up PR, I just wanted to note that we now have more control over the specific behavior now)

pllim · 2022-10-05T17:39:13Z

Are we waiting for SME to weight in or you want to get this in regardless?

kecnry · 2022-10-07T13:24:25Z

Updated to make use of glue-viz/glue-jupyter#320, I'll update the glue-jupyter pin once it is included in a release and then mark this as ready for review. If anyone feels like reviewing early, feel free to install that PR of glue-jupyter and do so, I only expect to change the version requirement on glue-jupyter.

pllim · 2022-10-07T13:42:10Z

Unless glue-jupyter is getting released in a few hours, I propose we turn this back to draft and slap a "upstream fix required" label on it. Thanks!

rosteen

Looks good to me, fixes all the bugs I'd seen previously. I couldn't break anything testing in Cubeviz.

pllim · 2022-10-10T22:35:14Z

This is marked as a bug, do we need to backport all the bug fixes or just the critical ones?

pllim · 2022-10-10T22:37:05Z

If we want to backport, you might have to manually squash the irrelevant commits so we can use "Merge pull request" to create a merge commit.

kecnry · 2022-10-11T13:10:43Z

do we need to backport all the bug fixes or just the critical ones?

I think we can decide case-by-case. I'm on the fence here - this does fix a bug, but it also has to make somewhat significant changes under-the-hood to do so.

If we want to backport, you might have to manually squash the irrelevant commits so we can use "Merge pull request" to create a merge commit.

I can do that if we decide to backport this (although I think we should also be able to cherry pick the "squash and merge" as long as we're doing it manually).

pllim · 2022-10-11T13:16:02Z

@kecnry , if you do not create merge commit, it is hard to verify using scripts like https://github.com/astropy/astropy-tools/blob/main/pr_consistency/4.check_consistency.py (not exactly that one because it is tailored for astropy but you get the idea)

* improve nested toolbar * single pixel to reset subset selection * bump glue-jupyter

kecnry · 2022-10-12T18:33:21Z

@pllim - squashed and re-milestoned as 3.0.2 (assuming we're ok with the version bump on glue-jupyter in the bugfix release)

duytnguyendtn

A couple of variable name comments, but otherwise fixes the behavior on my side! Thanks!

duytnguyendtn · 2022-10-12T19:38:08Z

jdaviz/configs/cubeviz/plugins/viewers.py

-             'jdaviz:panzoom', 'jdaviz:panzoom_x',
-             'jdaviz:panzoom_y', 'bqplot:xrange',
-             'jdaviz:selectslice', 'jdaviz:selectline']
-
    # categories: zoom resets, zoom, pan, subset, select tools, shortcuts
    tools_nested = [


Does tools_nested make sense anymore in context of this PR's changes?

I think so. This PR still exposes a property for viewer.tools which is a read-only flattened version of this nested list to match the same format that is set as tools by the parent classes. Alternatively we could call the nested list as tools but then would probably need to set inherit_tools=False to avoid conflicts with upstream logic.

duytnguyendtn · 2022-10-12T19:39:32Z

jdaviz/configs/mosviz/plugins/viewers.py

-             'jdaviz:panzoom', 'bqplot:rectangle',
-             'bqplot:circle']
-
+class MosvizImageView(JdavizViewerMixin, BqplotImageView):
    # categories: zoom resets, zoom, pan, subset, select tools, shortcuts
    tools_nested = [


Same as comment above

duytnguyendtn · 2022-10-12T19:39:48Z

jdaviz/configs/mosviz/plugins/viewers.py

-             'jdaviz:panzoom',
-             'jdaviz:panzoom_x',
-             'bqplot:xrange']
-
    # categories: zoom resets, zoom, pan, subset, select tools, shortcuts
    tools_nested = [


Same as comment above

remove BasicJupyterToolbar instance to avoid conflicts (cherry picked from commit f14b99c)

kecnry added this to the 2.11 milestone Sep 27, 2022

github-actions bot added cubeviz embed Regarding issues with front-end embedding imviz mosviz specviz labels Sep 27, 2022

kecnry force-pushed the no-duplicate-toolbar branch 2 times, most recently from f26628d to dd6af62 Compare September 27, 2022 17:06

kecnry added the bug Something isn't working label Sep 27, 2022

kecnry mentioned this pull request Sep 27, 2022

Imviz: Alt-click to create single-pixel region #1647

Merged

10 tasks

kecnry marked this pull request as ready for review September 27, 2022 17:22

kecnry requested review from duytnguyendtn, rosteen, javerbukh, ojustino and pllim as code owners September 27, 2022 17:22

rosteen reviewed Sep 28, 2022

View reviewed changes

kecnry marked this pull request as draft September 28, 2022 18:13

pllim added a commit to pllim/jdaviz that referenced this pull request Sep 28, 2022

To be compatible with spacetelescope#1679

c48b37b

Co-authored-by: Kyle Conroy <[email protected]>

pllim reviewed Sep 28, 2022

View reviewed changes

kecnry force-pushed the no-duplicate-toolbar branch from dd6af62 to 9bfcf26 Compare September 30, 2022 20:04

kecnry commented Sep 30, 2022

View reviewed changes

jdaviz/core/tools.py Outdated Show resolved Hide resolved

kecnry force-pushed the no-duplicate-toolbar branch 2 times, most recently from c1197b5 to aca911c Compare October 3, 2022 12:07

kecnry force-pushed the no-duplicate-toolbar branch from aca911c to ecb35ec Compare October 3, 2022 12:56

kecnry marked this pull request as ready for review October 3, 2022 13:36

kecnry force-pushed the no-duplicate-toolbar branch from ecb35ec to 81bb5c4 Compare October 3, 2022 14:22

kecnry force-pushed the no-duplicate-toolbar branch from 81bb5c4 to e1966d0 Compare October 7, 2022 13:22

kecnry added the Upstream fix required label Oct 7, 2022

rosteen approved these changes Oct 10, 2022

View reviewed changes

kecnry added Ready for final review and removed Upstream fix required labels Oct 10, 2022

rosteen modified the milestones: 2.11, 3.1 Oct 10, 2022

remove BasicJupyterToolbar instance to avoid conflicts

503d1c1

* improve nested toolbar * single pixel to reset subset selection * bump glue-jupyter

kecnry force-pushed the no-duplicate-toolbar branch from dd82caf to 503d1c1 Compare October 12, 2022 18:32

kecnry requested a review from bmorris3 as a code owner October 12, 2022 18:32

kecnry modified the milestones: 3.1, 3.0.2 Oct 12, 2022

duytnguyendtn approved these changes Oct 12, 2022

View reviewed changes

kecnry merged commit f14b99c into spacetelescope:main Oct 13, 2022

kecnry deleted the no-duplicate-toolbar branch October 13, 2022 16:43

rosteen pushed a commit to rosteen/jdaviz that referenced this pull request Oct 18, 2022

Merge pull request spacetelescope#1679 from kecnry/no-duplicate-toolbar

187a987

remove BasicJupyterToolbar instance to avoid conflicts (cherry picked from commit f14b99c)

This was referenced Oct 25, 2022

fix nested toolbar in specviz2d/mosviz 2d profile viewer #1778

Merged

Fix box zoom #1726

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

remove BasicJupyterToolbar instance to avoid conflicts #1679

remove BasicJupyterToolbar instance to avoid conflicts #1679

kecnry commented Sep 27, 2022 •

edited

Loading

codecov bot commented Sep 27, 2022 •

edited

Loading

rosteen left a comment

kecnry commented Sep 28, 2022

rosteen commented Sep 28, 2022

kecnry commented Sep 28, 2022

rosteen commented Sep 28, 2022

pllim Sep 28, 2022

kecnry Sep 28, 2022

pllim Oct 7, 2022

kecnry Oct 10, 2022

pllim commented Sep 29, 2022

kecnry commented Oct 3, 2022 •

edited

Loading

pllim commented Oct 5, 2022

kecnry commented Oct 7, 2022

pllim commented Oct 7, 2022

rosteen left a comment

pllim commented Oct 10, 2022

pllim commented Oct 10, 2022

kecnry commented Oct 11, 2022

pllim commented Oct 11, 2022

kecnry commented Oct 12, 2022

duytnguyendtn left a comment

duytnguyendtn Oct 12, 2022

kecnry Oct 12, 2022

duytnguyendtn Oct 12, 2022

duytnguyendtn Oct 12, 2022

remove BasicJupyterToolbar instance to avoid conflicts #1679

remove BasicJupyterToolbar instance to avoid conflicts #1679

Conversation

kecnry commented Sep 27, 2022 • edited Loading

Description

Change log entry

Checklist for package maintainer(s)

codecov bot commented Sep 27, 2022 • edited Loading

Codecov Report

rosteen left a comment

Choose a reason for hiding this comment

kecnry commented Sep 28, 2022

rosteen commented Sep 28, 2022

kecnry commented Sep 28, 2022

rosteen commented Sep 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pllim commented Sep 29, 2022

kecnry commented Oct 3, 2022 • edited Loading

pllim commented Oct 5, 2022

kecnry commented Oct 7, 2022

pllim commented Oct 7, 2022

rosteen left a comment

Choose a reason for hiding this comment

pllim commented Oct 10, 2022

pllim commented Oct 10, 2022

kecnry commented Oct 11, 2022

pllim commented Oct 11, 2022

kecnry commented Oct 12, 2022

duytnguyendtn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kecnry commented Sep 27, 2022 •

edited

Loading

codecov bot commented Sep 27, 2022 •

edited

Loading

kecnry commented Oct 3, 2022 •

edited

Loading