Skip to content

Fixed error when specifying general fields by correcting unwanted characters list#6643

Merged
Siedlerchr merged 4 commits into
JabRef:masterfrom
alex-petrov-vt:fix-for-6604
Jul 1, 2020
Merged

Fixed error when specifying general fields by correcting unwanted characters list#6643
Siedlerchr merged 4 commits into
JabRef:masterfrom
alex-petrov-vt:fix-for-6604

Conversation

@alex-petrov-vt

@alex-petrov-vt alex-petrov-vt commented Jun 27, 2020 •
edited by calixtus
Loading

Copy link
Copy Markdown
Contributor

Fixes #6604 by correcting the list of unwanted characters

  • Change in CHANGELOG.md described (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.

Siedlerchr
Siedlerchr reviewed Jun 27, 2020
View reviewed changes
Comment thread src/main/java/org/jabref/gui/customizefields/CustomizeGeneralFieldsDialogViewModel.java

String testString = CitationKeyGenerator.cleanKey(parts[1], preferences.getUnwantedCharacters());
if (!testString.equals(parts[1]) || (parts[1].indexOf('&') >= 0)) {
String unwantedChars = "#{}()~,^&-\"'`ʹ\\";

@Siedlerchr Siedlerchr Jun 27, 2020

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.

Please use the unwanted chars from the preferences

@alex-petrov-vt alex-petrov-vt Jun 27, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I thought the problem was that the list of unwanted chars from the preferences did not correspond to the unwanted characters for this particular case. I.e. the characters in the log message below are not the same that are stored in preferences. For example, ';' character is fine in this case because it's used to separate fields, but it is on the list of unwanted characters in preferences

@Siedlerchr Siedlerchr Jun 27, 2020

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.

Ah okay, thanks for clarifying this.

@Siedlerchr Siedlerchr Jun 27, 2020 •
edited
Loading

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.

Please also add a changelog entry (the checkstyle issue is not related)

@alex-petrov-vt alex-petrov-vt Jun 27, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Okay, I'll do later on tonight

@Siedlerchr Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Jun 27, 2020
@calixtus

calixtus commented Jun 27, 2020

Copy link
Copy Markdown
Member

Would be nice, if you could also add a short clearifying comment, so the next time someone works on this, one won't reintroduce the bug here, and we don't have to dig into the git history.

calixtus
calixtus approved these changes Jun 30, 2020
View reviewed changes

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

Thanks for the follow-up.
Codewise looks good to me.

@calixtus

calixtus commented Jun 30, 2020 •
edited
Loading

Copy link
Copy Markdown
Member

The markdown linter still produces errors:

CHANGELOG.md:12 MD022/blanks-around-headings/blanks-around-headers Headings should be surrounded by blank lines [Expected: 1; Actual: 0; Below] [Context: "### Added"]
CHANGELOG.md:13 MD032/blanks-around-lists Lists should be surrounded by blank lines [Context: "- We improved responsiveness o..."]
docs/adr.md:5:26 MD042/no-empty-links No empty links [Context: "[template.md]()"]
docs/getting-into-the-code/code-howtos.md:32 MD031/blanks-around-fences Fenced code blocks should be surrounded by blank lines [Context: "```"]
docs/getting-into-the-code/code-howtos.md:48 MD036/no-emphasis-as-heading/no-emphasis-as-header Emphasis used instead of a heading [Context: "TODO: Usage of status bar and ..."]

The last three errors are not about your changes.

@alex-petrov-vt

alex-petrov-vt commented Jun 30, 2020

Copy link
Copy Markdown
Contributor Author

Sorrry about that, accidentally deleted empty line after section header. Hope this fixed it

@Siedlerchr Siedlerchr merged commit 593b235 into JabRef:master Jul 1, 2020
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

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

Can't edit "Set up general fields"

3 participants

@alex-petrov-vt @calixtus @Siedlerchr