feat: Add Yes/No to all for batch asset import#3052
feat: Add Yes/No to all for batch asset import#3052Acissathar wants to merge 3 commits intostride3d:masterfrom
Conversation
|
Marked as a draft as I have a fair bit of test cases I want to run through locally before saying its ready, but I likely won't get to them until later this week/weekend so wanted to throw it up incase anyone had thoughts / nitpicks. Test cases I plan to run through (think this covers all scenarios) Single File Import
Multiple Files - Basic Flow
Mixed File Locations
Edge Cases
Button Index Verification
|
|
Ran through all my test cases and everything seems to work as expected and consistent with previous behavior where applicable, should be ready! |
| } | ||
| else | ||
| { | ||
| // If "Yes to all" we're going to assume they want to use the same directory as the initial file. |
There was a problem hiding this comment.
Is this a valid assumption? It sounds like it would flatten any hierarchy during the copy.
There was a problem hiding this comment.
I might be misunderstanding, but I believe this assumption is valid as this is consistent with the current behavior. If you select No to the default location, and specify a directory in the window that pops up, it puts it right where you tell it to without sub folder generation vs saying Yes to the default, which puts it in Resources/subfolder
| IReadOnlyList<DialogButtonInfo> copyPromptButtons = DialogHelper.CreateButtons(files is not null && files.Count > 1 ? | ||
| [ | ||
| Tr._p("Button", "Yes"), | ||
| Tr._p("Button", "No"), | ||
| Tr._p("Button", "Yes to all"), | ||
| Tr._p("Button", "No to all") | ||
| ] | ||
| : | ||
| [ | ||
| Tr._p("Button", "Yes"), | ||
| Tr._p("Button", "No") | ||
| ], 1, 2); | ||
|
|
||
| IReadOnlyList<DialogButtonInfo> overwritePromptButtons = DialogHelper.CreateButtons(files is not null && files.Count > 1 ? | ||
| [ | ||
| Tr._p("Button", "Yes"), | ||
| Tr._p("Button", "No"), | ||
| Tr._p("Button", "Yes to all") | ||
| ] | ||
| : | ||
| [ | ||
| Tr._p("Button", "Yes"), | ||
| Tr._p("Button", "No") | ||
| ], 1, 2); |
There was a problem hiding this comment.
This would produce mildly undesirable behavior when the user reaches the last file, by including the "to all" buttons.
…collection Also some minor grammar/formatting
| var message = Tr._p("Message", "Source file '{0}' is not inside of your project's resource folders, do you want to copy it?").ToFormat(file.FullPath); | ||
|
|
||
| var message = Tr._p("Message", "Source file '{0}' is not inside of your project's resource folders, do you want to copy it?").ToFormat(file.FullPath); | ||
| var copyResult = await Dialogs.MessageBoxAsync(message, files.Count > 1 && i != files.Count - 1 ? copyPromptWithToAllButtons : dialogDefaultButtons, MessageBoxImage.Warning); |
There was a problem hiding this comment.
Checking i != files.Count - 1 makes checking files.Count > 1 unnecessary.
| var message = Tr._p("Message", "The file '{0}' already exists, it will get overwritten if you continue, do you really want to proceed?").ToFormat(finalPath); | ||
|
|
||
| copyResult = await Dialogs.MessageBoxAsync(message, MessageBoxButton.YesNo, MessageBoxImage.Warning); | ||
| var copyResult = await Dialogs.MessageBoxAsync(message, files.Count > 1 && i != files.Count - 1 ? overwritePromptWithToAllButtons : dialogDefaultButtons, MessageBoxImage.Warning); |
There was a problem hiding this comment.
Checking i != files.Count - 1 makes checking files.Count > 1 unnecessary.
| else | ||
| { | ||
| // If "Yes to all" we're going to assume they want to use the same directory as the initial file. | ||
| finalPath = Path.Combine(Path.GetDirectoryName(finalPath), file.GetFileName()); |
There was a problem hiding this comment.
| finalPath = Path.Combine(Path.GetDirectoryName(finalPath), file.GetFileName()); | |
| finalPath = Path.Join(Path.GetDirectoryName(finalPath), file.GetFileName()); |
There was a problem hiding this comment.
I'm not opposed to the change personally, my only hang up is that the rest of Stride uses Path.Combine. I could not find any uses of Path.Join in the code base, so I'm hesitant to deviate from that consistency.
There was a problem hiding this comment.
Path.Join is newer, faster, and likely exhibits behavior closer to coder intentions. The primary difference between the two is:
- Path.Join is essentially concatenation.
- Path.Combine checks if later parameters are rooted and excludes earlier parameters if they are.
3 participants
PR Details
This change modifies the asset import to help with the flow of importing more than one item at a time. Importing multiple assets into Stride can be a bit of a pain, especially coming from Unity where drag and drop handles asset source copying for you.
While Stride does support multiple asset imports, it can very quickly add up as you're prompted with a dialogue box to copy the object to resources for every single asset, an additional one for the location (if choosing yes), and lastly, one more potential box if it already exists in that location.
Depending on the number of assets, this can quickly become a hassle to have to click 3+ times per asset just to get them imported.
The intent of this is to allow a user to pick once and handle it for all assets, or individually if still desired.
When picking a different location to copy the resources to, this makes the assumption (if you hit yes to all) that you want everything copied to that location. Otherwise, hitting Yes will prompt per asset as it currently does.
Related Issue
#2916
Types of changes
Checklist
There is currently a failure around BuildResult and an assembly reference in SampleTestFixture, but since this is mentioned in another PR, I don't believe its related to these changes.