Skip to content

Commit b960da1

Browse files
bdachtobiasdiez
authored andcommitted
[WIP] File link deletion dialog improvements (#3690)
* Only remove file links from entries by default (#3679) When a file link in a bibliography entry is deleted by the user, a dialog is displayed. Previously, the default (first) dialog option deleted the linked file from disk. This commit changes the default behaviour to just removing the file link from the entry. * Fix canceling bug in file link deletion dialog When clicking "Cancel" button in the file link deletion dialog, the file link was being deleted from the list. This commit fixes this behaviour by changing the return value of the delete() method when the "Cancel" button is chosen. * Tag link removal button as default (#3679) The "remove from entry" option in the file link removal dialog window has been tagged with the ButtonData.YES type to ensure correct positioning and behaviour on all platforms. * Add test cases for linked file removal dialog * Code style adjustments in dialog test cases * Added non-empty link to file in one test for clarity * Removed unnecessary newlines * Changed test method names to adhere to chosen naming convention
1 parent 6bac6ff commit b960da1

File tree

3 files changed

+157
-4
lines changed

3 files changed

+157
-4
lines changed

CHANGELOG.md

+3
Original file line numberDiff line numberDiff line change
@@ -23,13 +23,16 @@ For more details refer to the [field mapping help page](http://help.jabref.org/e
2323
- We added Facebook and Twitter icons in the toolbar to link to our [Facebook](https://www.facebook.com/JabRef/) and [Twitter](https://twitter.com/jabref_org) pages.
2424
- We no longer print empty lines when exporting an entry in RIS format [#3634](https://github.com/JabRef/jabref/issues/3634)
2525
- We improved file saving so that hard links are now preserved when a save is performed [#2633](https://github.com/JabRef/jabref/issues/2633)
26+
- We changed the default dialog option when removing a [file link](http://help.jabref.org/en/FileLinks#adding-external-links-to-an-entry) from an entry.
27+
The new default removes the linked file from the entry instead of deleting the file from disk. [#3679](https://github.com/JabRef/jabref/issues/3679)
2628

2729
### Fixed
2830
- We fixed the missing dot in the name of an exported file. [#3576](https://github.com/JabRef/jabref/issues/3576)
2931
- Autocompletion in the search bar can now be disabled via the preferences. [#3598](https://github.com/JabRef/jabref/issues/3598)
3032
- We fixed and extended the RIS import functionality to cover more fields. [#3634](https://github.com/JabRef/jabref/issues/3634) [#2607](https://github.com/JabRef/jabref/issues/2607)
3133
- Chaining modifiers in BibTeX key pattern now works as described in the documentation. [#3648](https://github.com/JabRef/jabref/issues/3648)
3234
- We fixed an issue where not all bibtex/biblatex fields would be exported as latex-free to MS-Office XML [koppor#284](https://github.com/koppor/jabref/issues/284)
35+
- We fixed an issue where linked files would be deleted from bibliography entries despite choosing the "Cancel" option in the dialog menu.
3336

3437
### Removed
3538
- We removed the [Look and Feels from JGoodies](http://www.jgoodies.com/freeware/libraries/looks/), because the open source version is not compatible with Java 9.

src/main/java/org/jabref/gui/fieldeditors/LinkedFileViewModel.java

+12-4
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,8 @@
4444
import org.slf4j.Logger;
4545
import org.slf4j.LoggerFactory;
4646

47+
import static javafx.scene.control.ButtonBar.ButtonData;
48+
4749
public class LinkedFileViewModel extends AbstractViewModel {
4850

4951
private static final Logger LOGGER = LoggerFactory.getLogger(LinkedFileViewModel.class);
@@ -54,15 +56,21 @@ public class LinkedFileViewModel extends AbstractViewModel {
5456
private final BooleanProperty downloadOngoing = new SimpleBooleanProperty(false);
5557
private final BooleanProperty isAutomaticallyFound = new SimpleBooleanProperty(false);
5658
private final BooleanProperty canWriteXMPMetadata = new SimpleBooleanProperty(false);
57-
private final DialogService dialogService = new FXDialogService();
59+
private final DialogService dialogService;
5860
private final BibEntry entry;
5961
private final TaskExecutor taskExecutor;
6062

6163
public LinkedFileViewModel(LinkedFile linkedFile, BibEntry entry, BibDatabaseContext databaseContext, TaskExecutor taskExecutor) {
64+
this(linkedFile, entry, databaseContext, taskExecutor, new FXDialogService());
65+
}
66+
67+
protected LinkedFileViewModel(LinkedFile linkedFile, BibEntry entry, BibDatabaseContext databaseContext,
68+
TaskExecutor taskExecutor, DialogService dialogService) {
6269
this.linkedFile = linkedFile;
6370
this.databaseContext = databaseContext;
6471
this.entry = entry;
6572
this.taskExecutor = taskExecutor;
73+
this.dialogService = dialogService;
6674

6775
downloadOngoing.bind(downloadProgress.greaterThanOrEqualTo(0).and(downloadProgress.lessThan(100)));
6876
canWriteXMPMetadata.setValue(!linkedFile.isOnlineLink() && linkedFile.getFileType().equalsIgnoreCase("pdf"));
@@ -274,14 +282,14 @@ public void moveToDefaultDirectory() {
274282

275283
public boolean delete() {
276284
Optional<Path> file = linkedFile.findIn(databaseContext, Globals.prefs.getFileDirectoryPreferences());
277-
ButtonType removeFromEntry = new ButtonType(Localization.lang("Remove from entry"));
285+
ButtonType removeFromEntry = new ButtonType(Localization.lang("Remove from entry"), ButtonData.YES);
278286

279287
if (file.isPresent()) {
280288
ButtonType deleteFromEntry = new ButtonType(Localization.lang("Delete from disk"));
281289
Optional<ButtonType> buttonType = dialogService.showCustomButtonDialogAndWait(AlertType.INFORMATION,
282290
Localization.lang("Delete '%0'", file.get().toString()),
283291
Localization.lang("Delete the selected file permanently from disk, or just remove the file from the entry? Pressing Delete will delete the file permanently from disk."),
284-
deleteFromEntry, removeFromEntry, ButtonType.CANCEL);
292+
removeFromEntry, deleteFromEntry, ButtonType.CANCEL);
285293

286294
if (buttonType.isPresent()) {
287295
if (buttonType.get().equals(removeFromEntry)) {
@@ -304,7 +312,7 @@ public boolean delete() {
304312
} else {
305313
LOGGER.warn("Could not find file " + linkedFile.getLink());
306314
}
307-
return true;
315+
return false;
308316
}
309317

310318
public void edit() {
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,142 @@
1+
package org.jabref.gui.fieldeditors;
2+
3+
import java.io.File;
4+
import java.io.IOException;
5+
import java.util.Optional;
6+
7+
import javafx.scene.control.Alert.AlertType;
8+
import javafx.scene.control.ButtonType;
9+
10+
import org.jabref.gui.DialogService;
11+
import org.jabref.gui.util.TaskExecutor;
12+
import org.jabref.model.database.BibDatabaseContext;
13+
import org.jabref.model.entry.BibEntry;
14+
import org.jabref.model.entry.LinkedFile;
15+
import org.jabref.model.metadata.FileDirectoryPreferences;
16+
17+
import org.junit.Before;
18+
import org.junit.Rule;
19+
import org.junit.Test;
20+
import org.junit.rules.TemporaryFolder;
21+
22+
import static org.junit.Assert.assertFalse;
23+
import static org.junit.Assert.assertTrue;
24+
import static org.mockito.ArgumentMatchers.any;
25+
import static org.mockito.ArgumentMatchers.anyString;
26+
import static org.mockito.Mockito.doReturn;
27+
import static org.mockito.Mockito.mock;
28+
import static org.mockito.Mockito.spy;
29+
import static org.mockito.Mockito.verify;
30+
import static org.mockito.Mockito.verifyZeroInteractions;
31+
import static org.mockito.Mockito.when;
32+
33+
public class LinkedFileViewModelTest {
34+
35+
@Rule public TemporaryFolder tempFolder = new TemporaryFolder();
36+
private LinkedFile linkedFile;
37+
private BibEntry entry;
38+
private BibDatabaseContext databaseContext;
39+
private TaskExecutor taskExecutor;
40+
private DialogService dialogService;
41+
42+
@Before
43+
public void setUp() {
44+
entry = new BibEntry();
45+
databaseContext = new BibDatabaseContext();
46+
taskExecutor = mock(TaskExecutor.class);
47+
dialogService = mock(DialogService.class);
48+
}
49+
50+
@Test
51+
public void deleteWhenFilePathNotPresentReturnsFalse() {
52+
// Making this a spy, so we can inject an empty optional without digging into the implementation
53+
linkedFile = spy(new LinkedFile("", "nonexistent file", ""));
54+
doReturn(Optional.empty()).when(linkedFile).findIn(any(BibDatabaseContext.class), any(FileDirectoryPreferences.class));
55+
56+
LinkedFileViewModel viewModel = new LinkedFileViewModel(linkedFile, entry, databaseContext, taskExecutor, dialogService);
57+
boolean removed = viewModel.delete();
58+
59+
assertFalse(removed);
60+
verifyZeroInteractions(dialogService); // dialog was never shown
61+
}
62+
63+
@Test
64+
public void deleteWhenRemoveChosenReturnsTrue() throws IOException {
65+
File tempFile = tempFolder.newFile();
66+
linkedFile = new LinkedFile("", tempFile.getAbsolutePath(), "");
67+
when(dialogService.showCustomButtonDialogAndWait(
68+
any(AlertType.class),
69+
anyString(),
70+
anyString(),
71+
any(ButtonType.class),
72+
any(ButtonType.class),
73+
any(ButtonType.class)
74+
)).thenAnswer(invocation -> Optional.of(invocation.getArgument(3))); // first vararg - remove button
75+
76+
LinkedFileViewModel viewModel = new LinkedFileViewModel(linkedFile, entry, databaseContext, taskExecutor, dialogService);
77+
boolean removed = viewModel.delete();
78+
79+
assertTrue(removed);
80+
assertTrue(tempFile.exists());
81+
}
82+
83+
@Test
84+
public void deleteWhenDeleteChosenReturnsTrueAndDeletesFile() throws IOException {
85+
File tempFile = tempFolder.newFile();
86+
linkedFile = new LinkedFile("", tempFile.getAbsolutePath(), "");
87+
when(dialogService.showCustomButtonDialogAndWait(
88+
any(AlertType.class),
89+
anyString(),
90+
anyString(),
91+
any(ButtonType.class),
92+
any(ButtonType.class),
93+
any(ButtonType.class)
94+
)).thenAnswer(invocation -> Optional.of(invocation.getArgument(4))); // second vararg - delete button
95+
96+
LinkedFileViewModel viewModel = new LinkedFileViewModel(linkedFile, entry, databaseContext, taskExecutor, dialogService);
97+
boolean removed = viewModel.delete();
98+
99+
assertTrue(removed);
100+
assertFalse(tempFile.exists());
101+
}
102+
103+
104+
@Test
105+
public void deleteWhenDeleteChosenAndFileMissingReturnsFalse() throws IOException {
106+
linkedFile = new LinkedFile("", "!!nonexistent file!!", "");
107+
when(dialogService.showCustomButtonDialogAndWait(
108+
any(AlertType.class),
109+
anyString(),
110+
anyString(),
111+
any(ButtonType.class),
112+
any(ButtonType.class),
113+
any(ButtonType.class)
114+
)).thenAnswer(invocation -> Optional.of(invocation.getArgument(4))); // second vararg - delete button
115+
116+
LinkedFileViewModel viewModel = new LinkedFileViewModel(linkedFile, entry, databaseContext, taskExecutor, dialogService);
117+
boolean removed = viewModel.delete();
118+
119+
verify(dialogService).showErrorDialogAndWait(anyString(), anyString());
120+
assertFalse(removed);
121+
}
122+
123+
@Test
124+
public void deleteWhenDialogCancelledReturnsFalse() throws IOException {
125+
File tempFile = tempFolder.newFile();
126+
linkedFile = new LinkedFile("desc", tempFile.getAbsolutePath(), "pdf");
127+
when(dialogService.showCustomButtonDialogAndWait(
128+
any(AlertType.class),
129+
anyString(),
130+
anyString(),
131+
any(ButtonType.class),
132+
any(ButtonType.class),
133+
any(ButtonType.class)
134+
)).thenAnswer(invocation -> Optional.of(invocation.getArgument(5))); // third vararg - cancel button
135+
136+
LinkedFileViewModel viewModel = new LinkedFileViewModel(linkedFile, entry, databaseContext, taskExecutor, dialogService);
137+
boolean removed = viewModel.delete();
138+
139+
assertFalse(removed);
140+
assertTrue(tempFile.exists());
141+
}
142+
}

0 commit comments

Comments
 (0)