-
-
Notifications
You must be signed in to change notification settings - Fork 3.2k
Fix #14821: Hide identifier action buttons when field is empty #14831
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
Fix #14821: Hide identifier action buttons when field is empty #14831
Conversation
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
|||||||||||||||||||||||
PR Code Suggestions ✨Explore these optional code suggestions:
|
|||||||||
68b98a3 to
6d92a10
Compare
478bf77 to
5a973f7
Compare
jabgui/src/main/java/org/jabref/gui/fieldeditors/identifier/IdentifierEditor.java
Outdated
Show resolved
Hide resolved
|
@abhishek9773 thanks for the contribution. |
CHANGELOG.md
Outdated
|
|
||
| ### Fixed | ||
|
|
||
| - Fixed redundant action icons showing in empty identifier fields (DOI, Eprint, ISBN) [#14831](https://github.com/JabRef/jabref/pull/14831) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3813a2a to
4e1fc3e
Compare
|
Hi @koppor, @subhramit, I also wanted to be fully transparent about my use of AI on this PR, as per the project policy. As a new contributor, I’ve been using AI as a tool to ensure my code and test cases actually reach the industry standards expected here. For example, in the I sincerely apologize for the 'chaos' with the force-pushes. I was trying to keep the history clean as I learned, but I see now that it triggered the build failures and made your review harder. |
Usage of AI to refine code is alright, and thanks for being transparent. However, please try to communicate in your own terms rather than copy pasting LLM responses. Please look at these guidelines. Since you seem to have understood your changes, you are also expected to write the PR description yourself instead of generating it using an LLM. |
|
@subhramit Thank you for pointing me out. from now onwards I will make sure that I am following all the points you have mentioned |
|
@abhishek9773 not sure why you clicked on "resolve conversation" without acting on review changes |
it was mistake . i made changes to "changelog.md" file but forgot to add those changes during ">git add" and mistakenly assumed i had done so.
|
look for this "outdated" label before resolving conversations: |
* upstream/main: (64 commits) New Crowdin updates (JabRef#14862) Make JDK25 available (JabRef#14861) Fix empty entries array when exporting group chat to JSON (JabRef#14814) feat: add right-click copy context menu to AI chat messages (JabRef#14722) FIX : generic error dialog and icon in Source Tab parsing (JabRef#14828) Factor out setup-* actions (JabRef#14859) Link .http files. Update dependency org.postgresql:postgresql to v42.7.9 (JabRef#14857) Add more commands to JabSrv (JabRef#14855) Fix JabRef#14821: Hide identifier action buttons when field is empty (JabRef#14831) Add GH_TOKEN to closed issues/PRs processing step New Crowdin updates (JabRef#14854) New Crowdin updates (JabRef#14849) Chore(deps): Bump jablib/src/main/resources/csl-styles from `0201999` to `f345aa8` (JabRef#14833) Add support for book front covers, again (JabRef#14777) Readd min width to button in new enty dialog (JabRef#14791) Replace plugin impl from jbang plugin (JabRef#14846) Revise AI policy wording Chore(deps): Bump jablib/src/main/resources/csl-locales (JabRef#14677) Update dependency com.konghq:unirest-modules-gson to v4.7.1 (JabRef#14845) ...
koppor
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The concept of JabRef is to DISABLE buttons, not to hide.
This PR trashed the functionality to look up a DOI. Look up means: If there is no DOI, search the net for it.
|
|
||
| Stream.of(fetchInformationByIdentifierButton, lookupIdentifierButton) | ||
| .forEach(button -> { | ||
| button.visibleProperty().bind(viewModel.textProperty().isNotEmpty()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did someone understand what "lookup" means?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I misunderstood the behavior and took the word “hide” literally.
I also understand that “lookup” should work even when there is no DOI, by searching online.
I can change the code to disable instead of hide. Is this what you mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the offer. Your past PR needed more than 10 comments of two different maintainers. Still the code did not work. Your code is just hacky - and did not build on existing (!) JavaFX code. Thus, we should save the time of both you and the team. -- In future, please first read around at the code base. The IdentifierEditor is a very nice example to learn about JavaFX bindings and general UI principles in JavaFX. You missed this opportunity.
I already fixed it at #14931.
| button.managedProperty().bind(button.visibleProperty()); | ||
| }); | ||
|
|
||
| shortenDOIButton.managedProperty().bind(shortenDOIButton.visibleProperty()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code totally overlooked org.jabref.gui.fieldeditors.identifier.BaseIdentifierEditorViewModel#canShortenIdentifier





User description
Closes #14821
Steps to test
Visual Verification
Screencast.from.11-01-26.03.01.55.PM.IST.webm
Mandatory checks
CHANGELOG.mdin a way that is understandable for the average user (if change is visible to the user)LLM USED: HELPED ME IN ARCHITECTURAL DECISION AND TEST CASE WRITING
PR Type
Enhancement
Description
Bind identifier action buttons visibility to text field state
Buttons automatically hide when field is empty, decluttering UI
Applies to Fetch, Lookup, and Shorten buttons for DOI, Eprint, ISBN fields
Managed property synced with visibility for proper layout expansion
Diagram Walkthrough
File Walkthrough
IdentifierEditor.java
Bind identifier buttons visibility to text field statejabgui/src/main/java/org/jabref/gui/fieldeditors/identifier/IdentifierEditor.java
viewModel.textProperty().isNotEmpty()