Skip to content

dev/lubl/storage-enum #8405

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

Closed
wants to merge 2 commits into from
Closed

dev/lubl/storage-enum #8405

wants to merge 2 commits into from

Conversation

lukeblevins
Copy link
Contributor

@lukeblevins lukeblevins commented Feb 17, 2022

TBA

For #7540

@lukeblevins lukeblevins requested a review from d2dyno1 February 17, 2022 18:58
private IntPtr hFile;
private WIN32_FIND_DATA findData;

public string WorkingDirectory
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would put it in BaseListedLayoutViewModel

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would complement what's in that view model. We just needed some way to have the path be tied to the service instance in order to enable checking if that service is supported for the file system location.

Copy link
Contributor Author

@lukeblevins lukeblevins Feb 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the "working directory" name doesn't sound right, we can always change it for the service. It could make sense to change it to reflect that it's tied to a particular instance of the service instead of an app navigation related construct.

Copy link
Member

@gave92 gave92 Feb 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not pass the path as a parameter? E.g Enumerate(string folderPath) / IsSupported(string folderPath)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes! That'd make it much more flexible

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gave92 @d2dyno1 I'm not a fan of that pattern. It seems more readable if we don't have duplicate opportunities to pass parameters for what will be the same object.

Moreover, the way I committed means we only have to validate the path once rather than for each method that takes it for an argument.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no real reason why we should keep the service in know of the current directory. The sole purpose of services is to process the provided data and return appropriate results. It shouldn't really know of the state of the navigated directory. There are some services which may know particular states, but this doesn't apply here. My intent was to create an abstract way directories are traversed, and so these enumerating services should only be supplied with desired path and should only supply back the result. There's no need to incorporate the current directory path nor any contextual data. That would be done in BaseListedLayout (or ListingModel) and shall be enough.

@duke7553

Copy link
Member

@d2dyno1 d2dyno1 Feb 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the other hand, I'm still thinking whether to make it transient or singleton. If we use the transient, we'll obviously be able to use the contextual data, but that's not my intent. The question is, how will the service behave with multithreading - will singleton affect it? (As of now, the service is injected as transient)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough. I'll close this then while we work out that detail.

@d2dyno1
Copy link
Member

d2dyno1 commented Feb 19, 2022

I'll work on it next week 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants