Skip to content

fix: fix content selector for custom fields#7508

Merged
Siedlerchr merged 3 commits into
JabRef:masterfrom
yinpeiqi:fix-for-issue-6819
Mar 10, 2021
Merged

fix: fix content selector for custom fields#7508
Siedlerchr merged 3 commits into
JabRef:masterfrom
yinpeiqi:fix-for-issue-6819

Conversation

@yinpeiqi

@yinpeiqi yinpeiqi commented Mar 9, 2021 •
edited by Siedlerchr
Loading

Copy link
Copy Markdown
Contributor

We Fix the issue that Content selector does not seem to work for custom fields. This issue actually do not affect the correctness of running and just show a ugly output in the frontend. So I simply change the 'toString()' method in UnkownField class. This method do not make any effect to other functions but the user interface shown in the issue.

fixes #6819

  • 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 documentation: Is the information available and up to date? If not created an issue at https://github.com/JabRef/user-documentation/issues or, even better, submitted a pull request to the documentation repository.

image

@tobiasdiez

tobiasdiez commented Mar 9, 2021

Copy link
Copy Markdown
Member

Thanks for your contribution! Very much appreciated.

You are already on the right track. However, instead of changing toString, which should be exclusively used for debugging, the dialog should use

jabref/src/main/java/org/jabref/model/entry/field/Field.java

Line 19 in 9aad6fc

default String getDisplayName() {

to display the field name. This would also improve the display of standard fields, e.g. Keyword instead of KEYWORD.

@tobiasdiez tobiasdiez added the status: changes-required Pull requests that are not yet complete label Mar 9, 2021
@yinpeiqi

yinpeiqi commented Mar 9, 2021

Copy link
Copy Markdown
Contributor Author

@tobiasdiez Thanks for your reviewing. Currently I get no idea on how to use getDisplayName() without any changes on toString(). Is there any other method to approach it? Thanks!

@tobiasdiez

tobiasdiez commented Mar 9, 2021

Copy link
Copy Markdown
Member

The code

jabref/src/main/java/org/jabref/gui/contentselector/ContentSelectorDialogView.java

Lines 69 to 70 in 5855bcf

initListView(fieldsListView, viewModel::getFieldNamesBackingList);
viewModel.selectedFieldProperty().bind(fieldsListView.getSelectionModel().selectedItemProperty());

says implicitly "please use toString" to render the Field. You can change this behavior by using a custom cell factory as its done for example here:

jabref/src/main/java/org/jabref/gui/preferences/preview/PreviewTab.java

Lines 120 to 122 in 5a11412

new ViewModelListCellFactory<PreviewLayout>()
.withText(PreviewLayout::getDisplayName)
.install(availableListView);

@yinpeiqi

yinpeiqi commented Mar 9, 2021

Copy link
Copy Markdown
Contributor Author

Thanks for your guidance!

@yinpeiqi

yinpeiqi commented Mar 10, 2021

Copy link
Copy Markdown
Contributor Author

I finished the behavior you mentioned above. Now the program can show display name without faults occurring in debug.

tobiasdiez
tobiasdiez approved these changes Mar 10, 2021
View reviewed changes

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

Looks good to me. Again, thanks a lot for your contribution!

@yinpeiqi

yinpeiqi commented Mar 10, 2021

Copy link
Copy Markdown
Contributor Author

You are welcome!

@tobiasdiez tobiasdiez added status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers and removed status: changes-required Pull requests that are not yet complete labels Mar 10, 2021
@Siedlerchr Siedlerchr changed the title fix: fix for issue 6819 fix: fix content selector for custom fields Mar 10, 2021
@Siedlerchr

Siedlerchr commented Mar 10, 2021

Copy link
Copy Markdown
Member

Thanks a lot for your contribution!

@Siedlerchr Siedlerchr merged commit c7593c9 into JabRef:master Mar 10, 2021
@yinpeiqi yinpeiqi deleted the fix-for-issue-6819 branch March 11, 2021 01:44
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

@tobiasdiez tobiasdiez tobiasdiez approved these changes

Assignees

No one assigned

Labels

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.

Content selector does not seem to work for custom fields

3 participants

@yinpeiqi @tobiasdiez @Siedlerchr