nandi/frq · 4083860

nandi/frqpublic⑂ Fork 0

⑂ 4083860

A row is named for its message, not its place

Two things, both about a tree that is rebuilt wholesale on every frame.

The member list is no longer remembered. It was, as of this morning,
along with the join/part and room-list toggles — but on a narrow window
the people panel is not beside the conversation, it *is* the pane. So
having asked for it once, every launch opened a room and showed a list
of names instead of the room. It is a way of looking at the moment you
are in, like the overview, and those start off.

And message rows were keyed by their index. A line arriving above, or
the join/part filter coming off, renumbers every row under it — and a
key that renumbers is a row Flutter tears down and builds again, losing
everything it held for it. The renderer now honours the `key` the core
has always put on these nodes, and the rows carry the message's own id.

That is also the best I can do about `ConcurrentModificationError ...
during a scheduler callback` out of SelectableRegion, which is a
framework bug in handling selectables that change during a live
selection: fewer of them change now. I could not reproduce it — three
attempts, driving a held drag against tree changes from the poll, from a
second pointer and from a send, all of which passed. The test that did
land is the one about element identity, and it took two tries to make it
discriminate: an arriving line lands at the bottom and renumbers
nothing, so the first version passed against both keyings.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

nandi committed 2026-09-19T10:47:59-07:00 Browse files

4083860 parent: abb8e36

modified flutter/lib/nim_renderer.dart +13 -0 Viewed

@@ -222,6 +222,19 @@ class _NimAppState extends State<NimApp> {
222	222	static const _column = 'column';
223	223
224	224	Widget _build(core.UiNode n, [String axis = _noAxis]) {
	225	+ // A node that names itself keeps its element across rebuilds.
	226	+ //
	227	+ // The tree is rebuilt wholesale from the core, so Flutter matches
	228	+ // children by position unless something says otherwise — and a position
	229	+ // is not an identity when a line can arrive above. Everything Flutter
	230	+ // holds per element is at stake: text controllers, scroll offsets, and
	231	+ // the selectables a live text selection is made of.
	232	+ final key = n.prop('key', '');
	233	+ final w = _buildNode(n, axis);
	234	+ return key.isEmpty ? w : KeyedSubtree(key: ValueKey(key), child: w);
	235	+ }
	236	+
	237	+ Widget _buildNode(core.UiNode n, String axis) {
225	238	final spacing = _d(n.props['spacing'], 0);
226	239	final flex = axis == _row \|\| axis == _column;
227	240

    static const _column = 'column';
    Widget _build(core.UiNode n, [String axis = _noAxis]) {
++    // A node that names itself keeps its element across rebuilds.
++    //
++    // The tree is rebuilt wholesale from the core, so Flutter matches
++    // children by position unless something says otherwise — and a position
++    // is not an identity when a line can arrive above. Everything Flutter
++    // holds per element is at stake: text controllers, scroll offsets, and
++    // the selectables a live text selection is made of.
++    final key = n.prop('key', '');
++    final w = _buildNode(n, axis);
++    return key.isEmpty ? w : KeyedSubtree(key: ValueKey(key), child: w);
++  }
++
++  Widget _buildNode(core.UiNode n, String axis) {
      final spacing = _d(n.props['spacing'], 0);
      final flex = axis == _row || axis == _column;

modified flutter/test/nim_layout_test.dart +36 -0 Viewed

@@ -241,6 +241,42 @@ void main() {
241	241	});
242	242	});
243	243
	244	+ group('identity', () {
	245	+ // Duplicate keys among siblings are an error Flutter throws at build
	246	+ // time, so this is mostly a guard on the tree the core emits: every
	247	+ // message row names itself now, and two rows must never name themselves
	248	+ // the same thing.
	249	+ testWidgets('every screen builds with the keys the core gives it',
	250	+ (tester) async {
	251	+ for (final screen in ['chat', 'chats', 'discover', 'settings']) {
	252	+ core.demoUi();
	253	+ if (screen != 'chat') core.dispatch('screen.$screen');
	254	+ await layOut(tester, sizes['desktop']!);
	255	+ expectLaidOut(tester, '$screen with keys');
	256	+ }
	257	+ });
	258	+
	259	+ testWidgets('a row keeps its element when one above it goes away',
	260	+ (tester) async {
	261	+ // Hiding the joins and parts takes the system line off the top of the
	262	+ // demo backlog, which renumbers every row under it. Keyed by position
	263	+ // that is a teardown and rebuild of all of them; keyed by the message
	264	+ // it is one row leaving.
	265	+ //
	266	+ // An arriving line would not show this — it lands at the bottom and
	267	+ // renumbers nothing, which is how the first version of this test
	268	+ // passed against both.
	269	+ core.demoUi();
	270	+ await layOut(tester, sizes['desktop']!);
	271	+ final before = tester.element(find.text('alice').first);
	272	+ core.dispatch('join-part.toggle');
	273	+ await tester.pump(const Duration(milliseconds: 150));
	274	+ expectLaidOut(tester, 'the backlog with the system lines hidden');
	275	+ expect(tester.element(find.text('alice').first), same(before),
	276	+ reason: 'the row was torn down rather than kept');
	277	+ });
	278	+ });
	279	+
244	280	group('emoji', () {
245	281	// ✏️ is U+270F plus a variation selector asking for emoji presentation,
246	282	// and DejaVu Sans claims U+270F — so ordinary fallback draws a monochrome

      });
    });
++  group('identity', () {
++    // Duplicate keys among siblings are an error Flutter throws at build
++    // time, so this is mostly a guard on the tree the core emits: every
++    // message row names itself now, and two rows must never name themselves
++    // the same thing.
++    testWidgets('every screen builds with the keys the core gives it',
++        (tester) async {
++      for (final screen in ['chat', 'chats', 'discover', 'settings']) {
++        core.demoUi();
++        if (screen != 'chat') core.dispatch('screen.$screen');
++        await layOut(tester, sizes['desktop']!);
++        expectLaidOut(tester, '$screen with keys');
++      }
++    });
++
++    testWidgets('a row keeps its element when one above it goes away',
++        (tester) async {
++      // Hiding the joins and parts takes the system line off the top of the
++      // demo backlog, which renumbers every row under it. Keyed by position
++      // that is a teardown and rebuild of all of them; keyed by the message
++      // it is one row leaving.
++      //
++      // An arriving line would not show this — it lands at the bottom and
++      // renumbers nothing, which is how the first version of this test
++      // passed against both.
++      core.demoUi();
++      await layOut(tester, sizes['desktop']!);
++      final before = tester.element(find.text('alice').first);
++      core.dispatch('join-part.toggle');
++      await tester.pump(const Duration(milliseconds: 150));
++      expectLaidOut(tester, 'the backlog with the system lines hidden');
++      expect(tester.element(find.text('alice').first), same(before),
++          reason: 'the row was torn down rather than kept');
++    });
++  });
++
    group('emoji', () {
      // ✏️ is U+270F plus a variation selector asking for emoji presentation,
      // and DejaVu Sans claims U+270F — so ordinary fallback draws a monochrome

modified nim/src/frq/reducer.nim +12 -8 Viewed

@@ -88,18 +88,22 @@ proc restoreRooms() =
88	88	trace("store", $saved.len & " rooms remembered")
89	89
90	90	proc restorePrefs() =
91		- ## The display toggles. Three booleans, and every one of them is the
92		- ## reader's answer to a question this client has no business re-asking on
93		- ## every run — whether the comings and goings are worth seeing, whether the
94		- ## member list is up, whether the room list is out of the way.
	91	+ ## The display toggles that are a standing answer rather than a passing
	92	+ ## one: whether the comings and goings are worth seeing, and whether the
	93	+ ## room list is out of the way.
	94	+ ##
	95	+ ## The member list is NOT among them, though it was for a day. On a narrow
	96	+ ## window the people panel is not beside the conversation, it is the
	97	+ ## pane — so a saved "on" meant opening a room and being shown a list of
	98	+ ## names instead of the room, every launch, having asked for it once. It is
	99	+ ## a way of looking at the moment you are in, like the overview, and those
	100	+ ## start off.
95	101	let prefs = loadPrefs()
96	102	app.hideJoinPart = prefs.getOrDefault("hideJoinPart", app.hideJoinPart)
97		- app.showUsers = prefs.getOrDefault("showUsers", app.showUsers)
98	103	app.hideChatList = prefs.getOrDefault("hideChatList", app.hideChatList)
99	104
100	105	proc rememberPrefs() =
101	106	discard savePrefs({"hideJoinPart": app.hideJoinPart,
102		- "showUsers": app.showUsers,
103	107	"hideChatList": app.hideChatList}.toTable)
104	108
105	109	var
@@ -386,15 +390,15 @@ proc dispatch*(event: JsonNode) =
386	390	of "search.clear": app.search = ""
387	391
388	392	# ---------------------------------------------------------- chat chrome
389		- # The three that outlive the run are written as they are pressed. There is
	393	+ # The two that outlive the run are written as they are pressed. There is
390	394	# no Save on this screen and no moment that is obviously the last one — the
391	395	# window closes when it closes.
392	396	of "chat-list.toggle":
393	397	app.hideChatList = not app.hideChatList
394	398	rememberPrefs()
395	399	of "users.toggle":
	400	+ # Not remembered; see `restorePrefs`.
396	401	app.showUsers = not app.showUsers
397		- rememberPrefs()
398	402	of "overview.toggle":
399	403	# Not kept: the overview is a way of looking at the moment you are in
400	404	# rather than a preference, and a client that reopened into it would be

    trace("store", $saved.len & " rooms remembered")
  proc restorePrefs() =
--  ## The display toggles. Three booleans, and every one of them is the
++  ## The display toggles that are a standing answer rather than a passing
--  ## reader's answer to a question this client has no business re-asking on
++  ## one: whether the comings and goings are worth seeing, and whether the
--  ## every run — whether the comings and goings are worth seeing, whether the
++  ## room list is out of the way.
--  ## member list is up, whether the room list is out of the way.
++  ##
++  ## The member list is NOT among them, though it was for a day. On a narrow
++  ## window the people panel is not beside the conversation, it *is* the
++  ## pane — so a saved "on" meant opening a room and being shown a list of
++  ## names instead of the room, every launch, having asked for it once. It is
++  ## a way of looking at the moment you are in, like the overview, and those
++  ## start off.
    let prefs = loadPrefs()
    app.hideJoinPart = prefs.getOrDefault("hideJoinPart", app.hideJoinPart)
--  app.showUsers = prefs.getOrDefault("showUsers", app.showUsers)
    app.hideChatList = prefs.getOrDefault("hideChatList", app.hideChatList)
  proc rememberPrefs() =
    discard savePrefs({"hideJoinPart": app.hideJoinPart,
--                     "showUsers": app.showUsers,
                       "hideChatList": app.hideChatList}.toTable)
  var
    of "search.clear": app.search = ""
    # ---------------------------------------------------------- chat chrome
--  # The three that outlive the run are written as they are pressed. There is
++  # The two that outlive the run are written as they are pressed. There is
    # no Save on this screen and no moment that is obviously the last one — the
    # window closes when it closes.
    of "chat-list.toggle":
      app.hideChatList = not app.hideChatList
      rememberPrefs()
    of "users.toggle":
++    # Not remembered; see `restorePrefs`.
      app.showUsers = not app.showUsers
--    rememberPrefs()
    of "overview.toggle":
      # Not kept: the overview is a way of looking at the moment you are in
      # rather than a preference, and a client that reopened into it would be

modified nim/src/frq/screens/chat.nim +8 -2 Viewed

@@ -210,7 +210,13 @@ proc messageRow(s: State, room: Room, i: int, m: Message): Node =
210	210	## on the sender's row, which a headerless line has nowhere to put.
211	211	let rid = rowId(m)
212	212	let highlit = rid.len > 0 and rid == s.highlight
213		- n("vbox", %*{"key": $i, "spacing": 2, "margin": 0, "marginRight": 10,
	213	+ # Keyed by the message and not by its position. A line arriving, or the
	214	+ # join/part filter coming off, renumbers every row under it — and a key
	215	+ # that renumbers is a row the renderer tears down and builds again, taking
	216	+ # with it whatever state Flutter held for it. A live text selection is the
	217	+ # loudest thing that state has been.
	218	+ n("vbox", %*{"key": (if rid.len > 0: rid else: "row-" & $i),
	219	+ "spacing": 2, "margin": 0, "marginRight": 10,
214	220	"marginTop": 10,
215	221	"scrollHere": rid.len > 0 and rid == s.jumpTo},
216	222	@[messageBody(s, room, m, highlit)])
@@ -237,7 +243,7 @@ proc messageRows*(s: State, room: Room, messages: seq[Message]): seq[Node] =
237	243	if m.at > 0:
238	244	let d = day(m.at)
239	245	if d != prevDay:
240		- result.add daySeparator("day-" & $i, dayLabel(m.at))
	246	+ result.add daySeparator("day-" & d, dayLabel(m.at))
241	247	prevDay = d
242	248	result.add messageRow(s, room, i, m)
243	249

    ## on the sender's row, which a headerless line has nowhere to put.
    let rid = rowId(m)
    let highlit = rid.len > 0 and rid == s.highlight
--  n("vbox", %*{"key": $i, "spacing": 2, "margin": 0, "marginRight": 10,
++  # Keyed by the message and not by its position. A line arriving, or the
++  # join/part filter coming off, renumbers every row under it — and a key
++  # that renumbers is a row the renderer tears down and builds again, taking
++  # with it whatever state Flutter held for it. A live text selection is the
++  # loudest thing that state has been.
++  n("vbox", %*{"key": (if rid.len > 0: rid else: "row-" & $i),
++               "spacing": 2, "margin": 0, "marginRight": 10,
                 "marginTop": 10,
                 "scrollHere": rid.len > 0 and rid == s.jumpTo},
      @[messageBody(s, room, m, highlit)])
      if m.at > 0:
        let d = day(m.at)
        if d != prevDay:
--        result.add daySeparator("day-" & $i, dayLabel(m.at))
++        result.add daySeparator("day-" & d, dayLabel(m.at))
        prevDay = d
      result.add messageRow(s, room, i, m)

modified nim/tests/tstore.nim +13 -2 Viewed

@@ -109,15 +109,26 @@ suite "the rooms file":
109	109
110	110	suite "the prefs file":
111	111	setup: clean()
112		- test "the three display toggles outlive the run":
	112	+ test "the display toggles outlive the run":
113	113	restore()
114	114	let wasJoinPart = app.hideJoinPart
115	115	dispatch(%*{"id": "join-part.toggle"})
116		- dispatch(%*{"id": "users.toggle"})
	116	+ dispatch(%*{"id": "chat-list.toggle"})
117	117	app = initState()
118	118	restore()
119	119	check app.hideJoinPart == not wasJoinPart
	120	+ check app.hideChatList
	121	+
	122	+ test "but the member list does not":
	123	+ # On a narrow window the people panel is the pane rather than something
	124	+ # beside it, so a remembered "on" opens a room and shows a list of names
	125	+ # instead of the room.
	126	+ restore()
	127	+ dispatch(%*{"id": "users.toggle"})
120	128	check app.showUsers
	129	+ app = initState()
	130	+ restore()
	131	+ check not app.showUsers
121	132	test "the overview is not one of them":
122	133	# It is a way of looking at the moment you are in rather than a
123	134	# preference, and reopening into it would answer a question nobody asked

  suite "the prefs file":
    setup: clean()
--  test "the three display toggles outlive the run":
++  test "the display toggles outlive the run":
      restore()
      let wasJoinPart = app.hideJoinPart
      dispatch(%*{"id": "join-part.toggle"})
--    dispatch(%*{"id": "users.toggle"})
++    dispatch(%*{"id": "chat-list.toggle"})
      app = initState()
      restore()
      check app.hideJoinPart == not wasJoinPart
++    check app.hideChatList
++
++  test "but the member list does not":
++    # On a narrow window the people panel is the pane rather than something
++    # beside it, so a remembered "on" opens a room and shows a list of names
++    # instead of the room.
++    restore()
++    dispatch(%*{"id": "users.toggle"})
      check app.showUsers
++    app = initState()
++    restore()
++    check not app.showUsers
    test "the overview is not one of them":
      # It is a way of looking at the moment you are in rather than a
      # preference, and reopening into it would answer a question nobody asked