Page MenuHomePhorge

Avoid separate per-line URIs in line number column
Needs RevisionPublic

Authored by aklapper on Apr 4 2024, 21:20.

Details

Reviewers
valerio.bozzolan
Group Reviewers
O1: Blessed Committers
Summary

Constructing a URI in the format X123$45 as the link target of line number 45 in the line number column of document X123 (resp. X123$1-10 for lines 1-10) provides the ability to highlight one or several lines, and accessing this URI highlights the corresponding lines and automatically scrolls to the highlight line(s) with 60px top margin.

The downside of using this custom JS-based approach instead of standard # HTML anchor fragments is that every URI linked from the line number column in Pastes and Diffusion file pages is semantically a separate URI to crawl (due to $ instead of # as a fragment separator).

As a compromise,

  • use standard # for the link target of each single line and when highlighting a single line. This makes us lose line highlighting and vastly reduces the number of links a crawler could follow when indexing a Paste or Diffusion file page,
  • keep using custom $ when explicitly highlighting multiple lines.

See T15670

Test Plan

Diff Detail

Repository
rP Phorge
Branch
crawl#linesDollarNo (branched from master)
Lint
Lint Passed
Unit
Test Failures
Build Status
Buildable 1151
Build 1151: arc lint + arc unit

Unit TestsFailed

TimeTest
184 msPhabricatorCelerityTestCase::testCelerityMaps
Assertion failed, expected 'true' (at PhabricatorCelerityTestCase.php:27): When this test fails, it means the Celerity resource map is out of date. Run `bin/celerity map` to rebuild it. ACTUAL VALUE
0 msPHUIListViewTestCase::testAppend
1 assertion passed.
0 msPHUIListViewTestCase::testAppendAfter
2 assertions passed.
0 msPHUIListViewTestCase::testAppendBefore
2 assertions passed.
2 msPHUIListViewTestCase::testAppendLabel
2 assertions passed.
View Full Test Results (1 Failed · 15 Passed)

Event Timeline

aklapper requested review of this revision.Apr 4 2024, 21:20

Bonus point: maybe instead of "#123" we can use "#L123" since both GitLab and GitHub use that (and that's not a stupid specification). Just because we can.

https://gitlab.com/ItalianLinuxSociety/ilsmanager/-/blob/master/README.md?ref_type=heads&plain=1#L9

https://github.com/phorgeit/phorge/blob/master/README.md?plain=1#L9

My last comment is also because the W3C does not allow to start an identifier with a number:

https://www.w3.org/TR/CSS21/syndata.html#value-def-identifier

they cannot start with a digit, two hyphens, or a hyphen followed by a digit.

That is probably the strong reason behind the wide adoption of #L123 - outside Phorge

valerio.bozzolan added inline comments.
src/view/layout/PhabricatorSourceCodeView.php
134–146

Maybe better to have L123 ids as already mentioned, to strictly follow W3C specifications.

So, a commodity variable $line_id may be useful

webroot/rsrc/js/core/behavior-line-linker.js
168–172
198

Something like this to highlight at startup

This revision now requires changes to proceed.Jul 15 2024, 06:00
webroot/rsrc/js/core/behavior-line-linker.js
168–172

At this point this line should also be removed:

path = path + '#' + o;