-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Right click create group #11476
base: main
Are you sure you want to change the base?
Right click create group #11476
Conversation
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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
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 |
+1 for context menu |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed failed tests and style guidelines
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
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quick first comments. Maybe, it fosters discusions / actions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
// Makes sure there is at least one group selected, and if there are multiple groups selected | ||
// all have the same parent node. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Convert to JavaDoc
@@ -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")), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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")
@@ -111,7 +111,7 @@ private void export(Path file, FileChooser.ExtensionFilter selectedExtensionFilt | |||
// All entries | |||
entries = stateManager.getActiveDatabase() | |||
.map(BibDatabaseContext::getEntries) | |||
.orElse(Collections.emptyList()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will just revert and see if the CI is green.
Reason: BibDatbase context code:
public List<BibEntry> getEntries() {
return database.getEntries();
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did my revert go through, because I did not see it?
Please double check that this file is not changed!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per my earlier comment -
This push changes the method signature of
getEntries()
(since it was already anObservableList
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.
@@ -570,7 +599,7 @@ private ContextMenu createContextMenuForGroup(GroupNodeViewModel group) { | |||
} | |||
|
|||
private void addNewGroup() { | |||
viewModel.addNewGroupToRoot(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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")
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:
Reason:
Did I get something wrong? |
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.
Conversion from radio button to checkbox (including the ability to set the preference) has been done. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quick comments. Need to be more awake for the rest.
@@ -111,7 +111,7 @@ private void export(Path file, FileChooser.ExtensionFilter selectedExtensionFilt | |||
// All entries | |||
entries = stateManager.getActiveDatabase() | |||
.map(BibDatabaseContext::getEntries) | |||
.orElse(Collections.emptyList()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will just revert and see if the CI is green.
Reason: BibDatbase context code:
public List<BibEntry> getEntries() {
return database.getEntries();
}
<VBox visible="${explicitRadioButton.selected}" spacing="10.0"> | ||
<CheckBox fx:id="explicitIncludeSelected" text="%Include selected entries in new subgroup"/> | ||
</VBox> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -78,6 +80,8 @@ public class GroupDialogViewModel { | |||
private final BooleanProperty typeTexProperty = new SimpleBooleanProperty(); | |||
|
|||
// Option Groups |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The heading does not match the following things.
Please move the Boolean Property.
@Nullable GroupTreeNode parentNode, | ||
FileUpdateMonitor fileUpdateMonitor | ||
) { | ||
this(dialogService, currentDatabase, preferencesService, editedGroup, parentNode, fileUpdateMonitor, new ArrayList<>(), Selection.IGNORE_SELECTED_ENTRIES); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
USe List.of()
instead of new ArrayList<>()
@@ -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( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why a temp group?
You can directly add to a group?
Please change back and just add to resultingGroup
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
} 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())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The old code did not work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -111,7 +111,7 @@ private void export(Path file, FileChooser.ExtensionFilter selectedExtensionFilt | |||
// All entries | |||
entries = stateManager.getActiveDatabase() | |||
.map(BibDatabaseContext::getEntries) | |||
.orElse(Collections.emptyList()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>
Done. I also set the button to be disabled when editing a group rather than creating one. |
@LoayGhreeb Will this create conflicts with your search float mode PR? |
Yeah, would be better to merge the floating mode PR first. |
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
CHANGELOG.md
described in a way that is understandable for the average user (if applicable)