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

Implemented TagsField for the Keywords field #10910

Merged
merged 20 commits into from
Mar 5, 2024

Conversation

LoayGhreeb
Copy link
Collaborator

@LoayGhreeb LoayGhreeb commented Feb 22, 2024

Closes #10560

  • Implemented GemsFX TagsField for the Keywords field.

image

Mandatory checks

  • Change in CHANGELOG.md described in a way that is understandable for the average user (if applicable)
  • Tests created for changes (if applicable)
  • Manually tested changed features in running JabRef (always required)
  • Screenshots added in PR description (for UI changes)
  • 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.

@calixtus
Copy link
Member

The code looks almost fine to me so far, yet you should always use our cell factory for the drop-down list. Otherwise there will be rendering errors.
A good start.

@Siedlerchr
Copy link
Member

in addition to @calixtus comment, you can find example code for the field libraryListView in the ImportEntriesDialog

@LoayGhreeb LoayGhreeb marked this pull request as ready for review February 25, 2024 02:07
@LoayGhreeb
Copy link
Collaborator Author

Hi @Siedlerchr, @calixtus can you review the changes I made and let me know if there's anything else that needs to be done

@LoayGhreeb LoayGhreeb changed the title Bring back content selectors Add selection box for content selectors next to keyword field Feb 26, 2024
@calixtus
Copy link
Member

Hi, codewise it looks good to me, yet I'm not happy with the UI. The Combobox always seems to stay empty, only the dropdown list is used. Can you replace the box with just an icon button with some symbol representing content selectors and keep the dropdown menu? Or can the width of the buttonbox be reduced?

@koppor
Copy link
Member

koppor commented Feb 26, 2024

While discussing the UI. We have some chip view on the Crossref field (if auto completion is on)

image

Would it be possible to rewrite the editor for the keyword field to have the similar functionality, but using the provided terms in the dropdown. (Hint: SuggestionProvider; should also return something if NO letter is input. Then, the list should always be filled)

@LoayGhreeb
Copy link
Collaborator Author

Thanks, I'll rewrite the keywords field to use Tags Field (task 4 of #10560) and ignore task 1.
Also, I noticed a bug with the Crossref field. When there's no content selector it works fine.
But if I add content selectors and enable autocomplete the dropdown doesn't show, and pressing enter doesn't add the tag.
I'll look into this while working on the keyword field.

@Siedlerchr
Copy link
Member

For understanding the AutoCompletion hierarchy, I recently explained this in another issue
#8145 (comment)

@koppor koppor marked this pull request as draft February 28, 2024 22:34
Copy link
Member

@koppor koppor left a comment

Choose a reason for hiding this comment

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

Content selectors should always be enabled.

image


Even if activated, it does not work here:

image

@LoayGhreeb
Copy link
Collaborator Author

LoayGhreeb commented Mar 2, 2024

Content selectors should always be enabled.

I've fixed autocomplete issues. Now:

I'm not sure what OpenRewrite test is or why it failed.

@LoayGhreeb
Copy link
Collaborator Author

LoayGhreeb commented Mar 2, 2024

Regarding the Keywords Field:

  • I tried using the ChipView as a tag view instead of the Label. Please check what is preferred for use.

    image

- I'm still unable to import the current keywords or write new ones. I attempted to implement the binding as it is used in the Crossref field, but I couldn't do it. (Done)


Additionally, there is a visual issue with TagsField. If there are saved values in the field the height of the field expands according to how many tags are in the field. However, if you start to write in the field, it fixes the height issue and returns to the normal height.

gif

@calixtus
Copy link
Member

calixtus commented Mar 2, 2024

Hi, we discussed the the exception of crossref autocomplete always enabled regardless of the preference option a few weeks ago internally and decided against it, since it could have heavy performance issues on large databases. Instead we decided, that we want to make the preferences more fine grained and enable autocomplete for crossref as default. But I would suggest to put this change into a new issue/pr to not widen the scope of this pr any further.

@calixtus
Copy link
Member

calixtus commented Mar 2, 2024

For now your changes to the suggestion provider are fine.

@calixtus
Copy link
Member

calixtus commented Mar 2, 2024

About the height change, maybe related to the automatic weight calculation in the entry editor?

Copy link
Member

@koppor koppor left a comment

Choose a reason for hiding this comment

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

Looks good! Very small comment...

image


Follow up proposals - filed as #10984

  1. On paste: If it is possible to add this keyword, directly add it without requiring the user to press Enter
  2. Double click on keyword: remove it and enter edit mode. Example:
    double click on "Where"
    image
    leads to
    image
  3. Reorder keywords by drag and drop
  4. Alphabetic ordering of keywords. In case the list is already alphabetically ordered, add the new keyword in alphabetical order. Otherwise, add it to the end

@koppor
Copy link
Member

koppor commented Mar 3, 2024

@LoayGhreeb Since you are in the context of keywords, may I ask if you could look into #8701 (comment)?

@koppor
Copy link
Member

koppor commented Mar 4, 2024

OMG, what (sometimes) does NOT work is to CLICK on an empty field:

image

Tabbing into the field works.

Similr for Crossref field. Sometimes, it doesn't accept the focus.

@LoayGhreeb
Copy link
Collaborator Author

OMG, what (sometimes) does NOT work is to CLICK on an empty field:

Similr for Crossref field. Sometimes, it doesn't accept the focus.

Yes, I noticed this issue. It is related to GemsFX TagsField, you have to click exactly at the position where the text will be written in the text field, or I think click on the same line.

image

koppor
koppor previously approved these changes Mar 4, 2024
@calixtus
Copy link
Member

calixtus commented Mar 4, 2024

About the clicking issue: Maybe the tagsfield itself is only one line high. I think the borders of the tags field are not shown, only the border of the surrounding editor.

koppor
koppor previously approved these changes Mar 4, 2024
CHANGELOG.md Outdated Show resolved Hide resolved
@LoayGhreeb
Copy link
Collaborator Author

About the clicking issue: Maybe the tagsfield itself is only one line high. I think the borders of the tags field are not shown, only the border of the surrounding editor.

Yes, that's exactly the issue. I tried it without removing the default StyleClass, and this is the result.

image

@calixtus
Copy link
Member

calixtus commented Mar 4, 2024

Maybe sthg like this works?

keywordTagsField.getEditor().prefHeightProperty().bind(this.heightProperty());

@calixtus
Copy link
Member

calixtus commented Mar 4, 2024

If not, restore the styleclass and we'll merge. Codewise looks already good to me.

@LoayGhreeb
Copy link
Collaborator Author

If not, restore the styleclass and well merge. Codewise looks already good to me.

It doesn't work correctly it expands while writing in the editor.

@calixtus calixtus added this pull request to the merge queue Mar 5, 2024
Merged via the queue into JabRef:main with commit 8374693 Mar 5, 2024
20 checks passed
@LoayGhreeb LoayGhreeb deleted the fix-for-issue-10560 branch March 5, 2024 00:32
@andr-groth
Copy link

The new tag system ignores hierarchical keywords. Only the parent keyword is shown.

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.

Bring back content selectors
5 participants