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 +24 -12
2 changed files with 24 additions and 12 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit ab77c98a36 - Show all commits

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

@ -16,7 +16,10 @@ import envoy.data.User;
*/ */
public class QuickSelectControl extends VBox { public class QuickSelectControl extends VBox {
User user;
public QuickSelectControl(User user, GroupCreationTab tab) { public QuickSelectControl(User user, GroupCreationTab tab) {
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`.
this.user = user;
setPrefWidth(37); setPrefWidth(37);
setMaxWidth(37); setMaxWidth(37);
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.
setMinWidth(37); setMinWidth(37);
@ -70,4 +73,6 @@ public class QuickSelectControl extends VBox {
getStyleClass().add("quick-select"); getStyleClass().add("quick-select");
mpk marked this conversation as resolved Outdated
delvh commented 2020-10-03 20:48:43 +02:00
Outdated
Review
Copy Link

Delete this blank line.

Delete this blank line.
} }
public User getUser() { return user; }
} }

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

@ -70,7 +70,7 @@ public class GroupCreationTab implements EventListener {
@FXML @FXML
private void initialize() { private void initialize() {
userList.setCellFactory(new ListCellFactory<>(ContactControl::new)); userList.setCellFactory(new ListCellFactory<>(ContactControl::new));
userList.getSelectionModel().setSelectionMode(SelectionMode.MULTIPLE); userList.getSelectionModel().setSelectionMode(SelectionMode.SINGLE);
createButton.setDisable(true); createButton.setDisable(true);
mpk marked this conversation as resolved Outdated
delvh commented 2020-10-03 21:09:45 +02:00
Outdated
Review
Copy Link

Wait... Isn't that already default behavior?

Wait... Isn't that already default behavior?
eventBus.registerListener(this); eventBus.registerListener(this);
userList.getItems() userList.getItems()
@ -81,9 +81,7 @@ public class GroupCreationTab implements EventListener {
.filter(not(localDB.getUser()::equals)) .filter(not(localDB.getUser()::equals))
.map(User.class::cast) .map(User.class::cast)
.collect(Collectors.toList())); .collect(Collectors.toList()));
quickSelectList.setPrefHeight(0); resizeQuickSelectSpace(0);
quickSelectList.setMaxHeight(0);
quickSelectList.setMinHeight(0);
} }
/** /**
@ -95,9 +93,9 @@ public class GroupCreationTab implements EventListener {
private void userListClicked() { private void userListClicked() {
createButton.setDisable(userList.getSelectionModel().isEmpty() || groupNameField.getText().isBlank()); createButton.setDisable(userList.getSelectionModel().isEmpty() || groupNameField.getText().isBlank());
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.
quickSelectList.getItems().add(new QuickSelectControl(userList.getSelectionModel().getSelectedItem(), this)); quickSelectList.getItems().add(new QuickSelectControl(userList.getSelectionModel().getSelectedItem(), this));
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.
quickSelectList.setPrefHeight(60); resizeQuickSelectSpace(60);
quickSelectList.setMaxHeight(60); userList.getItems().remove(userList.getSelectionModel().getSelectedItem());
quickSelectList.setMinHeight(60); userList.getSelectionModel().clearSelection();
} }
/** /**
@ -107,7 +105,7 @@ public class GroupCreationTab implements EventListener {
* @since Envoy Client v0.1-beta * @since Envoy Client v0.1-beta
*/ */
@FXML @FXML
private void textUpdated() { createButton.setDisable(userList.getSelectionModel().isEmpty() || groupNameField.getText().isBlank()); } private void textUpdated() { createButton.setDisable(quickSelectList.getItems().size() == 0 || groupNameField.getText().isBlank()); }
mpk marked this conversation as resolved Outdated
delvh commented 2020-10-03 16:11:42 +02:00
Outdated
Review
Copy Link

Use

setDisable(quickSelectList.getItems().isEmpty()||...);
Use ``` setDisable(quickSelectList.getItems().isEmpty()||...); ```
/** /**
* Sends a {@link GroupCreation} to the server and closes this scene. * Sends a {@link GroupCreation} to the server and closes this scene.
@ -135,6 +133,9 @@ public class GroupCreationTab implements EventListener {
// Restoring the original design as tabs will always be reused // Restoring the original design as tabs will always be reused
setErrorMessageLabelSize(0); setErrorMessageLabelSize(0);
groupNameField.clear(); groupNameField.clear();
quickSelectList.getItems().forEach(q -> userList.getItems().add(q.getUser()));
quickSelectList.getItems().clear();
resizeQuickSelectSpace(0);
} }
} }
@ -148,7 +149,7 @@ public class GroupCreationTab implements EventListener {
private void createGroup(String name) { private void createGroup(String name) {
Context.getInstance() Context.getInstance()
.getClient() .getClient()
.send(new GroupCreation(name, userList.getSelectionModel().getSelectedItems().stream().map(User::getID).collect(Collectors.toSet()))); .send(new GroupCreation(name, quickSelectList.getItems().stream().map(q -> q.getUser().getID()).collect(Collectors.toSet())));
} }
/** /**
@ -165,13 +166,19 @@ public class GroupCreationTab implements EventListener {
public void removeFromQuickSelection(QuickSelectControl element) { public void removeFromQuickSelection(QuickSelectControl element) {
quickSelectList.getItems().remove(element); quickSelectList.getItems().remove(element);
userList.getItems().add(element.getUser());
if (quickSelectList.getItems().size() == 0) { if (quickSelectList.getItems().size() == 0) {
quickSelectList.setPrefHeight(0); resizeQuickSelectSpace(0);
quickSelectList.setMaxHeight(0); createButton.setDisable(true);
quickSelectList.setMinHeight(0);
} }
} }
private void resizeQuickSelectSpace(int value) {
mpk marked this conversation as resolved Outdated
delvh commented 2020-10-03 16:10:11 +02:00
Outdated
Review
Copy Link

Use

if(quickSelectList.getItems().isEmpty()){
Use ``` if(quickSelectList.getItems().isEmpty()){ ```
quickSelectList.setPrefHeight(value);
quickSelectList.setMaxHeight(value);
quickSelectList.setMinHeight(value);
}
@FXML @FXML
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.
private void backButtonClicked() { private void backButtonClicked() {
eventBus.dispatch(new BackEvent()); eventBus.dispatch(new BackEvent());