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.

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

improve unlinked files dialog #12195

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@
<TableColumn fx:id="colMessage" prefWidth="320.0" text="%Message"/>
</columns>
<columnResizePolicy>
<TableView fx:constant="UNCONSTRAINED_RESIZE_POLICY"/>
<TableView fx:constant="CONSTRAINED_RESIZE_POLICY"/>
</columnResizePolicy>
</TableView>
</TitledPane>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -217,9 +217,11 @@ private void initResultTable() {

colStatus.setCellValueFactory(cellData -> cellData.getValue().icon());
colStatus.setCellFactory(new ValueTableCellFactory<ImportFilesResultItemViewModel, JabRefIcon>().withGraphic(JabRefIcon::getGraphicNode));
importResultTable.setColumnResizePolicy(param -> true);

colFile.setResizable(true);
colStatus.setResizable(true);
colMessage.setResizable(true);
importResultTable.setItems(viewModel.resultTableItems());
importResultTable.setColumnResizePolicy(TableView.CONSTRAINED_RESIZE_POLICY);
}

private void initButtons() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import java.nio.file.DirectoryStream.Filter;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.nio.file.StandardOpenOption;
import java.util.List;
import java.util.Optional;
Expand Down Expand Up @@ -145,9 +146,11 @@ public void startSearch() {
}

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

Choose a reason for hiding this comment

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

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();

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
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Also, the directory should already be an absolute path?

.collect(Collectors.toList());
if (fileList.isEmpty()) {
LOGGER.warn("There are no valid files checked");
Expand Down Expand Up @@ -202,7 +205,7 @@ public void startExport() {
} catch (IOException e) {
LOGGER.error("Error exporting", e);
}
}
}

public ObservableList<FileExtensionViewModel> getFileFilters() {
return this.fileFilterList;
Expand Down
3 changes: 3 additions & 0 deletions src/main/java/org/jabref/gui/util/ValueTableCellFactory.java
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import javafx.event.EventHandler;
import javafx.scene.Node;
import javafx.scene.control.ContextMenu;
import javafx.scene.control.OverrunStyle;
import javafx.scene.control.TableCell;
import javafx.scene.control.TableColumn;
import javafx.scene.control.Tooltip;
Expand Down Expand Up @@ -122,9 +123,11 @@ protected void updateItem(T item, boolean empty) {
if (StringUtil.isNotBlank(tooltipText)) {
Screen currentScreen = Screen.getPrimary();
double maxWidth = currentScreen.getBounds().getWidth();
setTextOverrun(OverrunStyle.LEADING_ELLIPSIS);
Tooltip tooltip = new Tooltip(tooltipText);
tooltip.setMaxWidth(maxWidth * 2 / 3);
tooltip.setWrapText(true);

setTooltip(tooltip);
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,126 @@
package org.jabref.gui.externalfiles;

import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.util.List;
import java.util.Optional;
import java.util.stream.Collectors;

import javafx.beans.property.SimpleListProperty;
import javafx.collections.FXCollections;
import javafx.scene.control.TreeItem;

import org.jabref.gui.StateManager;
import org.jabref.gui.preferences.GuiPreferences;
import org.jabref.gui.util.FileNodeViewModel;
import org.jabref.logic.FilePreferences;
import org.jabref.logic.util.TaskExecutor;
import org.jabref.model.database.BibDatabaseContext;

import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.mockito.Mock;
import org.mockito.MockitoAnnotations;

import static org.jabref.gui.edit.automaticfiededitor.AbstractAutomaticFieldEditorTabViewModel.LOGGER;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;

public class UnlinkedFilesDialogViewModelTest {

@Mock
private ImportHandler importHandler;
@Mock
private TaskExecutor taskExecutor;
@Mock
private GuiPreferences guiPreferences;
@Mock
private StateManager stateManager;
@Mock
private BibDatabaseContext bibDatabaseContext;

private UnlinkedFilesDialogViewModel viewModel;

@BeforeEach
public void setUp() throws Exception {
MockitoAnnotations.openMocks(this);

// Mock file preferences to return a base directory
FilePreferences filePreferences = mock(FilePreferences.class);
when(guiPreferences.getFilePreferences()).thenReturn(filePreferences);
when(filePreferences.getWorkingDirectory()).thenReturn(Paths.get("C:/test/base"));

// Mock the state manager to provide an active database
when(stateManager.getActiveDatabase()).thenReturn(Optional.of(bibDatabaseContext));

viewModel = new UnlinkedFilesDialogViewModel(
null, // dialogService, not used in this test
null, // undoManager, not used in this test
null, // fileUpdateMonitor, not used in this test
guiPreferences,
stateManager,
taskExecutor
);
}

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

@Siedlerchr Siedlerchr Nov 16, 2024

Choose a reason for hiding this comment

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

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

Path subDir = tempDir.resolve("subdir");
Files.createDirectories(subDir);

// Create test files: one in the main directory and one in the subdirectory
Path tempFile1 = Files.createTempFile(tempDir, "file1", ".pdf"); // Main directory
Path tempFile2 = Files.createTempFile(subDir, "file2", ".txt");

// Arrange: Mock file nodes with the absolute paths of the temporary files
FileNodeViewModel fileNode1 = mock(FileNodeViewModel.class);
FileNodeViewModel fileNode2 = mock(FileNodeViewModel.class);

// Mock the getPath method to return the expected paths
when(fileNode1.getPath()).thenReturn(tempFile1);
when(fileNode2.getPath()).thenReturn(tempFile2);

// Create TreeItem for each FileNodeViewModel
TreeItem<FileNodeViewModel> treeItem1 = new TreeItem<>(fileNode1);
TreeItem<FileNodeViewModel> treeItem2 = new TreeItem<>(fileNode2);

// Initialize SimpleListProperty and assign ObservableList
SimpleListProperty<TreeItem<FileNodeViewModel>> checkedFileListProperty =
new SimpleListProperty<>(FXCollections.observableArrayList(treeItem1, treeItem2));

// Assert that the list contains 2 items
assertEquals(2, checkedFileListProperty.get().size());
assertEquals(tempFile1, checkedFileListProperty.get().get(0).getValue().getPath());
assertEquals(tempFile2, checkedFileListProperty.get().get(1).getValue().getPath());

Path directory = tempDir; // Base directory for relativization

// Create list of relative paths
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
.collect(Collectors.toList());

// Check if fileList is empty
if (fileList.isEmpty()) {
LOGGER.warn("There are no valid files checked");
return;
}

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");
Comment on lines +116 to +120
Copy link
Member

Choose a reason for hiding this comment

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

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.

Files.deleteIfExists(tempFile1);
Files.deleteIfExists(tempFile2);
Files.deleteIfExists(subDir);
Files.deleteIfExists(tempDir);
Comment on lines +121 to +124
Copy link
Member

Choose a reason for hiding this comment

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

No need if youuse @TempDir

}
}
Loading