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.

Sign up for GitHub

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

Jump to bottom

Fix Issue Unable to remove multiline property from field #12040

Closed
wants to merge 8 commits into from
Closed

Fix Issue Unable to remove multiline property from field #12040

wants to merge 8 commits into from

Conversation

PressJump
Copy link

@PressJump PressJump commented Oct 20, 2024 •
edited
Loading

This pull request aims to fix the issue where the user is unable to remove a multi-line property from fields. This is done through the addition of a checkbox listener that also checks the multiline boxes in other entry types as they are by design global Ref: #11897 (comment). This not only gives visual feedback of the change to the user but also fixes the issue without refactoring CustomEntryTypesTabViewModel logic as the original issue stemmed from the for-loop inside storeSettings overriding boolean changes which was also originally mentioned in the issue.

To remove the multiline setting, I have to update it for all entry types.

#11897 (comment)

Setting the value of the field in all entry types utilizes this to fix the issue.

fields-jabref

Closes #11897

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

@PressJump PressJump marked this pull request as draft October 20, 2024 07:43
@PressJump PressJump mentioned this pull request Oct 20, 2024
Open
2 tasks
koppor
koppor requested changes Oct 20, 2024
View reviewed changes
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.

Besides the code duplication issue, I would recommend to dive into TestFX - see annotation @GUITest to simulate clicks of users and the expected changes.

src/main/java/org/jabref/gui/preferences/customentrytypes/CustomEntryTypesTab.java Outdated Show resolved Hide resolved
@Siedlerchr
Copy link
Member

Thanks for the fix! See the comment from @koppor and add a changelog entry

@Siedlerchr Siedlerchr added the status: changes required Pull requests that are not yet complete label Oct 20, 2024
@PressJump PressJump requested a review from koppor October 22, 2024 18:43
@PressJump PressJump marked this pull request as ready for review October 22, 2024 18:43
LoayGhreeb
LoayGhreeb previously approved these changes Oct 22, 2024
View reviewed changes
Copy link
Collaborator

@LoayGhreeb LoayGhreeb left a comment

Choose a reason for hiding this comment

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

Thanks for the fix! Tested it out and looks good.

@LoayGhreeb LoayGhreeb changed the title [WIP] Fix Issue Unable to remove multiline property from field Fix Issue Unable to remove multiline property from field Oct 22, 2024
@PressJump PressJump requested a review from koppor October 26, 2024 11:54
koppor
koppor requested changes Oct 26, 2024
View reviewed changes
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.

Does not work at all.

  1. Reset preferences

  2. Check entry types "Book" and "InProceedings". "Author"

image

image

  1. I set "Author" muliline for InProcedings.

image

  1. I click "Save"

  2. I open "Book"

Author is multiline 😭😭😭

image

Thus, I really ask you to think about the code and the comment at #11897 (comment).

@koppor koppor marked this pull request as draft October 26, 2024 20:25
@PressJump
Copy link
Author

PressJump commented Oct 28, 2024 •
edited
Loading

Hey @koppor, thank you for your review and I am sorry that this issue has dragged out this long. I would like to make sure I am on the same page before I update this pull request. My original thought while working on this issue was that multi-line fields were global between entry types per comment #11897 (comment) saying

As a general hint, fields are global and shared among the entry types.

If I understand correctly from your comment and provided images, the expected behavior is for multiline fields to act like required fields and be entry type independent than global? So any change made in "In Proceedings" would not affect "Book". Which makes sense why my work on this pull request needs change.

I actually thought this was the case originally per my comment #11897 (comment) detailing the changes that would be required. This is also why I backtracked on comment #11897 (comment) as the concern I had is that FieldPreferences and CLIPreferences would require significant modification on how Fields are Stored as they are currently stored as a string list but would need to be saved as a key-value pair as "FieldName", "Value".

Thank you for your helpful review and support.

koppor
koppor requested changes Oct 28, 2024
View reviewed changes
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.

Thank you for your comment. The JavaDoc / code comments should include this knowledge at "common" places. I gave you hints.

src/main/java/org/jabref/gui/preferences/customentrytypes/CustomEntryTypesTab.java
@@ -176,7 +177,8 @@ private void setupFieldsTable() {
makeRotatedColumnHeader(fieldTypeColumn, Localization.lang("Required"));

fieldTypeMultilineColumn.setCellFactory(CheckBoxTableCell.forTableColumn(fieldTypeMultilineColumn));
fieldTypeMultilineColumn.setCellValueFactory(item -> item.getValue().multilineProperty());
// The listener createMultilinePropertyListener updates all fields with the same name in all entry types otherwise multiline property is not updated
Copy link
Member

Choose a reason for hiding this comment

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

Move this as JavaDoc to the method createMultilinePropertyListener.

src/main/java/org/jabref/gui/preferences/customentrytypes/CustomEntryTypesTab.java
private BooleanProperty createMultilinePropertyListener(TableColumn.CellDataFeatures<FieldViewModel, Boolean> item) {
BooleanProperty property = item.getValue().multilineProperty();
property.addListener((obs, wasSelected, isSelected) -> {
viewModel.entryTypes().forEach(typeViewModel -> {
Copy link
Member

Choose a reason for hiding this comment

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

Add doc that all types need to be changed as the standard fields are global for each entry type.

@koppor
Copy link
Member

koppor commented Dec 9, 2024

Closing this issue due to inactivity 💤

@koppor koppor closed this Dec 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@calixtus calixtus calixtus left review comments

@koppor koppor koppor requested changes

@LoayGhreeb LoayGhreeb LoayGhreeb left review comments

Assignees
No one assigned
Labels
status: changes required Pull requests that are not yet complete
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Entry types preferences: Unable to remove multiline property from field
5 participants
@PressJump @Siedlerchr @koppor @calixtus @LoayGhreeb