jabref icon indicating copy to clipboard operation
jabref copied to clipboard

Right click create group

Open m-peeler opened this issue 1 year ago • 14 comments

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 https://github.com/JabRef/jabref/issues/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.

image

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

  • [X] Change in CHANGELOG.md described in a way that is understandable for the average user (if applicable)
  • [X] Tests created for changes (if applicable)
  • [X] 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.
  • [X] 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.

m-peeler avatar Jul 09 '24 22:07 m-peeler

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).

m-peeler avatar Jul 10 '24 01:07 m-peeler

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

Siedlerchr avatar Jul 10 '24 06:07 Siedlerchr

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

+1 for context menu

calixtus avatar Jul 10 '24 11:07 calixtus

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?

m-peeler avatar Jul 10 '24 20:07 m-peeler

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.

m-peeler avatar Jul 10 '24 23:07 m-peeler

This one should be selected:

image

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.

m-peeler avatar Jul 11 '24 10:07 m-peeler

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?

koppor avatar Jul 11 '24 16:07 koppor

  • 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.

m-peeler avatar Jul 11 '24 16:07 m-peeler

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

m-peeler avatar Jul 11 '24 22:07 m-peeler

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.

m-peeler avatar Jul 13 '24 04:07 m-peeler

Thus, please remove the context menu entry.

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

m-peeler avatar Jul 13 '24 10:07 m-peeler

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

Siedlerchr avatar Jul 24 '24 19:07 Siedlerchr

Will this create conflicts with your search float mode PR?

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

LoayGhreeb avatar Jul 28 '24 05:07 LoayGhreeb

@m-peeler We took long to get https://github.com/JabRef/jabref/pull/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?

koppor avatar Aug 04 '24 21:08 koppor

Ah, this will require conflict resolution again after Postgres search, I think?

subhramit avatar Oct 25 '24 12:10 subhramit

Ah, this will require conflict resolution again after Postgres search, I think?

Did not forsee that, but maybe yes. I see many conflicts.

koppor avatar Oct 25 '24 13:10 koppor

Closing this issue due to inactivity :zzz:

Please ping us if you intend to resume work on this one.

koppor avatar Dec 09 '24 20:12 koppor