fyne-io / fyne

Cross platform GUI toolkit in Go inspired by Material Design
https://fyne.io/
Other
25.25k stars 1.4k forks source link

FileDialog does not handle relative file URIs well. #5234

Closed steampoweredtaco closed 4 days ago

steampoweredtaco commented 3 weeks ago

Checklist

Describe the bug

When using a relative directory to file dialog's SetLocation method and it is a relative directory it will not produce any contents. If you select the listing icon then files will show, but not the breadcrumb buttons. You can then click grid again to see the files, but again no breadcrumb buttons.

How to reproduce

Simply create a file dialog and set the storage uri to a relative directory that exists to the running working directory of the program. Then click the list/grid buttons to see behaviors as described.

Screenshots

This is an example of what the bug looks like: ___1go_build_ttstick_runtimes_traceReader_x1Wtl3O6WC

Gif showing that the file dialog still sorta works when you hit the list/grid buttons after but due to the nature of this issue the breadcrumb buttons will not be available: ___go_build_ttstick_runtimes_traceReader_S6exROEjpN

The workaround is simple of course, first make sure the directory is an absolute directory before setting the location: ___go_build_ttstick_runtimes_traceReader_t4d10mk6Lc

Example code

Of course make sure ./logs exist with some files from the runtime directory when the program starts for this example to work.

package main

import (
    "context"
    "fyne.io/fyne/v2"
    "fyne.io/fyne/v2/app"
    "fyne.io/fyne/v2/dialog"
    "fyne.io/fyne/v2/storage"
)

func main() {
    _, cancel := context.WithCancel(context.Background())
    a := app.New()
    w := a.NewWindow("Trace Reader")
    w.Resize(fyne.NewSize(800, 600))
    w.SetOnClosed(cancel)

    var fileOpener *dialog.FileDialog
    fileOpener = dialog.NewFileOpen(func(reader fyne.URIReadCloser, err error) {
        if err != nil {
            dialog.ShowError(err, w)
        }
    }, w)

    p := "./logs"
    // Workaround is as follows
    // p, err := filepath.Abs(p)
    // if err != nil {
    //  panic(err)
    // }
    u := storage.NewFileURI(p)
    d, err := storage.ListerForURI(u)
    if err != nil {
        panic(err)
    }
    fileOpener.SetLocation(d)
    fileOpener.Show()
    w.SetMaster()
    w.ShowAndRun()
}

Fyne version

v2.5.2

Go compiler version

1.23.1

Operating system and version

Windows 10

Additional Information

I tracked down this issue to the code that creates the breadcrumbs loop /fyne/dialog/file.go:468 image This will take the location "./logs" and end up parsing it to "/" then "/logs" which does not exist. so the isDir checks fail.

Although the workaround is simple, the fact fyne doesn't return an error wit SetLocation there is no way from a client code perspective to know there was a problem at runtime (setLocation does return an error, but SetLocation does not).

I think if an error was returned from SetLocation stating it does not support relative file uri locations that would of been fine.

I suspect this should work though instead of throwing an error, but maybe file dialog needs a way to recognize if the storage provider url support an absolute function of some sort and use that before creating the bread crumbs?

Also, given the way the bread crumbs are made, I suspect this bug hits in other scenarios like when the user has permissions to list in the directory they opened and navigated in but in the parent paths they do not have listing permissions which is a possible edge case in some OS's. That kind of issue would manifest in the same way. It might make more sense if the breadcrumbs were made from the current directory up to parents instead of starting with the root parent; in this way a partial bread crumb could be listed at all times, even if it is only for the current directory, up until the path breaks.

andydotxyz commented 3 weeks ago

We do not need to add support for checking a URI for absolute. A URI must be absolute, there is no support for relative in the way URI is parsed or handled.

It may be a useful improvement for the "NewFileURI" to ensure the path is absolute before passing it to the URI code.

andydotxyz commented 3 weeks ago

The change you suggest to breadcrumbs is already underway in an open PR.

steampoweredtaco commented 3 weeks ago

We do not need to add support for checking a URI for absolute. A URI must be absolute, there is no support for relative in the way URI is parsed or handled.

It may be a useful improvement for the "NewFileURI" to ensure the path is absolute before passing it to the URI code.

Yes, if the intention for fyne all storage path URIs are absolute it would make sense if new file uri normalized it to be absolute to the working directory for Windows, not sure the answer for other platforms. That, or throwing an error, would of saved me time.

The interface doc probably should also mention that this normalization to absolute even if the URI was created with a relative path must happen if components in fyne will require it at all times. (Probably overkill to make a new AbsoluteUri interface/method).

Feel free to change the title of this issue to better reflect this is more of a bug related to NewFileUri or close it in favor of a new issue if you want.

Thanks!

andydotxyz commented 3 weeks ago

I'm not sure we are quite on the same page. A URI is always absolute, it is the parsing of file path into URI where the relative aspect can be. Thats why I felt this probably refers to the parsing code in NewFileURI. I don't think it needs any changes elsewhere, or new documentation, because the URI related APIs are all correct.

steampoweredtaco commented 3 weeks ago

I agree that that upon creation of the URI is where it should be resolved. I think we are on the same page there.

The documentation I am referring to for the interface of URI. Just as NewFileUri provided an implementation that did not resolve relative paths when returning the URI for usage, someone else implementing the interface may make the same mistake..I was thinking it was worth calling out is all.

andydotxyz commented 3 weeks ago

I think that PR resolves the issue. For sure it at least fixes the parsing of relative file paths.

steampoweredtaco commented 3 weeks ago

I think that PR resolves the issue. For sure it at least fixes the parsing of relative file paths.

Yes it does! It prevents the gotchya with FileDialog which should never get a non relative URI in the first place and the documentation is now clear. Thank you.

andydotxyz commented 4 days ago

It's on develop and will be in v2.5.3