Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enhancement - multichoice type ahead dropdownlist #2256

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

andromeda-224
Copy link
Collaborator

@andromeda-224 andromeda-224 commented Jan 9, 2025

Prerequisites

  • Reviewed the checklist

  • Reviewed feedback from the "Sonar Cloud" bot. Note that you have to wait
    for the "CI / Unit Tests") to complete first. Failed Unit tests can be
    debugged by adding the label "verbose logging" to the GitHub PR.

Description of the Change

The original ticket was to add ability to type ahead in MultiChoiceParameterType drop down lists.
But because there are a number of different versions of the Multichoice input field, this might be a good opportunity to create a united look & feel for all input fields, beginning with this one.

The main idea is to use ConstellationInput and ParameterInputPane and extend from them for specific types of input.
Each input field will allow the implementation of a left button, input area and a right button.
+----------------------------------------------------+
| Left Button | Input Area | Right Button |
+---------------------------------------------------+

Various listeners will allow communication between components.

Alternate Designs

The alternative is to work on each individual drop down list and customise.

Why Should This Be In Core?

This should be in core so that the look and feel of all input fields will have the same look and feel and work in the same way.

Benefits

This should be in core so that the look and feel of all input fields will have the same look and feel and work in the same way.

Possible Drawbacks

All input fields will have to be reviewed/modified.

Verification Process

  1. Open Constellation
  2. Select DA view -> Test Parameters
  3. Observe 'Earth' in the input area under 'Planets'.
  4. Type a comma (',') followed by the letter 'm'. The drop down list will list 'Mars' and 'Mercury'.
  5. Delete everything in the planets input field. Click in the field. A list of planets should drop down.
  6. Type 'V'. Only 'Venus' should appear in the drop down list.

Applicable Issues

#1012
#1877

@andromeda-224 andromeda-224 added the verbose-logging test code in CI with verbose output label Jan 20, 2025
@andromeda-224
Copy link
Collaborator Author

Fields that have been updated with the latest type-ahead MultiChoiceParameterType:

File -> Export -> ToGeoJSON/ To GeoPackage -> Attributes
Tools -> Compare Graph -> Ignore Node Attributes
Tools -> Compare Graph -> Ignore Transaction Attributes
Experimental -> Build Graph -> Complete Graph Builder -> Node types , Transaction types
Experimental -> Build Graph -> Preferential Attachment Graph Builder -> Node types , Transaction types
Experimental -> Build Graph -> Small World Graph Builder -> Node types , Transaction types

Data Access View:

  • Select Top N -> Specific Types
  • Test Parameters/Test Parameters (Batched) -> Planets

Analytic View:
Information -> Transaction Types

Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
47.0% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

Copy link
Collaborator

@antares1470 antares1470 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could've added more comments but the general patterns were:

  • add final where appropriate
  • fix formatting
  • make copyright 2025 on new classes

Also if you could add a whatsnew entry, that would be great.

Some feedback from testing: if you type an invalid value, the red indicating invalid isn't as visible as you would ideally like (only a faint border). If we could get the box fully coloured red when an invalid value is present, that would be great.

I also did note that the type ahead only worked when adding to the end of the list, and not in the middle. Perhaps that is an addition for later on though

Comment on lines 239 to 243
if (empty) {
setGraphic(null);
} else {
setGraphic(new ImageView(img));
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can this be made a ternary statement?

field.setId(mcPluginParameter.isRequired() && field.getCheckModel().isEmpty() ? "invalid selection" : "");
field.setStyle("invalid selection".equals(field.getId()) ? "-fx-color: #8A1D1D" : "");
@Override
public ConstellationInputListener getFieldChangeListener(PluginParameter<MultiChoiceParameterValue> parameter) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

parameter final

field.setStyle("invalid selection".equals(field.getId()) ? "-fx-color: #8A1D1D" : "");
@Override
public ConstellationInputListener getFieldChangeListener(PluginParameter<MultiChoiceParameterValue> parameter) {
return (ConstellationInputListener<List<ParameterValue>>) (List<ParameterValue> newValue) -> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

newValue can either be final or explicit type (and brackets) removed

isAdjusting = false;
@Override
public PluginParameterListener getPluginParameterListener() {
return (PluginParameter<?> parameter, ParameterChange change) -> Platform.runLater(() -> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

parameters final or remove explicit types

@SuppressWarnings("unchecked") //mcPluginParameter is a MultiChoiceParameter
final PluginParameter<MultiChoiceParameterValue> mcPluginParameter = (PluginParameter<MultiChoiceParameterValue>) parameter;
switch (change) {

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove extra whitespace here

primaryInput = area;
}
default -> {
TextField field = new TextField();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

final


//The up down arrows allow for navigation to the begining and start of a line
//This is being remapped to ALT + left and ALT + right for consistency between textArea and textField
field.addEventFilter(KeyEvent.KEY_PRESSED, (KeyEvent event) -> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

parameter final or omit specific type for lambda

* Binds the heightProperty of a Rectangle to the height property of the {@link TextInputControl}.
* @param bindables
*/
public void bindHeightProperty(Rectangle... bindables) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

parameter final

* @param bindables
*/
public void bindHeightProperty(Rectangle... bindables) {
for (Rectangle bindable : bindables){
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

final

* Sets the text value of the {@link TextInputControl}.
* @param stringValue
*/
public void setText(String stringValue) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

final (and finals elsewhere in the class)

Copy link
Collaborator

@Delphinus8821 Delphinus8821 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I gave it a test and it looks like its working well. I agree with the other review comments.
It would also be good to fix the code smells sonar has identified

Includes update on year in copyright, code formatting, added whatsnew and changelog, made input field all red on error
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
verbose-logging test code in CI with verbose output
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants