Add Desktop directory to file dialogs

[codesoap]

I believe that the "Desktop" directory is heavily used by many users. Thus I think it would be convenient, if it appeared in the favorites of file dialogs.

The changes are basically pure Cargo cult, but seem sensible to me and seem to work. Please correct me, if I've forgotten something.

I have not assigned an icon to this entry in the file dialogs, since the most common symbol seems to be a monitor and I couldn't find such a symbol in the theme package.

[coveralls]

coverage: 64.518% (-0.009%) from 64.527%
when pulling 55cd42c on codesoap:desktop
into 4d32cfc on fyne-io:develop.

[andydotxyz]

Thanks for this, but can you add it for Windows and macOS as well? We keep feature parity between all the systems (and file dialog is only shown on desktop)

[codesoap]

I'm afraid I don't see what is missing. I had grepped for the term "Videos" as a guidance for my change and found no other relevant places like this.

The getFavoriteLocations() functions in dialog/file_darwin.go and dialog/file_windows.go both seem to use the favorites from the getFavoriteOrder() function, which I adapted.

I had tried my change on Linux and on Windows and the "Desktop" favorite was available on both.

[andydotxyz]

I see that the locations are looked up, good point.
But I think the getFavouriteIcons is missing an understanding of Desktop - so this new item does not match the others.

[codesoap]

You're right, I didn't assign an icon. The most common symbol for the desktop seems to be a monitor and I couldn't find that in the theme package. What is the preferred way; should I try to find a different icon, that already exists in the theme package, or should a new icon be added?

[andydotxyz]

should I try to find a different icon, that already exists in the theme package, or should a new icon be added?

If you find one that works use it, if not you can add a new one :).
There is a good repository of material icons that will fit right in (see how we embed the current ones to add a new).

[codesoap]

I couldn't find a suitable icon, so I added one (well, actually two). After some research it seemed to me, that (at least some of) the existing symbols where taken from Polymer (source I found during research). Since Polymer allows use under BSD style licenses and fyne uses a BSD style license, I saw no problem in copying the icons.

Polymer had actually defined two separate icons for the desktop: desktop-mac and desktop-windows. I kept this decision and added the two icons to fyne.

go run ./theme/gen.go did generate a change for fynelogo, but I didn't commit it here, since it has nothing to do with this issue.

I have speculated, that this change will be released with fyne 2.5 and added according "Since" comments to IconNameDesktopMac and IconNameDesktopWindows.

[andydotxyz]

Nice improvement thanks. But is having two different icons for desktops needed? I'm in 2 minds and curious about @Jacalz, @dweymouth or @Bluebugs thoughts

[Jacalz]

I think the can just have the Windows one and call it just "Desktop". The difference is not significant enough to warrant two icons in my opinion

[codesoap]

No complaints from me. I have unified the icons.

[andydotxyz]

Please don't force push a PR that is going through reviews - in general we prefer to keep history.

[Jacalz]

All of the tests are failing. Would you mind having a look at that?
I also think you need to update the test package lookup functions for the renderer markup output.

[codesoap]

I took a closer look and found that 3 fails are due to timeouts and one is unhappy about the use of theme.DarkTheme. I feel like these are not problems I created with this PR.

Am I missing something? I'm afraid I'm not very experienced with interpreting the errors...

[Jacalz]

Please don't request a re-review without committing changes. I still have outstanding comments requests from my latest review and I haven't had time to answer your comment yet.

About the tests, yes there are unfortunately a few spurious failing tests on develop. However, all tests should not be failing as they are here. I haven't had time to look into it any further.

As noted in my previous review, there is also the case about needing to update the test package to know about the new icon.

[Jacalz]

The test failures may not be your fault but I haven't seen them all fail like this. It is strange. Will try to look at it when I can.

[andydotxyz]

It looks like a deadlock in theme lookup code - my guess would be that the test themes are not bringing along the required features that have been added.
This should happen locally if you just run "go test ./...".

[codesoap]

I'm afraid I cannot reproduce the problem locally. I only get a test failure in the TestWindow_CaptureTypedShortcut test case, but that one also fails on master for me. The tests in fyne.io/fyne/v2/dialog and fyne.io/fyne/v2/theme do not fail on my system.

[andydotxyz]

I cannot replicate but also cannot seem to kick the CI to try again (not sure why but these workflows seem frozen?).
Is it possible to try at your end, or push an empty commit that will trigger it @codesoap ?

[Jacalz]

Rebase on latest develop might fix things maybe?

[codesoap]

I have performed the suggested rebase and the tests don't fail anymore. Thanks!

[Jacalz]

Thanks. Looks better now. You just need to teach the test package about this new icon. See

fyne/test/markup_renderer.go

Line 413 in d8e9fe0

func knownResource(rsc fyne.Resource) string {

.

[codesoap]

I have added the requested entry in the test package, although I must admit that I don't understand what for.

[andydotxyz]

although I must admit that I don't understand what for.

The test package runs the apps when in unit test mode. Without it code that used that icon would not function correctly.

[Jacalz]

Sorry for the late review. Thanks for the contribution.

[codesoap]

Thanks for your thorough reviews!