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 online link detection in entry editor #8514

Merged
merged 11 commits into from
Mar 7, 2022
Merged

Fix online link detection in entry editor #8514

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
4120332
Fix online link detection in entry editor
Siedlerchr Feb 20, 2022
db3eff5
add changelog
Siedlerchr Feb 20, 2022
0e3fbae
Fix typo
koppor Feb 20, 2022
ea5a231
update linked file
Siedlerchr Feb 21, 2022
e2ea992
Merge branch 'fixurllink' of github.com:JabRef/jabref into fixurllink
Siedlerchr Feb 21, 2022
48c44a5
fix test
Siedlerchr Feb 21, 2022
e21e236
Merge remote-tracking branch 'upstream/main' into fixUrlLink
Siedlerchr Mar 7, 2022
bda2458
remove check for colon
Siedlerchr Mar 7, 2022
bc1495a
Merge remote-tracking branch 'upstream/main' into fixUrlLink
Siedlerchr Mar 7, 2022
6a14556
fix checkstyle
Siedlerchr Mar 7, 2022
f451c3e
fix checkstyle javadoc
Siedlerchr Mar 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ Note that this project **does not** adhere to [Semantic Versioning](http://semve

### Fixed

- We fixed an issue where online links in the file field were not detected correctyl and could produce an exception [#8150](https://github.com/JabRef/jabref/issues/8510)
- We fixed an issue where an exception could occur when saving the preferences [#7614](https://github.com/JabRef/jabref/issues/7614)
- We fixed an issue where "Copy DOI url" in the right-click menu of the Entry List would just copy the DOI and not the DOI url. [#8389](https://github.com/JabRef/jabref/issues/8389)
- We fixed an issue where opening the console from the drop-down menu would cause an exception. [#8466](https://github.com/JabRef/jabref/issues/8466)
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/org/jabref/logic/importer/util/FileFieldParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ public static List<LinkedFile> parse(String value) {
return files;
}

if (LinkedFile.isOnlineLink(value.trim())) {
if (!value.trim().startsWith(":") && LinkedFile.isOnlineLink(value.trim())) {
Copy link
Member

Choose a reason for hiding this comment

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

I think, isOnlineLink should be fixed. The file field can contain a description. In case the description is This is an awesome/somesome file, the method will still break.

Maybe, we should work on a more intuitive file field - see #98 for a discussion.

Copy link
Member Author

@Siedlerchr Siedlerchr Feb 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This code part is only handling the edge case for non valid file links See #7948 for the PR and #7882 for the original issue.
All other things are handled further down

Copy link
Member Author

Choose a reason for hiding this comment

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

See also the tests

Copy link
Member Author

Choose a reason for hiding this comment

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

I switched to a regex with the condition to only match at the beginning. The contains www was a culprit

Copy link
Member

Choose a reason for hiding this comment

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

I believe this was originally chose for performance reasons. Can we somehow benchmark this?

Copy link
Member Author

Choose a reason for hiding this comment

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

The regex is compiled, I doubt it will be a huge problem.

Copy link
Member

Choose a reason for hiding this comment

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

Can we remove the `!value.trim().startsWith(":")? part?

It should be working without that check.

// needs to be modifiable
try {
return List.of(new LinkedFile(new URL(value), ""));
Expand Down
13 changes: 12 additions & 1 deletion src/test/java/org/jabref/logic/importer/util/FileFieldParserTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,11 +144,22 @@ private static Stream<Arguments> stringsToParseTestData() throws Exception {
"https://books.google.de/"
),

// url with www
Arguments.of(
Collections.singletonList(new LinkedFile(new URL("https://www.google.de/"), "")),
"https://www.google.de/"
),

// url as file
Arguments.of(
Collections.singletonList(new LinkedFile("", new URL("http://ceur-ws.org/Vol-438"), "URL")),
":http\\://ceur-ws.org/Vol-438:URL"
)
),
// url as file with desc
Arguments.of(
Collections.singletonList(new LinkedFile("desc", new URL("http://ceur-ws.org/Vol-438"), "URL")),
"desc:http\\://ceur-ws.org/Vol-438:URL"
)
);
}

Expand Down