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

Add "Duplicate" feature to mediamanager #1083

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from

Conversation

damsfx
Copy link
Contributor

@damsfx damsfx commented Mar 11, 2024

Add a button for duplicating files or folders in the Media Manager menu bar.

Single item selection:

If a file or folder is selected, the user is prompted for the new name of the duplicated file/folder.

Multiple item selection:

If multiple files and/or folders are selected, a warning is displayed to the user that the files/folders will be duplicated with an automatically generated name.

Automatically generated names :

"-x" is added to the name of the item to be duplicated, where x is a numeric counter.
The folder where the item is located is scanned for occurrences with identical names.

For example, myfile.pdf will give myfile-1.pdf.
If myfile-1.pdf is already present, then the new name will be myfile-2.pdf.

* @param string $path Specifies a file path to check.
* @return string The generated file name.
*/
public function generateIncrementedFileName($path): string
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 a Str::unique() helper, and the original PR for it proposed a File::unique() helper as well. Any thoughts on either using the str helper or re-adding the proposed File::unique() helper: wintercms/storm#114?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@LukeTowers I've tested with Str::unique for directory names ... it seems correct.
However, for files it doesn't work, I think the File::unique() method is required.

Is there any particular reason why it was removed from the PR?

Copy link
Member

Choose a reason for hiding this comment

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

It wasn't being used at the time, the Str::unique() was in an internal project, but at the time the File::unique() wasn't being used at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can update the code for this PR if the feature makes a comeback in Winter's core.

Copy link
Member

Choose a reason for hiding this comment

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

Do you mind submitting a PR to Storm to add it back in?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will try to find the time this end's week.

Copy link
Member

Choose a reason for hiding this comment

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

@damsfx ping 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@LukeTowers pong 😉

@LukeTowers
Copy link
Member

@damsfx can you add some screenshots?

@damsfx
Copy link
Contributor Author

damsfx commented Aug 2, 2024

@LukeTowers
WinterCMS_Mediamanager_Duplicate.webm

WinterCMS_Mediamanager_Duplicate.webm

New button :
image

Popup to give new name (with suggested one) :
image

Popup for multiple selected files or folders :
image

Automatically generated names :

"_x" is added to the name of the item to be duplicated, where x is a numeric counter.
The folder where the item is located is scanned for occurrences with identical names.

For example, myfile.pdf will give myfile_1.pdf.
If myfile_1.pdf is already present, then the new name will be myfile_2.pdf.
If myfile_1.pdf and myfile_4.pdf is already present, then the new name will be myfile_5.pdf.

@LukeTowers
Copy link
Member

Fantastic @damsfx! What happens if they rename the proposed name in the duplicate file popup to the original name (ignoring the suggested name)?

@damsfx
Copy link
Contributor Author

damsfx commented Aug 5, 2024

@LukeTowers the golden question!
I don't remember to have make that test.
It use storage facade copy method witch is just a return of the PHP native one.

The php copy manual says:

Warning
If the destination file already exists, it will be overwritten.

So I think it may be the same here.

Test needed and if it's the case, maybe ask the user to confirm the overwrite?

@damsfx
Copy link
Contributor Author

damsfx commented Aug 20, 2024

@LukeTowers I can confirm that a file with the same name is overwritten.

What solution do you plan to use to solve this problem?
An alert informing the user/requesting his confirmation, a return with no action other than a flash message?

@LukeTowers
Copy link
Member

@damsfx I would say probably start with just throwing an ApplicationException which will popup the error. I'm not sure how we're currently handling it in the rename popup and / or on file upload, maybe take a quick look there and then we can standardize across all three?

@damsfx
Copy link
Contributor Author

damsfx commented Aug 24, 2024

@LukeTowers At this time, both upload and rename actions simply replace an existing file, no errors, no warnings.

@LukeTowers
Copy link
Member

@damsfx thats what I thought, I think there was an issue at some point to fix that by auto renaming to the suffixed form, any thoughts on the approach that we should take for all of the scenarios so that we can standardize on that? I think the options are:

  • Do nothing and silently allow the replacement
  • throw an applicationexception refusing to overwrite an existing file
  • allow the file put but force a rename to an auto suffixed filename
  • present a dialog offering the choice to the user

The last option is slightly more difficult to implement but I think is the best option. Do you have any other thoughts or perhaps any options I missed?

Also would like to get input from any of the other maintainers if they're available (@bennothommo, @mjauvin, @jaxwilko).

@mjauvin
Copy link
Member

mjauvin commented Aug 24, 2024

I would definitely prefer the following option:

allow the file put but force a rename to an auto suffixed filename

@bennothommo
Copy link
Member

I agree with @mjauvin. It's the least intrusive and destructive. If someone wants to be clever with their naming, they will most certainly be skilled enough to find the file with an additional suffix.

@damsfx
Copy link
Contributor Author

damsfx commented Aug 26, 2024

It's hard to say ...

Allowing an existing file to be overwritten (without warning) can be an advantage, but it can also be a risk.

Having a warning before any file overwrite action would be a bonus, but perhaps this behaviour should be configurable in the backend settings?

@LukeTowers
Copy link
Member

The way it's typically handled in OS level file managers is to present a dialog that asks the user if they want to Replace, Keep Both (auto rename), or Cancel. That would be my preferred way to handle it here, but I'm also fine with defaulting to Keep Both if that's easier to implement to start with.

@damsfx
Copy link
Contributor Author

damsfx commented Sep 4, 2024

@LukeTowers , @mjauvin , @bennothommo
Would you prefer this PR to include the changes so that the existing files are no longer overwritten by the uploaded/renamed/duplicated files, or would you prefer this to be done at a later date with a new PR?

@mjauvin
Copy link
Member

mjauvin commented Sep 4, 2024

I prefer to include it in this PR

@AIC-BV
Copy link
Contributor

AIC-BV commented Sep 10, 2024

looking GREAT! This will be used a lot by my team creating the content

@damsfx
Copy link
Contributor Author

damsfx commented Sep 10, 2024

@AIC-BV As a user likely to use this function a lot, what are your feelings about overwriting files already present with the same name (see previous messages)?

@AIC-BV
Copy link
Contributor

AIC-BV commented Sep 10, 2024

@AIC-BV As a user likely to use this function a lot, what are your feelings about overwriting files already present with the same name (see previous messages)?

It can work both ways but I think that the correct way, or the expected UX default behaviour, is to not automatically overwrite it.
Preventing accidental data loss is the most important.
Overwriting it is a risk, like you said above, and I think you should avoid risk if you can choose to.

When you do this in Windows File Explorer it automatically changes the name to 'filename - Copy' which makes sense to me. Just like you did with the _1, _2, _3, ...
It is up to the content creater to name the files correct, or remove the duplicate ones.

For me it doesn't need a popup, just the 'version' behind the string.
A popup can work too but that is not the default file explorer behaviour, but perhaps its different on Mac

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.

5 participants