Right click create group #11476

m-peeler · 2024-07-09T22:56:35Z

Added functionality to allow users to right click to make a new group from the selection. Updated all group creation and group editing to use the CreateGroupAction class, extending SimpleCommand so it would work with the right-click menu. Closes #11449 and should be ready for merging in.

Getting this functionality extracted into a SimpleCommand (needed by the right-click menu) that could be called with the available states stored in StateManager required some pretty large changes and bug fixes related to synchronizing the various lists of active and selected entries.

This has the changes from #11453 included in it, and slightly improved due to a few issues I faced when doing the other edits.

Additionally, fixed a bug where when new entries were dropped into a list, the displayed number of entries did not update.

I'll update the documentation when this gets approved for merge.

Future improvements may include letting the user pick if they want a group created above / beside / below the current group, or at root.

Mandatory checks

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.

When a new group is added, that group will automatically be selected and focused.

Updated changelog to reflect updates.

Added new option to the "Group Options" in the GroupDialog to allow the creation of a group that contains the currently selected entries. If there are no entries selected, this option is disabled. If there is more than one entry selected, this option is selected by default.

Fixes failed unit test (keyValueShouldBeEqualForEnglishPropertiesMessages) and style test ( '(' is preceded with whitespace.)

Created the 'Create group from Seletion' item for the right-click menu and a new item on the menu. Began refactoring GroupDialogView, GroupDialogViewModel and GroupTreeViewModel to allow for creating a DialogView that automatically prioritizes 'Current selection'.

Attempted to implement right-click-to-add functionality to the codebase. This ran into several problems and a few Gordian knots which I was attempting, and failing, to untie. This work included using a standardized SimpleCommand to launch the GroupDialogView each time, and accessed the parameters for that through the StateManager. To ensuring this was doable, however, required changing StateManager from tracking GroupNodeViewModels to tracking GroupTreeNodes. Something in this process broke the way that re-renders occur, and now I am facing persistent issues of: - Group numbers not updating when groups are added - Colors on entries not accurately reflecting the groups they are part of

Added functionality to allow users to right click to make a new group from the selection. Updated all group creation and group editing to use the CreateGroupAction class, extending SimpleCommand so it would work with the right-click menu. Getting this functionality extracted into an Action that could be called with the available states stored in stateManager required some pretty large changes and bug fixes related to synchronizing the various lists of active and selected entries. Additionally, fixed a bug where when new entries were dropped into a list, the displayed number of entries did not update.

koppor

Just a quick comment - it's late here.

Please check the output of the build pipeline (checkstyle, openrewrite)

Need to discuss with @JabRef/developers whether this context menu or the choice in the group creation dialog was better.

I didn't find the PR of the group creation dialog, otherwise I would have linked it.

CHANGELOG.md

src/main/java/module-info.java

m-peeler · 2024-07-10T01:04:53Z

Working to address the failed tests now! In terms of context menu vs. dialogue, this actually keeps the dialogue; the context menus is just an additional way to access it. This doesn't remove any of the previous functionality, just adds an additional access point to the same historic system (and pulls that system into a 'SimpleCommand' so that it can be run the same way from all three access points).

Co-authored-by: Oliver Kopp <kopp.dev@gmail.com>

…r/jabref into right-click-create-group

Siedlerchr · 2024-07-10T06:45:42Z

You seem to be missing the code style set up https://devdocs.jabref.org/getting-into-the-code/guidelines-for-setting-up-a-local-workspace/intellij-13-code-style.html

calixtus · 2024-07-10T11:32:52Z

Need to discuss with @JabRef/developers whether this context menu or the choice in the group creation dialog was better.

+1 for context menu

koppor

Discussion result: We have special context menus at the group pane. Also "Add selected entries to group". To have consistency here, we propose:

Left side: "Add subgroup using selected entries". Below "Add subgroup"

Addressed failed tests and style guidelines

m-peeler · 2024-07-10T20:53:23Z

Discussion result: We have special context menus at the group pane. Also "Add selected entries to group". To have consistency here, we propose:

Left side: "Add subgroup using selected entries". Below "Add subgroup"

That sounds good and should be fairly easily implemented with the changes I've made. I imagine it should be greyed out if there are multiple groups selected?

Fixed style issues, added new context menu option, fixed bug with update order when selecting a new active group after creating it

…r/jabref into right-click-create-group

m-peeler · 2024-07-10T23:27:51Z

This push should close out the changes, add add in @koppor's request for the addition to the second context menu. Let me know if you find any issues.

koppor

Quick first comments. Maybe, it fosters discusions / actions.

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

src/main/java/org/jabref/gui/StateManager.java

src/main/java/org/jabref/gui/maintable/RightClickMenu.java

koppor

General comments before I comment on details in the code. I think, following comments will lead to much code removal and code change.

This one should be selected:

When the user presses "OK", the entries in the main table are then already in the group.

Remove the context menu entry in the main table

Reason: If it was kept, one also would need two other entries in the context menu:

But then, the menu is too large. We removed it at #4695 - and no users complained since 2019. Thus, no change there.

koppor · 2024-07-11T09:09:03Z

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

+    // Makes sure there is at least one group selected, and if there are multiple groups selected
+    // all have the same parent node.


Convert to JavaDoc

koppor · 2024-07-11T09:16:14Z

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

@@ -36,6 +36,8 @@ public enum StandardActions implements Action {
    EXTRACT_FILE_REFERENCES_OFFLINE(Localization.lang("Extract references from file (offline)"), IconTheme.JabRefIcons.FILE_STAR),
    OPEN_URL(Localization.lang("Open URL or DOI"), IconTheme.JabRefIcons.WWW, KeyBinding.OPEN_URL_OR_DOI),
    SEARCH_SHORTSCIENCE(Localization.lang("Search ShortScience")),
+    ADD_GROUP_FROM_SELECTION(Localization.lang("Add group from selection")),


This can be removed? -- (I don't find the link to the comment I gave, maybe you remember the screenshot that "All entries" always have the context menu with "subgroups", not" groups")

koppor · 2024-07-11T09:28:09Z

src/main/java/org/jabref/gui/exporter/ExportCommand.java

@@ -111,7 +111,7 @@ private void export(Path file, FileChooser.ExtensionFilter selectedExtensionFilt
            // All entries
            entries = stateManager.getActiveDatabase()
                                  .map(BibDatabaseContext::getEntries)
-                                  .orElse(Collections.emptyList());


Is this change needed? I checked the caller - and it seems it is passed to org.jabref.logic.exporter.Exporter#export(org.jabref.model.database.BibDatabaseContext, java.nio.file.Path, java.util.List<org.jabref.model.entry.BibEntry>, java.util.List<java.nio.file.Path>, org.jabref.logic.journals.JournalAbbreviationRepository), which requires a list and nothing observable.

I would even try to use toList() since the list should not be modified (should it?)

I believe I just did this because the function signature changed (from Optional<List<..>> to Optional<ObservableList<...>> and I was trying to minimally fix to comply. Will add in the toList call and change it back.

I will just revert and see if the CI is green.

Reason: BibDatbase context code:

public List<BibEntry> getEntries() { return database.getEntries(); }

Did my revert go through, because I did not see it?

Please double check that this file is not changed!

Per my earlier comment -

This push changes the method signature of getEntries() (since it was already an ObservableList and something needed to be able to bind to it). Removing the .map(List::stream).map(Stream::toList) causes compile errors and seems to be the reason tests are currently failing. I've changed it to instead do .orElse(FXCollections.emptyObservableList()) and that seems to address the compile issue.

koppor · 2024-07-11T10:00:46Z

src/main/java/org/jabref/gui/groups/GroupTreeView.java

@@ -570,7 +599,7 @@ private ContextMenu createContextMenuForGroup(GroupNodeViewModel group) {
    }

    private void addNewGroup() {
-        viewModel.addNewGroupToRoot();


Please think of two options:

a) ENUM covering the options
b) two method (this is favored at Java by Comparison, item "Split Method with Boolean Parameters")

m-peeler · 2024-07-11T10:23:55Z

This one should be selected:

Currently, since there is usually one entry selected at all times, unless you ctrl-click it to unselect, I only have it adding the entries initially if they have multiple - that was if someone is trying to create an empty group it doesn't initially include entries they don't want. Should we maintain any sort of 'don't include selection' at any point, or just assume as such? If the latter is the case, it should lead to the removal of that boolean argument you wanted taken out.

koppor · 2024-07-11T16:20:35Z

Currently, since there is usually one entry selected at all times, unless you ctrl-click it to unselect, I only have it adding the entries initially if they have multiple - that was if someone is trying to create an empty group it doesn't initially include entries they don't want. Should we maintain any sort of 'don't include selection' at any point, or just assume as such? If the latter is the case, it should lead to the removal of that boolean argument you wanted taken out.

I understood your proposal as follows:

User selectes "Add subgroup" -> group dialog opens, user work as sual
User selects "Add subgroup from selected entries" -> group dialog opens, "explicit selection" selected. ~~User can use group dialog. User can change radio button. If may choose other group (e.g., keywords). Then, notification: "selected entries"..~~ User cannot choose other radio button. Meaning: user cannot change away from explicit selection. User enters data. User presses OK. Then, group with explicitly selection type is created. Selected entries added.

Reason:

It is not generally possible to deduct the keywords or other queries for the selected entries in a "nice" way, so that the user is satisfied in 80% of the cases. Thus, group type "explicit selection"
We trigger the explicit selection "externally" from the group dialog. My initial proposal was the checkbox inside the group dialog, which was not done. The implementation moved to some context menu in the main table, which we moved to the group panel.

Did I get something wrong?

m-peeler · 2024-07-11T16:27:50Z

We trigger the explicit selection "externally" from the group dialog. My initial proposal was the checkbox inside the group dialog, which was not done. The implementation moved to some context menu in the main table, which we moved to the group panel.

I think I somehow misread the original checkbox request as a radio button. Check box should be easy to add - I'll do that quickly - and should resolve the issues.

…ckbox & preference Revered changes to context menu on the main table. Removed 'Current selection' radio button in `GroupDialog` and replaced with a 'Include currently selected entries' checkbox, liked both to a parameter in `GroupDialogViewModel` and a preference added to `GroupPreferences`. Edited preference menu to include this new preference. Streamlined `CreateGroupAction` to better fit these changes. Fixed bug where dragging new empty article entries into a new group wouldn't increment the group count.

m-peeler · 2024-07-11T22:36:38Z

Conversion from radio button to checkbox (including the ability to set the preference) has been done.

…d.java

koppor

Quick comments. Need to be more awake for the rest.

koppor · 2024-07-12T22:55:07Z

src/main/java/org/jabref/gui/exporter/ExportCommand.java

@@ -111,7 +111,7 @@ private void export(Path file, FileChooser.ExtensionFilter selectedExtensionFilt
            // All entries
            entries = stateManager.getActiveDatabase()
                                  .map(BibDatabaseContext::getEntries)
-                                  .orElse(Collections.emptyList());


I will just revert and see if the CI is green.

Reason: BibDatbase context code:

public List<BibEntry> getEntries() { return database.getEntries(); }

koppor · 2024-07-12T22:56:29Z

src/main/java/org/jabref/gui/groups/GroupDialog.fxml

+                    <VBox visible="${explicitRadioButton.selected}" spacing="10.0">
+                        <CheckBox fx:id="explicitIncludeSelected" text="%Include selected entries in new subgroup"/>
+                    </VBox>


With this, we do not need the additional context menu entry in the context menu of the group dialog.

Please add a screenshot. Reason: There are interested persons scrolling through PRs only. they cannot read the code and imagine what has changed. You make life easier for those (and also for reviewers) if you add updated screenshots!

Here are the two changed menus

koppor · 2024-07-12T23:00:31Z

src/main/java/org/jabref/gui/groups/GroupDialogViewModel.java

@@ -78,6 +80,8 @@ public class GroupDialogViewModel {
    private final BooleanProperty typeTexProperty = new SimpleBooleanProperty();

    // Option Groups


The heading does not match the following things.

Please move the Boolean Property.

koppor · 2024-07-12T23:01:03Z

src/main/java/org/jabref/gui/groups/GroupDialogViewModel.java

+            @Nullable GroupTreeNode parentNode,
+            FileUpdateMonitor fileUpdateMonitor
+    ) {
+        this(dialogService, currentDatabase, preferencesService, editedGroup, parentNode, fileUpdateMonitor, new ArrayList<>(), Selection.IGNORE_SELECTED_ENTRIES);


USe List.of() instead of new ArrayList<>()

koppor · 2024-07-12T23:01:47Z

src/main/java/org/jabref/gui/groups/GroupDialogViewModel.java

@@ -310,10 +332,15 @@ public AbstractGroup resultConverter(ButtonType button) {
        try {
            String groupName = nameProperty.getValue().trim();
            if (typeExplicitProperty.getValue()) {
-                resultingGroup = new ExplicitGroup(
+                ExplicitGroup tempResultingGroup = new ExplicitGroup(


Why a temp group?

You can directly add to a group?

Please change back and just add to resultingGroup

add is only on EntriesChanger, which ExplicitGroup implements but AbstractGroup does not. I used the temp rather than casting for adding on line 341, but can cast if you prefer.

koppor · 2024-07-12T23:03:21Z

src/main/java/org/jabref/gui/groups/GroupNodeViewModel.java

        } else {
            children = EasyBind.mapBacked(groupNode.getChildren(), this::toViewModel);
        }
        if (groupNode.getGroup() instanceof TexGroup) {
            databaseContext.getMetaData().groupsBinding().addListener(new WeakInvalidationListener(onInvalidatedGroup));
        }
        hasChildren = new SimpleBooleanProperty();
-        hasChildren.bind(Bindings.isNotEmpty(children));
+        hasChildren.bind(Bindings.isNotEmpty(groupNode.getChildren()));


The old code did not work?

Think there was a potential problem with the main table context menu that this was for, but since that's been removed I will revert.

m-peeler · 2024-07-13T04:00:09Z

In reference to this comment - this push changes the method signature (since it was already an ObservableList and something needed to be able to bind to it). Removing the .map(List::stream).map(Stream::toList) causes compile errors and seems to be the reason tests are currently failing. I've changed it to instead do .orElse(FXCollections.emptyObservableList()) and that seems to address the compile issue.

…r/jabref into right-click-create-group

koppor

We have now both the possibility in the dialog to change the behavior and using the contextd menu

This is too much for this small functionality. We also store the default in the preferences (which is good)

Thus, please remove the context menu entry.

src/main/java/org/jabref/gui/groups/GroupDialog.fxml

koppor · 2024-07-13T07:56:40Z

src/main/java/org/jabref/gui/exporter/ExportCommand.java

@@ -111,7 +111,7 @@ private void export(Path file, FileChooser.ExtensionFilter selectedExtensionFilt
            // All entries
            entries = stateManager.getActiveDatabase()
                                  .map(BibDatabaseContext::getEntries)
-                                  .orElse(Collections.emptyList());


Did my revert go through, because I did not see it?

Please double check that this file is not changed!

Co-authored-by: Oliver Kopp <kopp.dev@gmail.com>

… a group

m-peeler · 2024-07-13T10:07:58Z

Thus, please remove the context menu entry.

Done. I also set the button to be disabled when editing a group rather than creating one.

Siedlerchr · 2024-07-24T19:27:06Z

@LoayGhreeb Will this create conflicts with your search float mode PR?

LoayGhreeb · 2024-07-28T05:12:38Z

Will this create conflicts with your search float mode PR?

Yeah, would be better to merge the floating mode PR first.

koppor · 2024-08-04T21:37:24Z

@m-peeler We took long to get #11510 finished, but we finally managed.💪. Can you have a look whetehr you can resolve the merge conflicts so that we can get this PR finished, too?

m-peeler added 15 commits July 5, 2024 02:58

Auto-Select Newly Created Group

910c857

When a new group is added, that group will automatically be selected and focused.

Update CHANGELOG.md

cbc0d7d

Updated changelog to reflect updates.

Addressed Style and Property Name Errors

e4c2281

Fixes failed unit test (keyValueShouldBeEqualForEnglishPropertiesMessages) and style test ( '(' is preceded with whitespace.)

Merge branch 'focus-group-on-creation' into right-click-create-group

9631067

Merge branch 'new-group-from-selected' into right-click-create-group

d4222a7

Merge branch 'focus-group-on-creation'

ca5e69f

Update CHANGELOG.md

8dd474a

Merge branch 'main' into right-click-create-group

12dedd4

Merge branch 'main' of https://github.com/JabRef/jabref

2c87cda

Merge branch 'main' into right-click-create-group

f3011ed

Update CHANGELOG.md

53f1544

koppor reviewed Jul 9, 2024

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

src/main/java/module-info.java Outdated Show resolved Hide resolved

m-peeler and others added 4 commits July 10, 2024 02:34

Update CHANGELOG.md

ec9b081

Co-authored-by: Oliver Kopp <kopp.dev@gmail.com>

Fixed formatting issues & addressed test failures

aabb4a3

Merge branch 'right-click-create-group' of https://github.com/m-peele…

f654aba

…r/jabref into right-click-create-group

Fixed unnecessary module

d772aaa

koppor requested changes Jul 10, 2024

View reviewed changes

Style & Bug Fixes

d3e1f1e

Addressed failed tests and style guidelines

m-peeler and others added 4 commits July 10, 2024 17:01

Inverted conditional & removed unnecessary dependency

e4ec775

Merge branch 'main' into right-click-create-group

fd088d1

Added "Add Subgroup from Selection" to groups context menu.

e2ad784

Fixed style issues, added new context menu option, fixed bug with update order when selecting a new active group after creating it

Merge branch 'right-click-create-group' of https://github.com/m-peele…

cf33b73

…r/jabref into right-click-create-group

Fixed text comparison issue

85d42b9

koppor requested changes Jul 11, 2024

View reviewed changes

m-peeler added 2 commits July 11, 2024 02:51

Fixed bad bindings

00de24b

Removed accidential imports

a1dfe75

koppor requested changes Jul 11, 2024

View reviewed changes

koppor reviewed Jul 11, 2024

View reviewed changes

koppor mentioned this pull request Jul 11, 2024

Synchronized scrolling at diff view tsantalis/RefactoringMiner#672

Open

Discard changes to src/main/java/org/jabref/gui/exporter/ExportComman…

f80e515

…d.java

koppor requested changes Jul 12, 2024

View reviewed changes

koppor added the groups label Jul 12, 2024

m-peeler added 2 commits July 13, 2024 00:00

Bug fixes, variable ordering & unnecessary change reversions

c5241f0

Merge branch 'right-click-create-group' of https://github.com/m-peele…

bba0522

…r/jabref into right-click-create-group

koppor requested changes Jul 13, 2024

View reviewed changes

m-peeler and others added 2 commits July 13, 2024 05:34

Update src/main/java/org/jabref/gui/groups/GroupDialog.fxml

1912281

Co-authored-by: Oliver Kopp <kopp.dev@gmail.com>

Removed context menu option, disabled 'include selected' when editing…

4931eac

… a group

m-peeler and others added 2 commits July 13, 2024 06:18

Fixed imports

f928a36

Merge branch 'main' into right-click-create-group

93f2df5

Merge branch 'main' into right-click-create-group

0367f7b

koppor mentioned this pull request Aug 4, 2024

Implemented "Current selection" group option #11455

Closed

6 tasks

		// Makes sure there is at least one group selected, and if there are multiple groups selected
		// all have the same parent node.

		@@ -78,6 +80,8 @@ public class GroupDialogViewModel {
		private final BooleanProperty typeTexProperty = new SimpleBooleanProperty();

		// Option Groups

Right click create group #11476

Are you sure you want to change the base?

Right click create group #11476

Conversation

m-peeler commented Jul 9, 2024

Mandatory checks

koppor left a comment

Choose a reason for hiding this comment

m-peeler commented Jul 10, 2024 • edited Loading

Siedlerchr commented Jul 10, 2024

calixtus commented Jul 10, 2024

koppor left a comment

Choose a reason for hiding this comment

m-peeler commented Jul 10, 2024

m-peeler commented Jul 10, 2024

koppor left a comment

Choose a reason for hiding this comment

koppor left a 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

Choose a reason for hiding this comment

m-peeler commented Jul 11, 2024 • edited Loading

koppor commented Jul 11, 2024

m-peeler commented Jul 11, 2024

m-peeler commented Jul 11, 2024

koppor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m-peeler Jul 13, 2024 • 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

m-peeler commented Jul 13, 2024 • edited Loading

koppor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m-peeler commented Jul 13, 2024

Siedlerchr commented Jul 24, 2024

LoayGhreeb commented Jul 28, 2024

koppor commented Aug 4, 2024

m-peeler commented Jul 10, 2024 •

edited

Loading

m-peeler commented Jul 11, 2024 •

edited

Loading

m-peeler Jul 13, 2024 •

edited

Loading

m-peeler commented Jul 13, 2024 •

edited

Loading