Skip to content

User specific comment#9727

Merged
Siedlerchr merged 37 commits into
JabRef:mainfrom
NiD133:user_specific_comment
Jun 6, 2023
Merged

User specific comment#9727
Siedlerchr merged 37 commits into
JabRef:mainfrom
NiD133:user_specific_comment

Conversation

@NiD133

@NiD133 NiD133 commented Apr 2, 2023

Copy link
Copy Markdown
Contributor

Fixes #543
1
2
3

### Compulsory checks
- [x] Change in `CHANGELOG.md` described in a way that is understandable for the average user (if applicable)
- [ ] Tests created for changes (if applicable)
- [x] Manually tested changed features in running JabRef (always required)
- [x] Screenshots added in PR description (for UI changes)
- [ ] [Checked developer's documentation](https://devdocs.jabref.org/): Is the information available and up to date? If not, I outlined it in this pull request.
- [ ] [Checked documentation](https://docs.jabref.org/): 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.

@koppor

koppor commented Apr 4, 2023

Copy link
Copy Markdown
Member

If possible, the new editor should also support editing org.jabref.model.entry.BibEntry#commentsBeforeEntry

koppor
koppor requested changes Apr 4, 2023
View reviewed changes

@koppor koppor left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

A good start 👍

Test cases are missing.

Comment thread src/main/java/org/jabref/model/entry/field/UserSpecificCommentField.java Outdated

public class UserSpecificCommentField implements Field {
private final String name;
private final Set<FieldProperty> properties;

@koppor koppor Apr 4, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This can be static final

Comment thread src/main/java/org/jabref/model/entry/field/UserSpecificCommentField.java Outdated
Comment thread src/main/java/org/jabref/model/entry/field/UserSpecificCommentField.java Outdated
Comment thread src/main/java/org/jabref/gui/entryeditor/OtherFieldsTab.java Outdated
Set<Field> otherFields = entry.getFields().stream().filter(field -> !allKnownFields.contains(field)).collect(Collectors.toCollection(LinkedHashSet::new));

Set<Field> otherFields = entry.getFields().stream()
.filter(field -> !allKnownFields.contains(field) && !field.getName().toLowerCase().contains("comment"))

@koppor koppor Apr 4, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It should be startsWith and comment-, because UserSpecificCommentField.java is implemented like that.

Maybe also check for plain comment in addition.

@koppor koppor Apr 4, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why does the following not work here?

(that code is from above)

   field -> field.equals(StandardField.COMMENT) || field instanceof UserSpecificCommentField

Comment thread src/main/java/org/jabref/gui/entryeditor/EntryEditor.java Outdated
Comment thread src/main/java/org/jabref/gui/entryeditor/CommentsTab.java Outdated
Comment thread CHANGELOG.md Outdated
@koppor koppor mentioned this pull request Apr 27, 2023
Open
@Siedlerchr

Siedlerchr commented May 6, 2023

Copy link
Copy Markdown
Member

Please have a look at the failing unit tests, most of them are related to the l10n https://devdocs.jabref.org/code-howtos/localization.html

@Siedlerchr Siedlerchr added the status: changes-required Pull requests that are not yet complete label May 6, 2023
@NiD133

NiD133 commented May 20, 2023

Copy link
Copy Markdown
Contributor Author

Please have a look at the failing unit tests, most of them are related to the l10n https://devdocs.jabref.org/code-howtos/localization.html
Got it! Working on it right now.

@Siedlerchr

Siedlerchr commented May 23, 2023

Copy link
Copy Markdown
Member

Please have a look at the failing tests!

@Siedlerchr

Siedlerchr commented Jun 3, 2023

Copy link
Copy Markdown
Member

Thanks for the continuing work here! What is the current status`Is this ready from your side? Anything left or unclear?

@NiD133

NiD133 commented Jun 4, 2023

Copy link
Copy Markdown
Contributor Author

Thanks for the continuing work here! What is the current status`Is this ready from your side? Anything left or unclear?

Yeah, I believe it's ready from my side. If there are any further changes or clarifications needed, please let me know. :)

@koppor koppor force-pushed the user_specific_comment branch 2 times, most recently from 30e01c4 to 0d7da65 Compare June 6, 2023 05:26
@koppor

koppor commented Jun 6, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

I made updates:

  • Merge "All Comments" into "Comments" tab. Thereby modifying the logic of the entry editor to prioritize the code over user-configuration 😇
  • Add more tests

@koppor

koppor commented Jun 6, 2023

Copy link
Copy Markdown
Member

In principle, this works. If one modifies the BibTeX source code, one has to switch to another entry and back to get the change updated in the Comments tab. I did not see an easy solution for this. Thus, it is good to go from my side!

@koppor koppor added status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers status: changes-required Pull requests that are not yet complete and removed status: changes-required Pull requests that are not yet complete labels Jun 6, 2023
@koppor koppor marked this pull request as ready for review June 6, 2023 06:09
@koppor koppor removed the status: changes-required Pull requests that are not yet complete label Jun 6, 2023
@NiD133

NiD133 commented Jun 6, 2023

Copy link
Copy Markdown
Contributor Author

I made updates:

  • Merge "All Comments" into "Comments" tab. Thereby modifying the logic of the entry editor to prioritize the code over user-configuration 😇
  • Add more tests

Got it!

@Siedlerchr

Siedlerchr commented Jun 6, 2023

Copy link
Copy Markdown
Member

Thanks a lot for this cool feature!

@Siedlerchr Siedlerchr merged commit 76e2ce7 into JabRef:main Jun 6, 2023
@koppor koppor mentioned this pull request Jun 14, 2023
Merged
6 tasks
@tobiasdiez tobiasdiez mentioned this pull request Sep 27, 2023
Closed
2 tasks
@koppor

koppor commented Oct 31, 2023

Copy link
Copy Markdown
Member

What we oversaw during review: Ordering of fields.

First, the bibtex standard field should come, then the user-specific comment fields. Since they are mixed, users are very confused and angry (see #10424). - I don't remember any other occasion where we have three thumbs ups ^^

I tried to fix it at #10610.

Just as information to @NiD133 if you work on UX in the future in other projects.

Siedlerchr added a commit that referenced this pull request Jan 1, 2024
@Siedlerchr
Merge remote-tracking branch 'upstream/main' into controlsfx-tags
3512118 
* upstream/main: (68 commits)
  Fix issue 9863 - Change Tab selection order (#9907)
  New Crowdin updates (#9994)
  Enable editing typo with double clicking on field name in custom entry type (#9977)
  Remove "Field name:" from localization - we already have "Field name" (#9991)
  Changed database to catalog in org.jabref.gui.slr and org.jabref.logic.crawler (#9989)
  Use environment variables for hostname detection (#9910)
  Add cleanup activity for URL field (#9970)
  Fix freezing when fetching IBSN and no results are found (#9987)
  Make Group(Node)TreeViewModel more OO (#9978)
  Fix container for group item count still visible if display count is off  (#9980)
  Fix paste of BibTeX data (#9985)
  Bring back SimplifyBoolean* and UnnecessaryParantheses (and refine guide) (#9981)
  Add new menu entry to remove braces from selection, aka unprotect it. (#9968)
  User specific comment (#9727)
  Bump org.openrewrite.recipe:rewrite-recipe-bom from 1.19.3 to 2.0.0 (#9975)
  Fix typos
  Remove non-needed link
  Fix typos
  Enable RemoveJavaDocAuthorTag (#9973)
  Bump com.fasterxml.jackson.datatype:jackson-datatype-jsr310 (#9974)
  ...
@koppor koppor mentioned this pull request Mar 12, 2024
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Siedlerchr Siedlerchr Siedlerchr approved these changes

@koppor koppor koppor approved these changes

Assignees

No one assigned

Labels

component: entry-editor status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add user-specific comment field

4 participants

@NiD133 @koppor @Siedlerchr @ThiloteE