Skip to content

Introduce abstract storage layer #9544

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 15 commits into from
Aug 11, 2022
Merged

Conversation

d2dyno1
Copy link
Member

@d2dyno1 d2dyno1 commented Jul 11, 2022

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

  • 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?

  • Built and ran the app

@cinqmilleans
Copy link
Contributor

@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)

@d2dyno1
Copy link
Member Author

d2dyno1 commented Jul 15, 2022

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
Copy link
Member Author

d2dyno1 commented Jul 15, 2022

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

@cinqmilleans
Copy link
Contributor

This is ok for me.

@cinqmilleans
Copy link
Contributor

@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).

@d2dyno1 d2dyno1 marked this pull request as ready for review August 7, 2022 23:34
@d2dyno1 d2dyno1 requested a review from gave92 August 7, 2022 23:34
@cinqmilleans
Copy link
Contributor

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 Create project Files.Filesystem #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.

@d2dyno1
Copy link
Member Author

d2dyno1 commented Aug 10, 2022

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

@yaira2 yaira2 added changes requested Changes are needed for this pull request and removed needs - code review labels Aug 11, 2022
yaira2
yaira2 previously approved these changes Aug 11, 2022
@yaira2 yaira2 added needs - code review and removed changes requested Changes are needed for this pull request labels Aug 11, 2022
@yaira2
Copy link
Member

yaira2 commented Aug 11, 2022

@gave92 can I merge this?

@gave92 gave92 added ready to merge Pull requests that are approved and ready to merge and removed needs - code review labels Aug 11, 2022
@yaira2 yaira2 merged commit 2d51a3d into files-community:main Aug 11, 2022
@d2dyno1 d2dyno1 deleted the infra_storage branch August 11, 2022 17:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Pull requests that are approved and ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants