jdaviz icon indicating copy to clipboard operation
jdaviz copied to clipboard

[FEAT] Make it possible to destroy the default viewer

Open eteq opened this issue 2 years ago • 5 comments
trafficstars

Jdaviz component

Imviz

What is the problem this feature will solve?

When in use cases where you want to look at two images side-by-side in different viewers, it's often useful to use appropriately-named viewers for them that help identify which image is which and why I'm looking at them. The name imviz-0 then serves as a distraction, because it's not being used and there's not really any way to hide it.

Describe the desired outcome

imviz.destroy_viewer('imviz-0') should successfully destroy the viewer. Fine to have it pop up a warning that says something like "this will make some things not work by default".

It might also make sense to have this be possible on the UI, but I'm less sure about that since someone just using the UI is less likely to recognize the consequences of that (and would miss the warning).

Additional context

🐱

eteq avatar Jul 31 '23 15:07 eteq

I think we have a separate ticket to "rename viewers" and I think that will solve your problem without having to destroy the default viewer?

I disabled it on purpose.

One could also say that it makes sense to promote the second viewer that is still open to default viewer when you destroy the real default viewer, but that would require refactoring.

pllim avatar Jul 31 '23 15:07 pllim

Ohh, good point @pllim, I think that's also a fine solution, although it's a bit awkward when combined with #2320. Basically what I really want here is a way to load two or more images in a loop in separate viewers, have them be named by their filename or similar, and not have to special-case viewer 0. I think anything that acheived that would be fine even if it doesn't actually destroy viewer 0.

Can you clarify why it's important to not to remove it though? I thought it was because some API calls use the default viewer by default? My thinking here is that we could treat destroying viewer 0 as a "advanced mode" where even if it breaks some of the convenience API a bit, that's ok as long as the user can still get around it for use cases where they aren't using the default viewer anyway?

eteq avatar Jul 31 '23 15:07 eteq

Currently we do have a lot of things that attempt to load data into the "default" viewer. For imviz, we could probably switch this logic to be the "first" existing viewer. But since we'd never really want to support having NO viewers, maybe renaming the default viewer to whatever you want would still support the same workflow?

kecnry avatar Jul 31 '23 16:07 kecnry

Yeah, the more you both talk about it, I think the "renaming" approach would be just fine as a way to functionally acheive the same as the "first" viewer workflow you're describing. The main catch is how to be able to do the thing I'm requesting in #2320 at the same time - i.e. I'd want to be able to do something like:

for im in images:
    imviz.load_data(im, viewer_label=label, ...some-other-keywords-maybe...)

And have it be smart enough to have the first load automatically do the rename. But I guess maybe it's just a matter of having some logic that checks if the default viewer has had anything loaded, and if not automatically do the rename inside load_data?

eteq avatar Jul 31 '23 17:07 eteq

The default viewer name is defined in the YAML. On top of my head, I am not sure how easy it is to change it on the fly.

pllim avatar Aug 01 '23 23:08 pllim