Don't overwrite existing files when DOWNLOAD_ALWAYS_ASK is false #9867

timborden · 2017-07-05T11:59:50Z

Fixes #7764

Submitter Checklist:

Submitted a ticket for my issue if one did not already exist.
Used Github auto-closing keywords in the commit message.
Added/updated tests for this change (for new code or code which already has tests).
Ran git rebase -i to squash commits (if needed).
Tagged reviewers and labelled the pull request as needed.

Test Plan:

Tested that the file was downloaded to the location specified in the settings
Tested that a second downloaded file would not replace an existing file of the same name

Reviewer Checklist:

Tests

Adequate test coverage exists to prevent regressions
Tests should be independent and work correctly when run individually or as a suite ref
New files have MPL2 license header

bridiver · 2017-07-05T17:50:32Z

app/filtering.js

@@ -520,14 +521,29 @@ function registerForDownloadListener (session) {
    }

    const defaultPath = path.join(getSetting(settings.DOWNLOAD_DEFAULT_PATH) || getSetting(settings.DEFAULT_DOWNLOAD_SAVE_PATH) || app.getPath('downloads'), itemFilename)
-    const savePath = ((process.env.SPECTRON || (!getSetting(settings.DOWNLOAD_ALWAYS_ASK) && !item.promptForSaveLocation())) ? defaultPath : dialog.showSaveDialog(win, { defaultPath }))
+    let savePath = ((process.env.SPECTRON || (!getSetting(settings.DOWNLOAD_ALWAYS_ASK) && !item.promptForSaveLocation())) ? defaultPath : dialog.showSaveDialog(win, { defaultPath }))


I think there should be a single if statement here that wraps all of this, but can you switch !getSetting(settings.DOWNLOAD_ALWAYS_ASK) && !item.promptForSaveLocation() to getSetting(settings.DOWNLOAD_ALWAYS_ASK) || item.promptForSaveLocation()? The former is confusing

bridiver · 2017-07-05T17:53:30Z

app/filtering.js


    // User cancelled out of save dialog prompt
    if (!savePath) {
      event.preventDefault()
      return
    }

+    if (!getSetting(settings.DOWNLOAD_ALWAYS_ASK) && !item.promptForSaveLocation()) {


please merge into else for if above

bridiver · 2017-07-05T17:54:17Z

app/filtering.js


    // User cancelled out of save dialog prompt
    if (!savePath) {
      event.preventDefault()
      return
    }

+    if (!getSetting(settings.DOWNLOAD_ALWAYS_ASK) && !item.promptForSaveLocation()) {
+      let willNotOverwrite = false


double negative checks are also hard to understand !willNotOverwrite -> willOverwrite

bridiver · 2017-07-05T17:55:07Z

app/filtering.js

+      let matchedFilenames = 0
+      let querySavePath
+      while (!willNotOverwrite) {
+        querySavePath = (matchedFilenames ? savePath.substring(0, savePath.lastIndexOf('.') >= 0 ? savePath.lastIndexOf('.') : savePath.length) + ` (${matchedFilenames})` + savePath.substring(savePath.lastIndexOf('.') >= 0 ? savePath.lastIndexOf('.') : savePath.length) : savePath)


I think this will break things like .tar.gz. Maybe get the first index?

timborden · 2017-07-06T10:19:23Z

Thanks for the feedback @bridiver

I've refactored the code based on your suggestions.

I also dug into the extension name edge cases (like tar.gz). As a result I switched from the substring() approach to using path.extname(). Sadly, it didn't solve the issue, but the code is shorter and more readable.

I did some searching for possible solutions. One thought was to use the MIME:

> var mime = require('mime-types')
undefined
> mime.lookup('package.json')
'application/json'
> mime.extension(mime.lookup('package.json'))
'json'

....sadly, it didn't work for tar.gz files

> mime.lookup('v0.17.13dev.tar.gz')
false

It seems that the most common approach is to split on the last "."...even the OS X save prompt defaults to using the last ".":

Short of rolling with a whitelist of file extensions, which feels like overkill....it looks like this may be the best option.

timborden · 2017-07-11T14:05:27Z

Just added tests....let me know if you think it needs more coverage.

luixxiul · 2017-08-05T23:32:02Z

@timborden would you please rebase on the latest master? thanks!

…7764)

timborden · 2017-08-09T15:49:09Z

Hey @luixxiul ...sorry for the delay (just changed my GitHub settings so I don't miss these notifications)

I just rebased....FYI

luixxiul · 2017-08-09T15:51:42Z

@timborden thanks! I'd like to see you keep up the nice work :-)

codecov-io · 2017-08-09T16:06:38Z

Codecov Report

Merging #9867 into master will decrease coverage by 0.03%.
The diff coverage is 6.66%.

@@            Coverage Diff             @@
##           master    #9867      +/-   ##
==========================================
- Coverage   53.67%   53.63%   -0.04%     
==========================================
  Files         238      238              
  Lines       21049    21063      +14     
  Branches     3256     3258       +2     
==========================================
+ Hits        11297    11298       +1     
- Misses       9752     9765      +13

Flag	Coverage Δ
#unittest	`53.63% <6.66%> (-0.04%)`	⬇️

Impacted Files	Coverage Δ
app/filtering.js	`18.07% <6.66%> (-0.27%)`	⬇️

bsclifton

Code works great 😄 👍 Good job on this one (including the tests too!)

bsclifton · 2017-08-13T20:27:16Z

@bridiver did you want to re-review before this gets merged?

bsclifton · 2017-08-14T05:48:45Z

Merged 😄 👌

timborden · 2017-08-14T07:22:03Z

Awesome....thanks @bsclifton, @bridiver and @luixxiul

timborden mentioned this pull request Jul 5, 2017

Downloading the same name file disabling the save file dialog should change the filename #7764

Closed

NejcZdovc added the feature/download label Jul 5, 2017

NejcZdovc assigned timborden Jul 5, 2017

NejcZdovc requested review from bridiver and bsclifton July 5, 2017 12:12

NejcZdovc added this to the 0.20.x (Nightly Channel) milestone Jul 5, 2017

timborden force-pushed the feature/download-filename-exists branch from 5d2ed55 to 11299f7 Compare July 5, 2017 16:53

bridiver suggested changes Jul 5, 2017

View reviewed changes

timborden force-pushed the feature/download-filename-exists branch from 11299f7 to 8a64255 Compare July 6, 2017 10:06

timborden force-pushed the feature/download-filename-exists branch from 8a64255 to 5b7a0b3 Compare July 11, 2017 14:02

luixxiul added the PR/needs-rebase ۞ label Aug 5, 2017

luixxiul modified the milestones: 0.21.x (Nightly Channel), 0.20.x (Developer Channel) Aug 7, 2017

Don't overwrite files when DOWNLOAD_ALWAYS_ASK is false (fixes brave#…

3cba178

…7764)

timborden force-pushed the feature/download-filename-exists branch from 5b7a0b3 to 3cba178 Compare August 9, 2017 15:47

luixxiul removed the PR/needs-rebase ۞ label Aug 9, 2017

timborden changed the title ~~Don't overwrite existing files when DOWNLOAD_ALWAYS_ASK is false (fix…~~ Don't overwrite existing files when DOWNLOAD_ALWAYS_ASK is false Aug 9, 2017

bsclifton approved these changes Aug 13, 2017

View reviewed changes

bsclifton merged commit 2f9ef72 into brave:master Aug 14, 2017

bbondy modified the milestones: 0.21.x (Developer Channel), 0.20.x (Beta Channel) Oct 25, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't overwrite existing files when DOWNLOAD_ALWAYS_ASK is false #9867

Don't overwrite existing files when DOWNLOAD_ALWAYS_ASK is false #9867

timborden commented Jul 5, 2017 •

edited by bsclifton

Loading

bridiver Jul 5, 2017

bridiver Jul 5, 2017

bridiver Jul 5, 2017

bridiver Jul 5, 2017

timborden commented Jul 6, 2017

timborden commented Jul 11, 2017 •

edited

Loading

luixxiul commented Aug 5, 2017

timborden commented Aug 9, 2017 •

edited

Loading

luixxiul commented Aug 9, 2017

codecov-io commented Aug 9, 2017 •

edited

Loading

bsclifton left a comment

bsclifton commented Aug 13, 2017

bsclifton commented Aug 14, 2017

timborden commented Aug 14, 2017

Don't overwrite existing files when DOWNLOAD_ALWAYS_ASK is false #9867

Don't overwrite existing files when DOWNLOAD_ALWAYS_ASK is false #9867

Conversation

timborden commented Jul 5, 2017 • edited by bsclifton Loading

bridiver Jul 5, 2017

Choose a reason for hiding this comment

bridiver Jul 5, 2017

Choose a reason for hiding this comment

bridiver Jul 5, 2017

Choose a reason for hiding this comment

bridiver Jul 5, 2017

Choose a reason for hiding this comment

timborden commented Jul 6, 2017

timborden commented Jul 11, 2017 • edited Loading

luixxiul commented Aug 5, 2017

timborden commented Aug 9, 2017 • edited Loading

luixxiul commented Aug 9, 2017

codecov-io commented Aug 9, 2017 • edited Loading

Codecov Report

bsclifton left a comment

Choose a reason for hiding this comment

bsclifton commented Aug 13, 2017

bsclifton commented Aug 14, 2017

timborden commented Aug 14, 2017

timborden commented Jul 5, 2017 •

edited by bsclifton

Loading

timborden commented Jul 11, 2017 •

edited

Loading

timborden commented Aug 9, 2017 •

edited

Loading

codecov-io commented Aug 9, 2017 •

edited

Loading