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

Add drop down for citation key patterns #12516

Merged
merged 19 commits into from
Feb 21, 2025

Conversation

priyanshu16095
Copy link
Contributor

@priyanshu16095 priyanshu16095 commented Feb 16, 2025

Fixes #12502

This PR introduces a functionality to display a drop-down list of matching suggestions when typing a citation key pattern.

Mandatory checks

  • I own the copyright of the code submitted and I licence 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 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.

Screenshot (130)

@koppor
Copy link
Member

koppor commented Feb 16, 2025

Screenshot?

@priyanshu16095
Copy link
Contributor Author

priyanshu16095 commented Feb 16, 2025

Recording.2025-02-16.mp4

The positioning of list is not perfect, it comes on top of it, else works fine and the list of patterns is hardcoded.

Copy link
Member

@subhramit subhramit left a comment

Choose a reason for hiding this comment

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

I know it's a draft, but requesting a minor change.
Really good work so far! Thanks for picking this up.

ObservableList<String> fullData = FXCollections.observableArrayList(
"[auth]", "[authFirstFull]", "[authForeIni]", "[auth.etal]", "[authEtAl]",
"[auth.auth.ea]", "[authors]", "[authorsN]", "[authIniN]", "[authN]", "[authN_M]",
"[authorIni]", "[authshort]", "[authorsAlpha]");
Copy link
Member

@subhramit subhramit Feb 16, 2025

Choose a reason for hiding this comment

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

Also add [authorsAlphaLNI] (added in #12499).
Also, the issue asks for special field markers, which also includes Editor-related field markers, Title-related field markers, Other field markers and Bibentry fields apart from the author-related ones. Please add them as well.

Copy link
Member

Choose a reason for hiding this comment

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

We should either think how to auto-populate this list - or add a Java comment on how to populate the list manually.

Copy link
Member

@subhramit subhramit Feb 17, 2025

Choose a reason for hiding this comment

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

Yes, @priyanshu16095 - lets make this neat.
This is what I brainstormed, need a second opinion from @Siedlerchr and @koppor so that we know this is the best way and doesn't break things:
There is a record class, CitationKeyPatterns.java.

  1. Modify it to make it a final class.
  2. Hardcode the patterns as constants there. Leave proper comments with //region - citation key patterns offered. for any new pattern, add them here and // endregion below.
  3. Make a public static function getAllPatterns, see if this works (I chatgpt'd this snippet - make any modifications needed):
Arrays.stream(CitationKeyPatterns.class.getDeclaredFields())
             .filter(field -> {
                 int modifiers = field.getModifiers();
                 return Modifier.isPublic(modifiers) && 
                        Modifier.isStatic(modifiers) && 
                        Modifier.isFinal(modifiers) &&
                        field.getType() == CitationKeyPattern.class &&
                        field != CitationKeyPattern.NULL_CITATION_KEY_PATTERN;
             })
             .map(field -> {
                 try {
                     return (CitationKeyPattern) field.get(null);
                 } catch (IllegalAccessException e) {
                     throw new RuntimeException("Could not access field", e);
                 }
             })
             .collect(Collectors.toList());
  1. Use that list returned in your observable array list

This way, the developer has to just change one line when adding a new pattern, without the overhead of finding the list where patterns are populated, or just using raw strings or using constants (you have to put them everywhere otherwise).

Copy link
Member

Choose a reason for hiding this comment

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

Actually no need to even change the class type. You can add the method in the record itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure! Great suggestions, thanks!

@priyanshu16095
Copy link
Contributor Author

priyanshu16095 commented Feb 17, 2025

Recording.2025-02-18.mp4

For CitationKeyPatternSuggestionCell, I found this solution on Stack Overflow and a similar gist on Github Gist and modified it.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Your code currently does not meet JabRef's code guidelines. We use OpenRewrite to ensure "modern" Java coding practices. The issues found can be automatically fixed. Please execute the gradle task rewriteRun, check the results, commit, and push.
You can check the detailed error output by navigating to your pull request, selecting the tab "Checks", section "Tests" (on the left), subsection "OpenRewrite".

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Your code currently does not meet JabRef's code guidelines. We use Checkstyle to identify issues. Please carefully follow the setup guide for the codestyle. Afterwards, please run checkstyle locally and fix the issues.
In case of issues with the import order, double check that you activated Auto Import. You can trigger fixing imports by pressing Ctrl+Alt+O to trigger Optimize Imports.

@priyanshu16095
Copy link
Contributor Author

There is a problem with using the context menu because when it reaches the bottom of the screen, it shifts upward, making the UI buggy.
Instead, we can use a VBox or any other suggestions?

@koppor
Copy link
Member

koppor commented Feb 18, 2025

There is a problem with using the context menu because when it reaches the bottom of the screen, it shifts upward, making the UI buggy. Instead, we can use a VBox or any other suggestions?

If you don't get an answer quickly, just assume that nobody has a clue and will need invest time to investigate. Thus, go ahead and try.

Note that we could accept a different UI using a popup and buttons -similar to "Select entry type" -- if this works better

grafik


With this, we could even recommend certain citation key paterns.

@priyanshu16095
Copy link
Contributor Author

Screenshot (126)

The feature is now complete.
For the last element, it works fine, and the context menu does not change its position.

@subhramit subhramit marked this pull request as ready for review February 20, 2025 19:19
Co-authored-by: Subhramit Basu Bhowmick <[email protected]>
@subhramit subhramit added status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers component: citationkey-generator labels Feb 20, 2025

private void populatePopup(List<String> searchResult) {
List<CustomMenuItem> menuItems = new ArrayList<>();
int maxEntries = 7;
Copy link
Member

@subhramit subhramit Feb 20, 2025

Choose a reason for hiding this comment

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

Move this to a class-level constant with a brief comment explaining its use and the value

subhramit
subhramit previously approved these changes Feb 21, 2025
Copy link
Member

@subhramit subhramit left a comment

Choose a reason for hiding this comment

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

Tested it, works fine.
Code-wise too I can see everything I asked for.

Someone a bit deeper into javafx should take a closer look at the gui files.

@priyanshu16095
Copy link
Contributor Author

I found this solution on Stack Overflow for GUI.

@subhramit
Copy link
Member

Update - tested a case - in case of compound patterns like [auth]_[year], however, the dropdown doesn't come except when typing the first [auth].
See comment #12502 (comment).
If this is possible to do, that would complete it, else too I guess it's better than not having it so I keep my approval.

@priyanshu16095
Copy link
Contributor Author

Will do. I'll ensure [auth]_[year] triggers the dropdown.

@subhramit subhramit dismissed their stale review February 21, 2025 09:45

WIP (extended)

@priyanshu16095
Copy link
Contributor Author

priyanshu16095 commented Feb 21, 2025

Now it works as expected.

Screenshot (131)

I have a suggestion, we can use something like this to further improve the user experience.

Screenshot (130)

@priyanshu16095
Copy link
Contributor Author

This pattern [auth]_[year] does not exist in the Java record.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Your code currently does not meet JabRef's code guidelines. We use Checkstyle to identify issues. Please carefully follow the setup guide for the codestyle. Afterwards, please run checkstyle locally and fix the issues.
In case of issues with the import order, double check that you activated Auto Import. You can trigger fixing imports by pressing Ctrl+Alt+O to trigger Optimize Imports.

@Siedlerchr
Copy link
Member

Wow this is a really epic feature! Looks great!

Siedlerchr
Siedlerchr previously approved these changes Feb 21, 2025
@Siedlerchr Siedlerchr added this pull request to the merge queue Feb 21, 2025
@Siedlerchr Siedlerchr removed this pull request from the merge queue due to a manual request Feb 21, 2025
@Siedlerchr
Copy link
Member

Please add a changelog entry!

Siedlerchr
Siedlerchr previously approved these changes Feb 21, 2025
@Siedlerchr Siedlerchr enabled auto-merge February 21, 2025 20:13
auto-merge was automatically disabled February 21, 2025 20:15

Head branch was pushed to by a user without write access

@Siedlerchr Siedlerchr added this pull request to the merge queue Feb 21, 2025
Merged via the queue into JabRef:main with commit 4401efd Feb 21, 2025
25 checks passed
@subhramit
Copy link
Member

This pattern [auth]_[year] does not exist in the Java record.

Yes, it is not supposed to, it is a compound pattern - what I meant was (as per #12502 (comment)) - the individual components of a compound pattern should trigger a dropdown. I have given an example. [auth] triggering a dropdown (as matches exist), _ not (as no matches) and [year] again individually triggering a matching dropdown, treating it as running text.

I like your solution more though, and hardcoding it into the record works as it's a common pattern. Any less used pattern can be manually typed anyway. The sub-dropdowns are an icing on the cake, so really good work. JabRef users will love this.

@priyanshu16095
Copy link
Contributor Author

Thanks for the clarification! I will try it in a follow-up PR.

@subhramit
Copy link
Member

subhramit commented Feb 21, 2025

Thanks for the clarification! I will try it in a follow-up PR.

Not a priority right now, maybe when you can't find things more striking to work on.

Also, to clarify:

The sub-dropdowns are an icing on the cake, so really good work. JabRef users will love this.

I was referring to what you did, not what is left :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: citationkey-generator status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add dropdown for selecting citation key pattern
4 participants