[WIP] Added Select all and Deselect all buttons to the "Review changes" dialog #3731

N4tus · 2018-02-17T01:39:13Z

Added "Select all" and "Deselect all" buttons to the "Review changes" dialog. The "Select all" button is the fix for koppor#280 made by janek greif. The "Deselect all" button is an addition to that issue.

Change in CHANGELOG.md described
Tests created for changes
Screenshots added (for bigger UI changes)
Manually tested changed features in running JabRef
Check documentation status (Issue created for outdated help page at help.jabref.org?)

…g (fix for issue JabRef#280)

Siedlerchr · 2018-02-17T11:08:39Z

CHANGELOG.md

@@ -27,6 +27,7 @@ For more details refer to the [field mapping help page](http://help.jabref.org/e
 - We improved file saving so that hard links are now preserved when a save is performed [#2633](https://github.com/JabRef/jabref/issues/2633)
 - We changed the default dialog option when removing a [file link](http://help.jabref.org/en/FileLinks#adding-external-links-to-an-entry) from an entry.
 The new default removes the linked file from the entry instead of deleting the file from disk. [#3679](https://github.com/JabRef/jabref/issues/3679)
+- Added a "Select all" [#280](https://github.com/koppor/jabref/issues/280) and "Deselect all" button in the "Review changes" dialog.


Please look at the other changelog issues and add koppor#280 as link text

To fix the merge conflict, just move the changelog line a view lines above this line. We will sort the CHANGELOG.md entries before a release.

Siedlerchr · 2018-02-17T11:09:08Z

src/main/java/org/jabref/gui/collab/ChangeDisplayDialog.java

@@ -50,8 +53,36 @@ public ChangeDisplayDialog(JFrame owner, final BasePanel panel,
        tree = new JTree(root);
        tree.addTreeSelectionListener(this);

+        //start fix issue 280


remove your comments as they are useless and don't carry any information

Siedlerchr · 2018-02-17T11:21:47Z

src/main/java/org/jabref/gui/collab/ChangeDisplayDialog.java

+            Enumeration<? extends Object> nodes = root.preorderEnumeration();
+            while (nodes.hasMoreElements()) {
+                Object o = nodes.nextElement();
+                if ((o instanceof ChangeViewModel) && (o != root)) {


I am just unsure about the o !=root, but otherwise the code is fine and works

Please add a source code comment at the line above on why this check is necessary.

koppor

Minor comments. In general LGTM. The code is straightforward, but better code requires separate methods and I am not sure whether this is worth for 10 SLOC.

koppor · 2018-02-18T13:44:18Z

src/main/java/org/jabref/gui/collab/ChangeDisplayDialog.java

+        buttonAlignment.add(selectAll);
+        buttonAlignment.add(deselectAll);
+        treePanel.add(buttonAlignment, BorderLayout.SOUTH);
+        //end fix issue 280


Remove this line

koppor · 2018-02-18T13:45:04Z

src/main/java/org/jabref/gui/collab/ChangeDisplayDialog.java

        JSplitPane pane = new JSplitPane();
-        pane.setLeftComponent(new JScrollPane(tree));
+        pane.setLeftComponent(new JScrollPane(treePanel)); //fix issue 280 change: tree -> treePanel


Remote the comment. You can elaborate on line 58 why you introduce a new treepanel: Code comments should be self-contained.

koppor · 2018-02-18T13:45:30Z

src/main/java/org/jabref/gui/collab/ChangeDisplayDialog.java

+            Enumeration<? extends Object> nodes = root.preorderEnumeration();
+            while (nodes.hasMoreElements()) {
+                Object o = nodes.nextElement();
+                if ((o instanceof ChangeViewModel) && (o != root)) {


Please add a source code comment at the line above on why this check is necessary.

koppor · 2018-02-18T13:46:03Z

CHANGELOG.md

@@ -27,6 +27,7 @@ For more details refer to the [field mapping help page](http://help.jabref.org/e
 - We improved file saving so that hard links are now preserved when a save is performed [#2633](https://github.com/JabRef/jabref/issues/2633)
 - We changed the default dialog option when removing a [file link](http://help.jabref.org/en/FileLinks#adding-external-links-to-an-entry) from an entry.
 The new default removes the linked file from the entry instead of deleting the file from disk. [#3679](https://github.com/JabRef/jabref/issues/3679)
+- Added a "Select all" [#280](https://github.com/koppor/jabref/issues/280) and "Deselect all" button in the "Review changes" dialog.


To fix the merge conflict, just move the changelog line a view lines above this line. We will sort the CHANGELOG.md entries before a release.

koppor · 2018-02-18T13:47:31Z

src/main/java/org/jabref/gui/collab/ChangeDisplayDialog.java

+        JButton selectAll = new JButton(Localization.lang("Select all"));
+        JButton deselectAll = new JButton(Localization.lang("Deselect all"));
+        ActionListener selectEvent = (event) -> {
+            boolean accept = event.getSource() == selectAll;


Please add braces around the expression:

boolean accept = (event.getSource() == selectAll);

Rename the variable to "selectStateSelectAll" (or similar). "accept" is not a speaking name, but "selectAll" is already taken by the button. Maybe, the button could be named "selectAllButton".

koppor · 2018-02-18T13:56:58Z

src/main/java/org/jabref/gui/collab/ChangeDisplayDialog.java

+        treePanel.add(tree, BorderLayout.CENTER);
+        JButton selectAll = new JButton(Localization.lang("Select all"));
+        JButton deselectAll = new JButton(Localization.lang("Deselect all"));
+        ActionListener selectEvent = (event) -> {


Maybe the event should be called "selectOrDelesectEvent" to mirror that it is called by both select and deselect button.

koppor · 2018-02-19T17:48:58Z

Please add a screenshot so that we can compare it with #3742.

lenhard · 2018-02-21T11:03:12Z

And added to that, please merge with master to resolve the conflicts in the Changelog.

koppor · 2018-03-09T19:26:32Z

At my last review, I decided that the code of #3742 looks better at first sight than this code.

@Siedlerchr could you please decide whether we continue with this code or the other one? Maybe the code of #3742 is very incomplete?

tobiasdiez · 2018-10-09T18:30:07Z

Since this is still implemented in Swing, it is probably easier to add the button in the progress (or after) converting the dialog to JavaFX. Hence, I'll close this PR now.

Added Select all and Deselect all Buttons to the Review changes dialo…

e3179ea

…g (fix for issue JabRef#280)

Siedlerchr reviewed Feb 17, 2018

View reviewed changes

Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Feb 17, 2018

koppor added the PE1718 label Feb 18, 2018

koppor requested changes Feb 18, 2018

View reviewed changes

Siedlerchr mentioned this pull request Feb 19, 2018

[WIP] Add Accept All Changes to External Changes Dialog #3742

Closed

5 tasks

koppor removed the PE1718 label Mar 9, 2018

koppor assigned Siedlerchr Mar 9, 2018

koppor removed the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Mar 9, 2018

koppor changed the title ~~Added Select all and Deselect all buttons to the Review changes dialog (fix for issue #280)~~ [WIP] Added Select all and Deselect all buttons to the "Review changes" dialog Mar 9, 2018

tobiasdiez closed this Oct 9, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Added Select all and Deselect all buttons to the "Review changes" dialog #3731

[WIP] Added Select all and Deselect all buttons to the "Review changes" dialog #3731

N4tus commented Feb 17, 2018 •

edited by koppor

Loading

Siedlerchr Feb 17, 2018

koppor Feb 18, 2018

Siedlerchr Feb 17, 2018

Siedlerchr Feb 17, 2018

koppor Feb 18, 2018

koppor left a comment

koppor Feb 18, 2018

koppor Feb 18, 2018

koppor Feb 18, 2018

koppor Feb 18, 2018

koppor Feb 18, 2018

koppor Feb 18, 2018

koppor Feb 18, 2018

koppor commented Feb 19, 2018

lenhard commented Feb 21, 2018

koppor commented Mar 9, 2018 •

edited

Loading

tobiasdiez commented Oct 9, 2018

[WIP] Added Select all and Deselect all buttons to the "Review changes" dialog #3731

[WIP] Added Select all and Deselect all buttons to the "Review changes" dialog #3731

Conversation

N4tus commented Feb 17, 2018 • edited by koppor 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

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

koppor commented Feb 19, 2018

lenhard commented Feb 21, 2018

koppor commented Mar 9, 2018 • edited Loading

tobiasdiez commented Oct 9, 2018

N4tus commented Feb 17, 2018 •

edited by koppor

Loading

koppor commented Mar 9, 2018 •

edited

Loading