Filedialog widget for magicgui

[GenevieveBuckley]

This is for discussion of https://github.com/napari/magicgui/issues/20 QFileDialog picker.

Useful to know:

• It's not enough to simply map from a Path type to a QFileDialog/MagicFileDialog widget in type2widget, we need to check isinstance instead. This takes care of platform specific things like WindowsPath, etc.

Still up for discussion:

• I think it'd be nicer to have a button & text label that you can push to launch the file dialog, rather than keeping the file dialog expanded in magicgui.
• The getter/setter for the file dialog here is only for files, we still need to work out what to do about files vs directories vs multiple selection, etc.

Here's a

"""FileDialog with magicgui."""
from pathlib import Path
from magicgui import event_loop, magicgui

@magicgui(call_button='Call Button')
def filepicker(filename=Path()):
    print("The filename is:", filename)
    return filename

with event_loop():
    gui = filepicker.Gui(show=True)
    gui.filename_changed.connect(print)
    # we can connect a callback function to the __call__ event on the function
    gui.called.connect(lambda x: gui.set_widget("result", str(x), position=-1))

File dialog GUI is embedded

Embedded GUI disappears completely once you select a file

Pairing file dialog with a version of the call button might be better, this launches the file picker when you need it and doesn't disappear

[tlambert03]

I think it'd be nicer to have a button & text label that you can push to launch the file dialog, rather than keeping the file dialog expanded in magicgui.

this is totally how this should be, great point! makes everything easier (and let's us use the native file dialogs). So we basically just need to make our own new widget that has a LineEdit and a button. I'll propose some changes that you can review.

[codecov]

Codecov Report

Merging #23 into master will increase coverage by 0.03%.
The diff coverage is 95.79%.

@@            Coverage Diff             @@
##           master      #23      +/-   ##
==========================================
+ Coverage   95.49%   95.52%   +0.03%     
==========================================
  Files           4        4              
  Lines         754      871     +117     
==========================================
+ Hits          720      832     +112     
- Misses         34       39       +5     

Impacted Files	Coverage Δ
magicgui/_qt.py	95.28% <94.73%> (-0.34%)	⬇️
magicgui/_tests/test_ qt.py	98.86% <97.67%> (-1.14%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e530e83...5ec9874. Read the comment docs.

[GenevieveBuckley]

I've merged your suggestion, so this is ready for review now. It still needs a couple of tests added.

I think we should stick with the class name MagicFileDialog instead of the less clear MagicPathLineEdit, but other than that I love it

[GenevieveBuckley]

I'm not sure how to properly test the _on_choose_clicked() method without the newly opened file dialog box blocking until it is manually closed. I did see this suggestion to use QTimer, but I don't know how to get the handle/reference of the spawned file dialog window we want to close.

[tlambert03]

I'm not sure how to properly test the _on_choose_clicked() method without the newly opened file dialog box blocking until it is manually closed. I did see this suggestion to use QTimer, but I don't know how to get the handle/reference of the spawned file dialog window we want to close.

yeah, that is a tricky one! i'm not immediately sure... might need to play around with that. but one immediate thought is that you might be able to use the QTimer pattern here, and then search QApplication.topLevelWidgets() for an instance of QFileDialog? alternatively... we can just instantiate (and save) the QFileDialog in our _on_choose_clicked method, and then call exec_ manually. but that seems unfortunate just for testing.

[tlambert03]

this sort of approach will get you a handle on the dialog during a test:

    def handle_dialog():
        dialog = next(
            child
            for child in filewidget.children()
            if isinstance(child, QFileDialog)
        )
        print(dialog)
        dialog.reject()  #still hangs

    QtCore.QTimer().singleShot(400, handle_dialog)
    filewidget._on_choose_clicked()

but I still can't close it. It might be necessary to move away from the pattern of using static methods as I did, and instead make a filedialog, use setFileMode, then call .exec_()

[GenevieveBuckley]

Oh that's really weird @tlambert03 - your suggestion ONLY hangs on Mac.

Edited to add: I've just put a @pytest.mark.skipif flag on the misbehaving test. We can open an issue for it if/when we merge this PR, so as not to let it block this entirely.

[GenevieveBuckley]

I'm very happy with this overall! I'd like to get it in soon.

To summarize where we're at:

1. I didn't understand this comment, so I'm not sure what to do here:

instead of "files", make it a List[Path] type hint? might run into some issues there, but let me know

2. I'm a hair under the goal test coverage, but I'm not sure how to improve that at this point.

3. I'm not sure how to trigger the mode setter except KeyError clause for MagicFileDialog. Maybe this doesn't really matter, I noticed because I was looking for ways to bump the test coverage.

[tlambert03]

1. I didn't understand this comment, so I'm not sure what to do here:

instead of "files", make it a List[Path] type hint? might run into some issues there, but let me know

That was in reference to @jni's https://github.com/napari/magicgui/issues/20#issuecomment-648504543 where he suggested that "it might be better to go with r/w for modes (existing file/new file), and Sequence[Path] for existing files." In other words, if the user wants to allow for selection of multiple files, they should annotate the argument as a Sequence of Path objects (and then we will render the appropriate widget). However, that would mean we can no longer just look at the options dict... so it does get a bit trickier. I can take a look later

[tlambert03]

regarding using Sequence[Path]... I'm fine leaving that for another PR. we might need to touch a some additional code to make that work.

[tlambert03]

Suggested change

	R = "getOpenFileNames"
	R = "getOpenFileName"

let's make this an alias for a single file. and in a followup (or here if you want to) we can do something like this:

@magicgui
def filepicker(filename=Sequence[Path("~")]):
    ...

and then in type2widget return functools.partial(MagicFileDialog, mode='EXISTING_FILES')

[tlambert03]

whoops, sorry. I committed this and it broke the test. can you fix that? Then I think this is good to go

[GenevieveBuckley]

Ok

[GenevieveBuckley]

regarding using Sequence[Path]... I'm fine leaving that for another PR. we might need to touch a some additional code to make that work.

That's my feeling too, glad it's not just me feeling lazy

[tlambert03]

thanks!

[imagesc-bot]

This pull request has been mentioned on Image.sc Forum. There might be relevant details there:

https://forum.image.sc/t/interactive-dialog-box-for-napari-io-plugin/48332/2