Hide unsupported subset export formats #3633
Conversation
* Use unicode traitlet to communicate bad choice of output format to user rather than raising an error in console. * Currently debugging a persistence issue.
* If a format is not supported, it no longer shows in the export dropdown. For spectral, this means that there is no longer a dropdown since only one format is supported. * If this design is chosen, the default format/filename must be fixed and some text placed to indicate that ecsv is chosen.
* Used for spectral subset (ecsv only)
* Removed old code and condense * Fixed up the logic for populating the API dropdown. There may be redundancy with `subset_format_options` in `_set_subset_formats()` but I would need to test some more to confirm.
* Removed line updating `self.subset_format_options`. In testing, it appears usability is not affected by updating the attribute.
* Removed tests related to checking for export disabling via disallowed format * Modified tests in Imviz and Cubeviz portions for checking the ValueError raised when bad formats are passed to `subset_format.selected` for both spatial and spectral subsets. * Also slight modifications to tests to avoid checking 'default' export format values where possible.
* To solve test failure in `test_export_stcs_circle_ellipse`
* Notably moved some tests into for loop to loop through valid subset formats and check valid output with the intent of making future testing easier for other potential formats. * Added variable names for things to make further testing easier. * Update logic on changing filename to confirm output. * Fixed some things in bad spatial subset format check.
github-actions
bot
added
the
plugin
MatthewPortman
requested review from
bmorris3,
cshanahan1,
gibsongreen,
haticekaratay,
javerbukh,
kecnry and
rosteen
as code owners
|
I will need someone else to update the label/milestone (still), thanks! |
|
My main concern actually feels pretty natural from the UI alone, but still think it could cause some confusion from the API. For example, imagine a case where subset 1 and 3 are spatial and subset 2 is spectral in cubeviz and now I realize I selected the wrong subset, I wanted the other spatial subst in the UI you see the format dropdown change, but it would be easy to miss this in the API, or could even be confusing if you do since there isn't as clear of an "order" it feels like my selection to override the default was lost. Related to this, if you're trying to reproduce the state of the plugin, then this is a place that order will matter. We do have other places where order matters, so maybe its not a big deal to add another if this is deemed to be the better UX from the UI-perspective. Lastly, if we go this route, I think we should at least apply it across the entire plugin and we currently use |
* Previous behavior defaulted to FITS no matter the previously selected format type. * Note that switching subsets in the same type (spatial -> spatial) does not select the previous type... this would mostly affect API users. * Testing another version using a dictionary...
* Use a dictionary and an observer now * Left in code (commented out) if desired UX is to retain subset format *across subsets* rather than *across types*
MatthewPortman
force-pushed
the
fresh-hide-export-formats
branch
2 times, most recently
from
bd365c9 to
6e1fef5
Compare
|
The persistence issue has been fixed! |
* Just to be safe copied bad format test from cubeviz into imviz
| def _on_subset_format_selected(self, event): | ||
| # To prevent an error upon object initialization | ||
| if hasattr(self, 'subset_format_dict'): | ||
| # Persisent across subsets |
There was a problem hiding this comment.
should this be removed?
There was a problem hiding this comment.
Which part exactly? I left the commented out version (persistence across subsets) in case we later decide that that behavior is preferred.
|
Marking as draft - we're moving forward with #3635 now and can come back to this idea in the future if we can find a way to apply it more consistently app-wide. |
4 participants
Description
This pull request is to address the ability of the user to select subset
export formats that we do not support in the UI/API. In this PR---
Fixes JDAT-5374
Change log entry
CHANGES.rst? If you want to avoid merge conflicts,list the proposed change log here for review and add to
CHANGES.rstbefore merge. If no, maintainershould add a
no-changelog-entry-neededlabel.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.
triviallabel.