zdm/envoy
zdm
/
envoy
Archived
3
Fork 0
Code Issues 22 Pull Requests Releases 1 Wiki Activity

Quick Select Support for the GroupCreationTab #77

Merged
mpk merged 9 commits from f/quick-group-select into develop 2020-10-04 21:28:57 +02:00
Conversation 1 Commits 9 Files Changed 5 +17 -13
2 changed files with 17 additions and 13 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 51b189e8f5 - Show all commits

12
client/src/main/java/envoy/client/ui/control/QuickSelectControl.java
View File

@ -11,17 +11,18 @@ import envoy.client.util.IconUtil;
import envoy.data.User;
/**
* Displays a user as a quickSelectControl which is used in the quickSelectList.
* Displays an {@link User} as a quickSelectControl which is used in the
mpk marked this conversation as resolved Outdated
delvh commented 2020-10-03 16:21:01 +02:00
Outdated
Review
Copy Link
... a {@link User} as ...
``` ... a {@link User} as ... ```
delvh commented 2020-10-04 11:05:46 +02:00
Outdated
Review
Copy Link

We don't use camelCase is Javadoc.

We don't use camelCase is Javadoc.
* quickSelectList.
*
* @author Maximilian Käfer
* @since Envoy Client v0.3-beta
*/
public class QuickSelectControl extends VBox {
mpk marked this conversation as resolved Outdated
kske commented 2020-10-03 15:56:20 +02:00
Outdated
Review
Copy Link

Please make this private.

Please make this `private`.
User user;
private User user;
/**
mpk marked this conversation as resolved Outdated
delvh commented 2020-10-03 16:25:44 +02:00
Outdated
Review
Copy Link

As I've already been conditioned by a certain member of the Envoy team (the coffee machine guy) to not use @link in constructors for the own component, I've since swapped to using @code, so maybe you should too.

It might be beneficial to create a convention for such cases from here on.

As I've already been conditioned by a certain member of the Envoy team (the coffee machine guy) to not use `@link` in constructors for the own component, I've since swapped to using `@code`, so maybe you should too. It might be beneficial to create a convention for such cases from here on.
* Creates an instance of the {@link QuickSelectControl}.
* Creates an instance of the {@code QuickSelectControl}.
*
* @param user the user whose data is used to create this instance.
* @param tab the parent tab ({@link GroupCreationTab}).
@ -49,7 +50,6 @@ public class QuickSelectControl extends VBox {
clip.setArcHeight(32);
clip.setArcWidth(32);
contactProfilePic.setClip(clip);
mpk marked this conversation as resolved
delvh commented 2020-10-03 16:29:35 +02:00
Review
Copy Link

This is, as far as I can see, unnecessary.
Have tested it, it is.

This is, as far as I can see, unnecessary. Have tested it, it is.
setAlignment(Pos.TOP_CENTER);
picHold.getChildren().add(contactProfilePic);
stackPane.getChildren().add(picHold);
@ -59,7 +59,7 @@ public class QuickSelectControl extends VBox {
hBox.setMinHeight(12);
var region = new Region();
hBox.getChildren().add(region);
hBox.setHgrow(region, Priority.ALWAYS);
HBox.setHgrow(region, Priority.ALWAYS);
mpk marked this conversation as resolved Outdated
delvh commented 2020-10-03 16:03:11 +02:00
Outdated
Review
Copy Link

This line should be

HBox.setHgrow(region, Priority.ALWAYS);

(setHgrow is a static method)

This line should be ``` HBox.setHgrow(region, Priority.ALWAYS); ``` (`setHgrow` is a static method)
var removeBtn = new Button();
removeBtn.setPrefSize(12, 12);
@ -85,7 +85,7 @@ public class QuickSelectControl extends VBox {
}
/**
* @return the user whose data is used in this instance.
* @return the user whose data is used in this instance
mpk marked this conversation as resolved Outdated
kske commented 2020-10-03 15:58:39 +02:00
Outdated
Review
Copy Link

The dot at the end is unnecessary.

The dot at the end is unnecessary.
* @since Envoy Client v0.3-beta
*/
public User getUser() { return user; }

18
client/src/main/java/envoy/client/ui/controller/GroupCreationTab.java
View File

@ -7,6 +7,7 @@ import java.util.stream.Collectors;
import javafx.application.Platform;
import javafx.fxml.FXML;
import javafx.scene.control.*;
import javafx.scene.input.MouseEvent;
import javafx.scene.layout.HBox;
import envoy.client.data.*;
@ -82,6 +83,7 @@ public class GroupCreationTab implements EventListener {
.map(User.class::cast)
.collect(Collectors.toList()));
resizeQuickSelectSpace(0);
quickSelectList.addEventFilter(MouseEvent.MOUSE_PRESSED, evt -> evt.consume());
}
/**
@ -91,11 +93,13 @@ public class GroupCreationTab implements EventListener {
*/
@FXML
delvh marked this conversation as resolved Outdated
delvh commented 2020-10-03 16:17:39 +02:00
Outdated
Review
Copy Link

Isn't that the wrong check here? Shouldn't it be

setDisable(quickSelectList.getItems().isEmpty() && userList.getSelectionModel().isEmpty() || ...);
Isn't that the wrong check here? Shouldn't it be ``` setDisable(quickSelectList.getItems().isEmpty() && userList.getSelectionModel().isEmpty() || ...); ```
mpk commented 2020-10-03 20:15:34 +02:00
Outdated
Review
Copy Link

Yes there was an issue but it was not the one you suggested.

Yes there was an issue but it was not the one you suggested.
private void userListClicked() {
mpk marked this conversation as resolved Outdated
delvh commented 2020-10-03 16:19:55 +02:00
Outdated
Review
Copy Link

You need to insert a null check for userList.getSelectionModel().getSelectedItem() as it can also happen that no element has been selected. This is especially the case if you click on the userList after every user has been removed from it.

You need to insert a null check for `userList.getSelectionModel().getSelectedItem()` as it can also happen that no element has been selected. This is especially the case if you click on the userList after every user has been removed from it.
createButton.setDisable(userList.getSelectionModel().isEmpty() || groupNameField.getText().isBlank());
quickSelectList.getItems().add(new QuickSelectControl(userList.getSelectionModel().getSelectedItem(), this));
resizeQuickSelectSpace(60);
userList.getItems().remove(userList.getSelectionModel().getSelectedItem());
userList.getSelectionModel().clearSelection();
if (userList.getSelectionModel().getSelectedItem() != null) {
quickSelectList.getItems().add(new QuickSelectControl(userList.getSelectionModel().getSelectedItem(), this));
createButton.setDisable(quickSelectList.getItems().isEmpty() || groupNameField.getText().isBlank());
resizeQuickSelectSpace(60);
userList.getItems().remove(userList.getSelectionModel().getSelectedItem());
userList.getSelectionModel().clearSelection();
}
}
/**
@ -105,7 +109,7 @@ public class GroupCreationTab implements EventListener {
* @since Envoy Client v0.1-beta
*/
@FXML
private void textUpdated() { createButton.setDisable(quickSelectList.getItems().size() == 0 || groupNameField.getText().isBlank()); }
private void textUpdated() { createButton.setDisable(quickSelectList.getItems().isEmpty() || groupNameField.getText().isBlank()); }
/**
* Sends a {@link GroupCreation} to the server and closes this scene.
@ -173,7 +177,7 @@ public class GroupCreationTab implements EventListener {
public void removeFromQuickSelection(QuickSelectControl element) {
quickSelectList.getItems().remove(element);
userList.getItems().add(element.getUser());
if (quickSelectList.getItems().size() == 0) {
if (quickSelectList.getItems().isEmpty()) {
resizeQuickSelectSpace(0);
createButton.setDisable(true);
mpk marked this conversation as resolved Outdated
delvh commented 2020-10-03 16:08:00 +02:00
Outdated
Review
Copy Link

You really do like it if no one except you can ever resize components, right?

You really do like it if no one except you can ever resize components, right?
mpk commented 2020-10-03 20:21:00 +02:00
Outdated
Review
Copy Link

I have absolutely no clue what you mean but as I see it this is just a usefull little method.

I have absolutely no clue what you mean but as I see it this is just a usefull little method.
}