Change FileKeystore and Folder fields to disable/enable#15685
Conversation
Review Summary by QodoConditionally enable keystore and folder fields in shared database dialog
WalkthroughsDescription• Conditionally disable/enable keystore and folder fields based on SSL and autosave preferences • Add validation rules that respect SSL and autosave checkbox states • Fix error messages for host and port validators (swapped labels) • Include keystoreValidator and folderValidator in form validation Diagramflowchart LR
SSL["SSL Checkbox"] -->|toggled| KeystoreDisable["Disable keystore field"]
Autosave["Autosave Checkbox"] -->|toggled| FolderDisable["Disable folder field"]
KeystoreDisable --> Validation["Update validation rules"]
FolderDisable --> Validation
Validation --> FormValidator["Add validators to form"]
FormValidator --> ConnectButton["Enable/disable connect button"]
File Changes1. jabgui/src/main/java/org/jabref/gui/shared/SharedDatabaseLoginDialogView.java
|
Code Review by Qodo
1.
|
…ile for automatically saving
…om/gandhroh000/jabref into fix_shared_database_fields_enabled
|
Resolved the qodo action and ensured that the save path was working. |
pluto-han
left a comment
There was a problem hiding this comment.
i think, a change in view is enough
| if (!autosave.get()) { | ||
| return true; | ||
| } else if (input != null) { | ||
| try { | ||
| Path p = Path.of(input.trim()); | ||
| p = p.getParent(); | ||
| return (p != null) && Files.isDirectory(p); | ||
| } catch (InvalidPathException e) { | ||
| return false; | ||
| } | ||
| } | ||
| return false; | ||
| }; |
There was a problem hiding this comment.
Thanks for checking, though this is the rule used in the validation function to ensure that either the autosave button is not enabled or the file path that the user has entered is indeed valid, as there is a manual input and browse input available. If this isn't set up and an incorrect directory location is passed, then the dialog will throw syntax errors and won't be able to progress further. In previous closed pull request (#15661) qodo recognized that these validators weren't used and inputs weren't being validating. That's why there was a need to implement this rule. Earlier tried to combine two different observables, but that didn't work (will explain in subsequent comments.)
| EasyBind.subscribe(autosave, selected -> { | ||
| String current = folder.getValue(); | ||
| folder.setValue(null); | ||
| folder.setValue(current); | ||
| }); |
There was a problem hiding this comment.
This "crude toggle technique" is used as when attempting to simplify the process and combine two observables using EasyBind, unfortunately the system is smart enough to only need to activate or check the validation rule when the input it needs is changed (folder in this case) which is why there was a need to use this technique and trigger a change in the folder, thereby the rule would be evaluated and the Connect button would enable or disable appropriately.
Hi @pluto-han thank you for your comments and I'd be happy to explain. The three comments you made regarding changes in ViewModel were necessary as the validation functions were firstly not using the autosave or useSSL fields to confirm that the information entered there was indeed accurate and should be processed (as these fields control whether the folder and keystore fields will be processed as input and checked). I will make further comments explaining each design choice, under each of your comments. |
Siedlerchr
left a comment
There was a problem hiding this comment.
Thanks for the comment/explanations!
* upstream/main: (21 commits) chore(deps): update dependency com.konghq:unirest-modules-gson to v4.10.0 (JabRef#15715) Add manual tests (JabRef#15351) Refactored the comments for UnlinkedFilesCrawler (JabRef#15709) Replace inline styles with CSS classes (JabRef#15694) add test case for multiple authors in csl citaiton (JabRef#15707) fix invalid desktop file for linux (JabRef#15702) Change FileKeystore and Folder fields to disable/enable (JabRef#15685) Chore(deps): Bump org.openrewrite.recipe:rewrite-recipe-bom from 3.30.0 to 3.30.1 (JabRef#15696) New Crowdin updates (JabRef#15693) Chore(deps): Bump dev.langchain4j:langchain4j-bom in /versions (JabRef#15698) Chore(deps): Bump org.apache.logging.log4j:log4j-to-slf4j in /versions (JabRef#15700) chore(deps): update dependency org.apache.logging.log4j:log4j-to-slf4j to v2.26.0 (JabRef#15699) Chore(deps): Bump org.openrewrite.rewrite from 7.32.1 to 7.32.2 (JabRef#15697) Chore(deps): Bump jablib/src/main/resources/csl-locales (JabRef#15689) Fix month checker regex (JabRef#15678) Chore(deps): Bump com.dlsc.gemsfx:gemsfx in /versions (JabRef#15692) Chore(deps): Bump org.hisp.dhis:json-tree in /versions (JabRef#15691) Chore(deps): Bump jablib/src/main/resources/csl-styles (JabRef#15690) New Crowdin updates (JabRef#15687) Chore(deps): Bump com.konghq:unirest-java-core in /versions (JabRef#15683) ...
Related issues and pull requests
Closes #15660
PR Description
In the "Connect to Shared Database" page, when the "Use SSL" checkbox was deselected, fileKeyStore would still be showing as a required field, which can be confusing to the user. For reference, only when the SSL connection checkbox is enabled, the fileKeyStore field would be enabled. To change this, it was necessary to add a condition to the SharedDatabaseLoginDialogView. java file that triggered the disabledProperty function, hiding the fileKeyStore entry field.
Steps to test
Disabled "Use SSL" button:
Enabled "Use SSL" button:
Disable the "Automatically save the library to" button:
Enable the "Automatically save the library to" button:
AI Disclosure
Utilized Google Antigravity IDE to debug and locate issues, provide context regarding overall file and JabRef project structure, and to assess tradeoffs of solution implementations. All code was tested in the JabRef application and actions align with guidelines set by the JabRef repository.
Checklist
CHANGELOG.mdin a way that can be understood by the average user (if change is visible to the user)