Add the ability to filter files in the File Dialog #1034

okratitan · 2020-05-24T15:44:28Z

Description:

This adds the ability to filter files in Fyne's File Dialog

Fixes #893

Checklist:

Tests included.
Lint and formatter run with no errors.
Tests all pass.

Where applicable:

Public APIs match existing style.
Any breaking changes have a deprecation path or have been discussed.

okratitan · 2020-05-24T15:45:12Z

Tests added.

andydotxyz

Sorry I missed that a public API from before was not using URI, should probably change to be consistent.
Mostly i am not sure if the filter parameter should be part of the constructing method, and perhaps we should split out a New version for people who want to customise?

dialog/fileicon.go

dialog/file.go

…ching

andydotxyz

You removed the old ShowFileOpen API, but I thought it was discussed that would stay in as a shortcut? And the new APIs gained a Dialog suffix

I noticed that FileDialog does not implement Dialog, was that decided against?

Otherwise the API seems nice and clean

cmd/fyne_demo/screens/window.go

dialog/file.go

dialog/file_test.go

dialog/fileicon_test.go

dialog/fileitem.go

…eDialog

okratitan · 2020-05-26T19:57:12Z

You removed the old ShowFileOpen API, but I thought it was discussed that would stay in as a shortcut? And the new APIs gained a Dialog suffix

I noticed that FileDialog does not implement Dialog, was that decided against?

Otherwise the API seems nice and clean

I added the old API back in as a shortcut and removed the Dialog suffix. FileDialog, now File, does not implement Dialog because I thought it would be messy to do with native pickers on mobile. For instance File is a type that can be used for both the desktop and mobile pickers to pass in configuration/settings. On Desktop, fileDialog is created from this. On mobile it will be used to setup options in the native picker. That being said, while we have a way to handle Show() gracefully if Dialog was implemented. I think other cases would get a little more complicated and I would need to look at the mobile side... For instance with Hide()... easiest enough to hide the fileDialog on desktop. Do we have a handle to the native filepicker that we could use to hide it? with SetOnClosed... The File type already takes in a callback that gets called on close as part of the design so that seems redundant. SetDismissText wouldn't be used either. So with all of that being the case and never reaching an agreement on it in the Slack channel - I just chose the simplest, cleanest way to solve the issue and that was just Show() without implementing the Dialog interface. I'm sure if there are objections, we could implement it simply enough.

okratitan · 2020-05-26T19:59:57Z

Sorry I missed that a public API from before was not using URI, should probably change to be consistent.
Mostly i am not sure if the filter parameter should be part of the constructing method, and perhaps we should split out a New version for people who want to customise?

Done. Thanks!

andydotxyz · 2020-05-26T20:40:28Z

Tests added.

This is all looking good, but I can't find the tests you mentioned above?

dialog/file.go

toaster

Looks good!
Just a couple of nits, a question and a suggestion.

dialog/file.go

dialog/fileicon.go

storage/uri_test.go

formatting/punctuation.

okratitan · 2020-05-27T16:38:12Z

Looks good!
Just a couple of nits, a question and a suggestion.

Thanks! I think I resolved all of those points.

dialog/file.go

stuartmscott · 2020-05-27T17:20:14Z

dialog/file.go

 	save     bool
+	callback interface{}


I wonder if we should have openCallback and saveCallback to avoid the type casts later on

I don't mind the type casts later, but perhaps others may have some thought here.

Unresolving this to see if others have an opinion here

I didn't see that this had been reopened, sorry for that.
I agree that it could be improved, it has evolved a little since the first commit and we learned a lot along the way. Let's do a follow-on following discussion in Slack

dialog/fileicon.go

andydotxyz

This approach seems fine, and inkeeping with the rest of the package.
I think this "isNative" is not needed though (comments inline).
One other thought is that the OnClosed is called sometimes before and sometimes after the file picking callback... it should porobably be consistent.

andydotxyz · 2020-05-27T21:35:13Z

dialog/file.go

 			f.win.Hide()
+			if f.file.onClosedCallback != nil {


Should this be conditional on file.callback not being nil?

file.callback is the read/write closer callback ... file.onClosedCallback is the onClosed callbacks ... They are unrelated as far as I can tell

Oh I see you were actually referring to code above. In that space we only hide immediately if there is no file.callback ... so we wouldn't want to call the onClosedCallback until the dialog is closed... in this case when file.callback is not nil, we go through things like validation that may keep the dialog open still - if validation passes we call the Hide() and onClosedCallbacks later in the method...

So long story short - I think this is appropriate.

Thanks for the explanation, but I don't quite follow - there is a return here so I don't see where it falls through for the extra processes?

Oh I see what you mean now sorry

dialog/file.go

Add the ability to filter files in the File Dialog

6d679ae

okratitan requested review from andydotxyz and stuartmscott May 24, 2020 15:44

andydotxyz requested changes May 24, 2020

View reviewed changes

dialog/fileicon.go Outdated Show resolved Hide resolved

dialog/file.go Outdated Show resolved Hide resolved

okratitan added 2 commits May 24, 2020 13:51

Use a separate constructor for file dialog with filters.

586c111

Make NewFileIcon take a fyne.URI instead of a path string.

03d7d80

stuartmscott reviewed May 25, 2020

View reviewed changes

dialog/file.go Outdated Show resolved Hide resolved

stuartmscott reviewed May 25, 2020

View reviewed changes

dialog/file.go Outdated Show resolved Hide resolved

okratitan added 2 commits May 25, 2020 12:17

Move the file dialog to use a New/Show pattern for its api.

92da698

Strip possible parameters off of user provide mimetypes and clean mat…

f42b46b

…ching

andydotxyz requested changes May 25, 2020

View reviewed changes

okratitan and others added 7 commits May 26, 2020 09:44

Add tests for file dialog filtering.

014cb9a

Rename FileDialog to File and Remove Duplication between File and fil…

d2b47ac

…eDialog

Remove dialog suffix and add back in Show... convenience functions.

25dcf5e

Make sure to prepend "file://" to new URIs.

8b60b8f

Fix mimetype file reading to work with uris.

69ea193

Include the fyne test import

eaf2009

Setup the uri test in it's own package and import fyne test

d0c5e6c

Use real image files for dialog test data.

83503d8

toaster reviewed May 27, 2020

View reviewed changes

dialog/file.go Outdated Show resolved Hide resolved

toaster requested changes May 27, 2020

View reviewed changes

Remove extension from return of mimeTypeGet and fix

7881477

formatting/punctuation.

stuartmscott reviewed May 27, 2020

View reviewed changes

okratitan added 2 commits May 27, 2020 13:17

Implement Dialog and rename mimeTypeGet to mimeTypeSplit

8409fcb

Rename label for dismiss text to avoid test failure.

1db0567

andydotxyz requested changes May 27, 2020

View reviewed changes

okratitan added 3 commits May 28, 2020 09:06

Rename mimeTypeSplit to splitMimeType and remove File.nativePicker

177ec83

Move comment to the correct place

a96ec41

Merge branch 'develop' into fileFilter

22685b3

andydotxyz approved these changes May 28, 2020

View reviewed changes

toaster approved these changes May 28, 2020

View reviewed changes

andydotxyz merged commit 3cf392a into fyne-io:develop May 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add the ability to filter files in the File Dialog #1034

Add the ability to filter files in the File Dialog #1034

okratitan commented May 24, 2020 •

edited

okratitan commented May 24, 2020 •

edited

andydotxyz left a comment

andydotxyz left a comment

okratitan commented May 26, 2020

okratitan commented May 26, 2020

andydotxyz commented May 26, 2020

toaster left a comment

okratitan commented May 27, 2020

stuartmscott May 27, 2020

okratitan May 27, 2020

stuartmscott May 28, 2020

andydotxyz May 28, 2020

andydotxyz left a comment

andydotxyz May 27, 2020

okratitan May 28, 2020

okratitan May 28, 2020

andydotxyz May 28, 2020

andydotxyz May 28, 2020

Add the ability to filter files in the File Dialog #1034

Add the ability to filter files in the File Dialog #1034

Conversation

okratitan commented May 24, 2020 • edited

Description:

Checklist:

Where applicable:

okratitan commented May 24, 2020 • edited

andydotxyz left a comment

Choose a reason for hiding this comment

andydotxyz left a comment

Choose a reason for hiding this comment

okratitan commented May 26, 2020

okratitan commented May 26, 2020

andydotxyz commented May 26, 2020

toaster left a comment

Choose a reason for hiding this comment

okratitan commented May 27, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andydotxyz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

okratitan commented May 24, 2020 •

edited

okratitan commented May 24, 2020 •

edited