Shared folder strategies #39
Conversation
|
CLA Assistant Lite bot CLA Assistant bot All Contributors have signed the CLA. |
|
I have read the CLA Document and I hereby sign the CLA |
| public async Task ExecuteAsync(BasePackage package) | ||
| { | ||
| var installedPackage = settingsManager | ||
| .Settings | ||
| .InstalledPackages | ||
| .Single(p => p.PackageName == package.Name); |
There was a problem hiding this comment.
A few things here -
-
In the
InstallerViewModel, this is called before theInstalledPackageobject has been added to the settings, so this will throw an exception unless they had a previous install of the same package (in which case it'd be using the wrong paths anyway). -
It is possible to have two different installs of the same package, so using
.Single()with thePackageNamehere could possibly throw. If possible, its best to use the GuidIdwhen trying to find anInstalledPackage. -
Since the name property is all that's used from the BasePackage, could the method parameter here just be the name of the package instead of the full thing?
| var json = await File.ReadAllTextAsync(configFilePath); | ||
| var job = JsonConvert.DeserializeObject<JObject>(json)!; |
There was a problem hiding this comment.
We've been using System.Text.Json for our json parsing, as it has improved quite a bit since the initial release. So that we don't have two different json parsing libraries involved, would it be possible to redo this using System.Text.Json? I believe it should have a similar class to JObject called JsonObject
| if (basePackage.SharedFolderStrategy is LinkedFolderSharedFolderStrategy) | ||
| sharedFolders.UpdateLinksForPackage(basePackage, packagePath); | ||
| else | ||
| await basePackage.SharedFolderStrategy.ExecuteAsync(basePackage); |
There was a problem hiding this comment.
nit: could you please put curly braces around these statements? According to our style guidelines:
Only omit curly braces from if statements if the statement immediately following is a return.
Thank you!
| /// <summary> | ||
| /// The shared folders that this package supports. | ||
| /// Mapping of <see cref="SharedFolderType"/> to the relative path from the package root. | ||
| /// </summary> | ||
| public virtual Dictionary<SharedFolderType, string>? SharedFolders { get; } | ||
|
|
||
|
|
||
| public abstract ISharedFolderStrategy SharedFolderStrategy { get; protected set; } |
There was a problem hiding this comment.
I wonder if it may be better to implement an abstract method, along the lines of ExecuteSharedFolderStrategy instead - we could inject the ISharedFolders to the implementations of this, so that the packages without custom strategies can just call the original SharedFolders method without the is LinkedFolderSharedFolderStrategy check.
You could still inject the strategies to the classes that need them, we just wouldn't expose the whole strategy object to the callers. You might also need to pass in the directory as a parameter, since the BasePackage doesn't know where it's installed (most of the time).
It might also eliminate the need for LinkedFolderSharedFolderStrategy and avoid the circular reference shenanigans.
I hope that made sense 😅
|
I'm quite eager to see this implemented. I hope @demoran23 is able to make these changes. |
Tangentially related to issue #33, this adds the concept of a shared folder strategy. For SD.Next, it will update the config.json file with the appropriate paths and use them. With that configuration, there should be no need to juggle symlinks in the filesystem.
A similar strategy can be applied to Comfy (cf #33), but it hasn't been implemented here.