Fix overwritting downloaded files with same name by gtam25 · Pull Request #6174 · JabRef/jabref

gtam25 · 2020-03-25T07:15:01Z

Added auto-numbering for downloaded file names, which conflict with file names in the current directory. #6068

Change in CHANGELOG.md described (if applicable)
Tests created for changes (if applicable)
Manually tested changed features in running JabRef (always required)

koppor

Thank you for your contribution.

Some nitpick comments inside 😇

Two more things:

Please keep FileNameCleaner and introduce FileNameUniqueness. Reason: There is no shared code between the two utility methods (no cohesion in the sence of data; only in the sense of the general setting). Small classes make it easier to maintain them.
Can you try to include a check of Files.mismatch (see https://stackoverflow.com/a/56680991/873282 for details) at download -> before the same file is linked, just do not link it instead of creating ... (1).pdf.

koppor · 2020-03-25T12:37:02Z

src/main/java/org/jabref/logic/util/io/FileNameHandler.java

+        Optional<String> extensionOptional = FileUtil.getFileExtension(fileName);
+
+        // the suffix include the '.' , if extension is present Eg: ".pdf"
+        String extensionSuffix = "";


Please put that into an else path in the if of line 91. (Declaring the variables here is OK). I find it hard if the contents of the variables are overwritten: The default case is that there is an extension.

koppor · 2020-03-25T12:37:53Z

src/main/java/org/jabref/logic/util/io/FileNameHandler.java

+        Path absolutePath = Paths.get(absoluteName);
+        String newFileName = fileName;
+
+        for (int counter = 1; Files.exists(absolutePath); ++counter) {


Suggested change

for (int counter = 1; Files.exists(absolutePath); ++counter) {

for (int counter = 1; Files.exists(absolutePath); counter++) {

++ at the end to be inline with the usual i++.

After thinking longer, please rewrite to a while loop. Not testing for the counter in the test of a for loop is uncommon. Thus, hard to read when maintaining the code.

koppor · 2020-03-25T12:40:43Z

src/main/java/org/jabref/logic/util/io/FileNameHandler.java

+            newFileName = fileNameWithoutExtension +
+                            " (" + counter + ")" +
+                            extensionSuffix;
+            absolutePath = targetDirectory.resolve(newFileName);


No need for that variable. Can be tesed in the while condition.

koppor · 2020-03-25T12:41:27Z

src/test/java/org/jabref/logic/util/io/FileNameHandlerTest.java

+    public void testGetNonOverWritingFileNameReturnsUniqueName(@TempDir Path tempDirectory) throws IOException {
+        String dummyFile1 = "default.txt";
+        String dummyFile2 = "default (1).txt";
+        String expectedFileName = "default (2).txt";


Please include that in the assertEquals directly.

koppor · 2020-03-25T12:47:07Z

src/test/java/org/jabref/logic/util/io/FileNameHandlerTest.java

+
+    @Test
+    public void testGetNonOverWritingFileNameReturnsUniqueName(@TempDir Path tempDirectory) throws IOException {
+        String dummyFile1 = "default.txt";


Please introduce Path dummyFile1 variable. In both usages of dummyFileX, you are resulting the name using the same path.

src/test/java/org/jabref/logic/util/io/FileNameHandlerTest.java

gtam25 · 2020-03-25T15:12:29Z

Thank you for your contribution.

Some nitpick comments inside 😇

Two more things:

Please keep FileNameCleaner and introduce FileNameUniqueness. Reason: There is no shared code between the two utility methods (no cohesion in the sence of data; only in the sense of the general setting). Small classes make it easier to maintain them.

Can you try to include a check of Files.mismatch (see https://stackoverflow.com/a/56680991/873282 for details) at download -> before the same file is linked, just do not link it instead of creating ... (1).pdf.

I think our problem is that different files are getting downloaded as "default.pdf" (when no BibTeX key exists).
If we use Files.mismatch (as you mentioned in number 2 above), since both paths refer to same file path, it would return a match and hence not download the other files (which we do not want I guess ?)

- fixed downloading file with same name - updated changelog

koppor · 2020-03-25T16:19:21Z

Can you try to include a check of Files.mismatch (see stackoverflow.com/a/56680991/873282 for details) at download -> before the same file is linked, just do not link it instead of creating ... (1).pdf.
I think our problem is that different files are getting downloaded as "default.pdf" (when no BibTeX key exists).

Yeah.

If we use Files.mismatch (as you mentioned in number 2 above), since both paths refer to same file path, it would return a match and hence not download the other files (which we do not want I guess ?)

Long solution would be:

default.pdf exists
Download as default (1).pdf.
Store it in the linked files
Check if default.pdf equals default (1).pdf.
If default.pdf equals default (1).pdf then a) remote default (1).pdf from the file system, b) remove default (1).pdf from the list of linked files, c) add a notification that the file already exists.

I thought that one can omit step 3. - step 5c is necessary in all cases.

I see that this is not strongly part of your PR, so can be left for future work.

Submitted as #6197

tobiasdiez

Thanks for your contribution! The code looks good to me. I've only a few very minor comments before we merge.

tobiasdiez · 2020-03-28T14:20:52Z

src/main/java/org/jabref/logic/util/io/FileNameUniqueness.java

+     * @return a file-name such that it does not match any existing files in targetDirectory.
+     * */
+    public static String getNonOverWritingFileName(Path targetDirectory, String fileName) {
+//        String absoluteName = targetDirectory.resolve(fileName)


Please remove this comment.

tobiasdiez · 2020-03-28T14:21:03Z

src/main/java/org/jabref/logic/util/io/FileNameUniqueness.java

+            fileNameWithoutExtension = fileName;
+        }
+
+//        Path absolutePath = Paths.get(absoluteName);


This one as well.

tobiasdiez · 2020-03-28T14:23:19Z

src/test/java/org/jabref/logic/util/io/FileNameUniquenessTest.java

+    protected Path tempDir;
+
+    @Test
+    public void testGetNonOverWritingFileNameReturnsSameName(@TempDir Path tempDirectory) throws IOException {


Is there a need to have the temporary directory as a variable as well as an argument? I would prefer if all tests use the same format (either variable or either paramater)

tobiasdiez · 2020-03-28T14:24:11Z

src/main/java/org/jabref/logic/util/io/FileNameUniqueness.java

+public class FileNameUniqueness {
+
+    /**
+     * Returns an absolute path to a file which does not match with any existing file names


The description seems to be off as no absolute path is returned but only the file name, right?

tobiasdiez · 2020-03-28T14:25:00Z

src/test/java/org/jabref/logic/util/io/FileNameUniquenessTest.java

+        String outputFileName = FileNameUniqueness.getNonOverWritingFileName(tempDir, "differentFile.txt");
+        assertEquals("differentFile (1).txt", outputFileName);
+
+        Files.delete(dummyFilePath1);


Deleting the file shouldn't be necessary as the TempDir should take care of this.

koppor · 2020-03-29T23:28:40Z

Comments addressed. There are some code style issues, which I will quickly fix in master after merge.

koppor · 2020-03-29T23:32:39Z

Thank you for having kept up the work. Looking forward to more contributions!

b9302fd Update APA styles for "event" macro (#6174) d4daec6 remove DOI for printed articles organizational-studies.csl (#6176) acfc620 Create liver-transplantation.csl (#6167) 129a775 Change "event" to "event-title" (#6164) git-subtree-dir: buildres/csl/csl-styles git-subtree-split: b9302fd

) b9302fd Update APA styles for "event" macro (#6174) d4daec6 remove DOI for printed articles organizational-studies.csl (#6176) acfc620 Create liver-transplantation.csl (#6167) 129a775 Change "event" to "event-title" (#6164) git-subtree-dir: buildres/csl/csl-styles git-subtree-split: b9302fd Co-authored-by: github actions <jabrefmail+webfeedback@gmail.com>

8d69f16 Create university-of-hull-harvard.csl (#6146) 139dfdd Create current organic synthesis.csl (#6139) bb006c8 Update acta-universitatis-agriculturae-sueciae.csl (#6143) 5815da0 Create food-science-and-biotechnology.csl (#6132) 2702a7c Update harvard-university-for-the-creative-arts.csl (#6104) ef34543 Update economic-geology.csl (#6128) 0adcd30 Bump mathieudutour/github-tag-action from 5.6 to 6.0 (#6141) 3c36e97 Create universite-du-quebec-a-montreal-prenoms.csl (#6073) 415bc05 Bump softprops/action-gh-release from 0.1.14 to 1 (#6142) ae8c5e4 Create politique-europeenne.csl (#6074) 09cbc09 Update cell-numeric-superscript.csl (#6188) 6ee1ace Update avian-conservation-and-ecology.csl (#6191) cb5c43f Update harvard-anglia-ruskin-university.csl (#6189) 5c4f4c0 Create anais-da-academia-brasileira-de-ciencias.csl (#6066) a60dfe9 Update cardiff-university-harvard.csl (#6190) 999a45c Create sociologia-urbana-e-rurale.csl (#6042) 1bc9d62 Bluebook (#6183) a4f2a72 Oxford Brookes (#6182) 88df8d5 Delete harvard-cardiff-university-old.csl (#6180) b9302fd Update APA styles for "event" macro (#6174) d4daec6 remove DOI for printed articles organizational-studies.csl (#6176) acfc620 Create liver-transplantation.csl (#6167) 129a775 Change "event" to "event-title" (#6164) git-subtree-dir: buildres/csl/csl-styles git-subtree-split: 8d69f16

gtam25 force-pushed the checkEntriesHaveBibtexKey branch from c4c9aa8 to 10ee0ef Compare March 25, 2020 09:01

koppor requested changes Mar 25, 2020

View reviewed changes

fixes JabRef#6068

155fbe3

- fixed downloading file with same name - updated changelog

gtam25 force-pushed the checkEntriesHaveBibtexKey branch from 7f4af8c to 155fbe3 Compare March 25, 2020 15:20

code check style

1bfb779

tobiasdiez requested changes Mar 28, 2020

View reviewed changes

Remove commented code and refactor

838539c

koppor mentioned this pull request Mar 28, 2020

Do not store downloaded file if it is already attached #6197

Closed

JabRef deleted a comment from codecov bot Mar 29, 2020

koppor merged commit 7ec1d0c into JabRef:master Mar 29, 2020

	for (int counter = 1; Files.exists(absolutePath); ++counter) {
	for (int counter = 1; Files.exists(absolutePath); counter++) {

Uh oh!

Conversation

gtam25 commented Mar 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

koppor left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gtam25 commented Mar 25, 2020

Uh oh!

koppor commented Mar 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tobiasdiez left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

koppor commented Mar 29, 2020

Uh oh!

koppor commented Mar 29, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gtam25 commented Mar 25, 2020 •

edited

Loading

koppor commented Mar 25, 2020 •

edited

Loading