Add feature relinking moved file #10954

RiLoGosh · 2024-03-01T15:35:00Z

We implemented a solution for the first implementation mentioned in the issue. This implementation is based on the work made in closed issue #10526. Their code was incomplete based on the suggestions made and we implemented those changes. Refactored the code and associated classes to allow for a FileNotFoundException. The class performs a move operation more traditionally than copying and deleting as previously. The relinkingFiles method now returns a list of not found file names. The test setup has been altered to not affect all tests. The searchFileInDirectoryAndSubdirectories method now returns List instead of string. Additional tests were made for the class.

Changes were produced primarily through test-driven development, wherein functionality was added or adapted after testing its associated mechanics.

We would love to hear your feedback on what is done so far. Were there limitations with the work done in issue #10526? After all we saw the comment about implementing a FileWatcher system recently, and were curious if that is a path worth pursuing still?

Mandatory checks

Change in CHANGELOG.md described in a way that is understandable for the average user (if applicable)
Tests created for changes (if applicable)
Manually tested changed features in running JabRef (always required)
Screenshots added in PR description (for UI changes)
Checked developer's documentation: Is the information available and up to date? If not, I outlined it in this pull request.
Checked documentation: Is the information available and up to date? If not, I created an issue at https://github.com/JabRef/user-documentation/issues or, even better, I submitted a pull request to the documentation repository.

…xception and refactored.

…[email protected]

…l.com

…[email protected]

We added file relinking functionality after a file is moved when clicking the "Automatically set file links" button. Implementation based and improved upon work done in closed issue [JabRef#10526]. [JabRef#9798]

koppor

I quickly ran through the code to provide initial feedback.

You text says, you would move a file, but your main code does not (which is good). I wonder why an exception is risen in the main code. If JabRef looks up files, there should be no exception thrown to the ultimate caller. Please catch the exception approriatly.

See, if a user moved a file and JabRef cannot relink, it should stay at the old location.

Please also add a test case for the exceptions - if possible. You could try https://github.com/google/jimfs for it.

You added a single test, not sure if that is enough. Maybe add the test case described int he issue. There is folder-a and I did not find the term folder-a in your code.

koppor · 2024-03-04T11:20:58Z

CHANGELOG.md

@@ -21,6 +21,7 @@ Note that this project **does not** adhere to [Semantic Versioning](https://semv
 - When pasting HTML into the abstract or a comment field, the hypertext is automatically converted to Markdown. [#10558](https://github.com/JabRef/jabref/issues/10558)
 - We added the possibility to redownload files that had been present but are no longer in the specified location. [#10848](https://github.com/JabRef/jabref/issues/10848)
 - We added the citation key pattern `[camelN]`. Equivalent to the first N words of the `[camel]` pattern.
+- We added file relinking functionality after a file is moved when clicking the "Automatically set file links" button. Implementation based and improved upon work done in closed issue [#10526]. [#9798]


CHANGELOG is only for user-facing changes. You can just remove the last part.

Please add a full link to the issue as shown in the other lines of the CHANGELOG.md

koppor · 2024-03-04T11:21:52Z

src/main/java/org/jabref/cli/ArgumentProcessor.java

@@ -188,7 +189,7 @@ private Optional<ParserResult> importFile(Path file, String importFormat) {
        }
    }

-    public void processArguments() {
+    public void processArguments() throws FileNotFoundException {


Please add JavaDoc @throws to state when this exception is thrown.

It is strange that upon startup of JabRef something is happening.

The issue itself has two headings "First implementation" and "Follow-up implementation". I cannot relate something happening on startup at each of the headings. Maybe you can explain?

koppor · 2024-03-04T11:23:14Z

src/main/java/org/jabref/gui/externalfiles/AutoSetFileLinksUtil.java

            fileExceptions.add(exception);
        }

        public List<BibEntry> getChangedEntries() {
            return changedEntries;
        }

-        public List<IOException> getFileExceptions() {
+        public List<String> getFileExceptions() {


Why reduction to weaker type? String should not be used if more finer-grainded data types are available.

See https://www.pearson.de/effective-java-9780134686073

koppor · 2024-03-04T11:25:14Z

src/main/java/org/jabref/gui/externalfiles/AutoSetFileLinksUtil.java

                if (changed) {
                    result.addBibEntry(entry);
                }
+                // Run Relinking Process


This comment does not say anything... You can extract a method to group statements together.

The algorithm itself should be explained

koppor · 2024-03-04T11:26:18Z

src/test/java/org/jabref/gui/externalfiles/AutoSetFileLinksUtilTest.java

+        List<String> exceptions = results.exceptions();
+        assertEquals(Collections.emptyList(), exceptions);
+
+        // Change Entry to match required result and run method on bib


"required result" is too unspecifiy. Explain, what is the intention.

koppor · 2024-03-04T11:33:56Z

src/main/java/org/jabref/gui/externalfiles/AutoSetFileLinksUtil.java

+
+        for (LinkedFile file : listlinked) {
+            Path path = Path.of(file.getLink());
+            if (!Files.exists(path)) {


This is incomplete.

Use org.jabref.logic.util.io.FileUtil#find(org.jabref.model.database.BibDatabaseContext, java.lang.String, org.jabref.preferences.FilePreferences)

Reason: JabRef has multiple directories to store files. See https://docs.jabref.org/finding-sorting-and-cleaning-entries/filelinks#directories-for-files for an explanation.

koppor · 2024-03-04T11:34:26Z

src/main/java/org/jabref/gui/externalfiles/AutoSetFileLinksUtil.java

+
+                String fileNameString = filePath.getFileName().toString();
+
+                List<Path> fileLocations = searchFileInDirectoryAndSubdirectories(Path.of(directoryPath), fileNameString);


Wrong start. Search in databaseContext.getFileDirectories(filePreferences) only.

koppor · 2024-03-04T11:36:13Z

src/main/java/org/jabref/gui/externalfiles/AutoSetFileLinksUtil.java

+                    exceptions.add(fileNameString);
+                    throw new FileNotFoundException();


No exception here. Just leave the link as is.

Exceptions are huge derivations from the normal results.

See, if I have a library wtih 10000 PDFs, moved 20 files and 19 can be relinked, the whole processing should not be stopped if one file cannot be found.

It is OK to report a list of files which could not be relinked. If this is hard, then just log the non-found files.

koppor · 2024-03-04T11:36:42Z

src/main/java/org/jabref/gui/externalfiles/AutoSetFileLinksUtil.java

+                 .filter(path -> path.getFileName().toString().equals(targetFileName))
+                 .forEach(paths::add);
+        } catch (IOException e) {
+            // Handle any exceptions here


This seems to be ChatGPT generated.

You can simply log the exception.

koppor · 2024-03-04T11:37:02Z

src/main/java/org/jabref/gui/externalfiles/AutoSetFileLinksUtil.java

+        List<Path> output = new ArrayList<>();
+        for (Path p : paths) {
+            output.add(p);
+        }
+        return output;


What is this?

Why not return paths?

calixtus · 2024-03-04T11:38:53Z

src/main/java/org/jabref/gui/externalfiles/AutoSetFileLinksUtil.java

@@ -75,7 +79,7 @@ public LinkFilesResult linkAssociatedFiles(List<BibEntry> entries, NamedCompound
            try {
                linkedFiles = findAssociatedNotLinkedFiles(entry);
            } catch (IOException e) {
-                result.addFileException(e);
+                result.addFileException(String.valueOf(e));


Can you please describe, why this change seems necessary?

koppor · 2024-03-05T15:01:35Z

At #9798 (comment) the authors said, they won't continue.

RiLoGosh and others added 5 commits March 1, 2024 11:17

feat: cloned u74981018's code and tested locally. Added FileNotFoundE…

27f0651

…xception and refactored.

fix: addressed owner's feedback suggestions. Co-authored-by: Serhan c…

2d94893

…[email protected]

feat: Added further tests. Co-authored-by: Serhan cakmakserhan02@gmai…

533eaa0

…l.com

fix: corrected checkstyle comments. Co-authored-by: Serhan cakmakserh…

ca0d08b

…[email protected]

Update CHANGELOG.md

dc57b2b

We added file relinking functionality after a file is moved when clicking the "Automatically set file links" button. Implementation based and improved upon work done in closed issue [JabRef#10526]. [JabRef#9798]

koppor requested changes Mar 4, 2024

View reviewed changes

calixtus reviewed Mar 4, 2024

View reviewed changes

koppor closed this Mar 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add feature relinking moved file #10954

Add feature relinking moved file #10954

RiLoGosh commented Mar 1, 2024 •

edited by calixtus

Loading

koppor left a comment

koppor Mar 4, 2024

koppor Mar 4, 2024

koppor Mar 4, 2024

koppor Mar 4, 2024

koppor Mar 4, 2024

koppor Mar 4, 2024

koppor Mar 4, 2024

koppor Mar 4, 2024

koppor Mar 4, 2024

koppor Mar 4, 2024

calixtus Mar 4, 2024

koppor commented Mar 5, 2024


		String fileNameString = filePath.getFileName().toString();

		List<Path> fileLocations = searchFileInDirectoryAndSubdirectories(Path.of(directoryPath), fileNameString);

		exceptions.add(fileNameString);
		throw new FileNotFoundException();

Add feature relinking moved file #10954

Add feature relinking moved file #10954

Conversation

RiLoGosh commented Mar 1, 2024 • edited by calixtus Loading

Mandatory checks

koppor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

koppor commented Mar 5, 2024

RiLoGosh commented Mar 1, 2024 •

edited by calixtus

Loading