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.

Sign up for GitHub

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

Jump to bottom

Deduplicate file names by adding a suffix on the file name #708

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

Deduplicate file names by adding a suffix on the file name #708

wants to merge 3 commits into from
+31 −2

Conversation

jenseralmeida
Copy link

@jenseralmeida jenseralmeida commented Aug 19, 2022 •
edited by bartekpacia
Loading

Fixes #620

@bartekpacia
Copy link
Collaborator

What about iOS?

bartekpacia
bartekpacia reviewed Aug 19, 2022
View reviewed changes
.vscode/settings.json Outdated Show resolved Hide resolved
@jenseralmeida jenseralmeida force-pushed the issue-620-download-error-if-file-exists branch from fea2165 to 7cf781a Compare August 19, 2022 17:20
@jenseralmeida
Copy link
Author

jenseralmeida commented Aug 19, 2022 •
edited
Loading

What about iOS?

iOS has a different model for the file system, where we do not share files between applications, without using the Share option. Besides that, the lines 948 to 950 in FlutterDownloaderPlugin.m, highlighted below remove the file before downloading it. In that way, duplicate file names does not appear to be an issue for iOS.

        if ([fileManager fileExistsAtPath:[destinationURL path]]) {
            [fileManager removeItemAtURL:destinationURL error:nil];
        }

So, my question is, should I change the above lines to replicate the Android behaviour? If you want to change this, I can do it on this PR, or as a separate PR.

@bartekpacia
Copy link
Collaborator

bartekpacia commented Aug 19, 2022 •
edited
Loading

Ah, OK, thanks for explaining :)

  1. What happens currently on Android when file name already exists?

  2. Regarding the issue – I think that the most important thing is to have the same behavior across all platforms so that if a file with the same name on iOS exists, it is deleted, then Android should work the same way. We risk overwriting some user files, but I don't see a better solution as of now. Alternatively, we could other way around and deduplicate file names on iOS as well – then I'd ask you to write some Objective-C to implement this on iOS.

  3. In the longer term, I think we could provide some mechanism like FileSaveStrategy, which would be an enum with 3 states: fail, duplicate, or overwrite. User would configure this strategy globally, in the first call to initialize(), and also per-call, in enqueue.

Overall, what do you think @jenseralmeida?

@jenseralmeida
Copy link
Author

jenseralmeida commented Aug 19, 2022 via email

@jenseralmeida
Copy link
Author

jenseralmeida commented Aug 19, 2022 via email

bartekpacia
bartekpacia requested changes Aug 20, 2022
View reviewed changes
android/src/main/java/vn/hunghd/flutterdownloader/DownloadWorker.java
Comment on lines +506 to +527
int deduplicationFileNumber = 0;
while (newFile.exists()) {
deduplicationFileNumber++;
int fileNameExtensionIndex = filename.lastIndexOf(".");
String fileNameWithoutExtension;
String fileExtension;
if (fileNameExtensionIndex != -1) {
fileNameWithoutExtension = filename.substring(0, fileNameExtensionIndex);
if (fileNameExtensionIndex + 1 < filename.length()) {
fileExtension = "." + filename.substring(fileNameExtensionIndex + 1);
} else if (fileNameExtensionIndex + 1 == filename.length()) {
fileExtension = ".";
} else {
fileExtension = "";
}
} else {
fileNameWithoutExtension = filename;
fileExtension = "";
}
newFile = new File(savedDir,
fileNameWithoutExtension + "(" + deduplicationFileNumber + ")" + fileExtension);
}
Copy link
Collaborator

@bartekpacia bartekpacia Aug 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I'd like to have this logic extracted to some method and unit-tested.

I know, there aren't any tests yet and the overall quality of the Java code in the plugin is not very good, but at least we can try to mitigate the problem :)

PS If we had Android side in Kotlin, we could simplify this code (see link). I'm thinking about converting to Kotlin soon.

@bartekpacia
Copy link
Collaborator

@jenseralmeida Okay, so I'm waiting for you to add deduplication of file name for iOS as well, and then I'll merge this because we'll have consistent behavior across iOS and Android.

Also, please take a look at my comment :)

@bartekpacia
Copy link
Collaborator

Also:

Together with the long-term solution, I think that a new SourceFileName or OriginalFileName property should be added to the DownloadTask class, allowing better control of what to do, by the plugin users.

I think this is a great idea. I needed this feature in my private app which uses this plugin (in the end, I think I found some workaround or something – don't remember exactly). If you have time and are willing to contribute this feature as well, I'd be more than happy to review it and (hopefully) merge :)

@bartekpacia bartekpacia mentioned this pull request Aug 20, 2022
Closed
@bartekpacia bartekpacia changed the title Deduplicate files names by adding a (#dedup-name) suffix on the file name Deduplicate file names by adding a suffix on the file name Aug 20, 2022
@IlyaMax
Copy link

IlyaMax commented Aug 31, 2022

Why is this done not on dart side, if you want this to be on both ios/android?

@bartekpacia
Copy link
Collaborator

@IlyaMax Currently all the Dart side does is forwarding methods to native side. I agree that having more logic on the Dart side would be desirable (some work is being done here), but I'd prefer not make such big changes in this PR.

@IlyaMax
Copy link

IlyaMax commented Aug 31, 2022

@bartekpacia I've made simple function and written tests for it. Why you think that big changes is bad in this case? It doesn't break the api as I understand.

class FileUtils {
  final FileOperator _fileOperator;

  FileUtils(this._fileOperator);
  String deduplicatedFileName(String savedDir, String fileName) {
    final fileExists = _fileOperator.isFileWithNameExists('$savedDir/$fileName');
    if (fileExists) {
      final startIndexOfExtension = fileName.lastIndexOf('.');
      final fileExtension = fileName.substring(startIndexOfExtension);
      final withoutExtension = fileName.substring(0, startIndexOfExtension);
      final startIndexOfPotentialDuplicationNumber =
          withoutExtension.lastIndexOf('(');
      final duplicationNumber = int.tryParse(withoutExtension.substring(
        startIndexOfPotentialDuplicationNumber + 1,
        withoutExtension.length - 1,
      ));
      if (duplicationNumber == null) {
        return '$withoutExtension(1)$fileExtension';
      } else {
        final withoutDuplicationNumber =
            fileName.substring(0, startIndexOfPotentialDuplicationNumber);
        return '$withoutDuplicationNumber(${duplicationNumber + 1})$fileExtension';
      }
    }
    return fileName;
  }
}

@bartekpacia
Copy link
Collaborator

@IlyaMax I'm only maintaining this package and would prefer not to break any dependents. Just trying to stay on the safe side. But now I see that indeed, we can duplicate file names on the Dart side.

After all, this is @jenseralmeida's PR, so it's up to him to implement this functionality. You might want to create a PR to his fork with your Dart changes.

@jenseralmeida
Copy link
Author

jenseralmeida commented Sep 8, 2022 •
edited
Loading

Hi @IlyaMax, I can take a look into it, and as I did not write it on Objective-C yet, and @bartekpacia feels that it does fit as a safe option then I will move it out from Java to Dart and apply it to both platforms.

On the timeframe side, I am stuck with other things, so I am not sure when I will work on this. If @IlyaMax wants to open another PR with his proposed changes, he can do it. When I find time to work on this, I will check if this is still a pending issue and complete its work.

@lugael
Copy link

lugael commented Oct 13, 2022

Hi, any news about this?

@bartekpacia
Copy link
Collaborator

@lugael Hi, unfortunately, no news. If you're willing to pick this up, then here's a rough TODO list:

  • add unit tests to the existing Java implementation (done by the author of this PR)
  • write deduplication for iOS in Obj-C
  • test it and make sure that it's not breaking

@bartekpacia bartekpacia mentioned this pull request Dec 8, 2022
Closed
@jenseralmeida jenseralmeida force-pushed the issue-620-download-error-if-file-exists branch from 3ce851e to cda1324 Compare December 15, 2024 11:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bartekpacia bartekpacia bartekpacia requested changes

@berhili098 berhili098 berhili098 approved these changes

@salmaahhmed salmaahhmed Awaiting requested review from salmaahhmed salmaahhmed is a code owner

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

download error if file exists
5 participants
@jenseralmeida @bartekpacia @IlyaMax @lugael @berhili098