fix(web): a selected line range is one block, its title names the whole range and its hidden lines stay selected once loaded
- commit
- a772c1b73988c8f24e32bc0b8fff651c3d566981
- author
- brice · · 2026-10-19 15:22 UTC
- parent
- 502935e
- signature
- GPG Verified
CD88F9E564E0102646FB6077A1A07C7C7C3E349D· arch-gpg Verified for the committer email
4 files · +52 -1
M internal/adapters/web/mr.go +6 -1
| @@ -910,10 +910,15 @@ func (s *Server) placeComments(p *MRPage, repo string, n int64, comments []domai | |||
| 910 | 910 | ln.Selected = true | |
| 911 | 911 | } | |
| 912 | 912 | } | |
| 913 | + | // The title names the whole range a shift + click selected: a.go:21-149. | |
| 914 | + | lines := itoa(on.line) | |
| 915 | + | if start < on.line { | |
| 916 | + | lines = itoa(start) + "-" + lines | |
| 917 | + | } | |
| 913 | 918 | if ln := find(on.path, side, on.line); ln != nil { | |
| 914 | 919 | ln.Form = &CommentForm{ | |
| 915 | 920 | StartLine: optionalLine(on.start), ID: "cf-line", Action: mrHref(repo, n, "comments"), Path: on.path, Line: itoa(on.line), Side: on.side, | |
| 916 | - | Title: "comment on " + visibleText(on.path) + ":" + itoa(on.line) + " (" + on.side + ")", | |
| 921 | + | Title: "comment on " + visibleText(on.path) + ":" + lines + " (" + on.side + ")", | |
| 917 | 922 | Cancel: withFrom(self, windowStart(fileAt[on.path])) + "#" + fileIDs[on.path], Focus: true, | |
| 918 | 923 | From: windowStart(fileAt[on.path]), | |
| 919 | 924 | } | |
| @@ -910,10 +910,15 @@ func (s *Server) placeComments(p *MRPage, repo string, n int64, comments []domai | |||||
| 910 | ln.Selected = true | 910 | ln.Selected = true | ||
| 911 | } | 911 | } | ||
| 912 | } | 912 | } | ||
| 913 | + | // The title names the whole range a shift + click selected: a.go:21-149. | |||
| 914 | + | lines := itoa(on.line) | |||
| 915 | + | if start < on.line { | |||
| 916 | + | lines = itoa(start) + "-" + lines | |||
| 917 | + | } | |||
| 913 | if ln := find(on.path, side, on.line); ln != nil { | 918 | if ln := find(on.path, side, on.line); ln != nil { | ||
| 914 | ln.Form = &CommentForm{ | 919 | ln.Form = &CommentForm{ | ||
| 915 | StartLine: optionalLine(on.start), ID: "cf-line", Action: mrHref(repo, n, "comments"), Path: on.path, Line: itoa(on.line), Side: on.side, | 920 | StartLine: optionalLine(on.start), ID: "cf-line", Action: mrHref(repo, n, "comments"), Path: on.path, Line: itoa(on.line), Side: on.side, | ||
| 916 | - | Title: "comment on " + visibleText(on.path) + ":" + itoa(on.line) + " (" + on.side + ")", | 921 | + | Title: "comment on " + visibleText(on.path) + ":" + lines + " (" + on.side + ")", |
| 917 | Cancel: withFrom(self, windowStart(fileAt[on.path])) + "#" + fileIDs[on.path], Focus: true, | 922 | Cancel: withFrom(self, windowStart(fileAt[on.path])) + "#" + fileIDs[on.path], Focus: true, | ||
| 918 | From: windowStart(fileAt[on.path]), | 923 | From: windowStart(fileAt[on.path]), | ||
| 919 | } | 924 | } | ||
M internal/adapters/web/static/css/55-diff.css +25 -0
| @@ -9,4 +9,29 @@ | |||
| 9 | 9 | ||
| 10 | 10 | @layer components { | |
| 11 | 11 | .sel { outline: 1px solid var(--ink-dim); outline-offset: -1px; } | |
| 12 | + | ||
| 13 | + | /* a range of selected diff lines (shift + click) is one block: one border | |
| 14 | + | around the run, none between its lines. Each edge is an inset shadow of | |
| 15 | + | the cells; the top one only on the first line of the run, the bottom one | |
| 16 | + | only on the last. In split, each side is a block of its own. */ | |
| 17 | + | .dt { | |
| 18 | + | --sel-edge: var(--ink-dim); | |
| 19 | + | & .sel { outline: none; } | |
| 20 | + | & tr.sel > td, & td.sel { | |
| 21 | + | box-shadow: var(--sel-l, 0 0 transparent), var(--sel-r, 0 0 transparent), var(--sel-t, 0 0 transparent), var(--sel-b, 0 0 transparent); | |
| 22 | + | --sel-t: inset 0 1px var(--sel-edge); --sel-b: inset 0 -1px var(--sel-edge); | |
| 23 | + | } | |
| 24 | + | /* unified: the row is selected */ | |
| 25 | + | & tr.sel > td:first-child { --sel-l: inset 1px 0 var(--sel-edge); } | |
| 26 | + | & tr.sel > td:last-child { --sel-r: inset -1px 0 var(--sel-edge); } | |
| 27 | + | & tr.sel + tr.sel > td { --sel-t: 0 0 transparent; } | |
| 28 | + | & tr.sel:has(+ tr.sel) > td { --sel-b: 0 0 transparent; } | |
| 29 | + | /* split: the cells of one side (number, mark, text) are selected */ | |
| 30 | + | & td.sel.ln { --sel-l: inset 1px 0 var(--sel-edge); } | |
| 31 | + | & td.sel.tx { --sel-r: inset -1px 0 var(--sel-edge); } | |
| 32 | + | & tr:has(> :nth-child(3).sel) + tr > :nth-child(-n + 3).sel, | |
| 33 | + | & tr:has(> :nth-child(6).sel) + tr > :nth-child(n + 4).sel { --sel-t: 0 0 transparent; } | |
| 34 | + | & tr:has(> :nth-child(3).sel):has(+ tr > :nth-child(3).sel) > :nth-child(-n + 3).sel, | |
| 35 | + | & tr:has(> :nth-child(6).sel):has(+ tr > :nth-child(6).sel) > :nth-child(n + 4).sel { --sel-b: 0 0 transparent; } | |
| 36 | + | } | |
| 12 | 37 | } | |
| @@ -9,4 +9,29 @@ | |||||
| 9 | 9 | ||||
| 10 | @layer components { | 10 | @layer components { | ||
| 11 | .sel { outline: 1px solid var(--ink-dim); outline-offset: -1px; } | 11 | .sel { outline: 1px solid var(--ink-dim); outline-offset: -1px; } | ||
| 12 | + | ||||
| 13 | + | /* a range of selected diff lines (shift + click) is one block: one border | |||
| 14 | + | around the run, none between its lines. Each edge is an inset shadow of | |||
| 15 | + | the cells; the top one only on the first line of the run, the bottom one | |||
| 16 | + | only on the last. In split, each side is a block of its own. */ | |||
| 17 | + | .dt { | |||
| 18 | + | --sel-edge: var(--ink-dim); | |||
| 19 | + | & .sel { outline: none; } | |||
| 20 | + | & tr.sel > td, & td.sel { | |||
| 21 | + | box-shadow: var(--sel-l, 0 0 transparent), var(--sel-r, 0 0 transparent), var(--sel-t, 0 0 transparent), var(--sel-b, 0 0 transparent); | |||
| 22 | + | --sel-t: inset 0 1px var(--sel-edge); --sel-b: inset 0 -1px var(--sel-edge); | |||
| 23 | + | } | |||
| 24 | + | /* unified: the row is selected */ | |||
| 25 | + | & tr.sel > td:first-child { --sel-l: inset 1px 0 var(--sel-edge); } | |||
| 26 | + | & tr.sel > td:last-child { --sel-r: inset -1px 0 var(--sel-edge); } | |||
| 27 | + | & tr.sel + tr.sel > td { --sel-t: 0 0 transparent; } | |||
| 28 | + | & tr.sel:has(+ tr.sel) > td { --sel-b: 0 0 transparent; } | |||
| 29 | + | /* split: the cells of one side (number, mark, text) are selected */ | |||
| 30 | + | & td.sel.ln { --sel-l: inset 1px 0 var(--sel-edge); } | |||
| 31 | + | & td.sel.tx { --sel-r: inset -1px 0 var(--sel-edge); } | |||
| 32 | + | & tr:has(> :nth-child(3).sel) + tr > :nth-child(-n + 3).sel, | |||
| 33 | + | & tr:has(> :nth-child(6).sel) + tr > :nth-child(n + 4).sel { --sel-t: 0 0 transparent; } | |||
| 34 | + | & tr:has(> :nth-child(3).sel):has(+ tr > :nth-child(3).sel) > :nth-child(-n + 3).sel, | |||
| 35 | + | & tr:has(> :nth-child(6).sel):has(+ tr > :nth-child(6).sel) > :nth-child(n + 4).sel { --sel-b: 0 0 transparent; } | |||
| 36 | + | } | |||
| 12 | } | 37 | } | ||
M internal/adapters/web/static/js/ermite.js +19 -0
| @@ -401,6 +401,24 @@ | |||
| 401 | 401 | // file whose lines are all loaded has no gap left. | |
| 402 | 402 | window.ermiteGaps = (() => { | |
| 403 | 403 | const pending = new Map(); | |
| 404 | + | // selectLoaded marks the loaded rows that fall in the range of lines the | |
| 405 | + | // URL selects (?on=new:21-149:a.go, a shift + click), as the server marks | |
| 406 | + | // the rows it sent: the range stays one block once its hidden lines show. | |
| 407 | + | // The loaded rows are context lines, the same on both sides. | |
| 408 | + | function selectLoaded(file, rows) { | |
| 409 | + | const m = /^(new|old):(?:(\d+)-)?(\d+):(.+)$/.exec(new URLSearchParams(location.search).get('on') || ''); | |
| 410 | + | if (!m || file.querySelector('summary .p')?.textContent !== m[4]) return; | |
| 411 | + | const end = Number(m[3]); | |
| 412 | + | const start = m[2] ? Number(m[2]) : end; | |
| 413 | + | for (const r of rows) { | |
| 414 | + | const cells = [...r.querySelectorAll('td')]; | |
| 415 | + | // unified: old, new, mark, text; split: old side, then new side | |
| 416 | + | const num = Number((cells.length === 4 ? cells[m[1] === 'old' ? 0 : 1] : cells[m[1] === 'old' ? 0 : 3])?.textContent.trim()); | |
| 417 | + | if (!(num >= start && num <= end)) continue; | |
| 418 | + | if (cells.length === 4) r.classList.add('sel'); | |
| 419 | + | else for (const c of cells) c.classList.add('sel'); | |
| 420 | + | } | |
| 421 | + | } | |
| 404 | 422 | // load fills the gap the button is in, in both tables of its file. | |
| 405 | 423 | function load(btn) { | |
| 406 | 424 | const src = btn.dataset.src; | |
| @@ -425,6 +443,7 @@ | |||
| 425 | 443 | if (!from || !at) continue; | |
| 426 | 444 | const rows = [...from.children].map((r) => r.cloneNode(true)); | |
| 427 | 445 | n = Math.max(n, rows.filter((r) => !r.classList.contains('gap')).length); | |
| 446 | + | selectLoaded(file, rows.filter((r) => !r.classList.contains('gap'))); | |
| 428 | 447 | at.replaceWith(...rows); | |
| 429 | 448 | } | |
| 430 | 449 | // The focus was on the button, which is gone: on the gap that is | |
| @@ -401,6 +401,24 @@ | |||||
| 401 | // file whose lines are all loaded has no gap left. | 401 | // file whose lines are all loaded has no gap left. | ||
| 402 | window.ermiteGaps = (() => { | 402 | window.ermiteGaps = (() => { | ||
| 403 | const pending = new Map(); | 403 | const pending = new Map(); | ||
| 404 | + | // selectLoaded marks the loaded rows that fall in the range of lines the | |||
| 405 | + | // URL selects (?on=new:21-149:a.go, a shift + click), as the server marks | |||
| 406 | + | // the rows it sent: the range stays one block once its hidden lines show. | |||
| 407 | + | // The loaded rows are context lines, the same on both sides. | |||
| 408 | + | function selectLoaded(file, rows) { | |||
| 409 | + | const m = /^(new|old):(?:(\d+)-)?(\d+):(.+)$/.exec(new URLSearchParams(location.search).get('on') || ''); | |||
| 410 | + | if (!m || file.querySelector('summary .p')?.textContent !== m[4]) return; | |||
| 411 | + | const end = Number(m[3]); | |||
| 412 | + | const start = m[2] ? Number(m[2]) : end; | |||
| 413 | + | for (const r of rows) { | |||
| 414 | + | const cells = [...r.querySelectorAll('td')]; | |||
| 415 | + | // unified: old, new, mark, text; split: old side, then new side | |||
| 416 | + | const num = Number((cells.length === 4 ? cells[m[1] === 'old' ? 0 : 1] : cells[m[1] === 'old' ? 0 : 3])?.textContent.trim()); | |||
| 417 | + | if (!(num >= start && num <= end)) continue; | |||
| 418 | + | if (cells.length === 4) r.classList.add('sel'); | |||
| 419 | + | else for (const c of cells) c.classList.add('sel'); | |||
| 420 | + | } | |||
| 421 | + | } | |||
| 404 | // load fills the gap the button is in, in both tables of its file. | 422 | // load fills the gap the button is in, in both tables of its file. | ||
| 405 | function load(btn) { | 423 | function load(btn) { | ||
| 406 | const src = btn.dataset.src; | 424 | const src = btn.dataset.src; | ||
| @@ -425,6 +443,7 @@ | |||||
| 425 | if (!from || !at) continue; | 443 | if (!from || !at) continue; | ||
| 426 | const rows = [...from.children].map((r) => r.cloneNode(true)); | 444 | const rows = [...from.children].map((r) => r.cloneNode(true)); | ||
| 427 | n = Math.max(n, rows.filter((r) => !r.classList.contains('gap')).length); | 445 | n = Math.max(n, rows.filter((r) => !r.classList.contains('gap')).length); | ||
| 446 | + | selectLoaded(file, rows.filter((r) => !r.classList.contains('gap'))); | |||
| 428 | at.replaceWith(...rows); | 447 | at.replaceWith(...rows); | ||
| 429 | } | 448 | } | ||
| 430 | // The focus was on the button, which is gone: on the gap that is | 449 | // The focus was on the button, which is gone: on the gap that is | ||
M internal/app/web_mr_test.go +2 -0
| @@ -268,6 +268,8 @@ func TestWebMergeRequestReviewAndFastForward(t *testing.T) { | |||
| 268 | 268 | assert.Contains(t, line.body, `name="path" value="search.go"`) | |
| 269 | 269 | assert.Contains(t, line.body, `name="line" value="3"`) | |
| 270 | 270 | assert.Contains(t, line.body, `name="side" value="new"`) | |
| 271 | + | assert.Contains(t, line.body, `comment on search.go:3 (new)`, "one line: the title names it") | |
| 272 | + | assert.Contains(t, b.get(base+"?on=new:1-3:search.go", as(token)).body, `comment on search.go:1-3 (new)`, "a range: the title names all of it") | |
| 271 | 273 | assert.Equal(t, 0, strings.Count(b.get(base+"?on=new:99:search.go", as(token)).body, `name="line" value="99"`), "a line the diff lacks has no form") | |
| 272 | 274 | assert.Equal(t, 0, strings.Count(b.get(base+"?on=new:3:search.go", nil).body, `<form`), "nor for a visitor") | |
| 273 | 275 | ||
| @@ -268,6 +268,8 @@ func TestWebMergeRequestReviewAndFastForward(t *testing.T) { | |||||
| 268 | assert.Contains(t, line.body, `name="path" value="search.go"`) | 268 | assert.Contains(t, line.body, `name="path" value="search.go"`) | ||
| 269 | assert.Contains(t, line.body, `name="line" value="3"`) | 269 | assert.Contains(t, line.body, `name="line" value="3"`) | ||
| 270 | assert.Contains(t, line.body, `name="side" value="new"`) | 270 | assert.Contains(t, line.body, `name="side" value="new"`) | ||
| 271 | + | assert.Contains(t, line.body, `comment on search.go:3 (new)`, "one line: the title names it") | |||
| 272 | + | assert.Contains(t, b.get(base+"?on=new:1-3:search.go", as(token)).body, `comment on search.go:1-3 (new)`, "a range: the title names all of it") | |||
| 271 | assert.Equal(t, 0, strings.Count(b.get(base+"?on=new:99:search.go", as(token)).body, `name="line" value="99"`), "a line the diff lacks has no form") | 273 | assert.Equal(t, 0, strings.Count(b.get(base+"?on=new:99:search.go", as(token)).body, `name="line" value="99"`), "a line the diff lacks has no form") | ||
| 272 | assert.Equal(t, 0, strings.Count(b.get(base+"?on=new:3:search.go", nil).body, `<form`), "nor for a visitor") | 274 | assert.Equal(t, 0, strings.Count(b.get(base+"?on=new:3:search.go", nil).body, `<form`), "nor for a visitor") | ||
| 273 | 275 | ||||