Quick Select Support for the GroupCreationTab #77
kske
delvh
Loading…
x
Reference in New Issue
Block a user
No description provided.
Delete Branch "f/quick-group-select"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
This PR provides a more refined and user friendly mechanism to select members for a group when creating it.
Fixes #60
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;Please make this
private.@ -0,0 +85,4 @@}/*** @return the user whose data is used in this instance.The dot at the end is unnecessary.
This is the unnecessary selection I mentioned in my review.
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.@ -0,0 +21,4 @@User user;/*** Creates an instance of the {@link QuickSelectControl}.As I've already been conditioned by a certain member of the Envoy team (the coffee machine guy) to not use
@linkin 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.
@ -0,0 +29,4 @@*/public QuickSelectControl(User user, GroupCreationTab tab) {this.user = user;setPadding(new Insets(1, 0, 0, 0));?
This centers the component (it is just 1 pixel but very noticable)
@ -0,0 +37,4 @@stackPane.setAlignment(Pos.TOP_CENTER);// Profile picturevar picHold = new VBox();Why exactly are you declaring a
VBoxinside aVBox?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.
@ -0,0 +49,4 @@clip.setArcHeight(32);clip.setArcWidth(32);contactProfilePic.setClip(clip);setAlignment(Pos.TOP_CENTER);This is, as far as I can see, unnecessary.
Have tested it, it is.
@ -0,0 +59,4 @@hBox.setMinHeight(12);var region = new Region();hBox.getChildren().add(region);hBox.setHgrow(region, Priority.ALWAYS);This line should be
(
setHgrowis a static method)@ -0,0 +65,4 @@removeBtn.setPrefSize(12, 12);removeBtn.setMaxSize(12, 12);removeBtn.setMinSize(12, 12);removeBtn.setOnMouseClicked(evt -> tab.removeFromQuickSelection(this));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:
This additionally brings the advantage of reusability.
When this is implemented, the
GroupCreationTabcan be removed from the declaration of this component.@ -0,0 +67,4 @@removeBtn.setMinSize(12, 12);removeBtn.setOnMouseClicked(evt -> tab.removeFromQuickSelection(this));removeBtn.setId("remove-button");hBox.getChildren().add(removeBtn);If I understand you correctly, the only purpose of the
HBoxis 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:
stackPane.getChildren(...))hBoxin line 70 topicHoldpicHoldfrom aVBoxto anHBoxJust 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)
Funny thing. They haven't. But still:
hBoxandregionpicHoldvariable should be anHBox, not aVBoxhBox.getChildren().add(removeBtn);should behBoxtostackPanecan be deletedI 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
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?
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);Unnecessary. Delete the line.
No it is defenitly not as this is centering the label!
@ -88,2 +92,3 @@@FXMLprivate void userListClicked() { createButton.setDisable(userList.getSelectionModel().isEmpty() || groupNameField.getText().isBlank()); }private void userListClicked() {createButton.setDisable(userList.getSelectionModel().isEmpty() || groupNameField.getText().isBlank());Isn't that the wrong check here? Shouldn't it be
Yes there was an issue but it was not the one you suggested.
@ -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));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.@ -96,3 +106,3 @@*/@FXMLprivate void textUpdated() { createButton.setDisable(userList.getSelectionModel().isEmpty() || groupNameField.getText().isBlank()); }private void textUpdated() { createButton.setDisable(quickSelectList.getItems().size() == 0 || groupNameField.getText().isBlank()); }Use
@ -154,0 +173,4 @@public void removeFromQuickSelection(QuickSelectControl element) {quickSelectList.getItems().remove(element);userList.getItems().add(element.getUser());if (quickSelectList.getItems().size() == 0) {Use
@ -154,0 +179,4 @@}}private void resizeQuickSelectSpace(int value) {You really do like it if no one except you can ever resize components, right?
I have absolutely no clue what you mean but as I see it this is just a usefull little method.
@ -147,0 +148,4 @@#quick-select-list .scroll-bar:horizontal{-fx-pref-height: 0;-fx-max-height: 0;-fx-min-height: 0;Is that even needed in this case?
Meant is only the last line...
Yes it is absolutly necessary.
Why? If the max height is already at 0, the pref height won't be larger than that, same for the min height...
No thats the thing I tried it and the scroll bars appear. I wondered to.
@ -147,0 +154,4 @@#quick-select-list .scroll-bar:vertical{-fx-pref-width: 0;-fx-max-width: 0;-fx-min-width: 0;Is that even needed in this case?
Meant is only the last line...
Yes it is absolutly necessary.
Why? If the max width is already 0, not even the pref width should matter, especially not the min width.
No thats the thing I tried it and the scroll bars appear. I wondered to.
👍
@ -0,0 +71,4 @@stackPane.getChildren().add(hBox);getChildren().add(stackPane);Delete this blank line.
@ -68,3 +72,3 @@private void initialize() {userList.setCellFactory(new ListCellFactory<>(ContactControl::new));userList.getSelectionModel().setSelectionMode(SelectionMode.MULTIPLE);userList.getSelectionModel().setSelectionMode(SelectionMode.SINGLE);Wait... Isn't that already default behavior?
@ -0,0 +11,4 @@import envoy.data.User;/*** Displays an {@link User} as a quickSelectControl which is used in theWe don't use camelCase is Javadoc.