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
5 changed files with 155 additions and 8 deletions

View File

@ -0,0 +1,92 @@
package envoy.client.ui.control;
import java.util.function.Consumer;
import javafx.geometry.*;
import javafx.scene.control.*;
import javafx.scene.image.ImageView;
import javafx.scene.layout.*;
import javafx.scene.shape.Rectangle;
import envoy.client.util.IconUtil;
import envoy.data.*;
/**
mpk marked this conversation as resolved Outdated
Outdated
Review
... a {@link User} as ...
``` ... a {@link User} as ... ```
Outdated
Review

We don't use camelCase is Javadoc.

We don't use camelCase is Javadoc.
* Displays an {@link User} as a quick select control which is used in the
* quick select list.
*
* @author Maximilian Käfer
* @since Envoy Client v0.3-beta
*/
public class QuickSelectControl extends VBox {
mpk marked this conversation as resolved Outdated
Outdated
Review

Please make this private.

Please make this `private`.
private User user;
mpk marked this conversation as resolved Outdated
Outdated
Review

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 {@code QuickSelectControl}.
*
* @param user the contact whose data is used to create this instance.
* @param action the action to perform when a contact is removed with this control as a parameter
* @since Envoy Client v0.3-beta
*/
public QuickSelectControl(User user, Consumer<QuickSelectControl> action) {
mpk marked this conversation as resolved Outdated
Outdated
Review

?

?
Outdated
Review

This centers the component (it is just 1 pixel but very noticable)

This centers the component (it is just 1 pixel but very noticable)
this.user = user;
setPadding(new Insets(1, 0, 0, 0));
setPrefWidth(37);
setMaxWidth(37);
setMinWidth(37);
var stackPane = new StackPane();
stackPane.setAlignment(Pos.TOP_CENTER);
mpk marked this conversation as resolved Outdated
Outdated
Review

Why exactly are you declaring a VBox inside a VBox?

Why exactly are you declaring a `VBox` inside a `VBox`?
Outdated
Review

It is more about the image view which this inner vBox is holding. An image view doesn't give me the option to add padding, so I had to do it with its parent: the vBox.

It is more about the image view which this inner vBox is holding. An image view doesn't give me the option to add padding, so I had to do it with its parent: the vBox.
// Profile picture
var picHold = new VBox();
picHold.setPadding(new Insets(2, 0, 0, 0));
picHold.setPrefHeight(35);
picHold.setMaxHeight(35);
picHold.setMinHeight(35);
var contactProfilePic = new ImageView(IconUtil.loadIconThemeSensitive("user_icon", 32));
final var clip = new Rectangle();
clip.setWidth(32);
clip.setHeight(32);
clip.setArcHeight(32);
clip.setArcWidth(32);
mpk marked this conversation as resolved
Review

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.
contactProfilePic.setClip(clip);
picHold.getChildren().add(contactProfilePic);
stackPane.getChildren().add(picHold);
var hBox = new HBox();
hBox.setPrefHeight(12);
hBox.setMaxHeight(12);
hBox.setMinHeight(12);
var region = new Region();
hBox.getChildren().add(region);
mpk marked this conversation as resolved Outdated
Outdated
Review

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)
HBox.setHgrow(region, Priority.ALWAYS);
var removeBtn = new Button();
removeBtn.setPrefSize(12, 12);
removeBtn.setMaxSize(12, 12);
removeBtn.setMinSize(12, 12);
mpk marked this conversation as resolved
Review

As far as I can see, this is the only reason, why you're limiting this component to be used only in the GroupCreationTab.

The OOP approach would be to define a setter that delegates what action to perform on the mouse click of the remove button. This would look something like this:

public void onRemoveButtonClicked(BiConsumer<? super MouseEvent, User> action){
	removeBtn.setOnMouseClicked(evt->action.accept(evt, user));
}

This additionally brings the advantage of reusability.

When this is implemented, the GroupCreationTab can be removed from the declaration of this component.

As far as I can see, this is the only reason, why you're limiting this component to be used only in the GroupCreationTab. The OOP approach would be to define a setter that delegates what action to perform on the mouse click of the remove button. This would look something like this: ``` public void onRemoveButtonClicked(BiConsumer<? super MouseEvent, User> action){ removeBtn.setOnMouseClicked(evt->action.accept(evt, user)); } ``` This additionally brings the advantage of reusability. When this is implemented, the `GroupCreationTab` can be removed from the declaration of this component.
removeBtn.setOnMouseClicked(evt -> action.accept(this));
removeBtn.setId("remove-button");
Review

If I understand you correctly, the only purpose of the HBox is to position the remove button to the far right of the component.

I've experimented a bit and found out that I can achieve the same result using two components (and a few lines) less:

  1. Remove lines 56-63 + 71 (stackPane.getChildren(...))
  2. Change hBox in line 70 to picHold
  3. Make picHold from a VBox to an HBox
  4. Done.
If I understand you correctly, the only purpose of the `HBox` is to position the remove button to the far right of the component. I've experimented a bit and found out that I can achieve the same result using two components (and a few lines) less: 1. Remove lines 56-63 + 71 (`stackPane.getChildren(...)`) 2. Change `hBox` in line 70 to `picHold` 3. Make `picHold` from a `VBox` to an `HBox` 4. Done.
Review

Just paste your modified code an a comment because it is very hard to understand what you want if you just give a description. (Because the lines have changed)

Just paste your modified code an a comment because it is very hard to understand what you want if you just give a description. (Because the lines have changed)
Review

Funny thing. They haven't. But still:

  1. Remove the variables hBox and region
  2. the picHold variable should be an HBox, not a VBox
  3. the line hBox.getChildren().add(removeBtn); should be
picHold.getChildren().add(removeBtn);
  1. The line below where you are adding hBox to stackPane can be deleted
Funny thing. They haven't. But still: 1. Remove the variables `hBox` and `region` 2. the `picHold` variable should be an `HBox`, not a `VBox` 3. the line `hBox.getChildren().add(removeBtn);` should be ``` picHold.getChildren().add(removeBtn); ``` 4. The line below where you are adding `hBox` to `stackPane` can be deleted
Review

I won't do this as it has no necessarity here. Btw I have the feeling that you did not understand the whole mechanism behind the component definition and usage.

Alternatively phrased, I won't do it because no. Sincerely @kske aka Jesus, das Massiv

I won't do this as it has no necessarity here. Btw I have the feeling that you did not understand the whole mechanism behind the component definition and usage. Alternatively phrased, I won't do it because no. Sincerely @kske aka Jesus, das Massiv
Review

So why exactly are we now allowing unnecessary overhead? I've shown you in detail that I can reproduce the same result using two components less. What's the deal with that?

So why exactly are we now allowing unnecessary overhead? I've shown you in detail that I can reproduce the same result using two components less. What's the deal with that?
Review

Oh, and by the way, if you have to resort to ' I won't do it because no' as the reasoning for something, then something is wrong with that thing.

Oh, and by the way, if you have to resort to ' I won't do it because no' as the reasoning for something, then something is wrong with that thing.
hBox.getChildren().add(removeBtn);
stackPane.getChildren().add(hBox);
getChildren().add(stackPane);
mpk marked this conversation as resolved Outdated
Outdated
Review

Delete this blank line.

Delete this blank line.
var nameLabel = new Label();
nameLabel.setPrefSize(35, 20);
nameLabel.setMaxSize(35, 20);
nameLabel.setMinSize(35, 20);
nameLabel.setText(user.getName());
nameLabel.setAlignment(Pos.TOP_CENTER);
mpk marked this conversation as resolved
Review

Unnecessary. Delete the line.

Unnecessary. Delete the line.
Review

No it is defenitly not as this is centering the label!

No it is defenitly not as this is centering the label!
nameLabel.setPadding(new Insets(0, 5, 0, 0));
getChildren().add(nameLabel);
getStyleClass().add("quick-select");
}
/**
* @return the user whose data is used in this instance
mpk marked this conversation as resolved Outdated
Outdated
Review

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; }
}

View File

@ -7,11 +7,12 @@ 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.*;
import envoy.client.event.BackEvent;
import envoy.client.ui.control.ContactControl;
import envoy.client.ui.control.*;
import envoy.client.ui.listcell.ListCellFactory;
import envoy.data.*;
import envoy.event.GroupCreation;
@ -58,6 +59,9 @@ public class GroupCreationTab implements EventListener {
@FXML
private HBox errorProceedBox;
@FXML
private ListView<QuickSelectControl> quickSelectList;
private String name;
private final LocalDB localDB = Context.getInstance().getLocalDB();
@ -67,7 +71,6 @@ public class GroupCreationTab implements EventListener {
@FXML
private void initialize() {
userList.setCellFactory(new ListCellFactory<>(ContactControl::new));
userList.getSelectionModel().setSelectionMode(SelectionMode.MULTIPLE);
createButton.setDisable(true);
mpk marked this conversation as resolved Outdated
Outdated
Review

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

Wait... Isn't that already default behavior?
eventBus.registerListener(this);
userList.getItems()
@ -78,6 +81,8 @@ public class GroupCreationTab implements EventListener {
.filter(not(localDB.getUser()::equals))
.map(User.class::cast)
.collect(Collectors.toList()));
resizeQuickSelectSpace(0);
quickSelectList.addEventFilter(MouseEvent.MOUSE_PRESSED, evt -> evt.consume());
}
/**
@ -86,7 +91,15 @@ public class GroupCreationTab implements EventListener {
* @since Envoy Client v0.1-beta
*/
@FXML
private void userListClicked() { createButton.setDisable(userList.getSelectionModel().isEmpty() || groupNameField.getText().isBlank()); }
private void userListClicked() {
delvh marked this conversation as resolved Outdated
Outdated
Review

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() || ...); ```
Outdated
Review

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.
if (userList.getSelectionModel().getSelectedItem() != null) {
mpk marked this conversation as resolved Outdated
Outdated
Review

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.getItems().add(new QuickSelectControl(userList.getSelectionModel().getSelectedItem(), this::removeFromQuickSelection));
createButton.setDisable(quickSelectList.getItems().isEmpty() || groupNameField.getText().isBlank());
resizeQuickSelectSpace(60);
userList.getItems().remove(userList.getSelectionModel().getSelectedItem());
userList.getSelectionModel().clearSelection();
}
}
/**
* Checks, whether the {@code createButton} can be enabled because text is
@ -95,7 +108,7 @@ public class GroupCreationTab implements EventListener {
* @since Envoy Client v0.1-beta
mpk marked this conversation as resolved Outdated
Outdated
Review

Use

setDisable(quickSelectList.getItems().isEmpty()||...);
Use ``` setDisable(quickSelectList.getItems().isEmpty()||...); ```
*/
@FXML
private void textUpdated() { createButton.setDisable(userList.getSelectionModel().isEmpty() || 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.
@ -123,6 +136,9 @@ public class GroupCreationTab implements EventListener {
// Restoring the original design as tabs will always be reused
setErrorMessageLabelSize(0);
groupNameField.clear();
quickSelectList.getItems().forEach(q -> userList.getItems().add(q.getUser()));
quickSelectList.getItems().clear();
resizeQuickSelectSpace(0);
}
}
@ -136,7 +152,7 @@ public class GroupCreationTab implements EventListener {
private void createGroup(String name) {
Context.getInstance()
.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())));
}
/**
@ -151,6 +167,27 @@ public class GroupCreationTab implements EventListener {
return localDB.getChats().stream().map(Chat::getRecipient).filter(Group.class::isInstance).map(Contact::getName).anyMatch(newName::equals);
}
/**
* Removes an element from the quickSelectList.
*
* @param element the element to be removed.
* @since Envoy Client v0.3-beta
*/
public void removeFromQuickSelection(QuickSelectControl element) {
mpk marked this conversation as resolved Outdated
Outdated
Review

Use

if(quickSelectList.getItems().isEmpty()){
Use ``` if(quickSelectList.getItems().isEmpty()){ ```
quickSelectList.getItems().remove(element);
userList.getItems().add(element.getUser());
if (quickSelectList.getItems().isEmpty()) {
resizeQuickSelectSpace(0);
createButton.setDisable(true);
}
mpk marked this conversation as resolved Outdated
Outdated
Review

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?
Outdated
Review

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

View File

@ -41,7 +41,7 @@
-fx-scale-y: 1.05;
}
.label {
.label, .quick-select {
-fx-background-color: transparent;
}
@ -144,3 +144,15 @@
visibility: hidden ;
-fx-padding: -20.0 0.0 0.0 0.0;
}
#quick-select-list .scroll-bar:horizontal{
-fx-pref-height: 0;
-fx-max-height: 0;
-fx-min-height: 0;
mpk marked this conversation as resolved
Review

Is that even needed in this case?

Is that even needed in this case?
Review

Meant is only the last line...

Meant is only the last line...
Review

Yes it is absolutly necessary.

Yes it is absolutly necessary.
Review

Why? If the max height is already at 0, the pref height won't be larger than that, same for the min height...

Why? If the max height is already at 0, the pref height won't be larger than that, same for the min height...
Review

No thats the thing I tried it and the scroll bars appear. I wondered to.

No thats the thing I tried it and the scroll bars appear. I wondered to.
}
#quick-select-list .scroll-bar:vertical{
-fx-pref-width: 0;
-fx-max-width: 0;
-fx-min-width: 0;
mpk marked this conversation as resolved
Review

Is that even needed in this case?

Is that even needed in this case?
Review

Meant is only the last line...

Meant is only the last line...
Review

Yes it is absolutly necessary.

Yes it is absolutly necessary.
Review

Why? If the max width is already 0, not even the pref width should matter, especially not the min width.

Why? If the max width is already 0, not even the pref width should matter, especially not the min width.
Review

No thats the thing I tried it and the scroll bars appear. I wondered to.

No thats the thing I tried it and the scroll bars appear. I wondered to.
}

View File

@ -18,7 +18,7 @@
-fx-background-color: lightgray;
}
#message-list, .text-field, .password-field, .tooltip, .pane, .pane .content, .vbox, .titled-pane > .title, .titled-pane > *.content, .context-menu, .menu-item {
#message-list, .text-field, .password-field, .tooltip, .pane, .pane .content, .vbox, .titled-pane > .title, .titled-pane > *.content, .context-menu, .menu-item, #quick-select-list {
-fx-background-color: #222222;
}
@ -69,7 +69,7 @@
-fx-background-color: transparent;
}
.scroll-bar:vertical .increment-arrow, .scroll-bar:vertical .decrement-arrow {
.scroll-bar:vertical .increment-arrow, .scroll-bar:vertical .decrement-arrow, .list-cell {
-fx-background-color: transparent;
}
@ -83,3 +83,8 @@
-fx-text-fill: white;
-fx-background-color: transparent;
}
#remove-button {
-fx-background-color: red;
-fx-background-radius: 1em;
}

View File

@ -64,6 +64,7 @@
<Insets bottom="5.0" top="5" />
</VBox.margin>
</Label>
<ListView fx:id="quickSelectList" id="quick-select-list" orientation="HORIZONTAL" prefHeight="60.0" />
<ListView id="chat-list" fx:id="userList" focusTraversable="false" onMouseClicked="#userListClicked" prefWidth="316.0" VBox.vgrow="ALWAYS">
<contextMenu>
<ContextMenu anchorLocation="CONTENT_TOP_LEFT" />