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

Enable automatic URL linking (bug 1019475) #19110

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Enable automatic URL linking (bug 1019475) #19110

wants to merge 1 commit into from

Conversation

ryzokuken
Copy link
Collaborator

@ryzokuken ryzokuken commented Nov 26, 2024 •
edited
Loading

Automatically detect links in the text content of a file and automatically generate link annotations at the appropriate locations to achieve automatic link detection and hyperlinking.

References:

github-advanced-security[bot]
github-advanced-security bot found potential problems Nov 26, 2024
View reviewed changes
web/pdf_page_view.js Fixed Show fixed Hide fixed
Snuffleupagus
Snuffleupagus requested changes Nov 26, 2024
View reviewed changes
Copy link
Collaborator

@Snuffleupagus Snuffleupagus left a comment •
edited
Loading

Choose a reason for hiding this comment

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

How does this perform, especially in documents that contain a lot of text?

Also, we probably want a new option/preference to be able to disable this functionality.

web/pdf_page_view.js Outdated Show resolved Hide resolved
web/pdf_page_view.js Outdated Show resolved Hide resolved
web/pdf_page_view.js Outdated Show resolved Hide resolved
web/pdf_page_view.js Outdated Show resolved Hide resolved
web/pdf_page_view.js Outdated Show resolved Hide resolved
web/pdf_page_view.js Outdated Show resolved Hide resolved
web/annotation_layer_builder.js Outdated Show resolved Hide resolved
@ryzokuken ryzokuken force-pushed the autolink-demo branch 2 times, most recently from a7d0024 to f3737b5 Compare December 10, 2024 23:11
github-advanced-security[bot]
github-advanced-security bot found potential problems Dec 10, 2024
View reviewed changes
web/autolinker.js Fixed Show fixed Hide fixed
@ryzokuken ryzokuken force-pushed the autolink-demo branch 2 times, most recently from cad781f to 93e5417 Compare December 10, 2024 23:16
@ryzokuken
Copy link
Collaborator Author

@Snuffleupagus

Besides, it isn't necessary since the textContent is already available once the textLayer has rendered; see

I was a bit unsure if I understood exactly what you were suggesting but how does this commit look? It "fetches" the textContents from the previous render of the textLayer and makes the processing step sync.

93e5417

@ryzokuken
Copy link
Collaborator Author

@Snuffleupagus nvm my last comment, I figured it out after looking at pdfPageView._textHighlighter.textDivs a couple of times it occurred to me what you were talking about.

https://github.com/mozilla/pdf.js/pull/19110/files#diff-71772f56be799df522c2076ab5fa476253ef15c607af5812536c41696d97cd59R73

@ryzokuken ryzokuken marked this pull request as ready for review December 16, 2024 22:10
@calixteman calixteman changed the title Enable automatic URL linking Enable automatic URL linking (bug 1019475) Jan 7, 2025
@marco-c marco-c linked an issue Jan 7, 2025 that may be closed by this pull request
Enable automatic URL hyperlinking #3172
Open
github-advanced-security[bot]
github-advanced-security bot found potential problems Jan 8, 2025
View reviewed changes
web/autolinker.js Fixed Show fixed Hide fixed
github-advanced-security[bot]
github-advanced-security bot found potential problems Jan 14, 2025
View reviewed changes
web/autolinker.js Fixed Show fixed Hide fixed
nicolo-ribaudo
nicolo-ribaudo reviewed Jan 14, 2025
View reviewed changes
web/autolinker.js Outdated Show resolved Hide resolved
@ryzokuken ryzokuken force-pushed the autolink-demo branch 4 times, most recently from 864c116 to 6aee919 Compare January 16, 2025 12:06
github-advanced-security[bot]
github-advanced-security bot found potential problems Jan 16, 2025
View reviewed changes
web/autolinker.js Fixed Show resolved Hide resolved
calixteman
calixteman reviewed Jan 16, 2025
View reviewed changes
web/autolinker.js Outdated Show resolved Hide resolved
@ryzokuken ryzokuken force-pushed the autolink-demo branch 5 times, most recently from 94c6ad3 to 2c22395 Compare January 24, 2025 15:40
@ryzokuken
Copy link
Collaborator Author

@calixteman thanks for the pointers! @Snuffleupagus your concern should be addressed now, new profile at https://share.firefox.dev/42yU2NG.

@calixteman
Copy link
Contributor

/botio test

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)

Received

Command cmd_test from @calixteman received. Current queue size: 1

Live output at: http://54.241.84.105:8877/d13db077206261e/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)

Received

Command cmd_test from @calixteman received. Current queue size: 1

Live output at: http://54.193.163.58:8877/67cd02400dd579c/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)

Failed

Full output at http://54.241.84.105:8877/d13db077206261e/output.txt

Total script time: 29.33 mins

  • Unit tests: Passed
  • Integration Tests: FAILED
  • Regression tests: Passed

Snuffleupagus
Snuffleupagus requested changes Jan 29, 2025
View reviewed changes
web/annotation_layer_builder.js Outdated Show resolved Hide resolved
web/annotation_layer_builder.js Outdated Show resolved Hide resolved
web/annotation_layer_builder.js Outdated Show resolved Hide resolved
web/annotation_layer_builder.js Outdated Show resolved Hide resolved
web/annotation_layer_builder.js Outdated Show resolved Hide resolved
web/autolinker.js Outdated Show resolved Hide resolved
@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)

Failed

Full output at http://54.193.163.58:8877/67cd02400dd579c/output.txt

Total script time: 59.42 mins

  • Unit tests: Passed
  • Integration Tests: FAILED
  • Regression tests: Passed

@ryzokuken ryzokuken force-pushed the autolink-demo branch 2 times, most recently from d648990 to e0b2146 Compare January 29, 2025 23:43
Snuffleupagus
Snuffleupagus reviewed Jan 30, 2025
View reviewed changes
web/annotation_layer_builder.js Show resolved Hide resolved
web/annotation_layer_builder.js Outdated Show resolved Hide resolved
web/annotation_layer_builder.js Outdated Show resolved Hide resolved
@ryzokuken
Copy link
Collaborator Author

Thanks for the amazing suggestions, @Snuffleupagus. I just implemented them.

@ryzokuken
Enable automatic URL linking
5c182a1 
Automatically detect links in the text content of a file and automatically
generate link annotations at the appropriate locations to achieve
automatic link detection and hyperlinking.
@calixteman
Copy link
Contributor

@Snuffleupagus anything else before I run the tests ?

@Snuffleupagus
Copy link
Collaborator

anything else before I run the tests ?

Not off the top of my head, since this is finally starting to look reasonable w.r.t. existing code-paths and edge-cases.

@calixteman
Copy link
Contributor

/botio test

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)

Received

Command cmd_test from @calixteman received. Current queue size: 0

Live output at: http://54.193.163.58:8877/5a2fadaeaeba323/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)

Received

Command cmd_test from @calixteman received. Current queue size: 0

Live output at: http://54.241.84.105:8877/addd119393d1d21/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)

Failed

Full output at http://54.241.84.105:8877/addd119393d1d21/output.txt

Total script time: 29.28 mins

  • Unit tests: Passed
  • Integration Tests: FAILED
  • Regression tests: Passed

@ryzokuken
Copy link
Collaborator Author

At a glance, the integration test failures seem unrelated to this PR, although I'll check to be sure.

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)

Failed

Full output at http://54.193.163.58:8877/5a2fadaeaeba323/output.txt

Total script time: 59.50 mins

  • Unit tests: Passed
  • Integration Tests: FAILED
  • Regression tests: Passed

calixteman
calixteman approved these changes Jan 30, 2025
View reviewed changes
Copy link
Contributor

@calixteman calixteman left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolo-ribaudo nicolo-ribaudo nicolo-ribaudo left review comments

@calixteman calixteman calixteman approved these changes

@Snuffleupagus Snuffleupagus Awaiting requested review from Snuffleupagus

Assignees

@ryzokuken ryzokuken

Labels
viewer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Enable automatic URL hyperlinking
6 participants
@ryzokuken @calixteman @Snuffleupagus @moz-tools-bot @nicolo-ribaudo @timvandermeij