Service Desk
opened 6 hours ago
-
-
Between two change hunks, each click grows the hidden gap from both ends (about 15 lines toward the hunk above and 15 toward the hunk below)
This is not entirely correct. The reason I created this issue is because only the lines above are being expanded.
-
@zaakiy Thanks — I re-checked that claim.
For a gap between two change hunks, the expand action does add about 15 more context lines on both sides of the remaining gap. The expander row is replaced in place, so the newly revealed lines on the top side appear where you clicked. Extra context just above the next hunk is added below the expander and is easy to miss.
For the expander at the start of a file, a click only reveals more lines above the first change. That matches “only the lines above are being expanded.”
There is still no control that expands only toward the hunk below, which is the gap you described in issue #3068.
On that question you chose 3B (both sides run the same expand). That keeps today’s middle-gap behavior and only makes the hit targets larger. It does not add a way to grow toward the next hunk without also growing from the top.
Please confirm what the two side-by-side controls should do:
- Same action on both sides (current issue #3069 scope)
- Independent directions: left expands only toward the hunk above; right expands only toward the hunk below
I will update the issue description and plan after you confirm.
-
- Independent directions: left expands only toward the hunk above; right expands only toward the hunk below
Retain the current behavior of expanding both sides by having a button in the middle of the left and right buttons.
Hope that makes sense. Don't assume. Ask high-level clarifying questions if needed.
- Numbered questions (1, 2, 3, etc)
- Lettered answer options (A, B, C, etc)
- Include a markdown table that contains the following for each question:
- Pros and Cons?
- Your recommendation (including justification)?
-
@zaakiy Got it: between two hunks the row should have three controls — left expands only toward the hunk above, right only toward the hunk below, and a middle control keeps today’s both-sides expand.
A few layout/behavior questions before I rewrite issue #3069. A reply like
1A, 2A, 3Ais enough.1. How should the three controls share the expander row?
A. Three equal-width hit targets across the full row (up | both | down). The skipped-line count can sit on the middle control or as small text on the row. B. Three small controls in the gutter only; keep the current “skipped N lines” cell as non-clickable space. C. Wide left and right targets, with a compact middle “both” control.
2. What should appear at the start or end of a file, where only one direction exists?
A. Keep a single expander (today’s control). B. Show only the applicable direction button. C. Show direction + both (they would do the same thing). D. Still show all three; hide or disable the impossible direction.
3. After one-sided expands, if one direction has no hidden lines left, what should the row become?
A. Drop to a single expander for the remaining direction (same idea as start/end). B. Keep the remaining direction plus the middle “both” control. C. Keep all three; hide or disable the exhausted direction.
# Pros and cons Recommendation 1 A keeps every action easy to click, including “both.” B is compact but the targets get small — the same problem that ruled out stacked icons. C favors up/down at the expense of the action you want to keep. A. Easy-to-click was the original constraint; three equal targets across the row is the most consistent way to keep all three usable, including in split view where the gutter is narrow. 2 A stays simple and matches your earlier start/end choice. B is slightly more consistent with directional controls. C duplicates the same action. D can look broken. A. Only show three controls when both directions exist. 3 A matches start/end and avoids a dead control. B keeps “both,” but “both” would then equal the remaining direction. C is consistent but noisy. A. Once only one direction remains, a single expander is enough. Once you confirm, I will update the issue #3069 description and implementation plan to this three-control design.
-
- A. Three equal-width hit targets across the full row (up | both | down). The skipped-line count can sit on the middle control or as small text on the row.
- A. Keep a single expander (today’s control).
- A. Drop to a single expander for the remaining direction (same idea as start/end).
-
Previous Value Current Value Split text-diff expander into two wide side-by-side controls
Split text-diff expander into up, both, and down controls
-
@zaakiy Confirmed as 1A, 2A, 3A. I updated the issue #3069 title, description, and implementation plan to the three-control design:
- Between hunks: three equal-width targets (up | both | down)
- Left expands only toward the hunk above; right only toward the hunk below; middle keeps today's both-sides expand
- File start/end: keep the current single expander
- After one-sided expands, if only one direction remains, drop to that single expander
-
-
@robin I could not prepare the issue checkout, so no code or verification could be performed.
tod issue checkout --for-write 3069failed while fetching the requiredserver-eesubmodule withfatal: expected 'acknowledgments'. The repository/submodule fetch configuration or access needs to be repaired before I can implement and verify the requested work with Playwright and e2e cases. -
-
Name Previous Value Current Value Assignee
robin
wh
-
-
Implemented independent up, both, and down expansion for middle text-diff gaps, including equal-width accessible controls, single-control fallback when one direction is exhausted, and shared support for blob and plain text diffs.
Verification passed:
mvn -pl server-core -am -Dtest=DiffExpandSupportTest -Dsurefire.failIfNoSpecifiedTests=false testPLAYWRIGHT_HTML_OPEN=never npx playwright test tests/text-diff-expander.spec.js --reporter=line
Submitted as PR #116.
| Type |
Improvement
|
| Priority |
Normal
|
| Assignee | |
| Labels |
No labels
|
Motivation
Text diffs use a single
Show more linescontrol. Between two change hunks, each click grows the hidden gap from both ends (about 15 lines toward the hunk above and 15 toward the hunk below). The expander row stays in the remaining gap. At the start of a file the only direction is more lines above the first change; at the end of a file the only direction is more lines below the last change.The current control is one small icon in the gutter. Reviewers need large, obvious hit targets, plus a way to expand toward only one hunk without also growing the other side.
Expected behavior
PlainTextDiffPanelsurfaces (blob edit preview, comment revisions, workspace changes).DIFF_EXPAND_SIZEmore lines from the top of the remaining gap).DIFF_EXPAND_SIZEfrom both ends). Put the skipped-line count on this control.DIFF_EXPAND_SIZEmore lines from the bottom of the remaining gap).Current implementation
Shared expand logic lives in
DiffExpandSupport:contextSizeper equal-block (defaultWebConstants.DIFF_CONTEXT_SIZE= 3). Each click addsWebConstants.DIFF_EXPAND_SIZE(15) to both ends.2 * contextSizecovers the block, the expander is omitted and the rest of the block is shown.Markup is built by:
BlobTextDiffPanel.appendExpander— commit/PR/compare text diffs (BlobDiffPaneluses this panel). Unified vs split and blame vs no-blame use different colspans. Icon isexpand2. Click callscallback('expand', blockIndex).PlainTextDiffPanel.appendExpander— other text diffs. Same single-icon row.Styling is in
server-core/src/main/java/io/onedev/server/web/asset/textdiff/text-diff.css(td.expander ais already a large block with hover). After expand,blob-text-diff.jsandplain-text-diff.jsreplacetr.expander{blockIndex}and re-bind tippy ontd.expander a.Implementation plan
Track top and bottom context independently for middle gaps.
DiffExpandSupportcurrently stores onecontextSizeper block andexpand()grows both ends. Keep that for first/last blocks and for the middle "both" action. For middle blocks, also store separate top/bottom sizes (or equivalent) so expand-up and expand-down can grow one end only. Cap so top + bottom never exceed the block length.Tell the renderer which controls to draw.
appendEqualsalready knows first / middle / last, and can know when only one end of a remaining gap can still grow. ExtendExpandCallback.appendExpanderwith enough information to render:expand().Render the controls. In
BlobTextDiffPanel.appendExpanderandPlainTextDiffPanel.appendExpander:<a>elements spanning the full row (expander + skipped cells) so they stay easy to click in split view. Left: up chevron, expand up. Middle:expand2(or equivalent) plus skipped-line count, expand both. Right: down chevron, expand down.expand2control and "skipped N lines" label.Wire direction through the Ajax callback.
BlobTextDiffPanelalready passesparam1…param8. Use a direction argument (up/down/both) with the existingexpandaction (or dedicated actions).PlainTextDiffPanelcurrently only passesactionandparam1; add a direction parameter there. First/last single expander keeps today's expand (only one direction exists).Icons. Keep
expand2for the single expander and the middle "both" control. For up/down, use existing arrow/chevron sprites (arrow,arrow2,arrow3,arrow4) rotated to point up and down, or add two dedicated chevron icons if those assets do not read clearly at this size. No angled-divider artwork.CSS. Update
text-diff.css(andblob-text-diff.cssonly if blob-specific borders/colspans need it):--primary/ white) and dark-mode colors.JS. Row replacement stays the same (
tr.expander{blockIndex}). Re-bind tippy on everytd.expander aafter expand (already the case). Selection-drag code that treatstr.expanderas a unit should keep working if the extra links stay inside that row.Translations. Tooltip can stay "Show more lines" for the single expander and the middle "both" control. Directional tooltips for left/right are fine if they stay in the same
_T(...)pattern. Do not edit generatedTranslation_*.javafiles.Verify.
PlainTextDiffPanelsurfaces (edit preview, comment revision).Related background (not required to implement): issue #3068.