The overview reads the way a conversation does
Oldest at the top, newest at the bottom -- where it is in every backlog beside it, and where the eye finishes. It read newest-first because nothing in the strip scrolled: the top was all there was, and the bottom was wherever the list happened to be cut off. So the strip is also opened at its end, because a conversation is not something you join at the beginning: what you came to it for is what has just happened, and it should be under your eyes without scrolling for it. `fromBottom` and not `stickToBottom` -- that one carries two meanings, read from the bottom and report whether the reader is at the present, and only the backlog has a present to be at. The turn-taking in `recentEverywhere` is untouched, and the distinction is worth keeping: it decides *which* lines are here -- the newest from each room, a turn each -- and the sort decides only where they sit. Reverting either half fails a test, and checking that took two goes. Reverting both at once passed everything, because newest-first read from the top shows the newest first too -- which is the old behaviour exactly, and the reason that check proved nothing. Reverted one at a time: the order fails in `tchat`, the opening position fails in the layout suite with "the newest entry is off screen". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
d4b430c parent: 1c7cb6d modified
flutter/lib/nim_renderer.dart +7 -1 | @@ -942,6 +942,12 @@ class _NimAppState extends State<NimApp> { | ||
| 942 | 942 | final scrollKey = n.prop('scrollKey', 'scroll'); |
| 943 | 943 | final c = _scrollers.putIfAbsent(scrollKey, ScrollController.new); |
| 944 | 944 | final stick = n.prop('stickToBottom', false); |
| 945 | + // Opens at its end, which is a different thing from being the | |
| 946 | + // backlog. `stickToBottom` carries two meanings -- read from the | |
| 947 | + // bottom, and report whether the reader is at the present -- and | |
| 948 | + // only the backlog has a present to be at. The overview wants the | |
| 949 | + // first of those and not the second. | |
| 950 | + final fromBottom = n.prop('fromBottom', false); | |
| 945 | 951 | |
| 946 | 952 | // "Jump to present": a tick that goes up, rather than a flag that |
| 947 | 953 | // would have to be cleared. A reverse scroll holds the present at |
| @@ -967,7 +973,7 @@ class _NimAppState extends State<NimApp> { | ||
| 967 | 973 | Widget body = SingleChildScrollView( |
| 968 | 974 | controller: c, |
| 969 | 975 | // The backlog reads from the bottom; a settings list from the top. |
| 970 | - reverse: stick, | |
| 976 | + reverse: stick || fromBottom, | |
| 971 | 977 | child: Column( |
| 972 | 978 | crossAxisAlignment: CrossAxisAlignment.start, |
| 973 | 979 | children: _spaced(kids, spacing, vertical: true)), |
| @@ -942,6 +942,12 @@ class _NimAppState extends State<NimApp> { | |||
| 942 | final scrollKey = n.prop('scrollKey', 'scroll'); | 942 | final scrollKey = n.prop('scrollKey', 'scroll'); |
| 943 | final c = _scrollers.putIfAbsent(scrollKey, ScrollController.new); | 943 | final c = _scrollers.putIfAbsent(scrollKey, ScrollController.new); |
| 944 | final stick = n.prop('stickToBottom', false); | 944 | final stick = n.prop('stickToBottom', false); |
| 945 | + // Opens at its end, which is a different thing from being the | ||
| 946 | + // backlog. `stickToBottom` carries two meanings -- read from the | ||
| 947 | + // bottom, and report whether the reader is at the present -- and | ||
| 948 | + // only the backlog has a present to be at. The overview wants the | ||
| 949 | + // first of those and not the second. | ||
| 950 | + final fromBottom = n.prop('fromBottom', false); | ||
| 945 | 951 | ||
| 946 | // "Jump to present": a tick that goes up, rather than a flag that | 952 | // "Jump to present": a tick that goes up, rather than a flag that |
| 947 | // would have to be cleared. A reverse scroll holds the present at | 953 | // would have to be cleared. A reverse scroll holds the present at |
| @@ -967,7 +973,7 @@ class _NimAppState extends State<NimApp> { | |||
| 967 | Widget body = SingleChildScrollView( | 973 | Widget body = SingleChildScrollView( |
| 968 | controller: c, | 974 | controller: c, |
| 969 | // The backlog reads from the bottom; a settings list from the top. | 975 | // The backlog reads from the bottom; a settings list from the top. |
| 970 | - reverse: stick, | 976 | + reverse: stick || fromBottom, |
| 971 | child: Column( | 977 | child: Column( |
| 972 | crossAxisAlignment: CrossAxisAlignment.start, | 978 | crossAxisAlignment: CrossAxisAlignment.start, |
| 973 | children: _spaced(kids, spacing, vertical: true)), | 979 | children: _spaced(kids, spacing, vertical: true)), |
modified
flutter/test/nim_layout_test.dart +18 -0 | @@ -603,6 +603,24 @@ void main() { | ||
| 603 | 603 | await tester.pumpAndSettle(); |
| 604 | 604 | expectLaidOut(tester, 'the overview scrolled to its end'); |
| 605 | 605 | }); |
| 606 | + | |
| 607 | + testWidgets('opens at the newest, not the oldest', (tester) async { | |
| 608 | + // The entries read oldest-first, the way a conversation does -- and a | |
| 609 | + // conversation is not something you join at the beginning. What you | |
| 610 | + // came to the strip for is what has just happened, so it should be | |
| 611 | + // under your eyes without scrolling for it. | |
| 612 | + core.demoUi(); | |
| 613 | + core.dispatch('overview.toggle'); | |
| 614 | + await layOut(tester, sizes['phone']!); | |
| 615 | + | |
| 616 | + final newest = find.textContaining('line 12 of a long day'); | |
| 617 | + expect(newest, findsOneWidget, reason: 'the newest entry is not built'); | |
| 618 | + | |
| 619 | + final view = Offset.zero & sizes['phone']!; | |
| 620 | + expect(view.contains(tester.getRect(newest).center), isTrue, | |
| 621 | + reason: 'the newest entry is off screen when the strip opens'); | |
| 622 | + expectLaidOut(tester, 'the overview opened at its end'); | |
| 623 | + }); | |
| 606 | 624 | testWidgets('and the whole entry is the control, not an arrow', |
| 607 | 625 | (tester) async { |
| 608 | 626 | core.demoUi(); |
| @@ -603,6 +603,24 @@ void main() { | |||
| 603 | await tester.pumpAndSettle(); | 603 | await tester.pumpAndSettle(); |
| 604 | expectLaidOut(tester, 'the overview scrolled to its end'); | 604 | expectLaidOut(tester, 'the overview scrolled to its end'); |
| 605 | }); | 605 | }); |
| 606 | + | ||
| 607 | + testWidgets('opens at the newest, not the oldest', (tester) async { | ||
| 608 | + // The entries read oldest-first, the way a conversation does -- and a | ||
| 609 | + // conversation is not something you join at the beginning. What you | ||
| 610 | + // came to the strip for is what has just happened, so it should be | ||
| 611 | + // under your eyes without scrolling for it. | ||
| 612 | + core.demoUi(); | ||
| 613 | + core.dispatch('overview.toggle'); | ||
| 614 | + await layOut(tester, sizes['phone']!); | ||
| 615 | + | ||
| 616 | + final newest = find.textContaining('line 12 of a long day'); | ||
| 617 | + expect(newest, findsOneWidget, reason: 'the newest entry is not built'); | ||
| 618 | + | ||
| 619 | + final view = Offset.zero & sizes['phone']!; | ||
| 620 | + expect(view.contains(tester.getRect(newest).center), isTrue, | ||
| 621 | + reason: 'the newest entry is off screen when the strip opens'); | ||
| 622 | + expectLaidOut(tester, 'the overview opened at its end'); | ||
| 623 | + }); | ||
| 606 | testWidgets('and the whole entry is the control, not an arrow', | 624 | testWidgets('and the whole entry is the control, not an arrow', |
| 607 | (tester) async { | 625 | (tester) async { |
| 608 | core.demoUi(); | 626 | core.demoUi(); |
modified
nim/src/frq/rooms.nim +11 -6 | @@ -117,7 +117,7 @@ func roundRobin(colls: seq[seq[Message]]): seq[Message] = | ||
| 117 | 117 | |
| 118 | 118 | func recentEverywhere*(channels: OrderedTable[string, Room], |
| 119 | 119 | current: string): seq[Message] = |
| 120 | - ## The newest lines from every buffer at once, newest first, and at most | |
| 120 | + ## The newest lines from every buffer at once, oldest first, and at most | |
| 121 | 121 | ## `overviewLimit` of them — a turn to each room until they run out. |
| 122 | 122 | ## |
| 123 | 123 | ## Bounded per room before anything else, so the cost is the number of rooms |
| @@ -140,11 +140,16 @@ func recentEverywhere*(channels: OrderedTable[string, Room], | ||
| 140 | 140 | result = roundRobin(colls) |
| 141 | 141 | if result.len > overviewLimit: |
| 142 | 142 | result = result[0 ..< overviewLimit] |
| 143 | - # Newest at the top, the other way round from a conversation and right for | |
| 144 | - # the same reason a conversation is the way it is: what you came to the | |
| 145 | - # strip for is what has just happened. The turn-taking above is about which | |
| 146 | - # lines are in it, not where they sit. | |
| 147 | - result.sort(proc (a, b: Message): int = cmp(b.at, a.at)) | |
| 143 | + # Oldest at the top, the way a conversation runs, now that the strip | |
| 144 | + # scrolls: the newest is at the bottom where the eye finishes, and where | |
| 145 | + # it is in every backlog on the screen. It read newest-first while nothing | |
| 146 | + # here scrolled, when the top was all there was and the bottom was | |
| 147 | + # wherever the list happened to be cut off. | |
| 148 | + # | |
| 149 | + # The turn-taking above is untouched, and the distinction matters: it | |
| 150 | + # decides *which* lines are here -- the newest from each room, a turn each | |
| 151 | + # -- and this decides only where they sit. | |
| 152 | + result.sort(proc (a, b: Message): int = cmp(a.at, b.at)) | |
| 148 | 153 | |
| 149 | 154 | func afterMarker*(ch: Room): seq[Message] = |
| 150 | 155 | ## The messages the reader has not seen: everything after the read marker. |
| @@ -117,7 +117,7 @@ func roundRobin(colls: seq[seq[Message]]): seq[Message] = | |||
| 117 | 117 | ||
| 118 | func recentEverywhere*(channels: OrderedTable[string, Room], | 118 | func recentEverywhere*(channels: OrderedTable[string, Room], |
| 119 | current: string): seq[Message] = | 119 | current: string): seq[Message] = |
| 120 | - ## The newest lines from every buffer at once, newest first, and at most | 120 | + ## The newest lines from every buffer at once, oldest first, and at most |
| 121 | ## `overviewLimit` of them — a turn to each room until they run out. | 121 | ## `overviewLimit` of them — a turn to each room until they run out. |
| 122 | ## | 122 | ## |
| 123 | ## Bounded per room before anything else, so the cost is the number of rooms | 123 | ## Bounded per room before anything else, so the cost is the number of rooms |
| @@ -140,11 +140,16 @@ func recentEverywhere*(channels: OrderedTable[string, Room], | |||
| 140 | result = roundRobin(colls) | 140 | result = roundRobin(colls) |
| 141 | if result.len > overviewLimit: | 141 | if result.len > overviewLimit: |
| 142 | result = result[0 ..< overviewLimit] | 142 | result = result[0 ..< overviewLimit] |
| 143 | - # Newest at the top, the other way round from a conversation and right for | 143 | + # Oldest at the top, the way a conversation runs, now that the strip |
| 144 | - # the same reason a conversation is the way it is: what you came to the | 144 | + # scrolls: the newest is at the bottom where the eye finishes, and where |
| 145 | - # strip for is what has just happened. The turn-taking above is about which | 145 | + # it is in every backlog on the screen. It read newest-first while nothing |
| 146 | - # lines are in it, not where they sit. | 146 | + # here scrolled, when the top was all there was and the bottom was |
| 147 | - result.sort(proc (a, b: Message): int = cmp(b.at, a.at)) | 147 | + # wherever the list happened to be cut off. |
| 148 | + # | ||
| 149 | + # The turn-taking above is untouched, and the distinction matters: it | ||
| 150 | + # decides *which* lines are here -- the newest from each room, a turn each | ||
| 151 | + # -- and this decides only where they sit. | ||
| 152 | + result.sort(proc (a, b: Message): int = cmp(a.at, b.at)) | ||
| 148 | 153 | ||
| 149 | func afterMarker*(ch: Room): seq[Message] = | 154 | func afterMarker*(ch: Room): seq[Message] = |
| 150 | ## The messages the reader has not seen: everything after the read marker. | 155 | ## The messages the reader has not seen: everything after the read marker. |
modified
nim/src/frq/screens/chat.nim +9 -2 | @@ -280,7 +280,7 @@ proc visible(s: State, messages: seq[Message]): seq[Message] = | ||
| 280 | 280 | result.add m |
| 281 | 281 | |
| 282 | 282 | proc overviewPane(s: State): Node = |
| 283 | - ## What is happening in every room but this one, newest first. | |
| 283 | + ## What is happening in every room but this one, oldest first. | |
| 284 | 284 | ## |
| 285 | 285 | ## A turn to each room rather than the newest lines outright — see |
| 286 | 286 | ## `rooms.recentEverywhere`. Taking the newest hundred would be the strip |
| @@ -326,7 +326,14 @@ proc overviewPane(s: State): Node = | ||
| 326 | 326 | # a scrolling list wants an `Expanded` inside this pane, and `expand` is |
| 327 | 327 | # already how the pane claims its own share of the column; a second one |
| 328 | 328 | # nested in the first asks a Column to shrink-wrap and fill at once. |
| 329 | - result = scroll(%*{"scrollKey": "overview", "orientation": "vertical"}, | |
| 329 | + # Opened at its end, where the newest is. The entries read oldest-first | |
| 330 | + # now, the way a conversation does, and a conversation is not something | |
| 331 | + # you join at the beginning: what you came to the strip for is what has | |
| 332 | + # just happened, and it should be under your eyes without scrolling for | |
| 333 | + # it. `fromBottom` and not `stickToBottom` -- the latter also reports | |
| 334 | + # whether the reader is at the present, and only the backlog has one. | |
| 335 | + result = scroll(%*{"scrollKey": "overview", "orientation": "vertical", | |
| 336 | + "fromBottom": true}, | |
| 330 | 337 | vbox(%*{"spacing": 4}, title2("Overview"), list)) |
| 331 | 338 | |
| 332 | 339 | proc lightboxPane(s: State): Node = |
| @@ -280,7 +280,7 @@ proc visible(s: State, messages: seq[Message]): seq[Message] = | |||
| 280 | result.add m | 280 | result.add m |
| 281 | 281 | ||
| 282 | proc overviewPane(s: State): Node = | 282 | proc overviewPane(s: State): Node = |
| 283 | - ## What is happening in every room but this one, newest first. | 283 | + ## What is happening in every room but this one, oldest first. |
| 284 | ## | 284 | ## |
| 285 | ## A turn to each room rather than the newest lines outright — see | 285 | ## A turn to each room rather than the newest lines outright — see |
| 286 | ## `rooms.recentEverywhere`. Taking the newest hundred would be the strip | 286 | ## `rooms.recentEverywhere`. Taking the newest hundred would be the strip |
| @@ -326,7 +326,14 @@ proc overviewPane(s: State): Node = | |||
| 326 | # a scrolling list wants an `Expanded` inside this pane, and `expand` is | 326 | # a scrolling list wants an `Expanded` inside this pane, and `expand` is |
| 327 | # already how the pane claims its own share of the column; a second one | 327 | # already how the pane claims its own share of the column; a second one |
| 328 | # nested in the first asks a Column to shrink-wrap and fill at once. | 328 | # nested in the first asks a Column to shrink-wrap and fill at once. |
| 329 | - result = scroll(%*{"scrollKey": "overview", "orientation": "vertical"}, | 329 | + # Opened at its end, where the newest is. The entries read oldest-first |
| 330 | + # now, the way a conversation does, and a conversation is not something | ||
| 331 | + # you join at the beginning: what you came to the strip for is what has | ||
| 332 | + # just happened, and it should be under your eyes without scrolling for | ||
| 333 | + # it. `fromBottom` and not `stickToBottom` -- the latter also reports | ||
| 334 | + # whether the reader is at the present, and only the backlog has one. | ||
| 335 | + result = scroll(%*{"scrollKey": "overview", "orientation": "vertical", | ||
| 336 | + "fromBottom": true}, | ||
| 330 | vbox(%*{"spacing": 4}, title2("Overview"), list)) | 337 | vbox(%*{"spacing": 4}, title2("Overview"), list)) |
| 331 | 338 | ||
| 332 | proc lightboxPane(s: State): Node = | 339 | proc lightboxPane(s: State): Node = |
modified
nim/tests/tchat.nim +20 -0 | @@ -266,6 +266,26 @@ suite "the overview": | ||
| 266 | 266 | .anyIt("hello" in it.props{"text"}.getStr() and |
| 267 | 267 | it.props{"text"}.getStr() != "hello") |
| 268 | 268 | |
| 269 | + test "the newest is last, and the strip opens at its end": | |
| 270 | + # Oldest first, the way a conversation runs -- and so the strip is | |
| 271 | + # opened at the bottom, because a conversation is not something you | |
| 272 | + # join at the beginning. `fromBottom` and not `stickToBottom`: the | |
| 273 | + # latter also reports whether the reader is at the present, and only | |
| 274 | + # the backlog has one. | |
| 275 | + app.overview = true | |
| 276 | + var later = app.rooms["#other"] | |
| 277 | + later.messages.add Message(id: "o2", frm: "zoe", text: "later", | |
| 278 | + at: 1_700_000_900_000) | |
| 279 | + app.rooms["#other"] = later | |
| 280 | + let t = cs.chatScreen(app, true) | |
| 281 | + let texts = t.find("text").mapIt(it.props{"text"}.getStr()) | |
| 282 | + check texts.find("elsewhere") < texts.find("later") | |
| 283 | + let sc = t.find("scroll").filterIt( | |
| 284 | + it.props{"scrollKey"}.getStr() == "overview") | |
| 285 | + check sc.len == 1 | |
| 286 | + check sc[0].props{"fromBottom"}.getBool() == true | |
| 287 | + check sc[0].props{"stickToBottom"}.getBool() == false | |
| 288 | + | |
| 269 | 289 | test "each line is a card that is itself the way there": |
| 270 | 290 | # It used to be a row ending in a "→" button. In a Wrap on a phone the |
| 271 | 291 | # button landed on a line of its own, so every entry cost three rows and |
| @@ -266,6 +266,26 @@ suite "the overview": | |||
| 266 | .anyIt("hello" in it.props{"text"}.getStr() and | 266 | .anyIt("hello" in it.props{"text"}.getStr() and |
| 267 | it.props{"text"}.getStr() != "hello") | 267 | it.props{"text"}.getStr() != "hello") |
| 268 | 268 | ||
| 269 | + test "the newest is last, and the strip opens at its end": | ||
| 270 | + # Oldest first, the way a conversation runs -- and so the strip is | ||
| 271 | + # opened at the bottom, because a conversation is not something you | ||
| 272 | + # join at the beginning. `fromBottom` and not `stickToBottom`: the | ||
| 273 | + # latter also reports whether the reader is at the present, and only | ||
| 274 | + # the backlog has one. | ||
| 275 | + app.overview = true | ||
| 276 | + var later = app.rooms["#other"] | ||
| 277 | + later.messages.add Message(id: "o2", frm: "zoe", text: "later", | ||
| 278 | + at: 1_700_000_900_000) | ||
| 279 | + app.rooms["#other"] = later | ||
| 280 | + let t = cs.chatScreen(app, true) | ||
| 281 | + let texts = t.find("text").mapIt(it.props{"text"}.getStr()) | ||
| 282 | + check texts.find("elsewhere") < texts.find("later") | ||
| 283 | + let sc = t.find("scroll").filterIt( | ||
| 284 | + it.props{"scrollKey"}.getStr() == "overview") | ||
| 285 | + check sc.len == 1 | ||
| 286 | + check sc[0].props{"fromBottom"}.getBool() == true | ||
| 287 | + check sc[0].props{"stickToBottom"}.getBool() == false | ||
| 288 | + | ||
| 269 | test "each line is a card that is itself the way there": | 289 | test "each line is a card that is itself the way there": |
| 270 | # It used to be a row ending in a "→" button. In a Wrap on a phone the | 290 | # It used to be a row ending in a "→" button. In a Wrap on a phone the |
| 271 | # button landed on a line of its own, so every entry cost three rows and | 291 | # button landed on a line of its own, so every entry cost three rows and |
modified
nim/tests/trooms.nim +5 -2 | @@ -132,9 +132,12 @@ suite "recentEverywhere": | ||
| 132 | 132 | chans["#c"] = c |
| 133 | 133 | check recentEverywhere(chans, "").allIt(it.text != "joined") |
| 134 | 134 | |
| 135 | - test "newest first": | |
| 135 | + test "oldest first, the way a conversation runs": | |
| 136 | + # The strip scrolls now, so the newest belongs at the bottom where the | |
| 137 | + # eye finishes and where it is in every backlog beside it. Newest-first | |
| 138 | + # was right while nothing here scrolled and the top was all there was. | |
| 136 | 139 | let ats = recentEverywhere(chans, "").mapIt(it.at) |
| 137 | - check ats == ats.sorted(SortOrder.Descending) | |
| 140 | + check ats == ats.sorted(SortOrder.Ascending) | |
| 138 | 141 | |
| 139 | 142 | test "a turn each, so a busy room cannot crowd a quiet one out": |
| 140 | 143 | # The reason for round-robin: taking the newest N outright would answer |
| @@ -132,9 +132,12 @@ suite "recentEverywhere": | |||
| 132 | chans["#c"] = c | 132 | chans["#c"] = c |
| 133 | check recentEverywhere(chans, "").allIt(it.text != "joined") | 133 | check recentEverywhere(chans, "").allIt(it.text != "joined") |
| 134 | 134 | ||
| 135 | - test "newest first": | 135 | + test "oldest first, the way a conversation runs": |
| 136 | + # The strip scrolls now, so the newest belongs at the bottom where the | ||
| 137 | + # eye finishes and where it is in every backlog beside it. Newest-first | ||
| 138 | + # was right while nothing here scrolled and the top was all there was. | ||
| 136 | let ats = recentEverywhere(chans, "").mapIt(it.at) | 139 | let ats = recentEverywhere(chans, "").mapIt(it.at) |
| 137 | - check ats == ats.sorted(SortOrder.Descending) | 140 | + check ats == ats.sorted(SortOrder.Ascending) |
| 138 | 141 | ||
| 139 | test "a turn each, so a busy room cannot crowd a quiet one out": | 142 | test "a turn each, so a busy room cannot crowd a quiet one out": |
| 140 | # The reason for round-robin: taking the newest N outright would answer | 143 | # The reason for round-robin: taking the newest N outright would answer |