Correct paths for favorites in file dialog

[PucklaJ]

Description:

Uses correct paths for favorite locations for different languages on Linux.
It uses the xdg-user-dir command to determine the location.

Fixes #1248

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.

[PucklaJ]

I want to add a test for the favorites, but they are inside a widget.Group. This struct does not export their children, so to test the favorites correctly, the children need to somehow be exported.

[PucklaJ]

I added a test by just using the slice returned by loadFavorites.

[andydotxyz]

Thanks for this. We would need a different approach for darwin as it also is not XDG.
More problematic is that my linux computer does not include the xdg-user-dir command (arch linux).
I read that it functions by finding a user-dirs.dirs file, but that does not exist either.

How universal is this feature? If it is fairly common then maybe we can go forward but with sensible fallbacks for other Unix systems?

[PucklaJ]

This will work with any linux distribution that has a DE which follow the freedesktop specification, which means: KDE, Gnome, XFCE and many more(exactly like the solution already posted by the OP).

Accordording to the first answer.

sensible fallbacks for other Unix systems

If the command fails an error is returned and the default one (eg. $HOME/Documents) is used.

We would need a different approach for darwin as it also is not XDG

According to this post these folders are handled similiar to Windows and just show the folder in a different language, but on the file system it is language independent.

[andydotxyz]

Apologies I misread the code when it comes to the actual lookup.
When it's chunks of code that are replaced based on OS we usually use different files for each variant (the app package has a better example) it's a little easier to read when the linux specific code is in a linux file, though in this case it's XDG rather than linux specifically...

It is unusual for DEs to install these utility executables on some systems - you can see in Arch that this is not a requirement of any of the desktops you listed: https://www.archlinux.org/packages/extra/x86_64/xdg-user-dirs/

[andydotxyz]

A more robust approach might be to use exec.LookPath to see if xdg-user-dirs is installed. If not then printing lots of errors is not really required.

[PucklaJ]

When it's chunks of code that are replaced based on OS we usually use different files for each variant

I actually wanted to do that, but I would have had to copy a lot of functions from file_other.go to file_linux.go.

[andydotxyz]

but I would have had to copy a lot of functions

I get your point - but there were basically 15 lines of code in 4 small functions.
That file is now 45 linux/xdg specific lines to avoid the small duplication...

[PucklaJ]

You got a point. I'll figure something out.

[andydotxyz]

getFavoriteLocations is not defined for non-linux unix systems like BSD.
You might see that in the app package we use the _xdg extension and use the build rule:

// +build linux openbsd freebsd netbsd
// +build !android

alternatively a file_bsd.go would work, if you think the behavior for BSD and Linux is different.

[andydotxyz]

Thanks. I just checked on BSD (Not on the travis run) and it's happy now :)

[andydotxyz]

Please don't force push after a review, it results in the following un-usable change set:

And so PRs must be re-reviewed in full...

[andydotxyz]

Would it not be better to assert that the correct directory is now set?

[PucklaJ]

I don't know the exact name of the directory since it depends on the username and language.

[andydotxyz]

I would have thought it existed in the map returned by favouriteLocations()?
I understand that it might not be a trivial lookup/comparison - but by not doing it you run the risk that this is simply setting the un-localised path which would be a failure and yet this test would pass...

[PucklaJ]

Yes, the paths are inside the map, but using those would be like checking strings against themselves. The unlocalised default paths are used if the xdg-user-dir command doesn't exist or if some other error occurs. The default paths are also used if the xdg-user-dir command returns the home directory. This happens when eg. the documents folder doesn't exist. This is the case for the CI tests.

[andydotxyz]

But this test is testing what happens when you tap the button.
To check that the favourites are created correctly is the remit of another test (one that does not create a file dialog UII).
Testing the dialog's new location against the string representing the location that should have been loaded when a button was pressed seems quite correct to me.
If your concern is that the localisation is not tested if we do that then yes another test can be added. But by ignoring the contents of the string I don't see that anything is being added.

[PucklaJ]

I changed it so that dlg.dialog.dir is checked against getFavoriteLocations.

[PucklaJ]

To check that the favourites are created correctly is the remit of another test

What test is this exactly? I couldn't find a test which is testing the favorites.

[andydotxyz]

I did not mean that it existed, I was more hinting that tests should test one thing, so maybe a test is missing?

[andydotxyz]

I think that this would be wrapped up nicely if we could add a test of the file_xdg contents - do you think that is possible? (clearly they would only run on linux :) )

[PucklaJ]

I did my best, but it is difficult to test this since the paths depend on the languange and sometimes don't even exist.

[andydotxyz]

Thanks for this update, a positive tests seems more complete.
Also good catch on the drive buttons, I had forgotten about them!

[andydotxyz]

I might have misunderstood the new tests, but hopefully my comments help

[andydotxyz]

I don't understand this test sorry.
It seems to iterate expected directories and confirms that they end with the expected string.
But the XDG code exists to make this configurable - so as soon as someone has that set these will fail won't they?

The purpose of testing the favourite location lookup would be to test the wrapper around xdg-user-dir I would think, which is based on environment variables, so can we manipualte them to test the code is working?

[PucklaJ]

The way it works is that xdg-user-dirs-update is run very early in the login phase. This program reads a configuration file, and a set of default directories. It then creates localized versions of these directories in the users home directory and sets up a config file in $(XDG_CONFIG_HOME)/user-dirs.dirs (XDG_CONFIG_HOME defaults to ~/.config) that applications can read to find these directories.

This is from here.

So it's not really about environment variables, but more about a config file. I think this is too "hacky" for me so I will leave it at that.

so as soon as someone has that set these will fail won't they?

The paths are only getting checked against those strings when the folders don't exist. So this test is not testing wether getFavoriteLocations returns the correct paths. It is only checking if the fallback paths are correct.

[andydotxyz]

Ah I understand thanks. I was struggling to follow the code - I played with it a bit, maybe some of these changes can help? Let me know what you think.

func TestFavoriteLocations(t *testing.T) {
	favoriteLocations, _ := getFavoriteLocations()

	// This is a map containing the base name
	// of every key that should be in favoriteLocations
	expected := map[string]string{
		"Home":      "",
		"Documents": "Documents",
		"Downloads": "Downloads",
	}

	homeDir, err := os.UserHomeDir()
	assert.Nil(t, err)

	for name, subdir := range expected {
		fav, ok := favoriteLocations[name]
		if !ok {
			t.Errorf("missing favourite location", name)
			continue
		}

		if subdir == "" {
			assert.Equal(t, homeDir, fav)
		} else {
//			assert.Equal(t, homeDir, filepath.Dir(fav)) // I don't think this assertion must be true, the user could specify a different parent directory
			_, err := os.Stat(fav)
			if err == nil {
				continue // folder found
			}
			if !os.IsNotExist(err) {
				t.Errorf("failed to read directory %s", fav)
				continue
			}
			fallbackName := filepath.Base(fav)
			if !strings.EqualFold(subdir, fallbackName) {
				t.Errorf("%s should equal fold with %s", subdir, fallbackName)
			}
		}
	}
}

[PucklaJ]

So the only thing that's different here is that

assert.Equal(t, homeDir, filepath.Dir(fav))

has been removed. Right?

[andydotxyz]

I probably should have said if the XDG tests are proving difficult then don't worry, I can merge this in and complete it myself.
Platform specifics can be a pain.

[PucklaJ]

I rewrote most of the code because of #1379. I used fyne.ListableURI everywhere where it should be used. But this needs a rebase and a force push of course. So just tell me when you are ready for that.

[andydotxyz]

Hi. In general we would prefer a merge into the PR with the conflict resolution commit.
However in this instance I can see that the majority of the code has changed, so go ahead.

[andydotxyz]

All of the Linux builds are failing so I don't think it's random error @PucklaMotzer09

[PucklaJ]

Now that I use fyne.ListableURI every favorite location has to exist. But the folders can be missing on some systems, so I can't really test them now.

[andydotxyz]

Looking at the latest change I see that you've removed an error when the favourite does not exist but also checking that an existing favourite cannnot be loaded.
It seems strange that both would be removed, consider the user experience - the left-panel should show those locations that we have found I think? So that list works from the favourite locations right? Then the end of the test that tries to find a favourite location should surely still pass, because it being in the list means that we found it?

(Apologies if that is not a correct representation of the current code, I was thinking out loud how it maybe should work).
What do you think?

[PucklaJ]

Every favorite location that has been found and exists will be added to the list. Every path that doesn't exist does not appear on the left panel. This happens because getFavoriteLocations now returns fyne.ListableURI and you can only create a ListableURI from an URI if the path exists. I think this behaviour is reasonable, but this makes it difficult to test since every system can have different paths or no paths at all.

[andydotxyz]

I think that sounds great, but if that is the case then why did the second part of the test need to be removed?
If we are only checking against existing favourite locations then we should be able to test for them...

[andydotxyz]

I think maybe we are not on the same page. I was asking that we re-instate some of the tests that should still pass, but now you have removed more?

[PucklaJ]

If a path doesn't exist then it doesn't get added to the favoriteLocations map. If I would add it to the map than that value would be nil, since I can't create a ListableURI from a non existing path. So the only favorite location we actually can test is Home. There is no need to check if the paths exist since we already know that they do.

[PucklaJ]

but now you have removed more?

I removed them because they were meaningless.

[PucklaJ]

Yes we should think about how to test them properly. The tests just didn't check something meaningful as they were.

[andydotxyz]

I thought the tests were confirming that the custom directory locations did in fact exist.
I appreciate that this may not have been meaningful on Travis, but surely they deserved to remain for the plethora of Linux devices that this is tested on during development?

[PucklaJ]

Yes you are right. I forgot about the other platforms and the drives. I added a check for the existance of the directories. But TestFavoriteLocations is pretty meaningless right now.