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

10984:Improve TagsField of keywords #11853

Conversation

MrunalKashid02
Copy link

@MrunalKashid02 MrunalKashid02 commented Sep 29, 2024

Fixes #10984

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.

@Siedlerchr
Copy link
Member

You need to merge upstream/main in your branch and resolve the merge conflicts

@MrunalKashid02
Copy link
Author

Merge conflicts because draft PR? or else

@Siedlerchr
Copy link
Member

Merge conflicts happen when the same code was edited by different people:
https://www.atlassian.com/git/tutorials/using-branches/merge-conflicts

You need to merge the upstream/main into your branch and then you can use Intellij to resolve them:
https://www.jetbrains.com/help/idea/resolve-conflicts.html

Copy link
Collaborator

@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.

Hi Mrunal. May I ask which AI you used to generate the code?

@MrunalKashid02
Copy link
Author

Is there anything wrong?

@subhramit
Copy link
Collaborator

Is there anything wrong?

Not with the fact that you considered contributing, but we don't appreciate it when PRs submitted have majority of the code generated by AI. It increases our effort to review and the candidate's effort to gather context about their own code and find fixes.

At JabRef, we strive to provide valuable learning experiences to contributors to help them grow their skills. The team invests significant time and resources, with a 2-to-1 ratio of senior developers mentoring each student. When a contribution lacks genuine effort, it not only affects maintainers' time but also takes away opportunities from other students who are eager to learn and contribute meaningfully.

@MrunalKashid02
Copy link
Author

I apologise for my mistake. will be ok if I again start by but it requires some time for each task because this is my first time for contribution. Once again sorry.

@MrunalKashid02
Copy link
Author

Please can if you give me some time to work on this issue.

@subhramit
Copy link
Collaborator

I apologise for my mistake. will be ok if I again start by but it requires some time for each task because this is my first time for contribution. Once again sorry.

It's alright. Try to understand what the code in your PR does properly and see if you can continue. If you are not confident about finishing it, you can ask to be unassigned and pick smaller issues to start with.

@MrunalKashid02
Copy link
Author

There are 1-4 task. I can do each task one by one this are small task I guess. if you feel that this issue is resolved quickly you may unassigned me. I really appreciate guidance and support.

@subhramit
Copy link
Collaborator

There are 1-4 task. I can do each task one by one this are small task I guess. if you feel that this issue is resolved quickly you may unassigned me. I really appreciate guidance and support.

If you can complete any sub-task of the issue fully, that's also fine.

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.
The tool reviewdog already placed comments on GitHub to indicate the places. See the tab "Files" in you PR.
Please carefully follow the setup guide for the codestyle.
Afterwards, please run checkstyle locally and fix the issues.

You can check review dog's comments at the tab "Files changed" of your pull request.

@MrunalKashid02 MrunalKashid02 marked this pull request as ready for review October 5, 2024 03:26
Bindings.bindContentBidirectional(keywordTagsField.getTags(), viewModel.keywordListProperty());
}

private void handlePasteAction() {
Clipboard clipboard = Clipboard.getSystemClipboard();
Copy link
Member

Choose a reason for hiding this comment

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

Use JabRef's ClipBoardManager with getcontents

String[] keywords = pastedText.split(String.valueOf(viewModel.getKeywordSeparator()));

for (String keywordText : keywords) {
Keyword keyword = viewModel.getStringConverter().fromString(keywordText.trim());
Copy link
Member

Choose a reason for hiding this comment

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

Use Keyword.of


// Add keyword if it's valid and not already present
if (keyword != null && !keywordTagsField.getTags().contains(keyword)) {
keywordTagsField.getTags().add(keyword);
Copy link
Member

Choose a reason for hiding this comment

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

You should not operate here on the keywordTagsField directly, instead use the viewModel.keywordListProperty
and simply call add (it already performs a contains check)

Copy link
Author

Choose a reason for hiding this comment

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

ok i will make changes

@Siedlerchr Siedlerchr added the status: changes required Pull requests that are not yet complete label Oct 7, 2024
@Siedlerchr
Copy link
Member

@MrunalKashid02 How is the status here? Would be great if you could address the comments and finish this

@MrunalKashid02
Copy link
Author

I am remain with some changes . I will commit changes tomorrow. Will it fine ?

@koppor
Copy link
Member

koppor commented Oct 16, 2024

I am remain with some changes . I will commit changes tomorrow. Will it fine ?

Sure. We just need to know whether there is any more activity intended. Otherwise, we will close the PR and look for another contributor.

String pastedText = ClipBoardManager.getContents();

if (!pastedText.isEmpty()) {
String[] keywords = pastedText.split(String.valueOf(viewModel.getKeywordSeparator()));
Copy link
Member

Choose a reason for hiding this comment

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

Please use org.jabref.model.entry.KeywordList

If it is not directly possible, because hierarchicalDelimiter is missing, use > as hierarchicalDelimiter.

Copy link
Author

Choose a reason for hiding this comment

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

Can elobarate more how this can be done

@@ -86,16 +93,84 @@ public KeywordsEditor(Field field,
event.consume();
}
});
keywordTagsField.getEditor().setOnKeyPressed(event -> {
if (event.isControlDown() && event.getCode() == KeyCode.V) {
Copy link
Member

Choose a reason for hiding this comment

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

this does not respect org.jabref.gui.keyboard.KeyBinding#PASTE. Please look at other codes how keyboard shortcuts are handled.

@@ -86,16 +93,84 @@ public KeywordsEditor(Field field,
event.consume();
}
});
keywordTagsField.getEditor().setOnKeyPressed(event -> {
if (event.isControlDown() && event.getCode() == KeyCode.V) {
handlePasteAction(); // Call paste handler
Copy link
Member

Choose a reason for hiding this comment

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

Remove code comments just repeating the method name. (This is an indication of AI-generated code...)

Copy link
Author

Choose a reason for hiding this comment

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

Yeah i made mistake previous by use ai. but the changes i am doing by understanding codebase and as per suggestion i didnt take help from ai for code. I will make changes.

});
keywordTagsField.getEditor().setOnContextMenuRequested(event -> {
ContextMenu contextMenu = new ContextMenu();
MenuItem pasteItem = new MenuItem("Paste");
Copy link
Member

Choose a reason for hiding this comment

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

Non-Localized. Please read at https://devdocs.jabref.org/code-howtos/localization.html how to Localize.

}
});
keywordTagsField.getEditor().setOnContextMenuRequested(event -> {
ContextMenu contextMenu = new ContextMenu();
Copy link
Member

Choose a reason for hiding this comment

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

This should be a class variable an initalized there?

@MrunalKashid02
Copy link
Author

I am getting this on mail what does it mean

Screenshot 2024-10-22 114558

@koppor
Copy link
Member

koppor commented Oct 27, 2024

I am getting this on mail what does it mean

That you forked JabRef and "our" workflows are running at your side, too.

You need to update the main branch in your fork, then the error should be gone!

@subhramit
Copy link
Collaborator

@MrunalKashid02 hey, are you still working on it? You can otherwise close the PR.

@MrunalKashid02
Copy link
Author

Yeah I am busy bcoz of exam I will update you tonight.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
keywords status: changes required Pull requests that are not yet complete
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve TagsField of keywords
5 participants