[WIP] File link deletion dialog improvements #3690
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,154 @@ | ||
| package org.jabref.gui.fieldeditors; | ||
|
|
||
| import java.io.File; | ||
| import java.io.IOException; | ||
| import java.util.Optional; | ||
|
|
||
| import javafx.scene.control.Alert.AlertType; | ||
| import javafx.scene.control.ButtonType; | ||
|
|
||
| import org.jabref.gui.DialogService; | ||
| import org.jabref.gui.util.TaskExecutor; | ||
| import org.jabref.model.database.BibDatabaseContext; | ||
| import org.jabref.model.entry.BibEntry; | ||
| import org.jabref.model.entry.LinkedFile; | ||
| import org.jabref.model.metadata.FileDirectoryPreferences; | ||
|
|
||
| import org.junit.Before; | ||
| import org.junit.Rule; | ||
| import org.junit.Test; | ||
| import org.junit.rules.TemporaryFolder; | ||
|
|
||
| import static org.junit.Assert.assertFalse; | ||
| import static org.junit.Assert.assertTrue; | ||
| import static org.mockito.ArgumentMatchers.any; | ||
| import static org.mockito.ArgumentMatchers.anyString; | ||
| import static org.mockito.Mockito.doReturn; | ||
| import static org.mockito.Mockito.mock; | ||
| import static org.mockito.Mockito.spy; | ||
| import static org.mockito.Mockito.verify; | ||
| import static org.mockito.Mockito.verifyZeroInteractions; | ||
| import static org.mockito.Mockito.when; | ||
|
|
||
| public class LinkedFileViewModelTest { | ||
|
|
||
| @Rule public TemporaryFolder tempFolder = new TemporaryFolder(); | ||
| private LinkedFile linkedFile; | ||
| private BibEntry entry; | ||
| private BibDatabaseContext databaseContext; | ||
| private TaskExecutor taskExecutor; | ||
| private DialogService dialogService; | ||
|
|
||
| @Before | ||
| public void setUp() { | ||
| entry = new BibEntry(); | ||
| databaseContext = new BibDatabaseContext(); | ||
| taskExecutor = mock(TaskExecutor.class); | ||
| dialogService = mock(DialogService.class); | ||
| } | ||
|
|
||
| @Test | ||
| public void deleteFilePathNotPresent() { | ||
| // Making this a spy, so we can inject an empty optional without digging into the implementation | ||
| linkedFile = spy(new LinkedFile("", "", "")); | ||
|
There was a problem hiding this comment. You could also pass "non-existent file" as the path as you do below. This might be a bit easier to understand. |
||
| doReturn(Optional.empty()).when(linkedFile).findIn( | ||
| any(BibDatabaseContext.class), any(FileDirectoryPreferences.class) | ||
| ); | ||
|
|
||
| LinkedFileViewModel viewModel = new LinkedFileViewModel( | ||
| linkedFile, entry, databaseContext, taskExecutor, dialogService | ||
| ); | ||
|
There was a problem hiding this comment. I'm not sure, but I think our style conventions say that the closing parenthesis has to go on the same line. Right, @koppor ? |
||
| boolean removed = viewModel.delete(); | ||
|
|
||
| assertFalse(removed); | ||
| verifyZeroInteractions(dialogService); // dialog was never shown | ||
| } | ||
|
|
||
| @Test | ||
| public void deleteDialogRemoveFromEntry() throws IOException { | ||
| File tempFile = tempFolder.newFile(); | ||
| linkedFile = new LinkedFile("", tempFile.getAbsolutePath(), ""); | ||
| when(dialogService.showCustomButtonDialogAndWait( | ||
| any(AlertType.class), | ||
| anyString(), | ||
| anyString(), | ||
| any(ButtonType.class), | ||
| any(ButtonType.class), | ||
| any(ButtonType.class) | ||
| )).thenAnswer(invocation -> Optional.of(invocation.getArgument(3))); // first vararg - remove button | ||
|
There was a problem hiding this comment. To explain myself with this -- the implementation uses strict reference equality for checking user choices (at least for the first two), so I thought about either getting out the parameter from the invocation like this, or modifying the equality checks to use the |
||
|
|
||
| LinkedFileViewModel viewModel = new LinkedFileViewModel( | ||
| linkedFile, entry, databaseContext, taskExecutor, dialogService | ||
| ); | ||
| boolean removed = viewModel.delete(); | ||
|
|
||
| assertTrue(removed); | ||
| assertTrue(tempFile.exists()); | ||
| } | ||
|
|
||
| @Test | ||
| public void deleteDialogDeleteFromEntry() throws IOException { | ||
| File tempFile = tempFolder.newFile(); | ||
| linkedFile = new LinkedFile("", tempFile.getAbsolutePath(), ""); | ||
| when(dialogService.showCustomButtonDialogAndWait( | ||
| any(AlertType.class), | ||
| anyString(), | ||
| anyString(), | ||
| any(ButtonType.class), | ||
| any(ButtonType.class), | ||
| any(ButtonType.class) | ||
| )).thenAnswer(invocation -> Optional.of(invocation.getArgument(4))); // second vararg - delete button | ||
|
|
||
| LinkedFileViewModel viewModel = new LinkedFileViewModel( | ||
| linkedFile, entry, databaseContext, taskExecutor, dialogService | ||
| ); | ||
| boolean removed = viewModel.delete(); | ||
|
|
||
| assertTrue(removed); | ||
| assertFalse(tempFile.exists()); | ||
| } | ||
|
|
||
|
|
||
| @Test | ||
| public void deleteDialogDeleteFromEntryException() throws IOException { | ||
| linkedFile = new LinkedFile("", "!!nonexistent file!!", ""); | ||
| when(dialogService.showCustomButtonDialogAndWait( | ||
| any(AlertType.class), | ||
| anyString(), | ||
| anyString(), | ||
| any(ButtonType.class), | ||
| any(ButtonType.class), | ||
| any(ButtonType.class) | ||
| )).thenAnswer(invocation -> Optional.of(invocation.getArgument(4))); // second vararg - delete button | ||
|
|
||
| LinkedFileViewModel viewModel = new LinkedFileViewModel( | ||
| linkedFile, entry, databaseContext, taskExecutor, dialogService | ||
| ); | ||
| boolean removed = viewModel.delete(); | ||
|
|
||
| verify(dialogService).showErrorDialogAndWait(anyString(), anyString()); | ||
| assertFalse(removed); | ||
| } | ||
|
|
||
| @Test | ||
| public void deleteDialogCanceled() throws IOException { | ||
|
There was a problem hiding this comment. In general we try to name the tests according to the scheme |
||
| File tempFile = tempFolder.newFile(); | ||
| linkedFile = new LinkedFile("desc", tempFile.getAbsolutePath(), "pdf"); | ||
| when(dialogService.showCustomButtonDialogAndWait( | ||
| any(AlertType.class), | ||
| anyString(), | ||
| anyString(), | ||
| any(ButtonType.class), | ||
| any(ButtonType.class), | ||
| any(ButtonType.class) | ||
| )).thenAnswer(invocation -> Optional.of(invocation.getArgument(5))); // third vararg - cancel button | ||
|
|
||
| LinkedFileViewModel viewModel = new LinkedFileViewModel( | ||
| linkedFile, entry, databaseContext, taskExecutor, dialogService | ||
| ); | ||
| boolean removed = viewModel.delete(); | ||
|
|
||
| assertFalse(removed); | ||
| assertTrue(tempFile.exists()); | ||
| } | ||
| } | ||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is fine. Could you please also add ButtonData.Yes as a second constructor argument for the
removeFromEntrybutton. With this the buttons are automatically ordered correctly according to the current OS specifications.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, not a problem. Would you like to have the change rebased into the first commit, or is a new one on top of the current two acceptable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just add them in a new commit. We usually squash all commits on merge