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 67 additions and 21 deletions
Showing only changes of commit 434d577c15 - Show all commits

View File

@ -1,9 +1,9 @@
package envoy.client.ui.control; package envoy.client.ui.control;
import javafx.geometry.Pos; import javafx.geometry.*;
import javafx.scene.control.*; import javafx.scene.control.*;
import javafx.scene.image.ImageView; import javafx.scene.image.ImageView;
import javafx.scene.layout.VBox; import javafx.scene.layout.*;
import javafx.scene.shape.Rectangle; import javafx.scene.shape.Rectangle;
import envoy.client.ui.controller.GroupCreationTab; import envoy.client.ui.controller.GroupCreationTab;
@ -17,19 +17,19 @@ import envoy.data.User;
public class QuickSelectControl extends VBox { public class QuickSelectControl extends VBox {
public QuickSelectControl(User user, GroupCreationTab tab) { public QuickSelectControl(User user, GroupCreationTab tab) {
Button removeBtn = new Button(); setPrefWidth(37);
removeBtn.setPrefSize(10, 10); setMaxWidth(37);
removeBtn.setMaxSize(10, 10); setMinWidth(37);
removeBtn.setMinSize(10, 10); var stackPane = new StackPane();
removeBtn.setAlignment(Pos.TOP_RIGHT); stackPane.setAlignment(Pos.TOP_CENTER);
removeBtn.setOnMouseClicked(evt -> {
tab.removeFromQuickSelection(this);
});
removeBtn.setId("remove-button");
getChildren().add(removeBtn);
// Profile picture // Profile picture
ImageView contactProfilePic = new ImageView(IconUtil.loadIconThemeSensitive("user_icon", 32)); 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(); final var clip = new Rectangle();
clip.setWidth(32); clip.setWidth(32);
clip.setHeight(32); clip.setHeight(32);
@ -37,14 +37,35 @@ public class QuickSelectControl extends VBox {
clip.setArcWidth(32); clip.setArcWidth(32);
contactProfilePic.setClip(clip); contactProfilePic.setClip(clip);
setAlignment(Pos.TOP_CENTER); setAlignment(Pos.TOP_CENTER);
getChildren().add(contactProfilePic); picHold.getChildren().add(contactProfilePic);
stackPane.getChildren().add(picHold);
Label nameLabel = new Label(); var hBox = new HBox();
hBox.setPrefHeight(12);
hBox.setMaxHeight(12);
hBox.setMinHeight(12);
var region = new Region();
hBox.getChildren().add(region);
hBox.setHgrow(region, Priority.ALWAYS);
var removeBtn = new Button();
removeBtn.setPrefSize(12, 12);
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.
removeBtn.setMaxSize(12, 12);
removeBtn.setMinSize(12, 12);
removeBtn.setOnMouseClicked(evt -> tab.removeFromQuickSelection(this));
removeBtn.setId("remove-button");
hBox.getChildren().add(removeBtn);
stackPane.getChildren().add(hBox);
getChildren().add(stackPane);
var nameLabel = new Label();
nameLabel.setPrefSize(35, 20); nameLabel.setPrefSize(35, 20);
nameLabel.setMaxSize(35, 20); nameLabel.setMaxSize(35, 20);
nameLabel.setMinSize(35, 20); nameLabel.setMinSize(35, 20);
nameLabel.setText(user.getName()); nameLabel.setText(user.getName());
nameLabel.setAlignment(Pos.TOP_CENTER); nameLabel.setAlignment(Pos.TOP_CENTER);
nameLabel.setPadding(new Insets(0, 5, 0, 0));
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.
getChildren().add(nameLabel); getChildren().add(nameLabel);
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.
getStyleClass().add("quick-select"); getStyleClass().add("quick-select");

View File

@ -81,6 +81,9 @@ 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);
quickSelectList.setMaxHeight(0);
quickSelectList.setMinHeight(0);
} }
/** /**
@ -92,6 +95,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());
quickSelectList.getItems().add(new QuickSelectControl(userList.getSelectionModel().getSelectedItem(), this)); quickSelectList.getItems().add(new QuickSelectControl(userList.getSelectionModel().getSelectedItem(), this));
quickSelectList.setPrefHeight(60);
quickSelectList.setMaxHeight(60);
quickSelectList.setMinHeight(60);
} }
/** /**
@ -157,7 +163,14 @@ public class GroupCreationTab implements EventListener {
return localDB.getChats().stream().map(Chat::getRecipient).filter(Group.class::isInstance).map(Contact::getName).anyMatch(newName::equals); return localDB.getChats().stream().map(Chat::getRecipient).filter(Group.class::isInstance).map(Contact::getName).anyMatch(newName::equals);
} }
public void removeFromQuickSelection(QuickSelectControl element) { quickSelectList.getItems().remove(element); } public void removeFromQuickSelection(QuickSelectControl element) {
quickSelectList.getItems().remove(element);
if (quickSelectList.getItems().size() == 0) {
quickSelectList.setPrefHeight(0);
quickSelectList.setMaxHeight(0);
quickSelectList.setMinHeight(0);
}
}
@FXML @FXML
private void backButtonClicked() { private void backButtonClicked() {

View File

@ -41,7 +41,7 @@
-fx-scale-y: 1.05; -fx-scale-y: 1.05;
} }
.label { .label, .quick-select {
-fx-background-color: transparent; -fx-background-color: transparent;
} }
@ -144,3 +144,15 @@
visibility: hidden ; visibility: hidden ;
-fx-padding: -20.0 0.0 0.0 0.0; -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; -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; -fx-background-color: #222222;
} }
@ -42,7 +42,7 @@
-fx-background-color: #191919; -fx-background-color: #191919;
} }
#chat-list, #top-bar, #search-panel, #note-background, .quick-select { #chat-list, #top-bar, #search-panel, #note-background {
-fx-background-color: #303030; -fx-background-color: #303030;
} }
@ -69,7 +69,7 @@
-fx-background-color: transparent; -fx-background-color: transparent;
} }
.scroll-bar:vertical .increment-arrow, .scroll-bar:vertical .decrement-arrow, #quick-select-list, .list-cell { .scroll-bar:vertical .increment-arrow, .scroll-bar:vertical .decrement-arrow, .list-cell {
-fx-background-color: transparent; -fx-background-color: transparent;
} }

View File

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