napari / napari

napari: a fast, interactive, multi-dimensional image viewer for python
https://napari.org
BSD 3-Clause "New" or "Revised" License
2.07k stars 410 forks source link

Save All Layers... uses by default napari-svg (.svg) and not napari builtins `Save to Folder` #6846

Open psobolewskiPhD opened 1 month ago

psobolewskiPhD commented 1 month ago

🐛 Bug Report

When having a set of layers, e.g. multi channel image, labels, and points, it's convenient to save all of them. When using File > Save All Layers... by default napari-svg will be used as the writer.

image

I assume this is because it can save 1 svg for all of the content? But it can't save 3D stacks and does anyone really want that? Seems pretty niche. Finally, if you do use that option, napari can't open the results! This is really unintuitive!

Instead, I think 99% of people would want to save the layers individually to a folder, akin to doing Save selected repeatedly. So the default should be napari builtins Save to Folder.

Also napari-svg is napari org, but still a plugin. I think I'd prefer the native options to be favored for default status.

💡 Steps to Reproduce

Open napari and add any two layers -- can open a sample or click new Points, Labels, Shapes, whatever -- they can be empty. Use File > Save All Layers... or Command/Control-Shift-S

Note the dropdown, which will show the writer being used. By default it will show napari-svg. If you use the dropdown, you can select the builtins Save to Folder.

💡 Expected Behavior

The napari builtins Save to Folder should be the default when more than one layer type is selected and napari-svg should be selectable from the menu.

🌎 Environment

napari: 0.4.19.post1 Platform: macOS-14.4.1-arm64-arm-64bit System: MacOS 14.4.1 Python: 3.11.7 | packaged by conda-forge | (main, Dec 23 2023, 14:38:07) [Clang 16.0.6 ] Qt: 5.15.8 PyQt5: 5.15.9 NumPy: 1.26.3 SciPy: 1.12.0 Dask: 2024.1.1 VisPy: 0.14.1 magicgui: 0.8.1 superqt: 0.6.1 in-n-out: 0.1.9 app-model: 0.2.4 npe2: 0.7.4

OpenGL:

Screens:

Settings path:

💡 Additional Context

No response

psobolewskiPhD commented 1 month ago

CC: @DragaDoncila do you have any thoughts on how to fix this behavior?

DragaDoncila commented 1 month ago

@psobolewskiPhD just to clarify - the main issue here is what writer is first shown in the dialog?

psobolewskiPhD commented 1 month ago

Yeah -- well for me on macOS to see other options I need to click on the dropdown, so it's not first vs second, it's default or not. By default, I see svg, when I should see builtins

Edit: I added a screenshot.

And just to emphasize, the svg writer is limited and the output cannot be opened by napari, so it's not viable as a way to save ones work. Save to folder on the other hand works perfectly, modulo issue with loading the folder back https://github.com/napari/napari/issues/5460

DragaDoncila commented 1 month ago

@psobolewskiPhD I understand. I will have a look at it. I think it's just a matter of us explicitly sorting the list of available writers to put the folder writer first. I don't think the dialogs have a default vs. not default option. But will check!

psobolewskiPhD commented 1 month ago

Not sure how easy to implement this is but: default in vanilla fresh install: if builtins and another plugin can work, show builtins, other plugins in dropdown if the user changes to a plugin, remember that for next time

DragaDoncila commented 1 month ago

if the user changes to a plugin, remember that for next time

next time what, the user selects Save All Layers...? Seems too broad to me to remember the preference. Save All Layers in general I think is so broad that we want to be cautious about remembering any kind of context.

psobolewskiPhD commented 1 month ago

I was just thinking -- biased by my own experiences -- that when people are working on a project they probably go through phases where they use the same file types or workflows. So if they're using svg, or some other plugin, rather than builtins, they'd want to keep using it. Save all layers is pretty broad, but I suspect the vast majority of GUI users with multiple layers are mixing: image, label, shape, points -- those are the ones available from GUI. Builtins happens to handle them perfectly, if you know to look. Meanwhile an advanced user doing something with vectors or something else fancy will never want to use builtins in favor of whatever cutting edge thing they use.

When opening a file we already ask what to remember to use, so this the idea would be something similar.

Edit: I could be totally looney though and just biased by helping with a project where there are images, labels, shapes, and points used in regularity.

DragaDoncila commented 1 month ago

Yeah so the order of the dropdown menus is just determined by how we order the writers. It's a little messy (the dropdown is actually populated by QFileDialog.getOpenFileName's filter parameter). But the good news is we can order that filter however we want, once we do decide what we want!

DragaDoncila commented 1 month ago

The function that's determining the filter is here. A quick and dirty way to bring Save to folder to the front would be:

def file_extensions_string_for_layers(
    layers: Sequence[Layer],
) -> tuple[Optional[str], list[WriterContribution]]:
    """Create extensions string using npe2.

    When npe2 can be imported, returns an extension string and the list
    of corresponding writers. Otherwise returns (None,[]).

    The extension string is a ";;" delimeted string of entries. Each entry
    has a brief description of the file type and a list of extensions. For
    example:

        "Images (*.png *.jpg *.tif);;All Files (*.*)"

    The writers, when provided, are the
    `npe2.manifest.io.WriterContribution` objects. There is one writer per
    entry in the extension string.
    """

    layer_types = [layer._type_string for layer in layers]
    writers = list(pm.iter_compatible_writers(layer_types))

    def _items():
        """Lookup the command name and its supported extensions."""
        for writer in writers:
            name = pm.get_manifest(writer.command).display_name
            title = (
                f'{name} {writer.display_name}'
                if writer.display_name
                else name
            )
            yield title, writer.filename_extensions

    # extension strings are in the format:
    #   "<name> (*<ext1> *<ext2> *<ext3>);;+"

    def _fmt_exts(es):
        return ' '.join(f'*{e}' for e in es if e) if es else '*.*'

    ######## bringing save to folder first ###########
    names_and_extensions = list(_items())
    default_writer_index = next((i for i, item in enumerate(names_and_extensions) if item[0] == 'napari builtins Save to Folder'), 0)
    names_and_extensions.insert(0, names_and_extensions.pop(default_writer_index))

    return (
        ';;'.join(f'{name} ({_fmt_exts(exts)})' for name, exts in names_and_extensions),
        writers,
    )

It's obviously brittle the way I've done it there, but we could sort earlier if we wanted, when we have all layer and writer information available.

psobolewskiPhD commented 3 weeks ago

Ran into this in a workshop I ran today--obviously solved with attention to detail on the part of the user, but still. Should builtins just get default prioritization unless the user changes in Prefs/Settings?

psobolewskiPhD commented 5 days ago

Ran into this in a different situation -- but again when doing a live code demo -- with napari-threedee. As it turns out, it has a defunct writer contribution and was the default when saving Points. I didn't realize (read carefully) and got errors. But what's worse is it also was used when using builtins Save to Folder. Shouldn't builtins prioritize builtins?

Czaki commented 4 days ago

Maybe we should do split (like Libre Office or Gimp) and add Export submenu? So save will be, a format that could be loaded and Export some formats like svg, pdf etc that may not be possible to load. Also, we should add supported dimensionality information to writers.

DragaDoncila commented 3 days ago

But what's worse is it also was used when using builtins Save to Folder. Shouldn't builtins prioritize builtins?

Yeah this, at least, shouldn't be happening, I will have to take a look.

psobolewskiPhD commented 3 days ago

Hmm, I can't reproduce, so I'm not sure what I did in the demo. Seems to be working OK right now. (and I fixed the non-existent writer in napari-threedee so it shouldn't happen again anyways)