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.

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

Fix issue: Auto-linking files with safe character replacements #9267 #9316

Conversation

Rita-Zhou
Copy link
Contributor

@Rita-Zhou Rita-Zhou commented Oct 27, 2022

Fixes #9267

Short Description

This aims to fix the issue where it is impossible to find the related file when the citation key involves some characters (e.g. :) that cannot be contained in the file name.
We fix this issue by searching the related file with the citation key where its unsafe characters are replaced with underscores _. (See #9267)

UI changes

  • Input a citation key with an unsafe character (colon ":")

313210096_495396219314658_7636164207235418215_n

  • Before

312984263_535109675291077_2673210671895727303_n

  • After

312436604_797747644827893_4867990044154713722_n
Note: The user should reopen the library to see the attachment

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

@@ -65,8 +65,13 @@ public List<Path> findAssociatedFiles(BibEntry entry, List<Path> directories, Li
return result.stream().sorted().collect(Collectors.toList());
}

public static String getSafeCiteKey(String citeKey) {
// replace the unsafe character with the underscore.
return citeKey.replaceAll("[:/*?<>|]", "_");
Copy link
Member

Choose a reason for hiding this comment

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

You should use the method FileNameCleaner.cleanFileName because this method is also called when the Filename is built from the citation key (when you have rename to pattern)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you! I changed the citation key filter to FileNameCleaner!

@Siedlerchr Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Oct 27, 2022
Copy link
Member

@Siedlerchr Siedlerchr left a comment

Choose a reason for hiding this comment

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

Thanks, lgtm!

@Siedlerchr
Copy link
Member

Thank you for the pull request. In future, please use the GitHub syntax Fixes #9304. Otherwise, GitHub does not automatically close the linked issue.

@Siedlerchr
Copy link
Member

TODO: That looks good so far, but I am unsure atm if it is also necessary to have this for exact matches
Can you add a test case for this as well, if the preference option is "only exact key matches"?

@ThiloteE ThiloteE removed the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Oct 28, 2022
@xianghao-wang
Copy link
Contributor

TODO: That looks good so far, but I am unsure atm if it is also necessary to have this for exact matches Can you add a test case for this as well, if the preference option is "only exact key matches"?

Many thanks, I just now added a test for both "exact" and "startsWith" searches.

StartsWith match: check whether a file name starts with a string and this string's unsafe characters will be converted to _. Exact match: check whether a file name exactly matches a string without allowing character replacement.

A suggestion: I notice there are 3 searches for auto-linking files - 1. start with 2. exact 3. regex. I think it would be better to replace "startwith" with a new search called "fuzzy search" which can match part of the filename intelligently as suggested #9322 .

@xianghao-wang
Copy link
Contributor

xianghao-wang commented Oct 30, 2022

Sorry for forgetting to fix the code style and I just now fixed it.

Copy link
Member

@calixtus calixtus left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@Siedlerchr
Copy link
Member

Thanks again for your PR!
I am unsure about the exact matching, but would leave it as it for now. If it's still an issue or requested by the users we can do a follow up.

@Siedlerchr Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Oct 30, 2022
@Siedlerchr Siedlerchr merged commit 46b7339 into JabRef:main Oct 30, 2022
Siedlerchr added a commit to tomazari/jabref that referenced this pull request Nov 1, 2022
…ok-info.com-JabRef#9145

* upstream/main:
  New Crowdin updates (JabRef#9333)
  Refresh example styles
  Squashed 'buildres/csl/csl-styles/' changes from 4eee79a..13fd98e
  Bump unirest-java from 3.13.11 to 3.13.12 (JabRef#9330)
  Bump checkstyle from 10.3.4 to 10.4 (JabRef#9331)
  Bump gittools/actions from 0.9.14 to 0.9.15 (JabRef#9332)
  Group context menu presents relevant options depending on number of subgroups (JabRef#9286)
  Removed BibTeX file type and included HTML and Markdown types (JabRef#9318)
  Fix issue: Auto-linking files with safe character replacements JabRef#9267 (JabRef#9316)
  Fix for issue 8806: Button highlights doesn't respect rounded corners (JabRef#9320)
  New Crowdin updates (JabRef#9324)
  Update CHANGELOG.md
  Try to relocate listener binding (JabRef#9238)
  Changed the messages after importing unlinked local files to past passive tense. (JabRef#9308)
  Changed the color of found text from red to high contrast  (JabRef#9315)

# Conflicts:
#	CHANGELOG.md
Siedlerchr added a commit to DavidOWade/jabref that referenced this pull request Nov 3, 2022
* upstream/main: (30 commits)
  New translations JabRef_en.properties (German) (JabRef#9336)
  Fix Abbreviation for Escaped Ampersand (JabRef#9288)
  Fix font size preference not updating in preference dialog 8386 (JabRef#9287)
  New Crowdin updates (JabRef#9333)
  Refresh example styles
  Squashed 'buildres/csl/csl-styles/' changes from 4eee79a..13fd98e
  Bump unirest-java from 3.13.11 to 3.13.12 (JabRef#9330)
  Bump checkstyle from 10.3.4 to 10.4 (JabRef#9331)
  Bump gittools/actions from 0.9.14 to 0.9.15 (JabRef#9332)
  Group context menu presents relevant options depending on number of subgroups (JabRef#9286)
  Removed BibTeX file type and included HTML and Markdown types (JabRef#9318)
  Fix issue: Auto-linking files with safe character replacements JabRef#9267 (JabRef#9316)
  Fix for issue 8806: Button highlights doesn't respect rounded corners (JabRef#9320)
  New Crowdin updates (JabRef#9324)
  Update CHANGELOG.md
  Try to relocate listener binding (JabRef#9238)
  Changed the messages after importing unlinked local files to past passive tense. (JabRef#9308)
  Changed the color of found text from red to high contrast  (JabRef#9315)
  New Crowdin updates (JabRef#9317)
  Add skips for MacOS X build (JabRef#9305)
  ...
Siedlerchr added a commit that referenced this pull request Nov 8, 2022
* upstream/main:
  New Crowdin updates (#9333)
  Refresh example styles
  Squashed 'buildres/csl/csl-styles/' changes from 4eee79a..13fd98e
  Bump unirest-java from 3.13.11 to 3.13.12 (#9330)
  Bump checkstyle from 10.3.4 to 10.4 (#9331)
  Bump gittools/actions from 0.9.14 to 0.9.15 (#9332)
  Group context menu presents relevant options depending on number of subgroups (#9286)
  Removed BibTeX file type and included HTML and Markdown types (#9318)
  Fix issue: Auto-linking files with safe character replacements #9267 (#9316)
  Fix for issue 8806: Button highlights doesn't respect rounded corners (#9320)
  New Crowdin updates (#9324)
  Update CHANGELOG.md
  Try to relocate listener binding (#9238)
  Changed the messages after importing unlinked local files to past passive tense. (#9308)
  Changed the color of found text from red to high contrast  (#9315)
  New Crowdin updates (#9317)
  Add skips for MacOS X build (#9305)
  New Crowdin updates (#9248)
  Fix for issue 9304 (#9313)

# Conflicts:
#	CHANGELOG.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
external files status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Auto-linking files with safe character replacements
5 participants