Allow basic editing for supported shapes and display rotation for rectangle and ellipse #1427
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1427 +/- ##
==========================================
+ Coverage 85.45% 85.52% +0.06%
==========================================
Files 94 94
Lines 9061 9111 +50
==========================================
+ Hits 7743 7792 +49
- Misses 1318 1319 +1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
pllim
force-pushed
the
you-spin-me-right-round-right-round
branch
from
5b002c6 to
32d2fe7
Compare
pllim
changed the title
pllim
changed the title
pllim
force-pushed
the
you-spin-me-right-round-right-round
branch
from
3b5b8ef to
1cff3f1
Compare
pllim
requested review from
duytnguyendtn,
rosteen,
javerbukh,
ojustino and
kecnry
as code owners
That is some front-end dark magic that I don't know how to do. If you do, please suggest the changes! Does |
| <v-text-field | ||
| :label="item.name" | ||
| v-model.number="item.value" | ||
| type="number" | ||
| :disabled="item.name=='Angle'" | ||
| ></v-text-field> |
There was a problem hiding this comment.
For the former, I think it would be something like this:
| <v-text-field | |
| :label="item.name" | |
| v-model.number="item.value" | |
| type="number" | |
| :disabled="item.name=='Angle'" | |
| ></v-text-field> | |
| <v-text-field | |
| :label="item.name" | |
| v-model.number="item.value" | |
| @keyup.enter="update_subset" | |
| type="number" | |
| :disabled="item.name=='Angle'" | |
| ></v-text-field> |
Injecting it in on myside seems to do the trick
There was a problem hiding this comment.
Hmm... but this will:
(a) make this plugin behavior different from the other ones; and
(b) might prematurely update a shape when someone accidentally hits enter but really want to also update other fields?
There was a problem hiding this comment.
(a) make this plugin behavior different from the other ones
I'm not sure if there's a consistent pattern here. Take, for instance, the Line Width field for plot options: when I update the text field, it immediately updates the line width, doesn't even wait for the user. I'd argue that it might be annoying to the user to have the field update while the user is trying to type the bounds, hence why I proposed Enter to accept. Though now that I say it, is the UPDATE button even necessary in this context? Why not just have it accept automatically? Which of course leads to...
want to also update other fields?
Is there a problem with updating one field at a time? I don't necessarily see it as an issue if I update the xcenter and THEN the ycenter rather than changing both fields simultaneously
There was a problem hiding this comment.
Come to think about it, I wonder if the disconnect here has to do with perspective. For Line Width I think of that field as an actual representation of the value, hence when I change the number, I expect it to immediately update. For Model Fit, it's more like a command, so I fill out all the necessary boxes and then tell it to activate via the button click.
As I tested it now, this PR appears to be more of the "command" like paradigm, where the user fills out all the information and then actively requests the plugin to "UPDATE" the subset by pressing the button. I suppose my request is really wondering whether it would be better to change this to the "representation" paradigm to have it behave more like plot options, like Line Width for instance.
There was a problem hiding this comment.
Subset update may be expensive, because it might affect other calculations etc. You would only want to update it when you are sure you want to update it.
There was a problem hiding this comment.
here are the options for updating a traitlet from a v-text-field with type="number" that are used elsewhere already:
- live updating on keypress: use
v-model(see slice plugin for a good example) and then@observein python. We use this quite often and it mimics what glue widgets do, so unless there are performance limitations, this might be worth trying first. - update on change (loss of focus) or mousepress on the up/down buttons: use
@changeand@mousedownto call a JS or python method to update the traitlet manually (play_pause widget does something similar - but this also usesv-modeland just triggers acting on it at those times, you could do the same to always have the traitlet update but only trigger the “update” when this happens, or if you want to have an@observestill in python, then you might need extra traitlets).
There was a problem hiding this comment.
Re: lag -- I tried with 6 4k x 2k and Imviz is already super laggy (the lag from Subset update doesn't matter much at that point because the whole thing is unusable, which is out of scope here). With 4 of those images, Imviz lag is tolerable but you can really see the lag in Subset shape catching up with each click to Update. So, I am still not convinced live-updating is a good idea.
You can try for yourselves like this:
- Open up Imviz example notebook.
- Load the example images twice (so you get 4 total) with this additional call:
imviz.load_data(acs_47tuc_1, data_label='acs_47tuc_3')
imviz.load_data(acs_47tuc_2, data_label='acs_47tuc_4')- Link them by WCS without fast approximation:
imviz.link_data(link_type='wcs', wcs_use_affine=False) - Draw a Subset.
- Use this new edit feature and change some field of the Subset, click Update. See lag.
There was a problem hiding this comment.
p.s. This step is also important: imviz.link_data(link_type='wcs', wcs_use_affine=False) (links them by WCS without fast approximation). I updated the instructions above.
There was a problem hiding this comment.
Down the road I think we can either look into improving the performance or perhaps have this plugin check the number of links (and if fast approximation is enabled) and switch itself between live-updating and the update button. But I'm ok considering that as a follow-up effort and moving forward with the PR with the update button.
There was a problem hiding this comment.
Having the button come and go is even more confusing than just allowing live-update unconditionally. But I agree this can be future work.
Please approve if the PR is okay as-is or if there are more changes needed here. Thanks!
Come to think about it, maybe is sufficient if my cursor stayed selected on the field. If the former suggestion works (press enter to auto update), and my cursor stayed on the same field, then I could just press tab to advance. The problem currently is when I press enter, it takes my cursor off, so I have to go back to my mouse to reselect on the next field |
But why are you pressing Enter in the first place? It currently does not do anything. I think this applies to all the plugins. |
Sorry, that comment was meant for if we go with the "press enter to update" direction |
There was a problem hiding this comment.
Ok here's my official review! Functionality and code looks good! Thanks Pey-Lian!
I have one comment concerning the spacing of the elements, and another in the above chain concerning the UPDATE button. IMHO, I feel these the subset tools plugin should behave more like an "inspector", similar to Plot Options. By that I mean modifications to the values should immediately (or if not immediately, then at least when the user "exits" the scope of the text field by clicking off or pressing ENTER. Therefore, the UPDATE button would be largely unnecessary, like how we don't have one in Plot Options. I'll try to remember to bring this up at parking lot tomorrow for the team to discuss
| <div v-if="is_editable"> | ||
| <v-row v-for="(item, index2) in subset_definitions[0]" no-gutters> | ||
| <v-col> | ||
| <v-text-field |
There was a problem hiding this comment.
Not sure whether we want to include this in scope, but I feel like this spacing feels awkwardly large to me. Thoughts @Jenneh?
There was a problem hiding this comment.
I'll be happy to reduce the spacing if you tell me how to do it.
I am still hesitant about the "update immediately" model. I feel like the performance hit is going to come back and bite us as we build more features. But we can discuss tomorrow.
There was a problem hiding this comment.
The extra spacing is caused by the nested v-cols inside v-rows. When there is only a single column, its easiest to just remove v-col entirely. Otherwise you'll want to add class="row-no-outside-padding" to the v-row. See the third bullet in the plugin style guide (and feel free to clean that up if any of it seems confusing)
There was a problem hiding this comment.
I removed the v-col (didn't make much difference) and neither did class="row-no-outside-padding", but the class="pt-0 pb-0 mt-0 mb-0" seems to work. I copy-pasted that from other parts of subset_plugin.vue.
pllim
force-pushed
the
you-spin-me-right-round-right-round
branch
from
3a14b2c to
0db834f
Compare
There was a problem hiding this comment.
Seems to work well! Is support for editing sub-subsets out-of-scope (it can display the sub-subsets, but doesn't show the logical operators between them)?
Is there a need for displaying both the edit box and the static read-only version of the subset parameters in cases where editing is supported? It seems to me that showing both sections adds quite a bit of code overhead (needing to trace the current widget value in addition to the original value), that might be simplified if the read-only section is just replaced with the widgets when editing is supported? But then without live-updating, we might want to have a cancel/reset button to revert the widget values to the current state?
|
This looks very nice!
|
That is a known issue and has existed before this PR. In fact, https://jdaviz.readthedocs.io/en/latest/imviz/plugins.html#subset-tools documents this in the second paragraph. |
I would say, yes. I have not found any use for compound subsets yet in Imviz. I guess it is more useful in Specviz but working on it would exceed the points for this ticket the PR is tied to. I mean, when someone edits a compound subset so that the two sub-subsets now overlap, do we just create a single non-compound subset or throw error? There are extra stuff we need to ponder. |
I originally combined them but find the display to be confusing because then I cannot be sure if the values are the ones I just entered or from the actual Subset state. When I edit a Subset, personally as a user, I find it useful to be able to see the current value and compare to what I just entered to make sure I am not trying to do something crazy. |
I'd be curious what @Jenneh thinks, but we can always iterate after merge if/when everything else is ready to go. |
That is the reason why we have fast approximation in the WCS transform. I do not recommend people turn fast approx. off but I turn it on to show you the worst case scenario with the lag. |
That was original request from @duytnguyendtn as well. But looks like we can defer this feature for now as Cami wants to try the plugin on different use cases first. |
pllim
force-pushed
the
you-spin-me-right-round-right-round
branch
from
0db834f to
bbc3593
Compare
There was a problem hiding this comment.
Still a few things to consider in future iterations (just summarizing points from multiple threads above, but I think we're ok to merge this as-is, once CI passes):
- re-consider live-updating and performance implications
- consult with @Jenneh if the read-only and edit version should both be shown
- re-implement popout button when issues with subsets failing to update are fixed
- enable support for angle rotation once supported upstream
- possible support for editing sub-subsets (see conversation above about why this might get difficult)
|
Thanks! I'll merge and can always open follow-up PRs if someone else submits a late review. |
|
Follow-up issues:
|
Description
This pull request is to allow basic shape editing. Also shows rotation angle for some shapes.
Related:
Address the centering part of #634 because rotation is blocked by glue-viz/glue-astronomy#74 .
TODO
For example, does Aperture Photometry take rotated Subset?ipypopout?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.CHANGES.rst?