ListableURI interface & Initial Implementation thereof

[charlesdaniels]

Description:

Relevant to #1108, this PR addresses the need to create a cross-platform "listable" URI, so that filesystem directories and other analogous things can be represented in a portable fashion.

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.

[charlesdaniels]

Would like commentary from @andydotxyz

One thing I wonder -- why did you make URI be the first instance of ListableURI in your proposed approach, which this sketch mirrors, thus embedding it. This seems inconsistent with the URIReadCloser and URIWriteCloser interfaces. If this was a deliberate decision, I would like to understand the logic.

I also think we could do better naming for CreateChild*URI(), I'll think on that some more.

I am lukewarm on having Writeable(). I think this may not be granular enough to represent all the different possible cases on all possible platforms. I am tempted to remove it, and let developers check if a write failed using the error codes. I think we need to address this though, because file->save dialogs exist.

I would definitely like commentary from people who work with mobile. I really only have UNIX-ey local filesystems as a point of reference, so I may be missing details that someone with more experience in that area would have.

These are the thoughts that come to mind to me after this first initial sketch.

[andydotxyz]

A good start thanks. I agree that Writable may not be right for inclusion - we don’t know what type of operation will be performed and on some systems you only know if it worked by attempting...

I agree the Child naming could be tidied - but also I wonder - does CreateChild even make sense? Some types of resource don’t get created ahead of writing, so all we need to do is support creating a name of a relative URI then writing could create it (as NewFileFromURI should do). So maybe RelativeURI or NewChildURI to mirror the package NewURI?

Which leaves the possibility that ListableURIs need to be created in this way perhaps?

[andydotxyz]

I think embedding URI was intended a list to return plain URIs or new lists, whereas URIReadCloser and URIWriteCloser are handling driver level input/output. Does that help?

[andydotxyz]

Thinking about it we might need something like storage.NewListableURI as well so that developers could do list := storage.NewListableURI("file://"+dirpath)

[charlesdaniels]

Just pushed up a new sketch based on what we've discussed recently.

This tries to crate a clear distinction between a URI as a reference to an abstract resource that presumably exists, and a new concept which I've dubbed URIHandler which defines a set of operations that can be done on a URI. For example, an SSH based URIHandler would "know" how to take an ssh://... URI and convert it to a URIWriteCloser (or throw an appropriate error).

I think maybe rather than in storage/file, it would make sense to have the implementation live in the driver. Perhaps you do something like fyne.CurrentApp().Driver().Handler() and that gives you a URI handler for all the platform-relevant schemes.

The user could also define their own URI handler to support things beyond the purview of the platform.

Maybe we want some kind of shortcut like fyne.DefaultHandler() that grabs the handler from the driver, to reduce verbosity.

RFC @andydotxyz

[andydotxyz]

I don't really understand what URIHandler adds as a public API.
It seems like internal detail that could be used to power the URI provider(s) in storage package.

[charlesdaniels]

The thought with it being a public API is so that downstream developers can define their own URI providers. Someone may want to handle URIs specific to their application or use case. I think it would be best to have that exposed for people that want it.

It would also implement our storage providers.

[andydotxyz]

I can see that as a desired outcome - but with URI being a public interface they can implement it directly - I think.
Perhaps some demo usage of this would help? I guess I'm not following how handlers support the creation of listable URI (the title of this PR).

[charlesdaniels]

It seemed to me like a list-able URI is a type of realization of a URI, much like a readable or writable URI. I thought bundling all the stuff relating to realizing URIs into one interface would make sense.

In principle, I could simply implement the necessary bits to do list-able URIs, and table the handler thing for some future point in time, which is probably better for keeping the size of this PR down, though I do think it's an idea work revisiting in the future.

[andydotxyz]

That all makese sense - but I'm not sure that ListableURI is a realization of URI in the same way that scp:// would be.
A handler would need to realize both types, but we don't have the base impl of listable yet.

[charlesdaniels]

It seems realization-ey to me. Retrieving a directory listing almost always implies a read operation of some kind.

[andydotxyz]

I guess by "realization" I thought you meant the re-hydration we disccussed.
Listable seemed like more of a feature-add on the existing hydration code - but maybe it's just semantics.

Looking more I think we don't have the same design in mind - perhaps this might help understand the differences:

• When I proposed ListableURI I thought it could extend URI
• This means that URI.(ListableURI).List() could work - equivalent of "IsDir"
  vs
• In the handler based design I see that URI has no logic built in and you are using handler.ListerOf(URI)
• in this case there is no need (in fact it's a big distraction) to extend URI, instead "URILister" is probably a better name.

On the face of it both work, but I'm not sure that having to go via a handler is the cleanest API - which is why I wondered if it could be an internal API (or at least not exposed on the happy path).

Another consideration could be can we shorten the APIs and make more available through URI(), for example

storage.NewURI(...).Open() (URIReadCloser, error)
storage.NewURI(...).List() ([]URI, error)

But this is just thinking out loud.
This would basically wrap driver code - as a necessity for current file handling - but could be extended.
For example we could create internal handlers for different types of URIs and explore exposing a public API to extend it more in the future.
Of course nothing stops developers directly implementing URI as long as they can avoid relying on storage.NewURI hydration.

[charlesdaniels]

I've added a working prototype of the list-able URI interface, and removed the URI handler bits that aren't needed. How do we feel about this?

I put it in the GLFW driver, since this feels like something that would be platform specific. Maybe we want some kind of standardized interface for ListableURIForURI that drivers can implement?

I've also added a basic test. Maybe this needs to be expanded a little.

RFC (@andydotxyz )

Edit: also, this does not preclude later adding helper methods in storage as you said that call into the current driver and wrap some of these things. If we do that, maybe ListableURIFromURI is lives in storage and the drivers have private interfaces for this? I think that's a separate discussion from whether or not this POC is in the right general direction or not though.

[andydotxyz]

Looks pretty solid. I guess ListableURIForURI just pops into the Driver interface.
Seems to fit neatly alongside the read/write code.
It is lkely that there will be commonality on different systems, but this abstraction fits - getting listings for "content://" etc in gomobile driver might be fun step.

To move to ListableURI in dialog package we probably don't need the storage API, but if we want to have SetStartLocation(ListableURI) we might need to expose something in storage.

[charlesdaniels]

I think this is ready for review. I decided to adopt the ListerForURI() that @andydotxyz discussed, since it's shorter and makes more sense than the previous naming convention. I also made this part of the driver interface, since each driver may have different sets of URIs it knows how to get listers for. I do not know enough about the mobile back end, so for now it just returns a not implemented error.

I can't figure out from the CI logs why Travis is failing, the tests and linter pass on my end.

[charlesdaniels]

Ahh, I think Travis was failing due to go mod tidy needing to be run. Hopefully that fixed it.

[charlesdaniels]

I have addressed the issues mentioned in today's meeting, this should be good to go.

Also, in case it wasn't obvious, I want to make it really clear that this is a potentially breaking change, in that the URI interface now has the Parent() method. I doubt there are many URI implementations in the wild at this point, and it is pretty simple to add.

[stuartmscott]

Can this be revisited? We probably don't need a full copy of an implementation for testing, and could probably get by with a simple type that saves its parameters and returns predetermined results, both of which can be asserted against.

[andydotxyz]

I think I agree with this. Copying implementation into tests is a bit of a smell - as you're not really testing that code but potentially copying bugs into the test code

[andydotxyz]

I quite like this API, thanks - but there are some fixes to be done to the testing (and potentially windows file URI parse...)

[andydotxyz]

I think I agree with this. Copying implementation into tests is a bit of a smell - as you're not really testing that code but potentially copying bugs into the test code

[charlesdaniels]

I think I agree with this. Copying implementation into tests is a bit of a smell - as you're not really testing that code but potentially copying bugs into the test code.

I only copied it over so that the interface would be satisfied. I don't know that the code in test/testfile.go is actually called?

How would you suggest that I address this @andydotxyz ? I think the code would end up looking very similar anyway, as the operations being implemented aren't exactly complex.

In principle, it would be nice to have a whole system for mocking the filesystem during testing, but I feel like that's a separate concern from what this PR addresses.

[andydotxyz]

I guess it's not a big deal. However the error messages are wrong as they have hard coded file names in that should not really copy over.
Possibly could just use the simpler

	if uri.Scheme() != "file" {
		return nil, fmt.Errorf("unsupported URL protocol")
	}

from the other file checks above?

[charlesdaniels]

I updated the error messages, I agree those shouldn't have been copied over.

Do we want to do something else for test/testfile.go?

[andydotxyz]

I think it’s ok for now, let’s get it landed, tested and iterated. In my last comment I meant to write “I guess it’s not a big deal” updated now, apologies!

[stuartmscott]

Nice work Charles!