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

merged 11 commits into from
Mar 7, 2022

Conversation

Siedlerchr
Copy link
Member

@Siedlerchr Siedlerchr commented Feb 20, 2022 •
edited
Loading

Fixes #8510

  • 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, I created an issue at https://github.com/JabRef/user-documentation/issues or, even better, I submitted a pull request to the documentation repository.

@Siedlerchr Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Feb 20, 2022
koppor
koppor requested changes Feb 20, 2022
View reviewed changes
src/main/java/org/jabref/logic/importer/util/FileFieldParser.java Outdated
@@ -22,7 +22,7 @@
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.

@Siedlerchr
Copy link
Member Author

i would like to merge this, as this is otherwise fails for a lot of users

@Siedlerchr
Merge remote-tracking branch 'upstream/main' into fixUrlLink
e21e236 
* upstream/main:
  Bump guava from 31.0.1-jre to 31.1-jre (#8543)
  Bump org.beryx.jlink from 2.24.4 to 2.25.0 (#8548)
  Bump postgresql from 42.3.2 to 42.3.3 (#8546)
  Bump richtextfx from 0.10.7 to 0.10.9 (#8547)
  Bump archunit-junit5-engine from 0.22.0 to 0.23.1 (#8545)
  Bump actions/checkout from 2 to 3 (#8542)
  Squashed 'buildres/csl/csl-styles/' changes from eb97405..8f69d4e
  Bump classgraph from 4.8.139 to 4.8.141 (#8535)
  Bump archunit-junit5-api from 0.22.0 to 0.23.1 (#8536)
  issue(8448): allow URL with + sign in entry tabs (#8508)
  Add initial telemetry development documentation (#8530)
  Add existing "Remote storage" to index
  Disable telemetry client due to incompatibilites with jakarta (#8526)
  Update to jakarta xml everywhere and update PDFbox (#8521)
  Bump flexmark-ext-gfm-tasklist from 0.62.2 to 0.64.0 (#8515)
  Bump xmlunit-core from 2.8.4 to 2.9.0 (#8519)
  Bump org.openjfx.javafxplugin from 0.0.11 to 0.0.12 (#8518)
  Bump mockito-core from 4.3.0 to 4.3.1 (#8520)
@Siedlerchr
remove check for colon
bda2458
@Siedlerchr
Merge remote-tracking branch 'upstream/main' into fixUrlLink
bc1495a 
* upstream/main:
  Add some JavaDoc to Fetchers
  Support two argument form of \abx@aux@cite macro in DefaultAuxParser (#8549)
@Siedlerchr
fix checkstyle
6a14556
@Siedlerchr
Copy link
Member Author

Fix olly's checkstyle...

@Siedlerchr Siedlerchr merged commit 0c439de into main Mar 7, 2022
@Siedlerchr Siedlerchr deleted the fixurllink branch March 7, 2022 21:58
Siedlerchr added a commit that referenced this pull request Mar 7, 2022
@Siedlerchr
Merge remote-tracking branch 'upstream/main' into dependabot/gradle/c…
dd798df 
…om.puppycrawl.tools-checkstyle-10.0

* upstream/main:
  Fix online link detection in entry editor (#8514)
  Add some JavaDoc to Fetchers
  Support two argument form of \abx@aux@cite macro in DefaultAuxParser (#8549)
  Bump guava from 31.0.1-jre to 31.1-jre (#8543)
  Bump org.beryx.jlink from 2.24.4 to 2.25.0 (#8548)
  Bump postgresql from 42.3.2 to 42.3.3 (#8546)
  Bump richtextfx from 0.10.7 to 0.10.9 (#8547)
  Bump archunit-junit5-engine from 0.22.0 to 0.23.1 (#8545)
  Bump actions/checkout from 2 to 3 (#8542)
Siedlerchr added a commit that referenced this pull request Mar 9, 2022
@Siedlerchr
Merge remote-tracking branch 'upstream/main' into fix-changed
ef1fd5c 
* upstream/main:
  Add Missing Fillers/Extractors for Supported Fields and Support Day Conversion (#8531)
  Bump checkstyle from 9.3 to 10.0 (#8544)
  Fix online link detection in entry editor (#8514)
  Add some JavaDoc to Fetchers
  Support two argument form of \abx@aux@cite macro in DefaultAuxParser (#8549)
  Bump guava from 31.0.1-jre to 31.1-jre (#8543)
  Bump org.beryx.jlink from 2.24.4 to 2.25.0 (#8548)
  Bump postgresql from 42.3.2 to 42.3.3 (#8546)
  Bump richtextfx from 0.10.7 to 0.10.9 (#8547)
  Bump archunit-junit5-engine from 0.22.0 to 0.23.1 (#8545)
  Bump actions/checkout from 2 to 3 (#8542)
  Squashed 'buildres/csl/csl-styles/' changes from eb97405..8f69d4e
  Bump classgraph from 4.8.139 to 4.8.141 (#8535)
  Bump archunit-junit5-api from 0.22.0 to 0.23.1 (#8536)

# Conflicts:
#	src/test/java/org/jabref/logic/xmp/XmpUtilReaderTest.java
Siedlerchr added a commit that referenced this pull request Mar 13, 2022
@Siedlerchr
Merge remote-tracking branch 'upstream/main' into updateCiteprocAlpha4
b751ffa 
* upstream/main:
  Update RemoteSetupTest.java, adding eq() function from mockito (#8561)
  readd encoding after merge
  Open office refactor finalization (formerly OObranch J cleanup) (#7795)
  Revert "Refine documentation (#8551)" (#8560)
  Refine documentation on logging (#8550)
  Revert "Refine documentation (#8551)" (#8559)
  Refine documentation (#8551)
  New Crowdin updates (#8557)
  New Crowdin updates (#8553)
  Fix missing metadata in BibDatabaseContext (#8556)
  Add encoding detection (and pin export to UTF-8) (#8506)
  Add Missing Fillers/Extractors for Supported Fields and Support Day Conversion (#8531)
  Bump checkstyle from 9.3 to 10.0 (#8544)
  Fix online link detection in entry editor (#8514)
  Add some JavaDoc to Fetchers
  Support two argument form of \abx@aux@cite macro in DefaultAuxParser (#8549)
@koppor koppor mentioned this pull request Mar 14, 2024
Open
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@calixtus calixtus calixtus left review comments

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

unknown trigger for MalformedURLException and possible 'file' field disappearance from some entries
3 participants
@Siedlerchr @koppor @calixtus