Fix for issue 9091 - store merge entries toolbar configuration #9712

dragos222 · 2023-03-28T20:08:54Z

Fixes #9091
The Merge Entries toolbar configuration now gets saved once the user hits 'Merge entries' button. More specifically, the plainTextOrDiff combo box, diffView combo box, and highlight words/highlight characters values are being stored using the GuiPreferences, and JabRefPreferences objects respectively. The 'Only show changed fields' check box was already handled, therefore I did not change its implementation.
I have a question regarding the showDiff method from ThreeWayMergeView class: should this method be left the way it is now, or should it get updated so that the user's preferences are loaded when invoking the showDiff method?

Compulsory checks

Give feedback

Change in CHANGELOG.md described in a way that is understandable for the average user (if applicable)
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.
Options

…ainTextOrDiff to JabrefPreferences

…ewaytoolbar fields

…ces diff view

…word preference in listener method

HoussemNasri · 2023-03-28T21:02:45Z

I couldn't assign @ssrpro to the PR, but so you know he is working on this PR with @dragos222 on the same team.

HoussemNasri · 2023-03-28T21:15:29Z

should this method be left the way it is now, or should it get updated so that the user's preferences are loaded when invoking the showDiff method?

Stored configuration should be loaded when the UI is created, e.g. at the constructor. Have a look at ThreeWayMergeView#initializeToolbar

src/main/java/org/jabref/gui/mergeentries/MergeEntriesDialog.java

…eferences to threewaymergeview object

dragos222 · 2023-03-29T06:57:46Z

Thank you for your feedback. The saveGuiPreferences has been delegated to the ThreeWayMergeView object and the MergeEntriesDialog object does not get access to the toolbar object.
The preferences are indeed loaded when the UI gets created. More specifically, these values are loaded in their according combo box/radio button in ThreeWayMergeToolbar#initialize.

Siedlerchr · 2023-03-29T16:57:27Z

src/main/java/org/jabref/gui/mergeentries/newmergedialog/toolbar/ThreeWayMergeToolbar.java

@@ -185,6 +196,14 @@ public enum PlainTextOrDiff {
            this.value = value;
        }

+        public static PlainTextOrDiff parse(String name) {
+            try {
+                return PlainTextOrDiff.valueOf(name);


You can use Guava's https://guava.dev/releases/15.0/api/docs/com/google/common/base/Enums.html#getIfPresent(java.lang.Class,%20java.lang.String) here, it's a little improvement as it does not throw an exception

…222/jabref into fix-for-issue-9091-group3

HoussemNasri · 2023-03-29T20:36:10Z

src/main/java/org/jabref/gui/mergeentries/newmergedialog/toolbar/ThreeWayMergeToolbar.java

@@ -176,6 +180,14 @@ public void setOnSelectRightEntryValuesButtonClicked(Runnable onClick) {
        selectRightEntryValuesButton.setOnMouseClicked(e -> onClick.run());
    }

+    public void saveGuiPreferences() {
+        preferencesService.getGuiPreferences().setMergePlainTextOrDiff(plainTextOrDiffComboBox.getValue());


When I created the PlainTextOrDiff enum type, I thought it would be useful because I can embed the "Plain Text" and "Show Diff" texts in there, and display the labels in UI. In preferences, I think a boolean should suffice. Maybe a boolean property named shouldShowDiffs?

This would have the advantage that you can then use bidrectional binding

Implemented this for both PlainTextOrDiff and DiffView enums.

HoussemNasri

The merge configurations are being saved perfectly in the MergeEntriesDialog, however, in the DuplicateResolverDialog they are not. This is a complication, because the three-way merge UI is being used in multiple dialogs, so it's not obvious when the user is done with the UI and thus when to save the configuration.

I would start by doing the same changes you did to MergeEntriesDialog to the DuplicateResolverDialog, but this time, you want to save the configurations on all actions but the "Cancel" one.

Overall, I think you did a good job implementing this. Once the duplicate resolver dialog is updated, I think we can merge.

dragos222 · 2023-04-01T18:48:09Z

The three way merge view toolbar preferences are now saved in DuplicateResolverDialog as well, on all actions but the "Cancel" one. There is a failing test, but it's not related to this PR and I notice that it also fails for other PRs.

- Also changed some names and code structure

HoussemNasri

I took the courtesy to adjust some names and restructure the code.

Also, while using the merge dialog, I expected the configuration to be saved even if I clicked "Cancel", so I changed that for both dialogs.

Thanks again for working on this enhancement. It would significantly improve the merging user experience, especially when resolving duplicates.

…group3

dragos222 added 11 commits March 26, 2023 14:02

Refactor out PlainTextOrDiff Enum(?); Use guiPreferences; add mergePl…

d0cd71e

…ainTextOrDiff to JabrefPreferences

Update preference; remove guiPreference redundant objects

998ac47

Update jabrefPreferences and GuiPreferences for all mergeentries thre…

1e52e74

…ewaytoolbar fields

Stop using record ShowDiffConfig; use preferences for diffViewComboBox

6a2222f

Remove redundant call functions; add easybind listener to guipreferen…

9bda40b

…ces diff view

Default value for highlight words TRUE; set the values for highlight …

a137fe4

…word preference in listener method

Remove redundant method & method call; remove unused var

8722fa1

Only save merge toolbar config when merge entries button is pressed

369104f

Readd deletions; fix typo in GuiPreferences

2f832e7

Fix typo

062d727

Add change to the changelog

92feafe

dragos222 changed the title ~~Fix for issue 9091 group3~~ Fix for issue 9091 Mar 28, 2023

dragos222 changed the title ~~Fix for issue 9091~~ Fix for issue 9091 - store merge entries toolbar configuration Mar 28, 2023

HoussemNasri assigned dragos222 Mar 28, 2023

HoussemNasri reviewed Mar 28, 2023

View reviewed changes

src/main/java/org/jabref/gui/mergeentries/MergeEntriesDialog.java Outdated Show resolved Hide resolved

ThiloteE added the preferences label Mar 28, 2023

MergeEntriesDialog does not need toolbar object; delegate save gui pr…

98ddc4a

…eferences to threewaymergeview object

dragos222 marked this pull request as ready for review March 29, 2023 10:05

Merge branch 'main' into fix-for-issue-9091-group3

5e60321

Siedlerchr reviewed Mar 29, 2023

View reviewed changes

Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Mar 29, 2023

Siedlerchr previously approved these changes Mar 29, 2023

View reviewed changes

Siedlerchr requested a review from HoussemNasri March 29, 2023 17:09

dragos222 added 2 commits March 29, 2023 20:33

Enum parse using getIfPresent to prevent exception being thrown

974b9ec

Merge branch 'fix-for-issue-9091-group3' of https://github.com/dragos…

afeea04

…222/jabref into fix-for-issue-9091-group3

dragos222 dismissed Siedlerchr’s stale review via afeea04 March 29, 2023 18:37

HoussemNasri reviewed Mar 29, 2023

View reviewed changes

HoussemNasri requested changes Mar 29, 2023

View reviewed changes

Siedlerchr added status: changes required Pull requests that are not yet complete and removed status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers labels Mar 31, 2023

dragos222 added 3 commits April 1, 2023 19:25

Store preference as boolean instead of enum

fcfa16f

Save three way merge view toolbar preferences in duplicateslverdialog

cfc5830

Fix typo

db6d569

dragos222 and others added 3 commits April 1, 2023 23:16

Refactor

7b636bf

Refactor ThreeWayMergeToolbar

9994c57

Save configuration regardless of the action performed (cancel included)

b382d39

- Also changed some names and code structure

HoussemNasri previously approved these changes Apr 1, 2023

View reviewed changes

HoussemNasri added 2 commits April 1, 2023 23:18

Merge remote-tracking branch 'upstream/main' into fix-for-issue-9091-…

98255f0

…group3

Fix CitationStyleTest#testDefaultCitation test

193aebe

HoussemNasri dismissed their stale review via 193aebe April 2, 2023 00:21

HoussemNasri approved these changes Apr 2, 2023

View reviewed changes

Siedlerchr merged commit 77ea49c into JabRef:main Apr 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for issue 9091 - store merge entries toolbar configuration #9712

Fix for issue 9091 - store merge entries toolbar configuration #9712

dragos222 commented Mar 28, 2023 •

edited by ThiloteE

Loading

Compulsory checks

HoussemNasri commented Mar 28, 2023

HoussemNasri commented Mar 28, 2023

dragos222 commented Mar 29, 2023

Siedlerchr Mar 29, 2023

HoussemNasri Mar 29, 2023

Siedlerchr Mar 29, 2023

dragos222 Apr 1, 2023

HoussemNasri left a comment

dragos222 commented Apr 1, 2023

HoussemNasri left a comment

Fix for issue 9091 - store merge entries toolbar configuration #9712

Fix for issue 9091 - store merge entries toolbar configuration #9712

Conversation

dragos222 commented Mar 28, 2023 • edited by ThiloteE Loading

Compulsory checks

HoussemNasri commented Mar 28, 2023

HoussemNasri commented Mar 28, 2023

dragos222 commented Mar 29, 2023

Siedlerchr Mar 29, 2023

Choose a reason for hiding this comment

HoussemNasri Mar 29, 2023

Choose a reason for hiding this comment

Siedlerchr Mar 29, 2023

Choose a reason for hiding this comment

dragos222 Apr 1, 2023

Choose a reason for hiding this comment

HoussemNasri left a comment

Choose a reason for hiding this comment

dragos222 commented Apr 1, 2023

HoussemNasri left a comment

Choose a reason for hiding this comment

dragos222 commented Mar 28, 2023 •

edited by ThiloteE

Loading