Added warning label that allows user to jump to already existing entry

[jayvardhanghildiyal]

Closes #13261

This pull request adds a checkDOI function, listener logic and some fxml to NewEntryView.java and NewEntry.fxml. This addition aims to add a UI component that warns the user when trying to add an already existing entry to a library, while also allowing them to jump to it instead.

Steps to test

demonstration.mp4

Mandatory checks

• I own the copyright of the code submitted and I license it under the MIT license
• Change in CHANGELOG.md described in a way that is understandable for the average user (if change is visible to the user)
• Tests created for changes (if applicable)
• Manually tested changed features in running JabRef (always required)
• Screenshots/video added in PR description (if change is visible to the user)
• Checked developer's documentation: Is the information available and up to date? If not, I outlined it in this pull request.
• Checked documentation: Is the information available and up to date? If not, I created an issue at https://github.com/JabRef/user-documentation/issues or, even better, I submitted a pull request to the documentation repository.

[jayvardhanghildiyal]

Oh binaries for different platforms are re-checked with every commit ? I'll try to make fewer commits.

[calixtus]

Hey @jayvardhanghildiyal , thank you for your PR and interest in JabRef.
About the Message "You must provide" - We are already using controlsfx validation badge in the preferences editor and the entry editor. Would you like to take a look in it and to modify your PR to display the message as a validation note on the doi field?

[calixtus]

O sorry, i just noticed, that message "you must provide" was not your change, that was already before....

but anyways, maybe you still want to take a look into it on the fly?

[jayvardhanghildiyal]

We are already using controlsfx validation

@calixtus I've looked into it and would like to explain what I think is happening. I'll post some code below mostly for my reference and understanding . Do correct me if I've missed the point of the request !

My Understanding of the validation logic

So this validator below helps control the visibility of FXML components.

idTextValidator = new FunctionBasedValidator<>(
            idText,
            StringUtil::isNotBlank,
            ValidationMessage.error(Localization.lang("You must specify an identifier.")));

public ReadOnlyBooleanProperty idTextValidatorProperty() {
        // logic to get validation status
    }

Validators like this can affect labels in the FXML file and the button in the new entry dialog window.

idErrorInvalidText.visibleProperty().....
idErrorInvalidFetcher.visibleProperty()....
generateButton.disableProperty().....

Requested Change

I have to:

• move the label below the textfield
• add it alongside all the other validation notes instead
• use validation logic to modify my implementation

....
      // moved label and hyperlink
    <Label fx:id="duplicateSelectLabel" text="Entry already existing in library."/>
    <Hyperlink fx:id="duplicateSelectLink" text="%Select entry"/>
    <Label fx:id="idErrorInvalidText" text="%You must provide an identifier."/>
    <Label fx:id="idErrorInvalidFetcher" text="%You must select an identifier type."/>

....

[jayvardhanghildiyal]

I think I've done it, but there is an error that says:

ERROR: Could not find citation style catalog

It comes from the CSLStyleLoader.java. I don't remember changing anything related to it. I'll try to see what it is about.

[calixtus]

Ignore the error, this might because you did not initialize the submodules. But it should run anyways

[koppor]

It comes from the CSLStyleLoader.java. I don't remember changing anything related to it. I'll try to see what it is about.

On the command line, execute git submodule update to have all submodules in the correct state.

Merge latest upstream/main and execute ./gradlew :jabgui:run (or similar in IntelliJ). Then, these files should be generated. :)

As Carl said: it can be ignored for this issue.

[koppor]

I think, something went wrong during merge of main - and the architecture needs to be fixed 😅

[Siedlerchr]

Hi,
first, please look at the failing tests.

We discussed this in our devcall:

• For consistency reason you should use a warning badge like e.g. in the entry editor

In the badge, show the text "Entry already exists in library XXXX" (can we show the library name?)

• Add a label "Jump to entry" to the right inside the text field in case it already exists
  

Use localization https://devdocs.jabref.org/code-howtos/localization.html#localization-in-fxml

[calixtus]

A helpful page about validation in ui:
https://uxplanet.org/designing-more-efficient-forms-assistance-and-validation-f26a5241199d

Wa had a discussion about that in the devcall:
We use the small warning badges in the entry editor, in the preferences etc. Therefor for the sake of consistency, it should work the same here. A small warning badge on the left, a popover saying "Entry already exists".

However in general, we think that the tooltips on the validation badges are not the best ui design. We go with it for now, as this is the current ui design approach in other places and we should think in the future to do this differently. Eventually when the entry editor will be rewritten (TM 😬) But for now, move forward as @Siedlerchr suggested.

Mind that in rtl languages the "Jump to entry" should appear on the left of the textinput control.

[jayvardhanghildiyal]

Hi @Siedlerchr ! I will keep looking at the tests.

In the badge, show the text "Entry already exists in library XXXX" (can we show the library name?)

Yes, that is doable ! In the commit before the latest one, I used the getOpenDatabases method to get access to all entries across all databases. I can make, say, a map that holds all BibEntrys and their corresponding LibraryTabs and use it to update the validation badge. But I don't think I can jump to another library from the currently active one. So, in case the entry does not belong to the current library tab, we won't be able to jump to it. We would only be able to show the validation badge.

To highlight the entry present in the current library I am using the showAndEdit method. But while using stateManager.getOpenDatabases(), I realised that the entry has to belong to the current library or the method just displays a blacked-out edit menu. I was thinking that maybe I can use the showLibraryTab method from LibraryTabContainer.java, followed by the showAndEdit method to solve that problem. But we don't have access to an instance of LibraryTabContainer either.

I'll try my best !

[calixtus]

A new map with all BibEntry(ies)? This is a bad idea. Think of a library with 10.000 entries. Not uncommon.

[jayvardhanghildiyal]

Hey there ! I've implemented some of the requested changes. I will continue working on the rest. Had some issues with the IDE so took more time than needed to. Some old logic not in use anymore is commented out, I'll remove it later ! Keeping it for my own reference for now.

For now, when a duplicate entry is detected, the triangle shows up next to the textfield. But the text doesn't. When I hover over the textfield at an unknown angle, the orange tooltip appears just for a second and disappears. Gotta figure that one out. Probably something to do with the existing tooltip trying to take precendence (I don't know I'll have to check xD) ?

So, a few questions I wanted to ask:

• I am using ValidationMessage.warning(Localization.lang(....)) for this. Since finding a duplicate entry isn't exactly an error, is this fine ? Or should I use ValidationMessage.error ?
• I've put the new key inside of the # New Entry section of jabref_en.properties. Let me know if I have placed it correctly !

[calixtus]

warning is perfectly fine.

[Siedlerchr]

Looks good so far, just remove the commented out code and i think it's ready

[jayvardhanghildiyal]

Looks good so far, just remove the commented out code and i think it's ready

Hey @Siedlerchr ! I wanted to talk about the jump to entry thingy.

Since I do not have access to other LibraryTabs (reason discussed before), let me know which implementation would be better:

• doiCache has access to all entries across all libraries to warn the user, but we don't give them the option to jump to that entry.
• doiCache has access to all entries in the currently active library and we give the user the ability to jump to that entry.

I think utility-wise, option 1 is better because the implementation will alert the user over all the entries. And if they want to add a new entry to a library, they will know that the same one exists somewhere else too. Not being able to jump to it sucks, but at least the user will be aware. I will now send out a commit that implements option 1, which I personally think will give out more consistent and useful warnings.

Do let me know ! In the meantime, I'll remove the commented code and add the suggested changes !

[Siedlerchr]

After thinking again of it, The new entry dialog is bound to a library, so it makes sense to only check the entries of the current library and then the jump to entry is useful as well.

And one thing, it's very hard to see the warning, because when I do mouseover I see the tooltip

so maybe disable that tooltip when the warning is shown?

[jayvardhanghildiyal]

Oh that's odd. The last time I hovered over the symbol, appeared every single time. I'll look into it once again.

[trag-bot]

@trag-bot didn't find any issues in the code! ✅✨

[jayvardhanghildiyal]

Hi again ! I've added the suggestions !

Working with the Tooltip idTextTooltip methods was a bit weird, because none of it's properties seem to affect it in any way. I figured out a work around.

Also, the jump to entry element that appears on the TextField idText is a hyperlink. Thought it was more intuitive for the user. I hope that is alright.