Skip to content
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

After creation of a group, the group should be focused #11449

Closed
koppor opened this issue Jul 4, 2024 · 6 comments · Fixed by #11453 · May be fixed by #11476
Closed

After creation of a group, the group should be focused #11449

koppor opened this issue Jul 4, 2024 · 6 comments · Fixed by #11453 · May be fixed by #11476
Assignees
Labels
good first issue An issue intended for project-newcomers. Varies in difficulty. groups ui

Comments

@koppor
Copy link
Member

koppor commented Jul 4, 2024

  1. Create a new library
  2. Ensure that "Groups" are visible:
    image
  3. Create new Group "Test": Click on "Add group" (below), enter "Test" as name, click "OK"
    image
  4. Result: Group "Test" created, group "All entries" focused:
    image

Expected result: Group "Test" focused:

image


This issue is very much about code reading and understanding how JavaFX and JabRef works. The fix itself probably will be three lines of code. The challenge is really: Which three lines and where to put these lines.

@koppor koppor added the good first issue An issue intended for project-newcomers. Varies in difficulty. label Jul 4, 2024
@github-project-automation github-project-automation bot moved this to Free to take in Good First Issues Jul 4, 2024
@github-project-automation github-project-automation bot moved this to Normal priority in JabRef UI Improvements Jul 4, 2024
@Divyam2806
Copy link

I think i know where to add those "three lines", in src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java there is a function addNewGroupToRoot() which is called when a new group is made using the "Add group button" but i cant figure out which function to call inside this function so that the newly added group should be focused. I think its maybe setSelectedGroup() function of StateManager class but i am not able to use it correctly, any help is appreciated.

@m-peeler
Copy link
Contributor

m-peeler commented Jul 5, 2024

Building on Divyam's sluthing, I've made a fork and a branch on it that is able to successfully select and focus the new node.

However, I wonder if this actually desirable behavior? In many use cases, I can imagine wanting to select a number of entries, create a new group, and add them to that group. In the current version, this is straightforward: select entries -> Add group -> right click on group -> Add selected entries.

The new behavior, however, means that changing the group will deselect those entries. Now, if a person attempts to select before making the group, their selection will be lost and they'll have to navigate back to the previous page before they can reselect -> right-click on group -> Add selected entries.

I might suggest two options, which sort of go hand-in-hand. One is a new option in the Add group dialogue's Collection options, which would be From selection and include all selected entries. The second would be that right clicking on an entry or a group of selected entries would allow the option to Add group from selection, which would launch the Add group dialogue with the From selection option pre-selected. Both would reduce the length of time it takes to generate a group in this manner, and seem fairly straight forward to implement.

I'll submit a pull request with the current fix, but think it may be wise to implement these sorts of feature additions before it is merged into main. If this seems reasonable, I would be happy to go ahead and work to implement those changes.

@Divyam2806
Copy link

Building on Divyam's sluthing, I've made a fork and a branch on it that is able to successfully select and focus the new node.

However, I wonder if this actually desirable behavior? In many use cases, I can imagine wanting to select a number of entries, create a new group, and add them to that group. In the current version, this is straightforward: select entries -> Add group -> right click on group -> Add selected entries.

The new behavior, however, means that changing the group will deselect those entries. Now, if a person attempts to select before making the group, their selection will be lost and they'll have to navigate back to the previous page before they can reselect -> right-click on group -> Add selected entries.

I might suggest two options, which sort of go hand-in-hand. One is a new option in the Add group dialogue's Collection options, which would be From selection and include all selected entries. The second would be that right clicking on an entry or a group of selected entries would allow the option to Add group from selection, which would launch the Add group dialogue with the From selection option pre-selected. Both would reduce the length of time it takes to generate a group in this manner, and seem fairly straight forward to implement.

I'll submit a pull request with the current fix, but think it may be wise to implement these sorts of feature additions before it is merged into main. If this seems reasonable, I would be happy to go ahead and work to implement those changes.

can i ask which function did you finally end up using to focus the added group, for my knowledge?

@m-peeler
Copy link
Contributor

m-peeler commented Jul 5, 2024

Had to change things in three spots:

  1. In GroupTreeViewModel.addNewSubgroup(), add the subgroup to the parent, and use the return value to instantiate a new GroupNodeViewModel that you put into GroupTreeViewModel.selectedGroups.
                GroupTreeNode newSubgroup = parent.addSubgroup(group);
                selectedGroups.setAll(new GroupNodeViewModel(database, stateManager, taskExecutor, newSubgroup, localDragboard, preferences));
  1. In GroupTreeView, when registering a bidirectional binding between the treeView's getSelectionModel().getSelectionItems() and the viewModel's getSelectionGroups(), I updated the callback function for changes in the treeView to clear the contents of groupTree.getSelectionModel() before adding new selections to it.
BindingsHelper.bindContentBidirectional(
                        groupTree.getSelectionModel().getSelectedItems(),
                        viewModel.selectedGroupsProperty(),
                        newSelectedGroups -> {
>>>                         groupTree.getSelectionModel().clearSelection();
                            newSelectedGroups.forEach(this::selectNode);
                        },
                        this::updateSelection
                ));
  1. In the StateManager, I similarly cleared the contents of StateManager.selectedGroups before adding new items.

Trickiest part was that the state was being stored in three separate places, and I had to figure out how to keep the updates between the three of them accurate.

@koppor
Copy link
Member Author

koppor commented Jul 5, 2024

I might suggest two options, which sort of go hand-in-hand. One is a new option in the Add group dialogue's Collection options, which would be From selection and include all selected entries. The second would be that right clicking on an entry or a group of selected entries would allow the option to Add group from selection, which would launch the Add group dialogue with the From selection option pre-selected. Both would reduce the length of time it takes to generate a group in this manner, and seem fairly straight forward to implement.

Both are cool. I think, the first one is even more simpler to implement

image

Add checkbox - and remember in the preference the state of the checkbox.

@m-peeler
Copy link
Contributor

m-peeler commented Jul 5, 2024

Add checkbox - and remember in the preference the state of the checkbox.

Yes, exactly what I was thinking. Since it really only matters as an option if you currently have entries selected, instead of messing with preferences, I think it would make more sense to pass GroupDialogViewModel the currently-selected entries and, if the list is not empty, default to this From selection option. I might also try greying out the option if there are no currently-selected entries?

@ThiloteE ThiloteE moved this from Free to take to In Progress in Candidates for University Projects Jul 8, 2024
@ThiloteE ThiloteE moved this from Free to take to In Progress in Good First Issues Jul 8, 2024
@github-project-automation github-project-automation bot moved this from Normal priority to Closed in JabRef UI Improvements Jul 9, 2024
@github-project-automation github-project-automation bot moved this from In Progress to Done in Candidates for University Projects Jul 9, 2024
@github-project-automation github-project-automation bot moved this from In Progress to Done in Good First Issues Jul 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue An issue intended for project-newcomers. Varies in difficulty. groups ui
Projects
Archived in project
Archived in project
3 participants