[GSOC22] - B - Implement merging fields in the three way merge UI #9022

HoussemNasri · 2022-08-03T21:48:15Z

Closes #8024
Depends on #8945
Contributes to #6190

It appears that every time a field changes, an event bus marks the library as edited. The library will remain marked as edited even after I undo groups merging and cancel the merge dialog. Right now, I can't think of a way to fix this.

Edit
Keywords and Comments can be merged too.

bandicam.2022-07-09.04-40-39-815.mp4

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.

- Thus commands can be moved to the view model

- Removed FieldNameCellFactory.java and MergeableFieldCell.java - Made it easier to add more side buttons in the future

…entries

- Initialized the change name column - Commented out 'getDialogPane().setContent(pane)' because it will be removed later

- Changes are applied only when all changes are resolved (changes table is empty). Like a transaction

…ced merge dialog

… 'EntryChangeViewModel'

…9019)

…groups

Siedlerchr · 2022-08-04T16:33:40Z

src/main/java/org/jabref/gui/actions/Action.java

+        }
+    }
+
+    class ActionImpl implements Action {


Why don't you use the JabRefAction together with the ActionFactory ? It should already cover the cases

I'm not sure how that would work. The idea behind the builder and ActionImpl is to create instances of org.jabref.gui.actions.Action without implementing it; which can be very verbose.

Well, you could either use them as anonymous classes or as separate classes. The latter is probably useful if you are reusing them in multiple placess.

Siedlerchr · 2022-08-04T16:37:52Z

src/main/java/org/jabref/gui/mergeentries/newmergedialog/fieldsmerger/FieldMergerFactory.java

+    }
+
+    public FieldMerger create(Field field) {
+        if (field.equals(StandardField.GROUPS)) {


Use a switch here?

And also some parts use == other use equal

I couldn't use a switch expression because Field is an interface. When I changed the type of the parameter to StandardField, it compiled fine but other parts of the codebase broke.

Ah okay, yeah I see. Then field is the correct approach

Siedlerchr · 2022-08-04T16:43:16Z

src/main/java/org/jabref/gui/mergeentries/newmergedialog/fieldsmerger/KeywordMerger.java

+        } else if (StringUtil.isBlank(keywordsB)) {
+            return keywordsA;
+        } else {
+            return Arrays.stream((keywordsA + keywordSeparator + keywordsB).split(keywordSeparator))


Not sure, but maybe you should use KeywordList ? This is relevant for keyword groups

Siedlerchr · 2022-08-04T21:12:10Z

src/main/java/org/jabref/gui/mergeentries/newmergedialog/FieldRowView.java

+
+    public class UnmergeCommand extends SimpleCommand {
+
+        public UnmergeCommand() {


empty default constructor can be removed

Siedlerchr · 2022-08-04T21:13:39Z

src/main/java/org/jabref/gui/mergeentries/newmergedialog/ThreeWayMergeView.java

+    public ThreeWayMergeView(BibEntry leftEntry, BibEntry rightEntry, String leftHeader, String rightHeader) {
+        getStylesheets().add(ThreeWayMergeView.class.getResource("ThreeWayMergeView.css").toExternalForm());
+        viewModel = new ThreeWayMergeViewModel((BibEntry) leftEntry.clone(), (BibEntry) rightEntry.clone(), leftHeader, rightHeader);
+        // TODO: Inject 'preferenceService' into the constructor


You can test if you can inject it as field here or in the caller component and pass it to this component in the constructor
@Inject private PreferencesService preferencesService

It throws a null pointer exception. I think this is happening because the injection is handled by mvvmfx but ThreeWayMergeView doesn't use a view loader. I thought that because @Inject is used in other classes and all of them also use view loader.

You need to inject it into DuplicateResolverDialog

And also pass it from the ChangeScanner -> EntryChangeViewModel -> ThreeWayMergeView via constructor

Siedlerchr · 2022-08-04T21:24:22Z

src/main/java/org/jabref/gui/duplicationFinder/DuplicateResolverDialog.java

@@ -39,7 +39,7 @@ public enum DuplicateResolverResult {
        BREAK
    }

-    private MergeEntries mergeEntries;
+    private ThreeWayMergeView threeWayMerge;
    private final DialogService dialogService;



Here you can use things with @Inject e.g @Inject PreferencesService preferencesService; and pass that to other

Injection doesn't work here either

Siedlerchr

overall looks good to me just a few minor improvements

…into GSOC-merge-fields

Siedlerchr · 2022-08-06T11:05:31Z

We are thinking of merging this so the early bird users can the new feature, Can PR A and B be merged?

HoussemNasri · 2022-08-06T11:59:13Z

PR A is complete. This one still needs some nitpicks, but it should be ready today or tomorrow.

# Conflicts: # src/main/java/org/jabref/gui/duplicationFinder/DuplicateResolverDialog.java # src/main/java/org/jabref/gui/icon/IconTheme.java # src/main/java/org/jabref/gui/mergeentries/MergeEntriesDialog.java # src/main/java/org/jabref/gui/mergeentries/newmergedialog/ThreeWayMergeView.css # src/main/java/org/jabref/gui/mergeentries/newmergedialog/ThreeWayMergeView.java # src/main/java/org/jabref/gui/mergeentries/newmergedialog/cell/FieldNameCell.java # src/main/java/org/jabref/gui/mergeentries/newmergedialog/cell/FieldValueCell.java # src/main/java/org/jabref/gui/mergeentries/newmergedialog/cell/HeaderCell.java # src/main/java/org/jabref/gui/mergeentries/newmergedialog/cell/MergedFieldCell.java # src/main/java/org/jabref/gui/mergeentries/newmergedialog/cell/OpenExternalLinkAction.java # src/main/resources/l10n/JabRef_en.properties

* upstream/main: (31 commits) Citavi Importer - Import all knowledge items (JabRef#9043) Fixed table update in eft preferences (JabRef#9051) Keep EOL setting at backups (JabRef#9048) ExternalFileTypes singleton refactor (JabRef#9044) Fix dead link (JabRef#9047) Fix performance regresssion (JabRef#9045) Update javafx to 18.02 import citavi knowledge items (JabRef#9033) Fix .gitattributes for CHANGELOG.md [GSOC22] - B - Implement merging fields in the three way merge UI (JabRef#9022) [GSOC22] - A - Implement a fully functional three way merge UI (JabRef#8945) Change button label from "Return to JabRef" to "Return to library" (JabRef#9039) Bump postgresql from 42.4.0 to 42.4.1 (JabRef#9036) Bump org.javamodularity.moduleplugin from 1.8.11 to 1.8.12 (JabRef#9037) Bump slf4j-api from 2.0.0-alpha7 to 2.0.0-beta1 (JabRef#9035) Bump slf4j-api from 2.0.0-alpha7 to 2.0.0-beta1 in /buildSrc (JabRef#9038) Update Gradle Wrapper from 7.5 to 7.5.1. (JabRef#9034) Refactor of DOI import failure dialog, import format reader and clipboard manager (JabRef#8839) Snapcraft and issue template Show development information\n\n+semver: minor ...

HoussemNasri added 30 commits July 25, 2022 12:37

Encode the resulting string from merging files

f6a281a

Refactor merge/unmerge fields command handling

470bc8b

Declare Actions with the builder design pattern

7354264

Remove MergeableFieldCell dependency from the merge/unmerge commands

c9b6db7

- Thus commands can be moved to the view model

Add logs

dab2813

Disable field's merge button when field values are equal

705e8cc

Update code documentation

0e9a784

Rename 'AbstractCell' to 'ThreeWayMergeCell'

2a512c5

Pass arguments to Localization.lang using '%0' not '%'

8081bc5

Decompose the ToggleMergeUnmergeButton into its own class (or component)

235a243

- Removed FieldNameCellFactory.java and MergeableFieldCell.java - Made it easier to add more side buttons in the future

Include original left and right entries in EntriesMergeResult

891feb6

Make entries merging logic reusable

feb0628

Rename ChangeDisplayDialog.java to ExternalChangesResolverDialog.java

328ce97

Create ExternalChangesResolverViewModel

b8ce549

Design the ExternalChangesResolverDialog

588db57

Implement a method for opening an advanced merge dialog for modified …

0af8175

…entries

Wrap the UI in a dialog pane

0e128d2

Convert change name to a StringProperty instead of a raw string

0f1366a

Populate table view with changes

241ae86

- Initialized the change name column - Commented out 'getDialogPane().setContent(pane)' because it will be removed later

Remove 'Accept changes' and 'Dismiss' buttons from the dialog

b3ddc88

Allow selecting multiple changes

6bf045b

Rename 'Accept Theirs' to 'Accept' and 'Accept Yours' to 'Deny'

0be74a6

Apply changes when before the dialog

5cd88eb

- Changes are applied only when all changes are resolved (changes table is empty). Like a transaction

Cleanup FXML

fe4b582

Remove old UI code

fc656d9

Close dialog when all changes are resolved

12ec6c5

Enable the 'Merge...' button only if the selected change has an advan…

f9292de

…ced merge dialog

Select the first change after opening the dialog

dd46057

Open the advanced merge dialog when 'Merge...' is clicked

57062ce

Don't open the advanced merge dialog when 'makeChange()' is called on…

d90c418

… 'EntryChangeViewModel'

Siedlerchr and others added 2 commits August 3, 2022 20:34

Revert "[WIP] [GSOC22] Improve the external changes resolver dialog" (#…

0a62304

…9019)

Merge branch 'GSOC-fully-functional-three-way-merge' into GSOC-merge-…

d43c3f9

…groups

calixtus added project: GSoC ui duplicateFinder labels Aug 3, 2022

Siedlerchr reviewed Aug 4, 2022

View reviewed changes

Siedlerchr approved these changes Aug 4, 2022

View reviewed changes

HoussemNasri added 2 commits August 5, 2022 14:32

Use '==' to compare enums

ec992a2

Merge branch 'GSOC-merge-fields' of https://github.com/JabRef/jabref …

bd7a01c

…into GSOC-merge-fields

HoussemNasri added 4 commits August 6, 2022 14:58

Move keyword merging logic to KeywordList class

11d87a6

Remove default constructor

331725a

Compare keywords equality without considering order

5e7a474

Test KeywordList#merge

4224dab

HoussemNasri added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Aug 6, 2022

HoussemNasri added 3 commits August 10, 2022 19:32

Fix I10n

ad14469

Fix I10n

d381e9e

Siedlerchr merged commit dcec4e2 into main Aug 10, 2022

Siedlerchr deleted the GSOC-merge-fields branch August 10, 2022 19:10

This was referenced Aug 13, 2022

merge entries should be able to merge keywords #8983

Closed

Merge entries should be able to merge keywords and groups koppor/jabref#12

Closed

koppor mentioned this pull request Sep 2, 2022

SLR should create groups where paper is from koppor/jabref#602

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GSOC22] - B - Implement merging fields in the three way merge UI #9022

[GSOC22] - B - Implement merging fields in the three way merge UI #9022

HoussemNasri commented Aug 3, 2022 •

edited by calixtus

Loading

Siedlerchr Aug 4, 2022 •

edited

Loading

HoussemNasri Aug 4, 2022

Siedlerchr Aug 4, 2022

Siedlerchr Aug 4, 2022

Siedlerchr Aug 4, 2022

HoussemNasri Aug 4, 2022

Siedlerchr Aug 4, 2022

Siedlerchr Aug 4, 2022

Siedlerchr Aug 4, 2022

Siedlerchr Aug 4, 2022 •

edited

Loading

HoussemNasri Aug 5, 2022 •

edited

Loading

Siedlerchr Aug 5, 2022

Siedlerchr Aug 5, 2022

Siedlerchr Aug 4, 2022 •

edited

Loading

HoussemNasri Aug 5, 2022

Siedlerchr left a comment

Siedlerchr commented Aug 6, 2022

HoussemNasri commented Aug 6, 2022


		public class UnmergeCommand extends SimpleCommand {

		public UnmergeCommand() {

[GSOC22] - B - Implement merging fields in the three way merge UI #9022

[GSOC22] - B - Implement merging fields in the three way merge UI #9022

Conversation

HoussemNasri commented Aug 3, 2022 • edited by calixtus Loading

Siedlerchr Aug 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Siedlerchr Aug 4, 2022 • edited Loading

Choose a reason for hiding this comment

HoussemNasri Aug 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Siedlerchr Aug 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Siedlerchr left a comment

Choose a reason for hiding this comment

Siedlerchr commented Aug 6, 2022

HoussemNasri commented Aug 6, 2022

HoussemNasri commented Aug 3, 2022 •

edited by calixtus

Loading

Siedlerchr Aug 4, 2022 •

edited

Loading

Siedlerchr Aug 4, 2022 •

edited

Loading

HoussemNasri Aug 5, 2022 •

edited

Loading

Siedlerchr Aug 4, 2022 •

edited

Loading