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

Feature: Added experimental support for flattening folders #15992

Merged
merged 28 commits into from
Aug 27, 2024

Conversation

devin-slothower
Copy link
Contributor

@devin-slothower devin-slothower commented Aug 11, 2024

Resolved / Related Issues

To prevent extra work, all changes to the Files codebase must link to an approved issue marked as Ready to build. Please insert the issue number following the hashtag with the issue number that this Pull Request resolves.

Steps used to test these changes

Stability is a top priority for Files and all changes are required to go through testing before being merged into the repo. Please include a list of steps that you used to test this PR.

  1. Tested many nested folders both blank and with data.
  2. Tested with merging duplicate files.
  3. Assured the Flatten popup only appears for a single selected folder.
  4. Ran through a bunch of scenarios trying to crash with/without a debugger.

Menu Preview
image

@yaira2 yaira2 changed the title Feature: Add support for flattening folders Feature: Added support for flattening folders Aug 11, 2024
@yaira2
Copy link
Member

yaira2 commented Aug 11, 2024

This is amazing @devin-slothower! Just a heads-up, you only need to add the strings to the en-us file. The other files will automatically update when we sync with Crowdin

Copy link
Member

@0x5bfa 0x5bfa left a comment

Choose a reason for hiding this comment

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

Using Actions is great idea!

src/Files.App/Actions/FileSystem/FlattenRecursiveAction.cs Outdated Show resolved Hide resolved
@yaira2
Copy link
Member

yaira2 commented Aug 12, 2024

@devin-slothower I pushed a commit to sort out the resource files. In short, the only resource file modified in this PR is now the en-us file. Thanks!

@yaira2
Copy link
Member

yaira2 commented Aug 12, 2024

Fyi @Jay-o-Way

@Jay-o-Way

This comment has been minimized.

@0x5bfa

This comment was marked as resolved.

@Jay-o-Way
Copy link
Contributor

I think the resulting action needs to be clear: what files will end up where? Specifically - what will become the root folder? A reasonable expectation can be that, when applying this on one folder, all the the SUBfolders will be dissolved, but the selected folder will remain. In stead, everything is "dumped" in the above folder. But I suppose this behavior makes sense to generalize the different triggering scenarios: selecting one or more folders, selecting one or more files...

@yaira2
Copy link
Member

yaira2 commented Aug 15, 2024

Would an optional dialog with a preview of the changes help?

@yaira2
Copy link
Member

yaira2 commented Aug 18, 2024

@devin-slothower what's the behavior for duplicate files?

@devin-slothower
Copy link
Contributor Author

devin-slothower commented Aug 18, 2024

@devin-slothower what's the behavior for duplicate files?

The current behavior is just to keep the duplicate files where they are and move all non duplicates to their respective place, only removing flattened folders once they no longer contain anything.

@yaira2
Copy link
Member

yaira2 commented Aug 18, 2024

I don’t want to complicate things, so this isn’t a blocker, but it would be great if we could handle this better. Here are some ideas:

  • Show a conflict dialog and let the user choose an action.
  • Add a prompt before flattening folders, allowing the user to set a default behavior for any conflicts.
  • Display a prompt at the end of the operation with a list of files that weren’t moved.

Personally, I lean towards the second idea, but I’m open to discussion. We should probably handle this in a separate PR once we agree on a solution.

@devin-slothower
Copy link
Contributor Author

I don’t want to complicate things, so this isn’t a blocker, but it would be great if we could handle this better. Here are some ideas:

  • Show a conflict dialog and let the user choose an action.
  • Add a prompt before flattening folders, allowing the user to set a default behavior for any conflicts.
  • Display a prompt at the end of the operation with a list of files that weren’t moved.

Personally, I lean towards the second idea, but I’m open to discussion. We should probably handle this in a separate PR once we agree on a solution.

I think the best option would be to handle conflicts similar to the default file manager where the user is prompted for an action, should a conflict arise, prior to anything being moved. I would recommend the following options (with some tweaking to the names):

  • Replace
  • Rename (Either prompting for a new name, or automatically appending a suffix like "file - 1.txt", "file - 2.txt", etc)
  • Skip
  • Cancel

@yaira2
Copy link
Member

yaira2 commented Aug 19, 2024

We already have a conflicts prompt with those options, so most of the work is already done.

@0x5bfa
Copy link
Member

0x5bfa commented Aug 19, 2024

It's a bit confusing but here's how to use. The dialog class is shared among multiple file system operations and dialog conflict resolve is one of them. There's an enum property to get how to resolve for each.

@yaira2 yaira2 changed the title Feature: Added support for flattening folders Feature: Added Experimental support for flattening folders Aug 26, 2024
@yaira2
Copy link
Member

yaira2 commented Aug 26, 2024

@devin-slothower I made some adjustments based on user feedback. I also updated the list in #12367 to track improvements to make at a later time.

  • Updated the wording to “Flatten to root” and changed the destination to be the selected folder.
  • Removed the "Flatten single action".
  • Added a "Flatten folder" action. This isn’t functional yet but opens a dialog that will contain more advanced options in the future.

From my perspective, this PR is ready to merge, but I’ll leave it open for a couple of hours to see if there is any other feedback.

Copy link
Member

@0x5bfa 0x5bfa left a comment

Choose a reason for hiding this comment

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

Looks good!

src/Files.App/Actions/FileSystem/FlattenToRootAction.cs Outdated Show resolved Hide resolved
Co-authored-by: 0x5BFA <62196528+0x5bfa@users.noreply.github.com>
@yaira2 yaira2 changed the title Feature: Added Experimental support for flattening folders Feature: Added experimental support for flattening folders Aug 26, 2024
Comment on lines 4 to 9
using Microsoft.Extensions.Logging;
using System.IO;
using Windows.Storage;
using Microsoft.UI.Xaml.Controls;
using Windows.Foundation.Metadata;
using Files.Shared.Helpers;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
using Microsoft.Extensions.Logging;
using System.IO;
using Windows.Storage;
using Microsoft.UI.Xaml.Controls;
using Windows.Foundation.Metadata;
using Files.Shared.Helpers;
using Files.Shared.Helpers;
using Microsoft.Extensions.Logging;
using Microsoft.UI.Xaml.Controls;
using System.IO;
using Windows.Storage;
using Windows.Foundation.Metadata;

@yaira2 yaira2 requested a review from hishitetsu August 27, 2024 02:08
Copy link
Member

@hishitetsu hishitetsu left a comment

Choose a reason for hiding this comment

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

LGTM

@yaira2 yaira2 merged commit 8f792f4 into files-community:main Aug 27, 2024
6 checks passed
@yaira2 yaira2 added ready to merge Pull requests that are approved and ready to merge and removed needs - code review labels Aug 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Pull requests that are approved and ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants