Repository navigation
Make long alert messages scrollable instead of silently clipped - #2833
Natalie-the-technician wants to merge 1 commit into
Conversation
showOptionDialog hands plain-text messages to Alert.setContentText, which routes them through DialogPane.createContentLabel. That method hard-codes label.setPrefWidth(360), so the dialog never grows sideways: a long message only grows downwards, until the window manager clips it at the bottom of the screen. The label's default ELLIPSIS overrun then drops the rest with no indication beyond a trailing ellipsis, and the buttons can be pushed off the screen along with it. Put the message in a scroll pane instead. Short messages keep the familiar 360px column and the layout they have today; long ones become scrollable and the buttons stay reachable. Each line becomes its own label whose leading spaces are turned into left padding, so a wrapped line hangs at the depth of the line it continues instead of falling back to the left margin. The line splitting is covered by AlertMessageLinesTest.
|
Hi, I am a bit concerned about that string-splitting code in this PR. I asked Claude if we could solve the problem in a simpler way, and it came up with this code: It seems to work, at least in my tests, and I could not see any clearly visible difference compared to your PR. Can you look at my branch and if it solves the problem, then maybe we can just accept it |
|
@Natalie-the-technician review ⬆️ |
|
Thanks for the pointer to I took the branch seriously and measured it instead of defending mine. Three results. 1. Your version does the job, and I would rather ship it than mine. I built
Long messages stop at the cap with the whole message still reachable; short messages keep the size they have today, to the pixel. That is the whole of what #2833 set out to do, in 60 lines instead of my 95. I ran the same four tests against my own branch as a control. Same four greens, and the same five numbers to the decimal — the two versions are indistinguishable on everything I could measure. Yours is shorter. On screen, on a 1024x768 display, your branch gives the same window your reviewers would have got from mine, to the pixel:
Scrolled to the end, the message finishes on the sentence it is supposed to finish on, and the two list items that were unreachable before are fully readable. A mid-length message in the same run came out 360 x 341 — under the cap, no scrollbar, sized to its content. 2. My tests did not survive the move, and that is a fair criticism of them. Dropping my 3. I rewrote them as behaviour tests, and they pass on your branch.
They need one change on your branch: the plain-text arm of What I dropped, and why it costs nothing. My version turned leading spaces into a hanging indent, so that a wrapped line stayed at the level of the line it continued. A single wrapped Happy to push the tests onto #2833 on top of your version, or to close #2833 if you would rather carry the change yourself — whichever is less work for you. One more thing worth flagging: your branch also routes the HTML/Markdown arm through the scroll pane, which is the subject of #2836. If |
|
@Natalie-the-technician, I merged my changes. If you find it worthwhile to port your tests on top of that, go ahead :) |
|
Thanks — and your branch is the right one to have landed. On porting the tests: I can't, and I think that is the correct outcome rather What I can offer instead is tests for what your version actually promises, none
The first is a headless preferred-height assertion like the numbers above. The Do you want them? I am happy to open a PR with just those, or to drop them into |
|
Not sure that focus-traversability is worth testing, but otherwise, why not |
|
The two tests are in #2848 — the height cap and the screen the cap is taken It is test code only; Closing this one. Its own tests only exercise |
2 participants
UIFacade.showOptionDialogrenders plain-text messages withAlert.setContentText. That path goes throughDialogPane.createContentLabel,which hard-codes
label.setPrefWidth(360). The dialog therefore never growssideways: a long message only grows downwards until the window manager clips it
at the bottom of the screen, and the label's default
ELLIPSISoverrun dropsthe remainder. Nothing is logged and nothing is offered — the only hint is the
trailing ellipsis — and the buttons can be pushed off the screen along with the
text.
Measured on a 1024x768 screen with a 2398-character message: the dialog stops
at 360x708, its top edge already at y=0, and 986 characters (41 %) are
unreachable. Two of the five listed items are not visible at all.
This puts the plain-text message in a
ScrollPane:Measured at three call sites (
ProjectUIFacadeImpl,NetworkOptionPageProvider,ProjectOpenStrategy): 360x149 before,360x152 after — same width, three pixels taller, no scroll bar, no box.
same message now measures 360x541 and every line can be reached.
Labelwhose leading spaces become left padding,so a wrapped line hangs at the depth of the line it continues instead of
falling back to the left margin.
Two details of
modena.cssmatter here and are commented in the code. Thecontent label is padded only while it is a direct child of the dialog pane, so
the padding has to be restored inside the scroll pane. And
-fx-backgroundmust be left alone:
-fx-text-background-coloris aladder()over it, sosetting it to
transparentpicks-fx-light-text-colorand turns every labelinside the scroll pane white on white.
The four alert types are untouched — the change sits after the
alertTypebranch and before the button types are set. Icon, header and buttons were
compared before and after for INFORMATION, WARNING, QUESTION and ERROR.
Testing:
AlertMessageLinesTestcovers the line splitting — indent, blanklines, whitespace-only lines, CRLF, empty input — in seven tests that need no
toolkit and no window. The full suite passes: 382 tests, 0 failures. What the
tests do not cover is the resulting window geometry: the sizes quoted above
were measured on screen with
xdotool getwindowgeometry, not asserted in atest.