Files icon indicating copy to clipboard operation
Files copied to clipboard

Proposal: Add factory methods to ListedItem

Open lukeblevins opened this issue 3 years ago • 5 comments

What's the Problem?

There doesn't presently exist a standardized way to create a ListedItem from external constructs such as IStorageItem-derived types, WIN32_FIND_DATA, and IShellItem. We currently expose numerous public properties on ListedItem and expect users of it to automatically and correctly populate all of them.

This expectation adds a level of redundancy and duplication everywhere we have to create ListedItems, and is not good architecture.

Solution/Idea

We should discuss improving code quality in this area with all of the tasks below:

1. Define factory methods inside ListedItem.cs (draft)

public static ListedItem FromFindData(WIN32_FIND_DATAW findData, string dateReturnFormat = null)

public static ListedItem FromStorageItem(IStorageItem storageItem, string dateReturnFormat = null)

public static ListedItem FromShellItem(IShellItem shellItem, string dateReturnFormat = null)

2. Make all properties on ListedItem read only

3. Add a way to determine what type of external construct(s) back a ListedItem instance

Alternatives

Add multiple constructor overloads to ListedItem: This idea might not be ideal because we may access the filesystem directly when constructing the item.

Priorities

Capability Priority
This proposal will allow users of ListedItem to easily create one from external constructs in a standard way Must
Add a way to determining the backing OS filesystem construct(s) Could

Files Version

v2

Windows Version

Windows 11 21H2

Additional comment

No response

lukeblevins avatar Nov 03 '21 17:11 lukeblevins

@files-community/files

yaira2 avatar Nov 03 '21 17:11 yaira2

We currently expose numerous public properties on ListedItem and expect users of it to automatically and correctly populate all of them.

How about we abstract out these properties so we're left only with the important ones and rest will go into a derived class for the UI?

d2dyno1 avatar Nov 03 '21 19:11 d2dyno1

@d2dyno1 I support that idea. ListedItem has kind of a mixture of entirely exposed UI and non-UI properties.

The alternative practice you mentioned is more typical. It appears to be a sound design, and we could separate the implementations in a UI focused way rather than arbitrarily.

Good feedback as always. 👍

lukeblevins avatar Nov 03 '21 19:11 lukeblevins

@cinqmilleans FYI in case you wanted to add or change the pattern I came up with here.

lukeblevins avatar Jan 31 '22 17:01 lukeblevins

@duke7553 Thank you. As mentioned on Discord, that's what I was planning to do.

cinqmilleans avatar Jan 31 '22 18:01 cinqmilleans

One year on, we're taking an entirely different approach with the storage layer.

This will be publicized through final specifications, blog posts and docs once myself, et. al agree to start implementing it; but it will be influenced by the work led by @Arlodotexe to create a CommunityToolkit.Storage abstraction.

ListedItem will be no more - at least in its current monolithic form. More details will come soon over the coming weeks + months.

lukeblevins avatar Oct 24 '22 17:10 lukeblevins