Files icon indicating copy to clipboard operation
Files copied to clipboard

Introduce abstract storage layer

Open d2dyno1 opened this issue 3 years ago • 5 comments

Details of Changes This PR continues work on the infrastructure, making the storage layer more broad and flexible. The motivation for this change is to eliminate the convoluted file system code that Files is built on - this requires major changes both in frontend and backend part of the codebase. This work is a stretch goal to ultimately replace all storage calls with Files.Sdk.Storage abstractions. This PR is a work-in-progress.

TODO The following list is limited to this PR and doesn't include future changes

  • [x] Implement FtpFile/Folder
  • [ ] ~~Implement NativeFile/Folder~~
  • [ ] ~~Implement ShellFile/Folde~~
  • [ ] ~~Implement SystemFile/Folder~~
  • [ ] ~~Implement ZipFile/Folder~~

(Crossed out items will be done in following PRs)

Validation How did you test these changes?

  • [x] Built and ran the app

d2dyno1 avatar Jul 11 '22 18:07 d2dyno1

@d2dyno1 We agree on the essentials. I will clarify my vocabulary so we understand the same thing.

Storage. This is a technical layer that uses IStorageItem from uwp. This roughly corresponds to the current Files.Uwp.Filesystem namespace. It is a technical layer but is used at high level by the application (wrongly). Storage uses FullTrust but this is a limitation of uwp. I hadn't planned to use an abstraction for this layer because ListedItem will encapsulate it and it will be used a lot less. However, your suggestion to add an abstraction convinced me. This will allow us to easily replace this layer if necessary, especially if we leave uwp.

Filesystem. This is an abstraction that encapsulates Storage (and libraries such as FluentFTP) to be able to manipulate ftp cloud zip and other system files and folders. It roughly corresponds to ListedItem and operations (copy, rename, ...). Currently, there is no abstraction of operations. The ListedItem abstraction has major flaws and is not standardized. Operations directly use the storage layer, which should be avoided.

Filesystem contains 2 parts. The sdk (public) and the implementation (private, except enumeration services or factory to be able to instantiate the services). I had planned to place these 2 parts in the same project. The separation can be done later if necessary. I had planned to place Storage in the Files.Filesystem project, without abstraction, but encapsulated.

An important point is the breakdown of the application into several projects. I tend to create a lot of projects for an application to have a clear and impossible to violate architecture. I didn't apply it here because there may be team objections to creating multiple projects.

Here is a suggestion of some projects we could create. Files.Sdk.Core (abstraction of basic services such as logger, localization, ...) Files.Sdk.Storage (abstraction) Files.Sdk.Filesystem (abstraction) Files.Shared.Extensions (implementation of generic extensions of string, linq, ...) Files.Backend.Storage (implementation) Files.Backend.Filesystem (implementation)

cinqmilleans avatar Jul 12 '22 15:07 cinqmilleans

Let's keep things simple for now. We'll try to start with this layout in mind:

Files.Sdk.Storage - storage abstractions Files.Uwp.Storage - implementation of those abstractions (satisfying UWP platform limitations)

We can later on decide and separate the ListedItem layer (we want to avoid confusion and have a clear foundation that we can expand upon later).

The current goal is to separate the storage (- ftp, win32, shell, etc.) from the backend (- ViewModels). The abstractions for backend view models are out of scope of this PR 🙂

@cinqmilleans

d2dyno1 avatar Jul 15 '22 11:07 d2dyno1

I've implemented storage abstractions for FTP. We'll need to implement other classes this way as well :)

d2dyno1 avatar Jul 15 '22 13:07 d2dyno1

This is ok for me.

cinqmilleans avatar Jul 16 '22 08:07 cinqmilleans

@yaichenbaum @d2dyno1 @gave92 Here are some ideas about Storage. Storage is the part of the application that manages file systems (whatever they are). You need a Storage backend and a Storage viewModel.

The backend needs an abstraction, many implementations (which may change if we leave uwp), tests, etc. This layer must be self-contained, with no dependencies on user settings or localization. This part is not specific to Files. The viewmodel will present Storage in a more convenient way. It will be independent of the real file system (cloud, ftp, ...) but will use user settings and location. This part is specific to Files. The most important point is therefore the backend. A CommunityToolkit.Storage project is in the works, but we can't afford to wait for it (if it comes to fruition).

I propose to no longer integrate Storage into the Files project. It is a standalone project that could be used by another application (which is not a file manager). We can create a new Repository project "Files Community/Storage". This will contain projects for abstraction, uwp implementations, testing, and its own documentation. This ensures our own independence.

In parallel, we can collaborate with CommunityToolkit.Storage to integrate our own improvements. This with the aim, in the long term, of using it to replace our solution (which allows us to have a head start).

cinqmilleans avatar Jul 30 '22 13:07 cinqmilleans

What bothers me most is the IModifiable interface.

  • The name CreateCopyOfAsync is inconsistent with MoveOfAsync. CopyOfAsync is simpler.
  • It is cleaner to use enums instead of boolean as method arguments.
  • The delete should be on an IModifiableStorage interface and delete the item itself. With the current model, you have to call the parent to delete the child.
  • CreateFileAsync/CreateFolderAsync could become CreateItemAsync, to be more consistent with other interfaces (GetItemsAsync). (Same remark for FileExistsAsync, GetFileFromPathAsync, ...)
  • It is not possible to know if an operation is possible before performing it. If the folder is read-only (in particular), we are forced to throw an exception. It's not efficient. Add CanCreateItemAsync, CanCopyItemAsync, ...
  • This interface does not allow advanced management, some of which are available on all other file managers (including File Explorer) or could bring great added value. (Pausing a copy, Percentage of completion).

One solution is to provide an IStorableAction type interface that contains IStorable Source IFolder Target StorableOperations operation (copy, move, create, delete, rename, ...) CreationCollisionOption collisionOption Status {get} (Running, Canceled, Pending, Initial) event StatusChanged Start(), Pause(), Cancel(), Stop() This can probably be further improved (one interface per operation + basic interface)

Note: A handy option to add would be an option in the copy list for limit to a single current copy. It's more efficient. It should be possible to choose which one is in progress. Steam has a similar behavior for multiple downloads.

  • In IStoragePropertiesCollection, DateAccessed is missing.
  • IStorageProperty, value should be read asynchronously (especially useful for cloud). Use GetValueAsync/SetValueAsync.
  • PathHelpers and FtpHelpers can become extension classes. It makes calls more concise and avoid unnecessary static classes. Additionally, PathExtensions should contain methods currently in StringExtensions and PathNormalization. Names can be more consistent and standardized. In my pr #9486, I published a version of these files.
  • There are 2 files called FtpHelpers in this pr.

I haven't read it all yet. This is a first list of remarks. It may seem like a lot of remarks but it's a great job and I quibble.

cinqmilleans avatar Aug 10 '22 15:08 cinqmilleans

The name CreateCopyOfAsync is inconsistent with MoveOfAsync. CopyOfAsync is simpler.

The current names are gramatically consistent


It is cleaner to use enums instead of boolean as method arguments.

I don't know what arguments you're referring to


The delete should be on an IModifiableStorage interface and delete the item itself. With the current model, you have to call the parent to delete the child.

The delete operation should be on parent folder since it needs to be contained within some container to be able to delete it. The parent folder also needs to be notified of the deletion


It is not possible to know if an operation is possible before performing it. If the folder is read-only (in particular), we are forced to throw an exception. It's not efficient. Add CanCreateItemAsync, CanCopyItemAsync, ...

If a folder is read-only, it cannot be cast to IModifiableFolder in the first place.


This interface does not allow advanced management, some of which are available on all other file managers (including File Explorer) or could bring great added value. (Pausing a copy, Percentage of completion).

These could be extended with additional interfaces or wrapped around interfaces like IStorageTransfer to allow for more in-depth control of the operation

@cinqmilleans

d2dyno1 avatar Aug 10 '22 17:08 d2dyno1

@gave92 can I merge this?

yaira2 avatar Aug 11 '22 16:08 yaira2