Page MenuHomePhorge

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

Authored by aklapper on Apr 4 2024, 21:20.
Tags
None
Referenced Files
F2192766: D25569.1715925815.diff
Thu, May 16, 06:03
Unknown Object (File)
Thu, May 9, 01:22
Unknown Object (File)
Thu, May 9, 01:16
Unknown Object (File)
Fri, Apr 26, 20:11
Unknown Object (File)
Fri, Apr 26, 20:11
Unknown Object (File)
Fri, Apr 26, 20:11
Unknown Object (File)
Thu, Apr 25, 23:02
Unknown Object (File)
Mon, Apr 22, 13:12

Details

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