Quick Select Support for the GroupCreationTab #77

Merged
mpk merged 9 commits from f/quick-group-select into develop 3 years ago
mpk commented 3 years ago
Owner

This PR provides a more refined and user friendly mechanism to select members for a group when creating it.
Fixes #60

This PR provides a more refined and user friendly mechanism to select members for a group when creating it. Fixes #60
mpk added this to the v0.3-beta milestone 3 years ago
mpk added the
client
M
labels 3 years ago
mpk self-assigned this 3 years ago
mpk requested review from delvh 3 years ago
mpk requested review from kske 3 years ago
kske requested changes 3 years ago
kske left a comment
Owner

Could you disable the selection of users already displayed inside the quick selector? At the moment selection is possible and serves no purpose.

Could you disable the selection of users already displayed inside the quick selector? At the moment selection is possible and serves no purpose.
@ -0,0 +18,4 @@
*/
public class QuickSelectControl extends VBox {
User user;
kske commented 3 years ago
Owner

Please make this private.

Please make this `private`.
mpk marked this conversation as resolved
@ -0,0 +85,4 @@
}
/**
* @return the user whose data is used in this instance.
kske commented 3 years ago
Owner

The dot at the end is unnecessary.

The dot at the end is unnecessary.
mpk marked this conversation as resolved
kske commented 3 years ago
Owner

This is the unnecessary selection I mentioned in my review.

grafik

This is the unnecessary selection I mentioned in my review. ![grafik](/attachments/96b16504-ae9f-41eb-bab9-c501a5925bc0)
7.0 KiB
delvh approved these changes 3 years ago
delvh left a comment
Owner

Noticed some fixes are possible.
Otherwise good job 👍.

Noticed some fixes are possible. Otherwise good job 👍.
@ -0,0 +11,4 @@
import envoy.data.User;
/**
* Displays a user as a quickSelectControl which is used in the quickSelectList.
delvh commented 3 years ago
Owner
... a {@link User} as ...
``` ... a {@link User} as ... ```
mpk marked this conversation as resolved
@ -0,0 +21,4 @@
User user;
/**
* Creates an instance of the {@link QuickSelectControl}.
delvh commented 3 years ago
Owner

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.
mpk marked this conversation as resolved
@ -0,0 +29,4 @@
*/
public QuickSelectControl(User user, GroupCreationTab tab) {
this.user = user;
setPadding(new Insets(1, 0, 0, 0));
delvh commented 3 years ago
Owner

?

?
mpk commented 3 years ago
Poster
Owner

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

This centers the component (it is just 1 pixel but very noticable)
mpk marked this conversation as resolved
@ -0,0 +37,4 @@
stackPane.setAlignment(Pos.TOP_CENTER);
// Profile picture
var picHold = new VBox();
delvh commented 3 years ago
Owner

Why exactly are you declaring a VBox inside a VBox?

Why exactly are you declaring a `VBox` inside a `VBox`?
mpk commented 3 years ago
Poster
Owner

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.
mpk marked this conversation as resolved
@ -0,0 +49,4 @@
clip.setArcHeight(32);
clip.setArcWidth(32);
contactProfilePic.setClip(clip);
setAlignment(Pos.TOP_CENTER);
delvh commented 3 years ago
Owner

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.
mpk marked this conversation as resolved
@ -0,0 +59,4 @@
hBox.setMinHeight(12);
var region = new Region();
hBox.getChildren().add(region);
hBox.setHgrow(region, Priority.ALWAYS);
delvh commented 3 years ago
Owner

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)
mpk marked this conversation as resolved
@ -0,0 +65,4 @@
removeBtn.setPrefSize(12, 12);
removeBtn.setMaxSize(12, 12);
removeBtn.setMinSize(12, 12);
removeBtn.setOnMouseClicked(evt -> tab.removeFromQuickSelection(this));
delvh commented 3 years ago
Owner

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.
mpk marked this conversation as resolved
@ -0,0 +67,4 @@
removeBtn.setMinSize(12, 12);
removeBtn.setOnMouseClicked(evt -> tab.removeFromQuickSelection(this));
removeBtn.setId("remove-button");
hBox.getChildren().add(removeBtn);
delvh commented 3 years ago
Owner

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 3 years ago
Poster
Owner

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 3 years ago
Owner

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 3 years ago
Poster
Owner

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 3 years ago
Owner

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 3 years ago
Owner

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.
@ -0,0 +77,4 @@
nameLabel.setMaxSize(35, 20);
nameLabel.setMinSize(35, 20);
nameLabel.setText(user.getName());
nameLabel.setAlignment(Pos.TOP_CENTER);
delvh commented 3 years ago
Owner

Unnecessary. Delete the line.

Unnecessary. Delete the line.
mpk commented 3 years ago
Poster
Owner

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

No it is defenitly not as this is centering the label!
mpk marked this conversation as resolved
@ -88,2 +92,3 @@
@FXML
private void userListClicked() { createButton.setDisable(userList.getSelectionModel().isEmpty() || groupNameField.getText().isBlank()); }
private void userListClicked() {
createButton.setDisable(userList.getSelectionModel().isEmpty() || groupNameField.getText().isBlank());
delvh commented 3 years ago
Owner

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 3 years ago
Poster
Owner

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.
delvh marked this conversation as resolved
@ -89,1 +93,3 @@
private void userListClicked() { createButton.setDisable(userList.getSelectionModel().isEmpty() || groupNameField.getText().isBlank()); }
private void userListClicked() {
createButton.setDisable(userList.getSelectionModel().isEmpty() || groupNameField.getText().isBlank());
quickSelectList.getItems().add(new QuickSelectControl(userList.getSelectionModel().getSelectedItem(), this));
delvh commented 3 years ago
Owner

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.
mpk marked this conversation as resolved
@ -96,3 +106,3 @@
*/
@FXML
private void textUpdated() { createButton.setDisable(userList.getSelectionModel().isEmpty() || groupNameField.getText().isBlank()); }
private void textUpdated() { createButton.setDisable(quickSelectList.getItems().size() == 0 || groupNameField.getText().isBlank()); }
delvh commented 3 years ago
Owner

Use

setDisable(quickSelectList.getItems().isEmpty()||...);
Use ``` setDisable(quickSelectList.getItems().isEmpty()||...); ```
mpk marked this conversation as resolved
@ -154,0 +173,4 @@
public void removeFromQuickSelection(QuickSelectControl element) {
quickSelectList.getItems().remove(element);
userList.getItems().add(element.getUser());
if (quickSelectList.getItems().size() == 0) {
delvh commented 3 years ago
Owner

Use

if(quickSelectList.getItems().isEmpty()){
Use ``` if(quickSelectList.getItems().isEmpty()){ ```
mpk marked this conversation as resolved
@ -154,0 +179,4 @@
}
}
private void resizeQuickSelectSpace(int value) {
delvh commented 3 years ago
Owner

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 3 years ago
Poster
Owner

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.
mpk marked this conversation as resolved
@ -147,0 +148,4 @@
#quick-select-list .scroll-bar:horizontal{
-fx-pref-height: 0;
-fx-max-height: 0;
-fx-min-height: 0;
delvh commented 3 years ago
Owner

Is that even needed in this case?

Is that even needed in this case?
delvh commented 3 years ago
Owner

Meant is only the last line...

Meant is only the last line...
mpk commented 3 years ago
Poster
Owner

Yes it is absolutly necessary.

Yes it is absolutly necessary.
delvh commented 3 years ago
Owner

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...
mpk commented 3 years ago
Poster
Owner

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.
mpk marked this conversation as resolved
@ -147,0 +154,4 @@
#quick-select-list .scroll-bar:vertical{
-fx-pref-width: 0;
-fx-max-width: 0;
-fx-min-width: 0;
delvh commented 3 years ago
Owner

Is that even needed in this case?

Is that even needed in this case?
delvh commented 3 years ago
Owner

Meant is only the last line...

Meant is only the last line...
mpk commented 3 years ago
Poster
Owner

Yes it is absolutly necessary.

Yes it is absolutly necessary.
delvh commented 3 years ago
Owner

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.
mpk commented 3 years ago
Poster
Owner

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.
mpk marked this conversation as resolved
delvh approved these changes 3 years ago
delvh left a comment
Owner

👍

👍
@ -0,0 +71,4 @@
stackPane.getChildren().add(hBox);
getChildren().add(stackPane);
delvh commented 3 years ago
Owner

Delete this blank line.

Delete this blank line.
mpk marked this conversation as resolved
delvh reviewed 3 years ago
@ -68,3 +72,3 @@
private void initialize() {
userList.setCellFactory(new ListCellFactory<>(ContactControl::new));
userList.getSelectionModel().setSelectionMode(SelectionMode.MULTIPLE);
userList.getSelectionModel().setSelectionMode(SelectionMode.SINGLE);
delvh commented 3 years ago
Owner

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

Wait... Isn't that already default behavior?
mpk marked this conversation as resolved
kske approved these changes 3 years ago
delvh requested changes 3 years ago
@ -0,0 +11,4 @@
import envoy.data.User;
/**
* Displays an {@link User} as a quickSelectControl which is used in the
delvh commented 3 years ago
Owner

We don't use camelCase is Javadoc.

We don't use camelCase is Javadoc.
mpk marked this conversation as resolved
mpk requested review from delvh 3 years ago
mpk merged commit 99867eb23a into develop 3 years ago
mpk deleted branch f/quick-group-select 3 years ago
This repo is archived. You cannot comment on pull requests.
Loading…
There is no content yet.