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

Fix mozilla/thimble.mozilla.org#1887: File overwrites without notifying user #847

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

humphd
Copy link

@humphd humphd commented Jul 20, 2017

Finishing work begun by @Simon66 in #712. I've rebased it and fixed some bugs, but I need to do another round of cleanup on this before it's ready for review, which I'll do soon.

@humphd
Copy link
Author

humphd commented Jul 20, 2017

OK, this is ready for review. Here's what it does, and how to test:

  • Drag some new file image.png into your file tree. You shouldn't get a dialog, and the file should import correctly.
  • Drag the same file image.png into your file tree again. This time you should get a dialog asking if you want to keep the existing file, use the new one, or cancel. Try repeating this process with all 3 options, and each should do what you expect.
  • Repeat the above process for a .zip file
  • Repeat the above process for a .tar file

@flukeout
Copy link

Testing now.

@flukeout
Copy link

Some suggestions this popup...

image

  • Let's just have two actions on the popup...
    • Use New File (Green)
    • Cancel (Gray)
  • Change the copy a bit....

Suggested popup...

image

@flukeout
Copy link

flukeout commented Jul 20, 2017

Use the upload file dialog and drag in a file that already exists. Then, select "Keep Existing". The upload dialog stays open with the "Uploading..." spinner visible, like this...

image

I think we should just close the upload dialog once the "Choose what to do" dialog opens.

Another small thing
This is totally an edge case, but if you drag in two files with the same name, this is the result...

image

This is a bit different in terms of behavior because we rename instead of prompting about duplication.

Copy link

@flukeout flukeout left a comment

Choose a reason for hiding this comment

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

Functionally this is working great. Let's address...

  • Popup content & buttons
  • Fixing the "upload dialog" issue

@humphd
Copy link
Author

humphd commented Jul 21, 2017

Good catch on the upload spinner--I fixed that originally, but somehow lost that part of the commit, maybe in a rebase. It's fixed now.

This is totally an edge case, but if you drag in two files with the same name, this is the result...

I don't know what this means. How do you drag in two files with the same name (e.g. your OS won't allow it).

Let's just have two actions on the popup...

We should discuss this a bit. The idea of the Cancel action is that you can stop the entire import. For example, if I drag in a zip file, and already have all the files, I don't want to have to click "Keep Existing" 50 times.

@flukeout
Copy link

double-file

^^^ What's up now @humphd ?!?!!?

Anyway, let me revisit the popup actions with multiple duplicated files. I hadn't tried that.

@flukeout
Copy link

flukeout commented Jul 21, 2017

If I drag in two files, both of which are duplicates, the popup shows up twice. Cancel dismissed each popup, but doesn't cancel the entire import.

If I drag in a ZIP file with two duplicated files, then I can cancel the entire upload process with the first click of Cancel - which also doesn't feel right, since it only references one file.

For the case of multiple files being duplicated, we could list them all in the popup, or say, there are some duplicate files here, [overwrite all] or [keep existing] for all

@humphd
Copy link
Author

humphd commented Jul 21, 2017

Talking with Luke, we're going to try doing this:

  • Try to do "Keep All" or "Replace All" vs. one-by-one for an import. That is, remember what the user says for an import "session" then clear that and ask again next time.

@flukeout to figure out the dialog strings.

@humphd humphd self-assigned this Jul 21, 2017
@flukeout
Copy link

@humph Dialog strings... thoughts?

We found a file with the same name

There is already a file called coin.mp3 in your project. We can overwrite it with the new file or keep the existing file.

[ Keep all existing files ] [ Use new files ]

@humphd
Copy link
Author

humphd commented Jul 21, 2017

I think that we should be careful about mixing the idea of this specific file vs. all files. The dialog message and the buttons aren't aligned in that way. What about this:

We found a file with the same name

There is already a file called coin.mp3 in your project. We can keep the existing file(s), or overwrite with the new file(s).

[ Keep all existing files ] [ Use new files ]

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.

3 participants