File dialog locations #1108

charlesdaniels · 2020-06-15T19:54:31Z

Description:

Fixes #821

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.~~ (N/A)

This swaps ~/ and ./ as the default directory. I have also made / the failover if both of those error. That *shouldn't* happen though. Note that / should be safe even on Windows, since NT supports POSIX-style paths. `/` should expand to NTFS's root, which is almost always `C:\`. XXX: might be worth setting the failover dir in the platform driver. `/` is probably not a good failover choice for iOS, although I'm not sure the file dialog even makes sense there, since they have their own system one? I'm not an iOS dev, so I'm not sure.

This adds the concept of a "starting directory", which may be set with a new setter method. If there is a problem with this directory, or it is an empty string, then the CWD is preferred, followed by the user home, followed by `/`. This also adds two new utility methods, ShowFileOpenAt, and ShowFileSaveAt which allow the starting directory to be explicitly set as arguments. This should not break the existing API.

charlesdaniels · 2020-06-15T20:02:32Z

I'm not sure how best to test this. Based on looking at file_test.go, it seems there wouldn't be a good way to get at the FileDialog object from the perspective of the test case. Even if we could, it there is no way to know what directories might exist on the system running the tests.

Aside from the uncertainty about testing, I think this can be considered for merging, pending any other feedback.

andydotxyz · 2020-06-15T20:25:19Z

The added complexity in the function to show should probably be factored out into a new function. If that was responsible for looking up starting directory that probably also answers how can it be tested :).

One stylistic thought - SetStartingDirectory could be omitted if the startingDirectory field was exported - would that be better or not? :)

andydotxyz · 2020-06-15T20:28:01Z

Im also not sure about the “where” string parameter to ShowFileOpenAt... - it’s not a very descriptive variable name but possibly more of a question is, should it not be a URI?

Jacalz · 2020-06-15T20:35:15Z

I agree with Andy and I think that a better name for the “where” variable might be “startdir” or something along the lines. Apart from that, I think that it all looks really good 👍 Good job and congratulations for making your first contribution to Fyne 🙂🎉

@andydotxyz

Per @andydotxyz I have moved the logic to calculate the starting directory for the file dialog widget to it's own function.

charlesdaniels · 2020-06-15T21:42:13Z

I agree with the proposed changes by @andydotxyz and @Jacalz and have implemented them in db857e1, I will add tests presently. Thank you for the feedback! Let me know if you think of anything else.

charlesdaniels · 2020-06-15T22:00:11Z

Tests are implemented as of 6fbdb2f, please let me know if there is further feedback.

charlesdaniels · 2020-06-15T22:12:21Z

Seems the build is failing do to inconsistent receiver names. To my knowledge, this was not caused by my code changes. I can fix this issue if desired, but I think it is out of scope for this PR.

andydotxyz · 2020-06-15T22:14:45Z

It looks like it is your new method - you used the reciever file instead of f.
An odd test failure can be chalked up to random - but here 3/4 of them are failing, probably indicates taking a closer look ;)

andydotxyz

Apart from the test failures this is good stuff. However I think that file paths are not cross-platform enough so may need to be URI for stating location

andydotxyz · 2020-06-15T23:02:59Z

dialog/file.go

+	parent            fyne.Window
+	dialog            *fileDialog
+	dismissText       string
+	StartingDirectory string


I think we need a URI based approach really, paths may not work across platforms

charlesdaniels · 2020-06-16T12:41:36Z

This is blocked while we determine how best to handle a URI based approach as @andydotxyz mentioned. I have opened #1111 to merge just changing the start directory to ./ for now.

This implements just the effectiveStartingDir() and a basic test from PR fyne-io#1108. It does provide the ability to set a custom starting dir.

andydotxyz · 2020-06-16T15:50:22Z

It is blocked I agree - but probably worth fixing the reciever name and probably rebasing on top of the merged #1111 in preparation :)

This implements just the effectiveStartingDir() and a basic test from PR fyne-io#1108. It does provide the ability to set a custom starting dir. Cherry picked from fyne-io#1111 to fyne-io#1108 to avoid future merge conflicts.

charlesdaniels · 2020-06-16T21:12:10Z

I have "rebased" (this was simple enough I just cherry picked) on top of the work one in #1111.

andydotxyz · 2020-06-17T17:25:56Z

Strange, that Windows failure was a keyboard test. I have restarted it to see if the issue is persistent.

andydotxyz · 2020-06-21T12:02:31Z

Woops, this should be off develop, I think you can just edit the PR, not certain...

We await the full solution in PR #1108

andydotxyz · 2020-07-17T09:29:31Z

Just to put some thoughts here from a discussion we had.

Clearly we want to be using URIs so to support the same cross-platform strategies that opening/saving files does
If the system is using URIs for location it realistically can't rely on file system lookups for getting a directory listing
Therefore we probably need to convert the file dialog to use URI functionality only internnaly, which leads to the challenge that we cannot list URIs currently.

Proposal / suggestion (This was my takeaway from our discussion - I know there were alternatives posed - this is to keep the discussion going).

Create a new type that is able to list children from a given URI:

type ListableURI interface {
    URI
    ListURIs() []URI
}

OK the naming probably wants some thought, but using this approach we could identify "directories" and list them like so:

dir, isDir := myURI.(fyne.ListableURI)
...
for _, child := range dir.ListURIs() {
...
}

And the new Location field of the FileDialog could be of type fyne.ListableURI.

andydotxyz · 2020-09-21T13:48:17Z

Yes, the URIs should never have windows separators in them - a file URI needs to be something that we can share irrespective of operating system.

Of course the varying locations of and is a fun challenge for the future :)...

charlesdaniels · 2020-09-21T22:35:55Z

@PucklaMotzer09

Thanks for checking this out. However as @andydotxyz the code I thought was broken was actually a red herring -- we need to make sure that any Windows-style paths get correctly changed to use / everywhere they are ingested, a change that I plan to implement.

Your PR looks good, but I was actually planning to move this inside of NewURI. I will merge it and make the needed tweaks on my end.

Thanks!

Fix some issues related to ListableURI and dialog

andydotxyz

Great work and really close.
A couple of things inline about tests mostly.
Also some windows code that looks like it can be removed and we're forcing file:// for filedialog but is that really needed now?

andydotxyz · 2020-09-24T09:32:00Z

storage/uri_test.go

+
+	p, _ = storage.Child(storage.NewURI("file:///foo/bar/"), "baz")
+	assert.Equal(t, "file:///foo/bar/baz", p.String())
+}


Can you add a child test for windows as well - it should behave the same obviously, but it's best to check given the OS specific code elsewhere

Shouldn't URIs always have / instead of \? If so then this test should work for all platforms.

I added a test that uses an URI with \ as input.

Yes, the slashes should always be the same, but I thought it was worth testing with the file://C:/ prefix version for windows

andydotxyz · 2020-09-24T09:33:19Z

storage/uri_test.go

+	if runtime.GOOS == "windows" {
+		// Only the Windows version of filepath will know how to handle
+		// backslashes.
+		parent, err = storage.Parent(storage.NewURI("file://C:\\foo\\bar\\baz\\"))


It's not clear here that storage.NewURI is doing the path conversion rather than parent...
Could it be split into two checks so that the result of NewURI is a correct URI, and then that is passed to the parent function?

I seperated it.

andydotxyz · 2020-09-24T11:03:45Z

storage/uri.go

-	// trim the scheme (and +1 for the :)
-	s = s[len(u.Scheme())+1:]
+	// trim the scheme
+	s = s[len(u.Scheme())+3:]


Why +3? the comment about the +1 has gone so it's not clear?
If it's for :// then beware that the "//" is optional in URI spec

beware that the "//" is optional in URI spec

We are using len(Scheme())+3 everywhere. There should be some functions like Path() which returns only the path of an URI to be complient with that.

But this code is improving the very way that URIs are being parsed and created - so we should be trying to improve rather than replicating the same issues in the previous implementations :).

The reason that Path() was not added immediately iis that if people thought this was just about files then they might do os.File operations on the result of URI.Path(), which is often not going to work.
In the future we can add it, once we're cleanly using abstractions in our own code. But this is a breaking change as the URI interface is public so it would not happen before 2.0.

What about path() for internal use?

I have pitched the idea of having a standardized method for retrieving the parts of the URI that aren't the scheme in the past. I still think it would be a good ideas as @PucklaMotzer09 mentioned, even if it was only for internal use. We could also put it in storage like Parent for now, if we wanted it to be public.

andydotxyz · 2020-09-24T11:04:39Z

storage/uri.go

+		if s[len(s)-1] == '\\' {
+			s = s[0 : len(s)-1]
+		}
+	}


Should this not be in an else? Would a windows path with / at the end expect to have it truncated?

I removed the check for windows since all URIs now have /

Yes, I think a trailing \ is no longer relevant if we are normalizing everything to /.

andydotxyz · 2020-09-24T11:07:04Z

storage/uri.go

+	components := strings.Split(location, "/")
+
+	if len(components) == 1 {
+		// trimmed <= 2 makes sure we handle UNIX-style paths on


This comment seems out of date somehow?

And if this method is explicitly for handling non-file URIs why is windows path handling in here?

I removed the comment

Is that the right fix? If a developer thought a comment was needed then surely there is a wider issue at play?

I thought the same, but I couldn't think of a proper comment, since I don't even know why the comment was there in the first place.

Yeah, I think this is not a relevant comment anymore. This was part of the logic for handling Windows drive letters.

andydotxyz · 2020-09-24T11:19:28Z

storage/uri.go

@@ -59,53 +61,131 @@ func (u *uri) String() string {
 	return u.raw
 }

+// parentGeneric is a generic function that returns the last element of a
+// path after splitting it on "/". It should be suitable for most URIs.
+func parentGeneric(location string) (string, error) {


It seems like this has not got tests because we're testing only the file version

I added multiple tests for this functions

Yeah, I never got around to testing it explicitly after I switched to using filepath. Thank you @PucklaMotzer09

andydotxyz · 2020-09-24T11:21:28Z

dialog/file_test.go

 	assert.False(t, open.Disabled())

 	test.Tap(open)
 	assert.Nil(t, win.Canvas().Overlays().Top())
 	assert.Nil(t, openErr)
-	assert.Equal(t, "file://"+target.path, chosen.URI().String())
+
+	targetLocationString := target.location.String()


This should not be needed as you are comparing a URI with a URI

I changed it back since now all URIs just have /

andydotxyz · 2020-09-24T11:22:56Z

dialog/file.go

+
+			// On windows replace '\\' with '/'
+			if runtime.GOOS == "windows" {
+				pathString := path.String()


path is now a URI so this should not be needed

I changed it back

andydotxyz · 2020-09-24T11:24:59Z

dialog/file.go

+func (f *fileDialog) loadFavorites() ([]fyne.CanvasObject, error) {
+	osHome, err := os.UserHomeDir()
+	if err != nil {
+		return nil, err


For this and all those below I don't think we should bomb out if one fails - let's add the favourites that we can and return the last error along with what we build...

I rewrote the function. Now it always runs until the end and returns the last error.

andydotxyz · 2020-09-24T11:27:09Z

dialog/file.go

-	buildDir := filepath.VolumeName(dir)
-	for i, d := range strings.Split(dir, string(filepath.Separator)) {
+
+	if dir.Scheme() != "file" {


Is this required? it seems like the dialog is abstracted now...

I removed the check. StartingLocation is ignored if it doesn't have file as scheme.

Why is it ignored? can we not allow nfs:// or ftp:// URIs in the directory picker?

can we not allow nfs:// or ftp:// URIs in the directory picker?

I don't know. That's just the way @charlesdaniels implemented it.

I wrote it that way since we don't actually have support for those things yet, and at the time I was under the impression that it would just cause it to break later. That might not be the case anymore, so I think we can remove this check if others are under the impression it won't break.

I don't remember specifically setting it to ignore StartingLocation with non-file schemes. That was probably an oversight on my part, or again I was assuming it would break later and wanted to catch the error early. I should have commented it better.

In effectiveStartingDir() the scheme gets checked against file and if it doesn't match the function just skips to PWD and HomeDir.

I don't think we should be enforcing the scheme type - if the StartingLocation is a valid ListableURI then we should use it.

Jacalz

Mostly a commented out callback that I think might be a mistake and the change to default starting location being PWD. I think we agreed on HomeDir being default as it makes more sense for users and that PWD mostly makes more sense to developers.

Jacalz · 2020-09-24T12:35:42Z

dialog/file.go

@@ -97,11 +114,11 @@ func (f *fileDialog) makeUI() fyne.CanvasObject {
 				func(ok bool) {
 					f.win.Hide()
 					if !ok {
-						callback(nil, nil)
+						// callback(nil, nil)


Is this supposed to be commented out?

Probably not. This wasn't really ready for review, I was probably testing something.

Jacalz · 2020-09-24T12:42:38Z

dialog/file.go

+	}
+
+	// Try to get ./
+	wd, err := os.Getwd()


My view is still that HomeDir() should be preferred over PWD. I think that this also was what we agreed on when talking about this in earlier discussions because PWD makes more sense for the developer than what it does for a user. A user of the application would probably be more happy to have Home than PWD.

Just a comment from a user.. When an app is launched from an icon, it isn't obvious what PWD would be. This may be as simple as it being documented somewhere but being able to rely on $HOME would be straight forward.

My proposal would be to have HomeDir the default and then developers wanting PWD could just put in a temporary different starting directory now that we, with this PR, have support for changing starting directory.

This was a debate we had previously in #1111, at which point we decided that ./ would be the best starting location.

We discussed this at today’s call and we (@stuartmscott, @andydotxyz and me) concluded that HomeDir should be the default as it is “the least likely to surprise the user” and having PWD can easily be toggled with few lines of code now that this work has landed.

Well if that was the decision.

Honestly, I think there has been enough contention on this point that maybe we should consider making it a preference the user can set? I find it infuriating that a few programs I use regularly open ~ rather than my working directory, since I then have to nagivate through many levels of file hierarchy.

A compromise might be to add ./ to "favorites"?

Yeah. I think adding PWD to favorites when PWD != HomeDir would be a good addition.

stuartmscott · 2020-09-24T15:07:21Z

@charlesdaniels consider using https://golang.org/pkg/strings/#TrimPrefix to remove the scheme:// instead of slicing. It may require a few invocations (1 to remove the scheme, 1 to remove the colon, and 1 to remove the optional double forward slash) but I think it could improve readability.

charlesdaniels · 2020-09-24T19:35:52Z

Most of the comments raised here seem reasonable, although this wasn't ready to review yet (sorry, I should have set it back to draft when the Windows problems cropped up). Many of these were things I had intended to tidy up. I will look through in more detail when I get a chance.

PucklaJ · 2020-09-25T10:04:57Z

@charlesdaniels Could you give me write access to your fork so that I can directly commit to it.

PucklaJ · 2020-09-25T12:34:02Z

I made some changes, but I want to directly push them to this fork. You can look my changes here

andydotxyz · 2020-09-25T17:15:45Z

Thanks for all the contributions @PucklaMotzer09, it's a great help.
I think that most developers around here use their own forks as only write access for themselves - the only collaborative spaces are really the upstream develop and occasional feature branches.
I know that @charlesdaniels is very busy this week but I am sure he will pull your changes into his branch soon so the result will be the same as if you'd written directly.
We're still a few weeks away from the proposed time of our next release so there is plenty of opportunity to get this all wrapped up in plenty of time :).

charlesdaniels · 2020-09-25T19:06:10Z

As Andy said, thanks for your help @PucklaMotzer09. I would be happy to review your changes and will probably merge them. Is there some reason you can't just open a PR against my fork, as you did before? That would make it a lot easier to review and comment on the changes.

PucklaJ · 2020-09-25T20:25:04Z

Okay I'll just open a PR. I just thought It would be easier to have write access. Because it would let me commit faster if there were still some changes I should make. But a PR is also fine.

File dialog locations

charlesdaniels · 2020-10-08T20:08:36Z

@andydotxyz per our Slack discussion, I think this is ready to turn over:

As far as I am aware, it should not error for non-file:// URIs, though it may not work. (e.g. startdir is ignored without error if it is non-file://. I think @PucklaMotzer09 addressed this in his changes.
I re-enabled the commented out code that @Jacalz noted.
I have reverted the default starting directory to be ~ rather than . (I would like to note for the record that I oppose this change, and think the start directory should continue to be the CWD, but the consensus is against me here).
I have removed the "open at" type constructors.
I have also modified the TestShowFileOpen test to explicitly assert that the first element of the path bar is /, which I think is the intended behavior, but I would like someone else double-check the changes I made to it in 5b5c3a3.

andydotxyz · 2020-10-09T13:45:06Z

I have also modified the TestShowFileOpen test to explicitly assert that the first element of the path bar is /, which I think is the intended behavior

That is correct on Unix :) (just not Windows)

andydotxyz · 2020-10-09T15:45:01Z

Thanks - really, really close. Replaced by #1379 where we will complete it.

charlesdaniels added 3 commits June 15, 2020 15:30

fixed linter errors

c5561ca

file dialog: break out effectiveStartingDir

db857e1

Per @andydotxyz I have moved the logic to calculate the starting directory for the file dialog widget to it's own function.

add tests for effectiveStartingDir()

6fbdb2f

charlesdaniels changed the title ~~WiP: File dialog locations~~ File dialog locations Jun 15, 2020

charlesdaniels marked this pull request as ready for review June 15, 2020 21:59

andydotxyz requested changes Jun 15, 2020

View reviewed changes

charlesdaniels mentioned this pull request Jun 16, 2020

File dialog wd #1111

Merged

5 tasks

charlesdaniels added a commit to charlesdaniels/fyne that referenced this pull request Jun 16, 2020

break out just effectiveStartingDir

76c6dde

This implements just the effectiveStartingDir() and a basic test from PR fyne-io#1108. It does provide the ability to set a custom starting dir.

break out just effectiveStartingDir

be9c018

This implements just the effectiveStartingDir() and a basic test from PR fyne-io#1108. It does provide the ability to set a custom starting dir. Cherry picked from fyne-io#1111 to fyne-io#1108 to avoid future merge conflicts.

Merge branch 'master' into file_dialog_locations

a20d4ed

andydotxyz changed the base branch from master to develop June 28, 2020 18:26

andydotxyz added a commit that referenced this pull request Jun 28, 2020

Reverting PWD change as discussed

3f05efa

We await the full solution in PR #1108

andydotxyz added this to the 1.4 - List, Table, Animations and Input Validation milestone Jul 28, 2020

charlesdaniels mentioned this pull request Aug 2, 2020

ListableURI interface & Initial Implementation thereof #1233

Merged

5 tasks

Fix save and open dialog tests

9c055c7

Merge pull request #1 from PucklaMotzer09/file_dialog_locations_fixes

f3e0660

Fix some issues related to ListableURI and dialog

andydotxyz requested changes Sep 24, 2020

View reviewed changes

Jacalz requested changes Sep 24, 2020

View reviewed changes

PucklaJ added 6 commits September 25, 2020 12:31

Replace backslashes in NewURI

347e49a

Add tests for parentGeneric

5681882

Improve tests and always end Parent with slash

60a46e4

Don't return as soon as one fails in loadFavorites

b7a6dd8

Update breadcrumbs creation and add test for them

20cbea5

Remove checking for "file" scheme in setDirectory

dc79d37

Fix staticcheck

d5ce136

PucklaJ mentioned this pull request Sep 25, 2020

File dialog locations charlesdaniels/fyne#2

Merged

charlesdaniels and others added 4 commits October 6, 2020 15:52

Merge pull request #2 from PucklaMotzer09/file_dialog_locations

ac81a66

File dialog locations

revert starting dir to ~, remove OpenAt functions

45233fe

re-enable overwrite callback

ae3b91f

update file dialog tests for ~ starting dir

5b5c3a3

andydotxyz mentioned this pull request Oct 9, 2020

File dialog locations #1379

Merged

4 tasks

andydotxyz closed this Oct 9, 2020

File dialog locations #1108

File dialog locations #1108

Conversation

charlesdaniels commented Jun 15, 2020 • edited

Description:

Checklist:

Where applicable:

charlesdaniels commented Jun 15, 2020

andydotxyz commented Jun 15, 2020

andydotxyz commented Jun 15, 2020

Jacalz commented Jun 15, 2020 • edited

charlesdaniels commented Jun 15, 2020

charlesdaniels commented Jun 15, 2020

charlesdaniels commented Jun 15, 2020

andydotxyz commented Jun 15, 2020

andydotxyz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charlesdaniels commented Jun 16, 2020

andydotxyz commented Jun 16, 2020

charlesdaniels commented Jun 16, 2020

andydotxyz commented Jun 17, 2020

andydotxyz commented Jun 21, 2020

andydotxyz commented Jul 17, 2020

andydotxyz commented Sep 21, 2020

charlesdaniels commented Sep 21, 2020

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

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

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

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

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

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

Choose a reason for hiding this comment

Jacalz 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jacalz Sep 25, 2020 • edited

Choose a reason for hiding this comment

stuartmscott commented Sep 24, 2020

charlesdaniels commented Sep 24, 2020

PucklaJ commented Sep 25, 2020

PucklaJ commented Sep 25, 2020 • edited

charlesdaniels commented Jun 15, 2020 •

edited

Jacalz commented Jun 15, 2020 •

edited

Jacalz Sep 25, 2020 •

edited

PucklaJ commented Sep 25, 2020 •

edited