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

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

@ -1,18 +1,19 @@
package envoy.client.ui.control; package envoy.client.ui.control;
import java.util.function.Consumer;
import javafx.geometry.*; 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.*; import javafx.scene.layout.*;
import javafx.scene.shape.Rectangle; import javafx.scene.shape.Rectangle;
import envoy.client.ui.controller.GroupCreationTab;
import envoy.client.util.IconUtil; import envoy.client.util.IconUtil;
import envoy.data.User; import envoy.data.*;
/** /**
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.
* Displays an {@link User} as a quickSelectControl which is used in the * Displays an {@link User} as a quick select control which is used in the
* quickSelectList. * quick select list.
* *
* @author Maximilian Käfer * @author Maximilian Käfer
* @since Envoy Client v0.3-beta * @since Envoy Client v0.3-beta
@ -24,11 +25,11 @@ public class QuickSelectControl extends VBox {
/** /**
* Creates an instance of the {@code QuickSelectControl}. * Creates an instance of the {@code QuickSelectControl}.
* *
* @param user the user whose data is used to create this instance. * @param user the contact whose data is used to create this instance.
* @param tab the parent tab ({@link GroupCreationTab}). * @param action the action to perform when a contact is removed with this control as a parameter
* @since Envoy Client v0.3-beta * @since Envoy Client v0.3-beta
*/ */
public QuickSelectControl(User user, GroupCreationTab tab) { public QuickSelectControl(User user, Consumer<QuickSelectControl> action) {
mpk marked this conversation as resolved Outdated
delvh commented 2020-10-03 16:27:20 +02:00
Outdated
Review
Copy Link

?

?
mpk commented 2020-10-03 20:03:28 +02:00
Outdated
Review
Copy Link

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; this.user = user;
setPadding(new Insets(1, 0, 0, 0)); setPadding(new Insets(1, 0, 0, 0));
setPrefWidth(37); setPrefWidth(37);
@ -65,13 +66,12 @@ public class QuickSelectControl extends VBox {
removeBtn.setPrefSize(12, 12); removeBtn.setPrefSize(12, 12);
removeBtn.setMaxSize(12, 12); removeBtn.setMaxSize(12, 12);
removeBtn.setMinSize(12, 12); removeBtn.setMinSize(12, 12);
mpk marked this conversation as resolved
delvh commented 2020-10-03 16:33:08 +02:00
Review
Copy Link

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 -> tab.removeFromQuickSelection(this)); removeBtn.setOnMouseClicked(evt -> action.accept(this));
removeBtn.setId("remove-button"); removeBtn.setId("remove-button");
delvh commented 2020-10-03 16:54:21 +02:00
Review
Copy Link

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.
mpk commented 2020-10-03 20:24:22 +02:00
Review
Copy Link

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)
delvh commented 2020-10-03 20:52:42 +02:00
Review
Copy Link

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
mpk commented 2020-10-04 16:50:53 +02:00
Review
Copy Link

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
delvh commented 2020-10-04 18:19:04 +02:00
Review
Copy Link

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?
delvh commented 2020-10-04 18:21:28 +02:00
Review
Copy Link

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); hBox.getChildren().add(removeBtn);
stackPane.getChildren().add(hBox); stackPane.getChildren().add(hBox);
getChildren().add(stackPane); getChildren().add(stackPane);
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.
var nameLabel = new Label(); var nameLabel = new Label();
nameLabel.setPrefSize(35, 20); nameLabel.setPrefSize(35, 20);
nameLabel.setMaxSize(35, 20); nameLabel.setMaxSize(35, 20);

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

@ -71,7 +71,6 @@ 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.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()
@ -94,7 +93,7 @@ public class GroupCreationTab implements EventListener {
@FXML @FXML
private void userListClicked() { private void userListClicked() {
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.
if (userList.getSelectionModel().getSelectedItem() != null) { if (userList.getSelectionModel().getSelectedItem() != null) {
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.getItems().add(new QuickSelectControl(userList.getSelectionModel().getSelectedItem(), this)); quickSelectList.getItems().add(new QuickSelectControl(userList.getSelectionModel().getSelectedItem(), this::removeFromQuickSelection));
createButton.setDisable(quickSelectList.getItems().isEmpty() || groupNameField.getText().isBlank()); createButton.setDisable(quickSelectList.getItems().isEmpty() || groupNameField.getText().isBlank());
resizeQuickSelectSpace(60); resizeQuickSelectSpace(60);
userList.getItems().remove(userList.getSelectionModel().getSelectedItem()); userList.getItems().remove(userList.getSelectionModel().getSelectedItem());