Fix for issue 4682 : Restructuring the side pane structure having additional functionality and merging the remove group menus #7714

apurvachuri · 2021-05-08T06:57:26Z

Rearrange side pane structure having add button moved to the bottom of the side pane and merging the remove group menus.

Fixes #4682

Following are the changes that we implemented:

Add group functionality
- Remove '+' button beside filter group tab
- Add group button at the bottom of the side pane
- Button dim after adding 10 groups.

Fix the header title in the Add Group/Subgroup Dialog box

Merge 'Remove group' having two options, with or without subgroups

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 documentation: Is the information available and up to date? If not created an issue at https://github.com/JabRef/user-documentation/issues or, even better, submitted a pull request to the documentation repository.

…ps and Remove group, keep subgroup

Siedlerchr

Overall looks already good!
From my side only a few small remarks/suggestions regarding the code style

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

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

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

Siedlerchr · 2021-05-08T10:11:21Z

Please look at the failing unit + checkstyle /reviewdog and also change the PR title to a more meaningful name

src/main/java/org/jabref/gui/groups/GroupTree.css

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

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

…java to store enum

…gHeader.java to store enum" This reverts commit c9cdff3.

…oupDialogHeader.java to store enum"" This reverts commit c69b9cb.

…gHeader.java to store enum" This reverts commit c9cdff3.

…java to store enum

calixtus · 2021-05-14T12:15:13Z

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

+    private void setNewGroupButtonStyle(TreeTableView<GroupNodeViewModel> groupTree) {
+        if (groupTree.getRoot() != null) {
+            if (groupTree.getExpandedItemCount() <= 10) {
+                addNewGroup.setStyle("-fx-border-width: 0px; -fx-background-color: -jr-theme;" +
+                        " -fx-padding: 0.5em; -fx-text-fill: -jr-white;");
+            } else {
+                addNewGroup.setStyle("-fx-border-width: 0px; -fx-background-color: -jr-icon-background-active;" +
+                        " -fx-padding: 0.5em; -fx-text-fill: -jr-black;");
+            }
+        } else {
+            addNewGroup.setStyle("-fx-border-width: 0px; -fx-background-color: -jr-theme;" +
+                    " -fx-padding: 0.5em; -fx-text-fill: -jr-white;");
+        }
+    }


Instead of hardcoding the css in the java file, please make this a pseudoclass in css, which should be very easy to implement. See BindingsHelper::includePseudoClassWhen !

Hi @calixtus , i've checked this BindingsHelper::includePseudoClassWhen needs a parameter of node and the css changes should be applied to the nodes whenever a condition is applied. however, in my change, what I'm changing is the color of Add Group button whenever the user added 10 groups (doesn't include the count of subgroups) and not the nodes. Any advise is greatly appreciated.

The node should be the addNewGroup.
A new Binding to be checked for can be created in some initialize method or the constructor of the class. You only need one Observable which is triggered when change to the list happens.
Please take a look at other occurences of includePseudoClassWhen to get an impression, how to implement this. Again, in theory this should be very straightforward. If you really don't get this to work, I can help a little bit in the next days, im just a bit busy today.

calixtus · 2021-05-14T12:16:44Z

src/main/resources/l10n/JabRef_en.properties

@@ -1782,7 +1782,7 @@ No\ full\ text\ document\ found\ for\ entry\ %0.=No full text document found for
 Delete\ Entry=Delete Entry
 Next\ library=Next library
 Previous\ library=Previous library
-add\ group=add group
+add\ group=Add group


Translation string must be consistent with english translation.

Suggested change

add\ group=Add group

Add\ group=Add group

calixtus

Some small remarks

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

damassh · 2021-05-19T02:36:46Z

Please look at the failing unit + checkstyle /reviewdog and also change the PR title to a more meaningful name

@Siedlerchr , I am sorry I'm new in checking the unit test. Is it possible if you can point me to the right direction on how to check it? I've clicked the details and directed me to a https://github.com/JabRef/jabref/pull/7714/checks?check_run_id=2604593213 but not sure how to investigate on the code itself on how to debug it and fix it.

…nd removed unused entries in english language file

calixtus · 2021-05-20T11:20:05Z

Please look at the failing unit + checkstyle /reviewdog and also change the PR title to a more meaningful name

@Siedlerchr , I am sorry I'm new in checking the unit test. Is it possible if you can point me in the right direction on how to check it? I've clicked the details and directed me to a https://github.com/JabRef/jabref/pull/7714/checks?check_run_id=2604593213 but not sure how to investigate on the code itself on how to debug it and fix it.

The provided link returns me a 404 😞

calixtus · 2021-05-20T11:22:22Z

The checkstyle fails on checking the changelog file and says the following;

CHANGELOG.md:105 MD022/blanks-around-headings/blanks-around-headers Headings should be surrounded by blank lines [Expected: 1; Actual: 0; Below] [Context: "### Removed"]
CHANGELOG.md:106 MD032/blanks-around-lists Lists should be surrounded by blank lines [Context: "- We removed add group button ..."]

Looks just like you forgot a blank line before or after a heading in changelog.md

calixtus

Few comments

calixtus · 2021-05-20T11:23:51Z

src/main/java/org/jabref/gui/groups/GroupTree.css


-#newGroupButton {
-    -fx-padding: 0.1em 1.5em 0.1em 1.5em;
+#addNewGroup {
+    /*-fx-padding: 0.1em 1.5em 0.1em 1.5em;*/
 }


Why keeping it, when commented? For version history we got git.

calixtus · 2021-05-20T11:24:39Z

CHANGELOG.md

 ### Removed
+- We removed add group button beside the filter group tab. [#4682](https://github.com/JabRef/jabref/issues/4682)


Keep a blank line before and after a heading.

Added a blank line after the heading "#Removed"

In this case, Visual Studio Code helps - with following plugin: https://marketplace.visualstudio.com/items?itemName=DavidAnson.vscode-markdownlint

I am not aware of any similar plugin for IntelliJ.

calixtus · 2021-05-20T11:30:56Z

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

+    private void setNewGroupButtonStyle(TreeTableView<GroupNodeViewModel> groupTree) {
+        if (groupTree.getRoot() != null) {
+            if (groupTree.getExpandedItemCount() <= 10) {
+                addNewGroup.setStyle("-fx-border-width: 0px; -fx-background-color: -jr-theme;" +
+                        " -fx-padding: 0.5em; -fx-text-fill: -jr-white;");
+            } else {
+                addNewGroup.setStyle("-fx-border-width: 0px; -fx-background-color: -jr-icon-background-active;" +
+                        " -fx-padding: 0.5em; -fx-text-fill: -jr-black;");
+            }
+        } else {
+            addNewGroup.setStyle("-fx-border-width: 0px; -fx-background-color: -jr-theme;" +
+                    " -fx-padding: 0.5em; -fx-text-fill: -jr-white;");
+        }
+    }


The node should be the addNewGroup.
A new Binding to be checked for can be created in some initialize method or the constructor of the class. You only need one Observable which is triggered when change to the list happens.
Please take a look at other occurences of includePseudoClassWhen to get an impression, how to implement this. Again, in theory this should be very straightforward. If you really don't get this to work, I can help a little bit in the next days, im just a bit busy today.

damassh · 2021-05-20T11:41:45Z

Please look at the failing unit + checkstyle /reviewdog and also change the PR title to a more meaningful name

@Siedlerchr , I am sorry I'm new in checking the unit test. Is it possible if you can point me in the right direction on how to check it? I've clicked the details and directed me to a https://github.com/JabRef/jabref/pull/7714/checks?check_run_id=2604593213 but not sure how to investigate on the code itself on how to debug it and fix it.

The provided link returns me a 404 😞

This also gives us an error 404 and was about raise it as well. The provided link for CUSTOM_EXPORTS_NAME_FORMATTER doesn't exist. @Siedlerchr @tobiasdiez may you help to advise if this is expected?

Added a blank line after the heading "#Removed"

calixtus · 2021-05-20T13:24:15Z

This also gives us an error 404 and was about raise it as well. The provided link for CUSTOM_EXPORTS_NAME_FORMATTER doesn't exist. @Siedlerchr @tobiasdiez may you help to advise if this is expected?

The HelpFileTests are unrelated to your changes and are caused bay changes to our help system gitbook. They should be gone after merging the most recent main-branch into your branch an can be ignored.

calixtus

Sorry for the delay here, we all were a bit busy the last few weeks.
One quick question on this one, everything else looks good to me.

src/main/java/org/jabref/gui/groups/GroupTree.css

Siedlerchr · 2021-06-08T16:40:43Z

From my side lgtm

calixtus

Thank you very much for this contribution. I personally want to add that I really appreciate your Will to stay on this, fix issues with the PR and including our remarks, until the PR can be merged!

Merging now!

* upstream/main: Added auto-key-generation task to task-progress (JabRef#7797) cleanup temporary files, use prefix "jabref-" (JabRef#7811) Add easier how-to for checklist (JabRef#7813) Fix annotation + package of ACMPortalParserTest (JabRef#7812) Implemented a select all button for the library import function (issue JabRef#7786) (JabRef#7808) Fix for issue 4682 : Restructuring the side pane structure having additional functionality and merging the remove group menus (JabRef#7714)

josipovicjohn and others added 12 commits April 15, 2021 15:15

removed three menu titles: Remove subgroups, Remove group and subgrou…

4ffcffb

…ps and Remove group, keep subgroup

Move the add group button to the bottom of the group view pane

8bd0748

created remove group submenu function in group tree view

53c75c7

Fix the header tile in the add group/subgroup dialog box

22549b3

implemented remove group submenu in view only

f851464

fixed menu titles

955523f

implemented subgroup behaviours in menu

96f69d4

about to merge upstream with local

e196041

Ready to test implementation of submenu

783198a

fixed the header title with the latest code

35b9a06

Update CHANGELOG.md with fixes for JabRef#4682

705f29f

Merge branch 'main' into fix-for-issue-4682

f97c656

apurvachuri mentioned this pull request May 8, 2021

Groups context menu: Structure remove groups and subgroups #4682

Open

Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label May 8, 2021

Siedlerchr requested changes May 8, 2021

View reviewed changes

Siedlerchr added status: changes required Pull requests that are not yet complete and removed status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers labels May 10, 2021

Change the maxWidth of the add group button to infinity

d8ab8bb

apurvachuri changed the title ~~Fix for issue 4682~~ Fix for issue 4682 : Restructuring the side pane structure having additional functionality and merging the remove group menus May 11, 2021

calixtus reviewed May 12, 2021

View reviewed changes

src/main/java/org/jabref/gui/groups/GroupTree.css Outdated Show resolved Hide resolved

calixtus reviewed May 12, 2021

View reviewed changes

src/main/java/org/jabref/gui/groups/GroupTree.fxml Outdated Show resolved Hide resolved

calixtus reviewed May 12, 2021

View reviewed changes

src/main/java/org/jabref/gui/groups/GroupDialogViewModel.java Outdated Show resolved Hide resolved

damassh and others added 6 commits May 12, 2021 21:05

Add the maxwidth of add group button in fxml and remove it form css

1cc0946

Changed isGroup boolean to enum declaration, added GroupDialogHeader.…

c9cdff3

…java to store enum

Revert "Changed isGroup boolean to enum declaration, added GroupDialo…

c69b9cb

…gHeader.java to store enum" This reverts commit c9cdff3.

Revert "Revert "Changed isGroup boolean to enum declaration, added Gr…

29f51b3

…oupDialogHeader.java to store enum"" This reverts commit c69b9cb.

Revert "Changed isGroup boolean to enum declaration, added GroupDialo…

9ff3879

…gHeader.java to store enum" This reverts commit c9cdff3.

Changed isGroup boolean to enum declaration, added GroupDialogHeader.…

fb2ffc8

…java to store enum

calixtus reviewed May 14, 2021

View reviewed changes

calixtus requested changes May 14, 2021

View reviewed changes

calixtus reviewed May 14, 2021

View reviewed changes

src/main/java/org/jabref/gui/groups/GroupDialogHeader.java Show resolved Hide resolved

josipovicjohn and others added 3 commits May 19, 2021 16:31

passed LocalizationConsistencyTest

051c5d6

Delete dupllicate add group

28b1761

renamed langauge localization keys in UndoableAddOrRemoveGroup.java a…

b9f7012

…nd removed unused entries in english language file

apurvachuri requested review from calixtus and Siedlerchr May 20, 2021 11:02

calixtus requested changes May 20, 2021

View reviewed changes

Update CHANGELOG.md

6d9a554

Added a blank line after the heading "#Removed"

calixtus self-assigned this May 24, 2021

Use PseudoClass to implement the changes of NewGroup button

edd440c

apurvachuri requested review from calixtus and tobiasdiez May 26, 2021 23:23

calixtus reviewed Jun 3, 2021

View reviewed changes

src/main/java/org/jabref/gui/groups/GroupTree.css Show resolved Hide resolved

Siedlerchr approved these changes Jun 8, 2021

View reviewed changes

Siedlerchr added status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers and removed status: changes required Pull requests that are not yet complete labels Jun 8, 2021

calixtus approved these changes Jun 8, 2021

View reviewed changes

calixtus merged commit 9d1c448 into JabRef:main Jun 8, 2021

mlep mentioned this pull request Oct 4, 2021

UI: Dark theme: "Add group" button seems inactivated #8051

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for issue 4682 : Restructuring the side pane structure having additional functionality and merging the remove group menus #7714

Fix for issue 4682 : Restructuring the side pane structure having additional functionality and merging the remove group menus #7714

apurvachuri commented May 8, 2021

Siedlerchr left a comment

Siedlerchr commented May 8, 2021

calixtus May 14, 2021

damassh May 20, 2021

calixtus May 20, 2021

calixtus May 14, 2021

damassh May 20, 2021

calixtus left a comment

damassh commented May 19, 2021 •

edited

Loading

calixtus commented May 20, 2021

calixtus commented May 20, 2021

calixtus left a comment

calixtus May 20, 2021

calixtus May 20, 2021

apurvachuri May 20, 2021

koppor May 20, 2021

calixtus May 20, 2021

damassh commented May 20, 2021

calixtus commented May 20, 2021

calixtus left a comment

Siedlerchr commented Jun 8, 2021

calixtus left a comment

		### Removed
		- We removed add group button beside the filter group tab. [#4682](https://github.com/JabRef/jabref/issues/4682)

Fix for issue 4682 : Restructuring the side pane structure having additional functionality and merging the remove group menus #7714

Fix for issue 4682 : Restructuring the side pane structure having additional functionality and merging the remove group menus #7714

Conversation

apurvachuri commented May 8, 2021

Siedlerchr left a comment

Choose a reason for hiding this comment

Siedlerchr commented May 8, 2021

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

calixtus left a comment

Choose a reason for hiding this comment

damassh commented May 19, 2021 • edited Loading

calixtus commented May 20, 2021

calixtus commented May 20, 2021

calixtus 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

damassh commented May 20, 2021

calixtus commented May 20, 2021

calixtus left a comment

Choose a reason for hiding this comment

Siedlerchr commented Jun 8, 2021

calixtus left a comment

Choose a reason for hiding this comment

damassh commented May 19, 2021 •

edited

Loading