[WIP] File link deletion dialog improvements #3690

bdach · 2018-02-03T22:16:39Z

This pull request fixes the following issues:

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. The default behaviour has been changed to just removing the file link from the entry.

This is a fix to issue Delete from disk should not be default option/selected on DELETE in file tab #3679
Fixes Delete from disk should not be default option/selected on DELETE in file tab #3679
When clicking the "Cancel" button in the file link deletion dialog, the file was being deleted from the list. This behaviour has been fixed by changing the return value of the delete() method when the "Cancel" button is chosen.

Example screenshot:

Checklist:

Change in CHANGELOG.md described
Tests created for changes
Screenshots added
Manually tested changed features in running JabRef
Check documentation status (Issue created for outdated help page at help.jabref.org?)

I am unsure whether the remaining checklist items are applicable to this pull request.
I would be willing to provide tests for the delete() method; however, the DialogService member cannot be stubbed-in/mocked as it is (as it is marked final), and therefore I can't control the returned chosen option from inside the unit test.

I would appreciate any pointers and suggestions.

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.

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.

tobiasdiez · 2018-02-03T22:36:47Z

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

@@ -281,7 +281,7 @@ public boolean delete() {
            Optional<ButtonType> buttonType = dialogService.showCustomButtonDialogAndWait(AlertType.INFORMATION,
                    Localization.lang("Delete '%0'", file.get().toString()),
                    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."),
-                    deleteFromEntry, removeFromEntry, ButtonType.CANCEL);
+                    removeFromEntry, deleteFromEntry, ButtonType.CANCEL);


This change is fine. Could you please also add ButtonData.Yes as a second constructor argument for the removeFromEntry button. With this the buttons are automatically ordered correctly according to the current OS specifications.

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?

Just add them in a new commit. We usually squash all commits on merge

Siedlerchr · 2018-02-03T22:40:22Z

Thanks for your contribution! I have tested the changes locally and it works as expected.
A little step for you, a great step for ~~humanity~~ JabRef users ;)
Just add the suggestions by @tobiasdiez and we can merge it in.

PS: If you include the word Fixes #xxxx then the corresponding issue will be closed automatically when the PR is merged.

tobiasdiez

Also a big thank you for your contribution from me!

If you wan't to add tests (which would be a nice addition, but no strict requirement), the strategy would be to extract the dialog service dependency as a constructor argument and then mock it in the tests.

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.

bdach · 2018-02-03T23:38:04Z

Thank you for your pointers! I have locally added ButtonData.YES, and now I will attempt to prepare a test suite for the remove() method at least, and push everything out when it's ready. 😄

bdach · 2018-02-04T01:10:15Z

Okay, I pushed out the changes. Adding the button data restored the original button order (at least on Linux), but the remove button is now clearly the default, and as far as I can tell from JavaFX docs this is the desired behaviour.

The test code I'm not too proud of -- there's some weird hacking going on there to get the tests running, and I had to balance between not making them brittle and not worsening the implementation's readability just for tests' sake. If you have ideas for improvements, I'd be happy to go back and fix the ugliness.

bdach · 2018-02-04T01:20:49Z

src/test/java/org/jabref/gui/fieldeditors/LinkedFileViewModelTest.java

+                any(ButtonType.class),
+                any(ButtonType.class),
+                any(ButtonType.class)
+        )).thenAnswer(invocation -> Optional.of(invocation.getArgument(3))); // first vararg - remove button


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 ButtonData field in the button types, which made the implementation very weird looking.

tobiasdiez

You are right, the mocking in the tests needs a bit of thinking. However, I don't have an idea on how to improve the code (and it's not that bad).

I have a few suggestions, but these are so minor that you can decide if you want to revise your code or tackle the next issue instead. Regardless, you have my +1 for merge.

tobiasdiez · 2018-02-04T12:39:46Z

src/test/java/org/jabref/gui/fieldeditors/LinkedFileViewModelTest.java

+    @Test
+    public void deleteFilePathNotPresent() {
+        // Making this a spy, so we can inject an empty optional without digging into the implementation
+        linkedFile = spy(new LinkedFile("", "", ""));


You could also pass "non-existent file" as the path as you do below. This might be a bit easier to understand.

tobiasdiez · 2018-02-04T12:41:02Z

src/test/java/org/jabref/gui/fieldeditors/LinkedFileViewModelTest.java

+
+        LinkedFileViewModel viewModel = new LinkedFileViewModel(
+                linkedFile, entry, databaseContext, taskExecutor, dialogService
+        );


I'm not sure, but I think our style conventions say that the closing parenthesis has to go on the same line. Right, @koppor ?

tobiasdiez · 2018-02-04T12:44:04Z

src/test/java/org/jabref/gui/fieldeditors/LinkedFileViewModelTest.java

+    }
+
+    @Test
+    public void deleteDialogCanceled() throws IOException {


In general we try to name the tests according to the scheme methodUnderTest_scenario_expectedBehavior (without the underscore). So for this test here, it would be e.g. deleteWhenDialogCancelledReturnsFalse.

* Added non-empty link to file in one test for clarity * Removed unnecessary newlines * Changed test method names to adhere to chosen naming convention

bdach · 2018-02-04T22:44:37Z

Well, if I'm this close, might as well try and make this PR as good as possible!

I've added the suggested changes. However, I did leave some of the parentheses as they were -- near those long mock setup calls to be precise, because I couldn't make them look decent otherwise.

tobiasdiez · 2018-02-05T11:04:02Z

Nice, thanks again for your contributions. We are looking forward to see more PRs from you 😄 .

* upstream/master: (155 commits) Update DEVELOPERS Update README.md [WIP] File link deletion dialog improvements (#3690) Update guava (#3692) Fix some FX Thread issue (#3691) Fixed typo, mayor to major (#3685) add Office xml test for latex free fields (#3680) Migrate importer tests to JUnit5 (#3665) fix typo Update slf4j-api from 1.8.0-beta0 -> 1.8.0-beta1 Update richtext from 0.8.1 -> 0.8.2 Update junit from 5.1.0-M1 -> 5.1.0-M2 Update checkstyle from 8.7 -> 8.8 Export all fields with their latex free equivalent (#3675) try around with exckluding tag (#3676) Fixes #3648: Chained modifiers work again (#3670) Update gradle from 4.4.1 to 4.5 and tweak .gitattributes Fix 2633 saving creates new database (#3674) New translations JabRef_en.properties (French) Implements #1664: group based on aux file (#3444) ... # Conflicts: # CHANGELOG.md # CONTRIBUTING.md # build.gradle # gradle/wrapper/gradle-wrapper.jar # gradle/wrapper/gradle-wrapper.properties # scripts/syncLang.py # src/main/java/org/jabref/FallbackExceptionHandler.java # src/main/java/org/jabref/Globals.java # src/main/java/org/jabref/JabRefMain.java # src/main/java/org/jabref/cli/ArgumentProcessor.java # src/main/java/org/jabref/cli/JabRefCLI.java # src/main/java/org/jabref/collab/EntryChange.java # src/main/java/org/jabref/collab/EntryDeleteChange.java # src/main/java/org/jabref/collab/FileUpdateListener.java # src/main/java/org/jabref/collab/StringAddChange.java # src/main/java/org/jabref/collab/StringChange.java # src/main/java/org/jabref/collab/StringNameChange.java # src/main/java/org/jabref/collab/StringRemoveChange.java # src/main/java/org/jabref/gui/BasePanel.java # src/main/java/org/jabref/gui/ClipBoardManager.java # src/main/java/org/jabref/gui/DefaultInjector.java # src/main/java/org/jabref/gui/DialogService.java # src/main/java/org/jabref/gui/EntryTypeDialog.java # src/main/java/org/jabref/gui/JabRefFrame.java # src/main/java/org/jabref/gui/actions/IntegrityCheckAction.java # src/main/java/org/jabref/gui/collab/ChangeScanner.java # src/main/java/org/jabref/gui/collab/DatabaseChangeMonitor.java # src/main/java/org/jabref/gui/copyfiles/CopyFilesTask.java # src/main/java/org/jabref/gui/customjfx/CustomJFXPanel.java # src/main/java/org/jabref/gui/desktop/JabRefDesktop.java # src/main/java/org/jabref/gui/entryeditor/DeprecatedFieldsTab.java # src/main/java/org/jabref/gui/entryeditor/EntryEditor.css # src/main/java/org/jabref/gui/entryeditor/EntryEditor.java # src/main/java/org/jabref/gui/entryeditor/FieldsEditorTab.java # src/main/java/org/jabref/gui/entryeditor/OptionalFields2Tab.java # src/main/java/org/jabref/gui/entryeditor/OptionalFieldsTab.java # src/main/java/org/jabref/gui/entryeditor/OtherFieldsTab.java # src/main/java/org/jabref/gui/entryeditor/RequiredFieldsTab.java # src/main/java/org/jabref/gui/entryeditor/SourceTab.java # src/main/java/org/jabref/gui/entryeditor/UserDefinedFieldsTab.java # src/main/java/org/jabref/gui/entryeditor/fileannotationtab/FileAnnotationTabController.java # src/main/java/org/jabref/gui/entryeditor/fileannotationtab/FileAnnotationTabViewModel.java # src/main/java/org/jabref/gui/exporter/ExportAction.java # src/main/java/org/jabref/gui/exporter/ExportFileFilter.java # src/main/java/org/jabref/gui/externalfiles/AutoSetFileLinksUtil.java # src/main/java/org/jabref/gui/externalfiles/AutoSetLinks.java # src/main/java/org/jabref/gui/fieldeditors/EditorValidator.java # src/main/java/org/jabref/gui/fieldeditors/FieldEditors.java # src/main/java/org/jabref/gui/fieldeditors/FileListEditorTransferHandler.java # src/main/java/org/jabref/gui/fieldeditors/LinkedFileViewModel.java # src/main/java/org/jabref/gui/fieldeditors/LinkedFilesEditor.java # src/main/java/org/jabref/gui/fieldeditors/LinkedFilesEditorViewModel.java # src/main/java/org/jabref/gui/fieldeditors/MapBasedEditorViewModel.java # src/main/java/org/jabref/gui/groups/GroupDialog.java # src/main/java/org/jabref/gui/groups/GroupSidePane.java # src/main/java/org/jabref/gui/groups/GroupTreeController.java # src/main/java/org/jabref/gui/help/AboutDialogViewModel.java # src/main/java/org/jabref/gui/help/HelpAction.java # src/main/java/org/jabref/gui/importer/ImportFileFilter.java # src/main/java/org/jabref/gui/importer/ImportFormats.java # src/main/java/org/jabref/gui/maintable/MainTableSelectionListener.java # src/main/java/org/jabref/gui/preftabs/AppearancePrefsTab.java # src/main/java/org/jabref/gui/preftabs/EntryEditorPrefsTab.java # src/main/java/org/jabref/gui/preftabs/PreviewPrefsTab.java # src/main/java/org/jabref/gui/search/GlobalSearchBar.java # src/main/java/org/jabref/gui/util/BindingsHelper.java # src/main/java/org/jabref/gui/util/DefaultFileUpdateMonitor.java # src/main/java/org/jabref/gui/util/DefaultTaskExecutor.java # src/main/java/org/jabref/gui/util/FileUpdateListener.java # src/main/java/org/jabref/gui/worker/CitationStyleToClipboardWorker.java # src/main/java/org/jabref/logic/bibtexkeypattern/BibtexKeyGenerator.java # src/main/java/org/jabref/logic/bibtexkeypattern/BracketedPattern.java # src/main/java/org/jabref/logic/citationstyle/CitationStyle.java # src/main/java/org/jabref/logic/citationstyle/CitationStyleCache.java # src/main/java/org/jabref/logic/citationstyle/CitationStyleGenerator.java # src/main/java/org/jabref/logic/exporter/BibTeXMLExporter.java # src/main/java/org/jabref/logic/exporter/ExportFormats.java # src/main/java/org/jabref/logic/exporter/IExportFormat.java # src/main/java/org/jabref/logic/exporter/MSBibExporter.java # src/main/java/org/jabref/logic/exporter/ModsExporter.java # src/main/java/org/jabref/logic/exporter/OpenDocumentSpreadsheetCreator.java # src/main/java/org/jabref/logic/exporter/OpenOfficeDocumentCreator.java # src/main/java/org/jabref/logic/exporter/TemplateExporter.java # src/main/java/org/jabref/logic/importer/ImportFormatReader.java # src/main/java/org/jabref/logic/importer/WebFetchers.java # src/main/java/org/jabref/logic/importer/fetcher/IacrEprintFetcher.java # src/main/java/org/jabref/logic/integrity/FieldCheckers.java # src/main/java/org/jabref/logic/integrity/IntegrityCheck.java # src/main/java/org/jabref/logic/journals/JournalAbbreviationRepository.java # src/main/java/org/jabref/logic/shared/DBMSProcessor.java # src/main/java/org/jabref/logic/shared/DBMSSynchronizer.java # src/main/java/org/jabref/logic/util/FileType.java # src/main/java/org/jabref/logic/util/JavaVersion.java # src/main/java/org/jabref/logic/util/io/CiteKeyBasedFileFinder.java # src/main/java/org/jabref/logic/util/io/FileFinder.java # src/main/java/org/jabref/logic/util/io/FileUtil.java # src/main/java/org/jabref/logic/util/io/RegExpBasedFileFinder.java # src/main/java/org/jabref/model/auxparser/AuxParserResult.java # src/main/java/org/jabref/model/database/BibDatabaseContext.java # src/main/java/org/jabref/model/entry/FieldProperty.java # src/main/java/org/jabref/model/pdf/FileAnnotation.java # src/main/java/org/jabref/model/util/FileUpdateListener.java # src/main/java/org/jabref/preferences/CustomExportList.java # src/main/java/org/jabref/preferences/JabRefPreferences.java # src/test/java/org/jabref/TestArchitectureTests.java # src/test/java/org/jabref/gui/externalfiles/AutoSetFileLinksUtilTest.java # src/test/java/org/jabref/gui/importer/EntryFromFileCreatorManagerTest.java # src/test/java/org/jabref/logic/bibtexkeypattern/BibtexKeyGeneratorTest.java # src/test/java/org/jabref/logic/bibtexkeypattern/MakeLabelWithDatabaseTest.java # src/test/java/org/jabref/logic/exporter/MsBibExportFormatTest.java # src/test/java/org/jabref/logic/importer/ImportFormatReaderTestParameterless.java # src/test/java/org/jabref/logic/importer/fetcher/ArXivTest.java # src/test/java/org/jabref/logic/importer/fetcher/AstrophysicsDataSystemTest.java # src/test/java/org/jabref/logic/importer/fetcher/CrossRefTest.java # src/test/java/org/jabref/logic/importer/fetcher/DBLPFetcherTest.java # src/test/java/org/jabref/logic/importer/fetcher/DiVATest.java # src/test/java/org/jabref/logic/importer/fetcher/DoiFetcherTest.java # src/test/java/org/jabref/logic/importer/fetcher/FulltextFetcherTest.java # src/test/java/org/jabref/logic/importer/fetcher/GvkFetcherTest.java # src/test/java/org/jabref/logic/importer/fetcher/IacrEprintFetcherTest.java # src/test/java/org/jabref/logic/importer/fetcher/IsbnFetcherTest.java # src/test/java/org/jabref/logic/importer/fetcher/IsbnViaChimboriFetcherTest.java # src/test/java/org/jabref/logic/importer/fetcher/IsbnViaEbookDeFetcherTest.java # src/test/java/org/jabref/logic/importer/fetcher/LibraryOfCongressTest.java # src/test/java/org/jabref/logic/importer/fetcher/MedlineFetcherTest.java # src/test/java/org/jabref/logic/importer/fetcher/MrDLibFetcherTest.java # src/test/java/org/jabref/logic/importer/fetcher/SpringerLinkTest.java # src/test/java/org/jabref/logic/importer/fetcher/TitleFetcherTest.java # src/test/java/org/jabref/logic/importer/fileformat/BibtexParserTest.java # src/test/java/org/jabref/logic/importer/fileformat/FreeCiteImporterTest.java # src/test/java/org/jabref/logic/l10n/LocalizationConsistencyTest.java # src/test/java/org/jabref/logic/shared/DBMSConnectionTest.java # src/test/java/org/jabref/logic/shared/DBMSProcessorTest.java # src/test/java/org/jabref/logic/shared/DBMSSynchronizerTest.java # src/test/java/org/jabref/logic/shared/DBMSTypeTest.java # src/test/java/org/jabref/logic/shared/SynchronizationTestEventListener.java # src/test/java/org/jabref/logic/shared/SynchronizationTestSimulator.java # src/test/java/org/jabref/logic/shared/TestConnector.java # src/test/java/org/jabref/logic/util/BracketedPatternTest.java # src/test/java/org/jabref/logic/util/io/CiteKeyBasedFileFinderTest.java # src/test/java/org/jabref/logic/util/io/RegExpBasedFileFinderTests.java # src/test/java/org/jabref/testutils/category/FetcherTest.java

bdach added 2 commits February 3, 2018 22:26

Fix canceling bug in file link deletion dialog

d1d70a1

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.

bdach mentioned this pull request Feb 3, 2018

Delete from disk should not be default option/selected on DELETE in file tab #3679

Closed

Siedlerchr approved these changes Feb 3, 2018

View reviewed changes

tobiasdiez reviewed Feb 3, 2018

View reviewed changes

Tag link removal button as default (#3679)

467b70a

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

3be1659

bdach commented Feb 4, 2018

View reviewed changes

tobiasdiez approved these changes Feb 4, 2018

View reviewed changes

Code style adjustments in dialog test cases

2b86362

* Added non-empty link to file in one test for clarity * Removed unnecessary newlines * Changed test method names to adhere to chosen naming convention

tobiasdiez merged commit b960da1 into JabRef:master Feb 5, 2018

stefan-kolb mentioned this pull request Feb 7, 2018

Delete dialog in file attachements is not focused #3707

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] File link deletion dialog improvements #3690

[WIP] File link deletion dialog improvements #3690

bdach commented Feb 3, 2018 •

edited

Loading

tobiasdiez Feb 3, 2018 •

edited

Loading

bdach Feb 3, 2018

Siedlerchr Feb 3, 2018

Siedlerchr commented Feb 3, 2018

tobiasdiez left a comment

bdach commented Feb 3, 2018

bdach commented Feb 4, 2018 •

edited

Loading

bdach Feb 4, 2018 •

edited

Loading

tobiasdiez left a comment

tobiasdiez Feb 4, 2018

tobiasdiez Feb 4, 2018

tobiasdiez Feb 4, 2018

bdach commented Feb 4, 2018

tobiasdiez commented Feb 5, 2018

[WIP] File link deletion dialog improvements #3690

[WIP] File link deletion dialog improvements #3690

Conversation

bdach commented Feb 3, 2018 • edited Loading

Example screenshot:

Checklist:

tobiasdiez Feb 3, 2018 • edited Loading

Choose a reason for hiding this comment

bdach Feb 3, 2018

Choose a reason for hiding this comment

Siedlerchr Feb 3, 2018

Choose a reason for hiding this comment

Siedlerchr commented Feb 3, 2018

tobiasdiez left a comment

Choose a reason for hiding this comment

bdach commented Feb 3, 2018

bdach commented Feb 4, 2018 • edited Loading

bdach Feb 4, 2018 • edited Loading

Choose a reason for hiding this comment

tobiasdiez left a comment

Choose a reason for hiding this comment

tobiasdiez Feb 4, 2018

Choose a reason for hiding this comment

tobiasdiez Feb 4, 2018

Choose a reason for hiding this comment

tobiasdiez Feb 4, 2018

Choose a reason for hiding this comment

bdach commented Feb 4, 2018

tobiasdiez commented Feb 5, 2018

bdach commented Feb 3, 2018 •

edited

Loading

tobiasdiez Feb 3, 2018 •

edited

Loading

bdach commented Feb 4, 2018 •

edited

Loading

bdach Feb 4, 2018 •

edited

Loading