Skip to content
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

Code Quality: Introduce IStorageQueryService and IStorageEnumerationService #8974

Open
5 tasks
cinqmilleans opened this issue Apr 21, 2022 · 7 comments
Open
5 tasks
Assignees
Labels
codebase quality Issues that are not bugs, but still might be worth improving (eg, code hygiene or maintainability)

Comments

@cinqmilleans
Copy link
Contributor

cinqmilleans commented Apr 21, 2022

Decoupling Items in the Filesystem Folder

This issue discusses the migration from Storage to the backend. This step is preliminary to the ListedItem refactor.
With this discussion, we can parallelize the changes to save time. @lukeblevins @d2dyno1 @gave92

Objectives

Items in the Filesystem folder should be removed from unwanted dependencies.

  • Concerning localized texts. These dependencies can remain because the backend uses them. There are some cases where localized text is used to identify types. It would be better to use a enum or non-localized string but that can be done after the migration.
  • Regarding dependencies on ListedItem and its derivatives. They prevent migration but are easy to remove by reversing the dependencies.
  • The ViewModel and App dependencies are very annoying and you have to find the right way to remove them.
  • Settings dependencies should be replaced by properties. This is the case with enumerators.

We take the opportunity to rework the style to clarify the files. The order of the elements is completely random. PRs are already pending. We can also check what is should be public or not. It will also be necessary to manage nullables because the backend manages them differently.

Potential work item ideas

  • Clarify the utility and rationale for BaseStorageFolder|File in the docs
  • Replace FolderSearch.cs with an implementation of a standard Backend service
  • Add IStorageQueryService with the member: bool IsAvailable(string directoryPath) to easily verify support for the implementation's filesystem API on a provided location (this would be done before performing the query)
  • Create a separate IStorageEnumeratorService which only handles instantiation of backend file or folder items from already retrieved paths or native filesystem API constructs (IStorageQueryService will return IList<string>)
  • Evaluate different types of parameters for the member functions of IStorageEnumeratorService. I like the concept choosing whether the items should have partial, full, or extended properties populated upon creation, but we could optionally go further and support a set of specific, core property system strings here.

------ And even more coming soon ------

@gave92
Copy link
Member

gave92 commented Apr 21, 2022

Could you point me a couple of examples where ViewModel and App are referenced in Storage?

@gave92 gave92 added codebase quality Issues that are not bugs, but still might be worth improving (eg, code hygiene or maintainability) triage approved labels Apr 21, 2022
@cinqmilleans
Copy link
Contributor Author

FilesystemOperations depends on IShellPage to access the ItemViewModel, for example here:
FilesystemResult<BaseStorageFolder> destinationResult = await associatedInstance.FilesystemViewModel.GetFolderFromPathAsync(PathNormalization.GetParentDir(destination));
GetFolderFromPathAsync depends on the current page. This dependency must therefore be circumvented.

In FilesystemHelpers, we find for example:
App.JumpList.RemoveFolder(source.Path); // Remove items from jump list

@gave92
Copy link
Member

gave92 commented Apr 21, 2022

Ah ok, I assumed this was just about the StorageFile/Folder implementations 👍

@lukeblevins
Copy link
Contributor

Edited to include my ideas for #8405 which was somewhat informed by @d2dyno1's code review suggestions a couple months ago.

RefactoringEnum

@yaira2 yaira2 changed the title Refactor - Storage Migration Code Quality: Introduce IStorageQueryService and IStorageEnumerationService May 17, 2024
@0x5bfa 0x5bfa self-assigned this May 21, 2024
@0x5bfa
Copy link
Member

0x5bfa commented May 23, 2024

Overview

Posting new operation status should be taken place in each rich command that executes operation.
Below denotes how and what layers we should implement without detailed information such as method arguments and long comments.

Copy

Copies to clipboard with Copy type.

graph TD;
    CommandManager.CopyItem;
Loading

Cut

Copies to clipboard with Move type.

graph TD;
    CommandManager.CutItem;
Loading

Paste

Pastes to the current folder in Copy mode.

graph TD;
    CommandManager.PasteItem-->IStorageOperationService.CopyAsync-->IModifiableStorable.CopyAsync
Loading

Pastes to the current folder in Cut mode.

graph TD;
    CommandManager.PasteItem-->IStorageOperationService.MoveAsync-->IModifiableStorable.MoveAsync
Loading

Delete

Deletes permanently.

graph TD;
    CommandManager.DeleteItem;
    CommandManager.DeleteItem-->IStorageOperationService.DeleteAsync;
    IStorageOperationService.DeleteAsync-->IModifiableStorable.DeleteAsync
Loading

Remove (Move to Trash)

Moves to Trash (Recycle Bin on Windows). Redirects to permanent deletion in some file systems that don’t support recycling.

graph TD;
    CommandManager.RemoveItem-->IStorageOperationService.RemoveAsync-->IRemovableStorable.RemoveAsync
Loading

Query

Performs Rapid Load - load of file names only to show on the UI rapidly.

graph TD;
    ShellViewModel.QueryAllAsync-->IStorageQueryService.GetAllAsync-->IFolder.GetChildrenAsync
Loading

Enumeration

Performs Lazy Load - delayed load of basic properties to show file names rapidly.

graph TD;
    ShellViewModel.EnumerateAllAsync-->IStorageEnumerationService.GetAllAsync;
Loading

Search

Queries with queryable syntax, such as RegEx, AQS and SQL, either in deep(recursively) and in shallow.

graph TD;
    AddressToolbarViewModel.SearchAsync-->IStorageQueryService.GetAllAsync-->IFolder.GetChildrenAsync
Loading

Properties Query

Queries properties of a storable .
Since there're too many properties to retrieve on-loaded, this layer should be accessed on-demand, such as in Details Pane and in Properties window.

graph TD;
    IStorageEnumerationService.GetAllAsync-->StandardStorageItem.GetProperties
Loading
graph TD;
    DetailsPaneViewModel.LoadAsync-->StandardStorageItem.GetProperties
Loading
graph TD;
    BasePropertiesViewModel.LoadAsync-->StandardStorageItem.GetProperties
Loading

Details

Services

Provides services for enumerations, searching and operations. Those services are intended to use without regarding file system capabilities.

Here’s abstracted services:

  • IStorageQueryService
  • IStorageEnumerationService
  • IStorageOperationsService
See All

IStorageQueryService

Task<IAsyncEnumerable<string>> GetAllAsync(
    string path,
    string query = "*",
    bool recursive = false,
    CancellationToken? token = default);
Task<IAsyncEnumerable<string>> GetAllWithAQSAsync(
    string path,
    string query = "",
    bool recursive = false,
    CancellationToken? token = default);
Task<IAsyncEnumerable<string>> GetAllWithSQLAsync(
    string path,
    string query = "",
    bool recursive = false,
    CancellationToken? token = default);

IStorageEnumerationService

Task<IAsyncEnumerable<IStandardStorageItem>> GetAllAsync(
    IReadOnlyList<string> items,
    StoragePropertiesGenerationKind propertiesGenerationKind = StoragePropertiesGenerationKind.Standard,
    CancellationToken? token = default);
Task<IAsyncEnumerable <IStandardStorageItem>> GetAllAsync(
    IReadOnlyList<IStandardStorageItem> items,
    StoragePropertiesGenerationKind propertiesGenerationKind = StoragePropertiesGenerationKind.Extended,
    CancellationToken? token = default);

IStorageOperationsService

FILES_ERROR CopyAsync(/**/);
FILES_ERROR MoveAsync(/**/);
FILES_ERROR RemoveAsync(/**/);
FILES_ERROR DeleteAsync(/**/);

Each file system operation services

  • IWin32StorageService
  • IFtpStorageService
  • IShellStorageService
  • IArchiveStorageService
  • ISftpStorageService (unsupported yet)
  • IWebDAVStorageService (unsupported yet)

Enums

See All

StoragePropertiesGenerationKind

internal enum StoragePropertiesGenerationKind
{
    Standard, // Used for the first enum in the rapid load
    Extended, // Used for the second enumeration in the lazy load
}

Structs

See All

FILES_ERROR

Represents error code and provides sets of extended functions.

public struct FILES_ERROR
{
    bool IsSucceed { get; set; }
    bool IsFailed { get; set; }
    WIN32_ERROR Win32Error { get; set; }
    
    FILES_ERROR ThrowIfFailed();
    FILES_ERROR ThrowIf(FILES_ERROR errorCode);

    static FILES_ERROR FromHResult(uint hResult);
    static FILES_ERROR FromException(Exception ex);
}

Exceptions

See All

StorageOperationFailedException

Let’s use this for every file system operation exception and write in the log.

public sealed class StorageOperationFailedException : Exception
{
    private FILES_ERROR _errorCode;

    public StorageOperationFailedException(error code)
    {
        _errorCode = errorCode;
    }
}

Watchers

The app needs watchers to keep eye on file modifications from outside of the application.

See All

We’ve made watchers through IFilesystemWatcher as demanded, as the result, watcher is everywhere and causing an issue where some watchers are not in operation in some file systems.
Thus, watchers also should be unified as below:

  • IWatcher
  • IFolderWatcher
  • IDeviceWatcher
  • ITrashWatcher

Basic Storage Items

See All

Fundamentals (interface)

  • IStorable
  • IFile
  • IFolder

ILocatables (interface)

Represents interface for storables that resides in a folder structure.

// string Path

IModifiables (interface)

Represents interface for storables that can be modified.

// bool IsTrashAvailable

FILES_ERROR CopyAsync(/**/);
FILES_ERROR MoveAsync(/**/);
FILES_ERROR MoveToTrashAsync(/**/);
FILES_ERROR DeleteAsync(/**/);
FILES_ERROR CreateChildAsync(/**/); // For IFolders only

INestables (interface)

Represents interface for storables to get their parent.

FILES_ERROR GetParentAsync(/**/);

NativeStorage

This uses Shell Win32API through CsWin32 to support accessing unauthorized folders with COM elevation.

FtpStorage

This uses FluentFtp, no worth it making our own one.

ArchiveStorage

This uses SevenZipSharp and 7zip.

Storage UI Items

See All

IStandardStorageItem

This interface allows the layout pages including Home and Sidebar to display all item kinds as below and get necessary standard properties and extended properties shown in each item row and its details pane.

internal interface IStandardStorageItem : ILocatableStorable
{
// FrameworkElement? Icon
// int Id
// string Name
// string Path
// ulong Size
// DateTimeOffset DateCreated
// DateTimeOffset DateAccessed
// DateTimeOffset DateModified
// string DateCreatedHumanized
// string DateAccessedHumanized
// string DateModifiedHumanized
// IStorageProperties Properties
}

StandardStorageItem

(previously ListedItem, LocationItem)

This has ‘AsRecycleBinItem‘ and ‘AsGitItem‘ properties as well to support all properties in single DataTemplate.

internal abstract StandardStorageItem : IStandardStorageItem
{
// Inherits from the interface
}

StandardShellItem

(previously ShellItem and RecentItem)

internal abstract StandardShellItem : IStandardStorageItem
{
// string ShellPath
}

StandardLibraryItem

(previously SidebarLibraryItem and ShellLinkItem)

internal abstract StandardLibraryItem : StandardStorageItem
{
}

StandardDriveItem

(previously DriveItem)

internal class StandardDriveItem : StandardStorageItem
{
}

StandardShortcutItem

(previously RecentItem, ShellLinkItem)

internal class StandardShortcutItem : StandardStorageItem
{
// string TargetPath
}

StandardGitItem

(previously GitItem)

internal class StandardGitItem : StandardStorageItem
{
// string GitLastCommitAuthor
// string GitLastCommitDate
// string GitLastCommitMessage
// [Enum] GitFileModifyStatus
}

StandardFileTagItem

(previously FileTagItem and 3 more)

internal class StandardFileTagItem : StandardStorageItem
{
// string ColorHex
}

StandardRecycleBinItem

(previously RecycleBinItem)

internal class StandardRecycleBinItem : StandardStorageItem
{
// string OriginalPath
// DateTimeOffset DateDeleted
// string DateDeletedHumanized
}

Storage UI Properties

Those properties are intended to use in Details pane and Properties window. Because they are very detailed, places where to be used is very limited but still necessary to keep better experience on Windows

See All

IStorableProperties

This represents extended properties, loading of which has to be lazy loading in UI thread with low priority without deadlock.

Especially when you want to get music properties for mp3 file, for example, you can specify the combined flag Music to the kind parameter, then the method gets them through native properties in System.Kind such as System.Audio.Format and System.Category and returns as NativeProperties.

internal interface IStorageProperties
{
    Task<IAsyncEnumerable<IStorableProperties>> GetAllAsync(
        StorablePropertiesKind kind = StorablePropertiesKind.All);

    Task<IStorableProperties> GetAsync(
        StorablePropertiesKind kind);
}

IStorageProperty

Supports all kind of properties.

internal interface IStorageProperty<T> : IStorable
{
    public string Kind { get; private set; }

    public string Name { get; private set; }
    public string NameHumanized { get; private set; }

    public T Value { get; private set; }
    public string ValueHumanized { get; private set; }

    public bool IsReadOnly { get; private set; }
}

StoragePropertySection

This is used in Details tab of properties window.

internal class StoragePropertySection : IEnumerable<IStorableProperty>
{
    public string SectionName { get; private set; }
    public string SectionNameHumanized { get; private set; }
    public int SectionDisplayOrder { get; private set; }

    public StoragePropertySection(IEnumerable<ISotrageProperty> properties) : base(properties)
    {
    }
}

NativeProperties

ShellProperties

DriveProperties

CloudDriveProperties

NetworkProperties

GitProperties

Usages

See All

Collection of items acquisition

ShellViewModel.GetAllAsync(CancellationToken? token = default)
↓
var paths = StorageQueryService.GetAllAsync(
    path,
    token: token);var items = StorageEnumerationService.GetAllAsync(
    paths,
    StorablePropertiesGenerationKind.Standard,
    token);

Properties acquisition

ShellViewModel.GetExtendedPropertiesAsync(CancellationToken? token = default)
↓
var items = StorageEnumerationService.GetAllAsync(
    items,
    StorablePropertiesGenerationKind.Extended,
    token);

Search results acquisition

ShellViewModel.SearchAsync(CancellationToken? token = default)
↓
var items = StorageQueryService.GetAllAsync(
    path,
    "*.docx",
    true,
    token);

@Arlodotexe
Copy link

Arlodotexe commented Jun 11, 2024

IStorageQueryService feels like an anti-pattern, and it's one I've been actively avoiding in my own apps for some time. Best to start with some given root folder.

@0x5bfa
Copy link
Member

0x5bfa commented Jun 11, 2024

Even it’s to support some type of query languages that are supported only on Windows such as AQS and Windows Search SQL via Win32API - Windows Search API v3?
If we use extension methods the app has to cast to NativeStorageFolder in search control view model. I think wrapping with interface protects codebase from being modified unintentionally and I want to avoid view models think about platform capability and try to use abstracted interfaces for it.

You're totally true. When it comes to Storage Search, we might as well create an extension SearchAsync as IStorageQueryService.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
codebase quality Issues that are not bugs, but still might be worth improving (eg, code hygiene or maintainability)
Projects
Status: 📋 Planning stage
Development

No branches or pull requests

6 participants