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

Fixes OK button enabling property in Group Dialog #4799

Closed
wants to merge 5 commits into from

Conversation

samiyac
Copy link
Contributor

@samiyac samiyac commented Mar 21, 2019

Fixes issue #4783

Binded nameField.textProperty() to the OK button so that OK button gets enabled as soon as we enter name of the Group.


  • Change in CHANGELOG.md described
  • Tests created for changes
  • Manually tested changed features in running JabRef
  • Screenshots added in PR description (for bigger UI changes)
  • Ensured that the git commit message is a good one
  • Check documentation status (Issue created for outdated help page at help.jabref.org?)

Copy link
Member

@tobiasdiez tobiasdiez left a comment

Choose a reason for hiding this comment

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

Thanks for the quick update. This works but is bit too complicated. The updateComponents is called whenever the state of the dialog is updated and thus there is no need to use bindings. In the meantime somebody opened another PR #4801 on this issue and I would consider his fix to be more straightforward. Thus, please don't be disappointed if we accept #4801 as the fix for this issue.

However, you were on the right track and we should use the power of JavaFX bindings instead of the old-style update mechanism. For validation there is a framework https://github.com/sialcasa/mvvmFX/wiki/Validation which allows to implement this kind of logic in a clean way. May I ask you to rework the code completely using this framework? If you are not in the mood to do this and prefer to work on something else, I would perfectly understand this.

Sorry for the strange situation.

@samiyac
Copy link
Contributor Author

samiyac commented Mar 22, 2019

Hi @tobiasdiez I understand the situation and I do agree with you about the other PR being more suitable.

However, you were on the right track and we should use the power of JavaFX bindings instead of the old-style update mechanism. For validation there is a framework https://github.com/sialcasa/mvvmFX/wiki/Validation which allows to implement this kind of logic in a clean way. May I ask you to rework the code completely using this framework? If you are not in the mood to do this and prefer to work on something else, I would perfectly understand this.

I would like to work on this. Could you tell me exactly what all I will have to re-implement?
I will make a different PR for all the changes to GroupDIalog and close this one.

Thank you for updating me about the situation and for the guidance. I look forward to keep contributing to JabRef.

@samiyac samiyac closed this Mar 22, 2019
@tobiasdiez
Copy link
Member

Thanks for your understanding.

As of now the data in the group dialog is validated in the updateComponents method. Instead it is better to have a collection of validation rules that are then combined in a so-called composite validation rule. This composition rule can then be used to control the disable status of the save button.

You can find this strategy live for example here:

  • Define rules:
    databaseValidator = new FunctionBasedValidator<>(database, notEmpty, ValidationMessage.error(Localization.lang("Required field \"%0\" is empty.", Localization.lang("Library"))));
    hostValidator = new FunctionBasedValidator<>(host, notEmpty, ValidationMessage.error(Localization.lang("Required field \"%0\" is empty.", Localization.lang("Port"))));
    portValidator = new FunctionBasedValidator<>(port, notEmpty, ValidationMessage.error(Localization.lang("Required field \"%0\" is empty.", Localization.lang("Host"))));
    userValidator = new FunctionBasedValidator<>(user, notEmpty, ValidationMessage.error(Localization.lang("Required field \"%0\" is empty.", Localization.lang("User"))));
    folderValidator = new FunctionBasedValidator<>(folder, notEmptyAndfilesExist, ValidationMessage.error(Localization.lang("Please enter a valid file path.")));
    keystoreValidator = new FunctionBasedValidator<>(keystore, notEmptyAndfilesExist, ValidationMessage.error(Localization.lang("Please enter a valid file path.")));
  • Combine them:
    formValidator = new CompositeValidator();
    formValidator.addValidators(databaseValidator, hostValidator, portValidator, userValidator);
  • Bind to button:
    btnConnect.disableProperty().bind(viewModel.formValidation().validProperty().not());
  • Moreover, it is easy to display a notification to the user in case the data is invalid:
    visualizer.initVisualization(viewModel.dbValidation(), database, true);
    visualizer.initVisualization(viewModel.hostValidation(), host, true);
    visualizer.initVisualization(viewModel.portValidation(), port, true);
    visualizer.initVisualization(viewModel.userValidation(), user, true);

@samiyac samiyac deleted the fix-for-issue-4783 branch March 30, 2019 07:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants