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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


### Fixed
- We fixed an issue where pressing space caused the cursor to jump to the start of the text field. [#3471](https://github.com/JabRef/jabref/issues/3471)
Expand Down
33 changes: 32 additions & 1 deletion src/main/java/org/jabref/gui/collab/ChangeDisplayDialog.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
package org.jabref.gui.collab;

import java.awt.BorderLayout;
import java.awt.Dimension;
import java.awt.FlowLayout;
import java.awt.Insets;
import java.awt.event.ActionListener;
import java.util.Collections;
import java.util.Enumeration;

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

//start fix issue 280
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

this.setMinimumSize(new Dimension(400, 400));
JPanel treePanel = new JPanel(new BorderLayout());
JPanel buttonAlignment = new JPanel(new FlowLayout(FlowLayout.LEADING));
treePanel.add(tree, BorderLayout.CENTER);
JButton selectAll = new JButton(Localization.lang("Select all"));
JButton deselectAll = new JButton(Localization.lang("Deselect all"));
ActionListener selectEvent = (event) -> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

boolean accept = event.getSource() == selectAll;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add braces around the expression:

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

((ChangeViewModel) o).setAccepted(accept);
}
}
cb.setSelected(accept);
if (selected != null) {
cb.setEnabled(selected.isAcceptable());
}
};
selectAll.addActionListener(selectEvent);
deselectAll.addActionListener(selectEvent);
buttonAlignment.add(selectAll);
buttonAlignment.add(deselectAll);
treePanel.add(buttonAlignment, BorderLayout.SOUTH);
//end fix issue 280
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this line


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

JPanel infoBorder = new JPanel();
pane.setRightComponent(infoBorder);

Expand Down