Skip to content

Fixed the inconsistent behavior for accents in search#8640

Merged
Siedlerchr merged 4 commits into
JabRef:mainfrom
LingZhang22:fix-issue-6815
Apr 8, 2022
Merged

Fixed the inconsistent behavior for accents in search#8640
Siedlerchr merged 4 commits into
JabRef:mainfrom
LingZhang22:fix-issue-6815

Conversation

@LingZhang22

@LingZhang22 LingZhang22 commented Apr 4, 2022

Copy link
Copy Markdown
Contributor

Currently, the characters with accents are not fully supported in the search functionality as described in #6815. This PR aims to solve it.

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

@LingZhang22 LingZhang22 mentioned this pull request Apr 4, 2022
Closed
@Siedlerchr

Siedlerchr commented Apr 4, 2022

Copy link
Copy Markdown
Member

I just noticed that we have a method stripAccents in our StringUtil class org.jabref.model.strings;
Does that work as well?

koppor
koppor requested changes Apr 4, 2022
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.

Please add test cases to https://github.com/JabRef/jabref/blob/main/src/test/java/org/jabref/model/strings/StringUtilTest.java to check whether stripAccents works.

Note that this method calls https://commons.apache.org/proper/commons-lang/apidocs/org/apache/commons/lang3/StringUtils.html#stripAccents-java.lang.String-, which should be similar to the one of the JDK

Comment thread src/main/java/org/jabref/model/search/rules/SearchRule.java Outdated
Comment on lines +16 to +20
// convert the given string to ASCII string
static String convertToASCII(String string) {
String normalizedString = Normalizer.normalize(string, Normalizer.Form.NFD);
return normalizedString.replaceAll("[^\\p{ASCII}]", "");
}

@koppor koppor Apr 4, 2022

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.

Similar to @Siedlerchr's comment: Move such helper method to StringUtils.

There is IMHO need to convert SearchRule from an interface to a class.

@LingZhang22 LingZhang22 Apr 5, 2022 •
edited
Loading

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 created the tests in the StringUtilsTest class and changed the SearchRule back to interface.

Comment thread CHANGELOG.md Outdated
- We fixed an issue where opening the changelog from withing JabRef led to a 404 error [#8563](https://github.com/JabRef/jabref/issues/8563)
- We fixed an issue where not all found unlinked local files were imported correctly due to some race condition. [#8444](https://github.com/JabRef/jabref/issues/8444)
- We fixed an issue where Merge entries dialog exceeds screen boundaries.
- We fixed an issue where accent search does not perform consistently. [#6815]([https://github.com/JabRef/jabref/issues/6815])

@koppor koppor Apr 4, 2022

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 correct markdown syntax

Suggested change
- We fixed an issue where accent search does not perform consistently. [#6815]([https://github.com/JabRef/jabref/issues/6815])
- We fixed an issue where accent search does not perform consistently. [#6815](https://github.com/JabRef/jabref/issues/6815)

@LingZhang22 LingZhang22 Apr 5, 2022

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 deleted the "[]". Thanks!

@koppor

koppor commented Apr 4, 2022

Copy link
Copy Markdown
Member

Proposal for continuing:

Start from the main branch, add test cases. Then work on the new main; meanwhile we merged #8636, which reorganizes to code a bit.

@LingZhang22

LingZhang22 commented Apr 5, 2022

Copy link
Copy Markdown
Contributor Author

I just noticed that we have a method stripAccents in our StringUtil class org.jabref.model.strings; Does that work as well?

Thanks! I will use the stripAccents method.

@LingZhang22

LingZhang22 commented Apr 5, 2022

Copy link
Copy Markdown
Contributor Author

Proposal for continuing:

Start from the main branch, add test cases. Then work on the new main; meanwhile we merged #8636, which reorganizes to code a bit.

Thanks for the info. I reset the branch and added the changes to the current main.

@Siedlerchr Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Apr 8, 2022
calixtus
calixtus approved these changes Apr 8, 2022
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.

Codeweise Looks good to me!

@Siedlerchr

Siedlerchr commented Apr 8, 2022

Copy link
Copy Markdown
Member

Thanks for the follow up! LGTM!

@Siedlerchr Siedlerchr merged commit a6489cb into JabRef:main Apr 8, 2022
Siedlerchr added a commit that referenced this pull request Apr 11, 2022
@Siedlerchr
Merge remote-tracking branch 'upstream/main' into dependabot/gradle/c…
dcac8a4 
…om.github.tomtung-latex2unicode_2.12-0.3.0

* upstream/main:
  Fixed the inconsistent behavior for accents in search (#8640)
  Only show exit warning if running tasks will not be recovered (#8651)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@koppor koppor koppor requested changes

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

4 participants

@LingZhang22 @Siedlerchr @koppor @calixtus