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

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

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

* 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)
* upstream/main:
  Add some JavaDoc to Fetchers
  Support two argument form of \abx@aux@cite macro in DefaultAuxParser (#8549)
@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
…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
* 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
* 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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

unknown trigger for MalformedURLException and possible 'file' field disappearance from some entries
3 participants