improve unlinked files dialog #12195

agondekova · 2024-11-16T21:27:03Z

Description of changes:

I modified the display of file paths in the "File" column so that relative paths, starting from the directory specified in the "Directory" field, are shown instead of full absolute paths. I also added the ability to resize the columns in the table, making them customizable for the user. Additionally, I changed the alignment of the text in the column, which was previously aligned to the right, to the left, as described in the issue, to ensure proper display when the text doesn’t fit within the available space.

This change improves the clarity and flexibility when working with the table.

Closes #11878.

Mandatory checks

I own the copyright of the code submitted and I licence it under the MIT license
Change in CHANGELOG.md described in a way that is understandable for the average user (if change is visible to the user)
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.

…: Improve dialog

github-actions

Your code currently does not meet JabRef's code guidelines.
We use OpenRewrite to ensure "modern" Java coding practices.
The issues found can be automatically fixed.
Please execute the gradle task rewriteRun, check the results, commit, and push.

You can check the detailed error output by navigating to your pull request, selecting the tab "Checks", section "Tests" (on the left), subsection "OpenRewrite".

Siedlerchr · 2024-11-16T22:27:58Z

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

+    @Test
+    public void testStartImportWithValidFiles() throws Exception {
+        // Create temporary test files
+        Path tempDir = Files.createTempDirectory("testDir");


You can use the @tempdir annotaiton from junit jupiter https://www.baeldung.com/junit-5-temporary-directory#1-tempdir-as-a-method-parameter
it has the advantage of auto cleanup of files, so no manual deleting necessary

Siedlerchr

codewise looks good already, just some small improvements.
will do a proper test tomorrow

Siedlerchr · 2024-11-16T22:30:53Z

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

@@ -145,9 +146,11 @@ public void startSearch() {
    }

    public void startImport() {
+        Path directory = Paths.get(this.getSearchDirectory().toString()); // Get the base directory


This is more complicated than it needs to be:

Suggested change

Path directory = Paths.get(this.getSearchDirectory().toString()); // Get the base directory

Path directory = this.getSearchDirectory();

Changes implemented

Siedlerchr · 2024-11-16T22:34:20Z

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

        List<Path> fileList = checkedFileListProperty.stream()
                                                     .map(item -> item.getValue().getPath())
                                                     .filter(path -> path.toFile().isFile())
+                                                     .map(path -> directory.toAbsolutePath().relativize(path.toAbsolutePath())) // Convert to relative path


remoe the comment, it's clear from the code what is done.
Despite that I am not sure if you really need the second toAbsolutePath

Also, the directory should already be an absolute path?

No, currently it is only using relativize.

koppor · 2024-11-16T23:04:58Z

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

+        assertEquals(2, fileList.size(), "fileList should contain 2 paths");
+        assertTrue(fileList.contains(directory.toAbsolutePath().relativize(tempFile1.toAbsolutePath())),
+                "fileList should contain the relative path of file1.pdf");
+        assertTrue(fileList.contains(directory.toAbsolutePath().relativize(tempFile2.toAbsolutePath())),
+                "fileList should contain the relative path of file2.txt");


Do one assertEquals

assertEquals(List.of(directory.toAbsolutePath().relativize(tempFile1.toAbsolutePath()), fileList.contains(directory.toAbsolutePath().relativize(tempFile2.toAbsolutePath())), fileList)

Then, you checked both size and the containment.

Changes implemented

koppor · 2024-11-16T23:05:09Z

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

+        Files.deleteIfExists(tempFile1);
+        Files.deleteIfExists(tempFile2);
+        Files.deleteIfExists(subDir);
+        Files.deleteIfExists(tempDir);


No need if youuse @TempDir

changes implemented

subhramit · 2024-11-20T12:01:45Z

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

+import org.mockito.Mock;
+import org.mockito.MockitoAnnotations;
+
+import static org.jabref.gui.edit.automaticfiededitor.AbstractAutomaticFieldEditorTabViewModel.LOGGER;


Woah, what? And why?

You're right, the LOGGER wasn’t needed here. I’ve removed it. Let me know if there’s anything else. :)

That was one part, but I was primarily wondering why you would import the logger from AbstractAutomaticFieldEditorTabViewModel even if you are to use one.

The usage of AI in this PR is pretty high, so it is important that you understand what you are doing, why you would or would not use something.
Rest looks good from my side.

github-actions

Your code currently does not meet JabRef's code guidelines.
We use OpenRewrite to ensure "modern" Java coding practices.
The issues found can be automatically fixed.
Please execute the gradle task rewriteRun, check the results, commit, and push.

You can check the detailed error output by navigating to your pull request, selecting the tab "Checks", section "Tests" (on the left), subsection "OpenRewrite".

AnickaG added 3 commits November 9, 2024 23:36

Implement changes for issue JabRef#11878: Search Unlinked local files…

e118e2c

…: Improve dialog

test: add unit test for verifying relative path functionality

07ccdf0

Merge branch 'main' into fix-for-issue-11878

c4c9a97

github-actions bot reviewed Nov 16, 2024

View reviewed changes

Siedlerchr changed the title ~~Fix for issue 11878~~ improve unlinked files dialog Nov 16, 2024

Siedlerchr reviewed Nov 16, 2024

View reviewed changes

Siedlerchr requested changes Nov 16, 2024

View reviewed changes

Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Nov 16, 2024

koppor requested changes Nov 16, 2024

View reviewed changes

subhramit reviewed Nov 20, 2024

View reviewed changes

refactor: changes based on PR feedback

96a3ce3

github-actions bot reviewed Nov 21, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improve unlinked files dialog #12195

improve unlinked files dialog #12195

agondekova commented Nov 16, 2024

github-actions bot left a comment

Siedlerchr Nov 16, 2024 •

edited

Loading

Siedlerchr left a comment

Siedlerchr Nov 16, 2024

agondekova Nov 21, 2024

Siedlerchr Nov 16, 2024

koppor Nov 16, 2024

agondekova Nov 21, 2024

koppor Nov 16, 2024

agondekova Nov 21, 2024

koppor Nov 16, 2024

agondekova Nov 21, 2024

subhramit Nov 20, 2024

agondekova Nov 21, 2024

subhramit Nov 22, 2024

github-actions bot left a comment

	Path directory = Paths.get(this.getSearchDirectory().toString()); // Get the base directory
	Path directory = this.getSearchDirectory();

improve unlinked files dialog #12195

Are you sure you want to change the base?

improve unlinked files dialog #12195

Conversation

agondekova commented Nov 16, 2024

Mandatory checks

github-actions bot left a comment

Choose a reason for hiding this comment

Siedlerchr Nov 16, 2024 • edited Loading

Choose a reason for hiding this comment

Siedlerchr 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

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

Siedlerchr Nov 16, 2024 •

edited

Loading