Skip to content

msglist: Share recipient headers across messages #300

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Sep 8, 2023

Conversation

gnprice
Copy link
Member

@gnprice gnprice commented Sep 7, 2023

Fixes: #174

Screenshot with recipients changing:
Screenshot from 2023-09-07 12-47-47

Screenshot with the date changing:
Screenshot from 2023-09-07 12-48-03

This was initially written when we didn't depend on the store in
the actual build phase.  But now we do, so we have it around
at this point and might as well use it.
This makes room to add a bit more complexity to some of the cases.
… bottom

We're already relying on this assumption in the code to add a new
message, in _processMessage.  We'll add the needed complications there
when they become needed.
Instead of showing recipient headers as part of each
MessageListMessageItem, they're now their own message-list items.
At this stage, we still show one for each message.
Copy link
Collaborator

@chrisbobbe chrisbobbe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! See one comment below, on the tests.

// are present just where [canShareRecipientHeader] requires them.
// In [checkInvariants] we check the current state against that invariant,
// so here we just need to exercise that code through all the relevant cases.
// Each [checkNotifiedOnce] call ensures there's been a [checkInvariants] call.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does each checkNotifiedOnce call ensure there's been a checkInvariants call? It doesn't call checkInvariants itself, right:

  void checkNotified({required int count}) {
    check(notifiedCount).equals(count);
    notifiedCount = 0;
  }
  void checkNotNotified() => checkNotified(count: 0);
  void checkNotifiedOnce() => checkNotified(count: 1);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. But the only way for notifiedCount to have been set greater than zero is in this listener:

    model = MessageListView.init(store: store, narrow: narrow)
      ..addListener(() {
        checkInvariants(model);
        notifiedCount++;
      });

and that calls checkInvariants.

So if checkNotifiedOnce succeeds (or checkNotified with any positive argument), then there must have been a call to that listener and it must have successfully called checkInvariants.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh! OK, yeah, that makes sense.

@gnprice gnprice force-pushed the pr-recipient-headers branch from b727adb to c5e7027 Compare September 8, 2023 00:12
@gnprice
Copy link
Member Author

gnprice commented Sep 8, 2023

Thanks for the review! Expanded that comment to answer your question.

@chrisbobbe
Copy link
Collaborator

Great, thanks! Merging.

@chrisbobbe chrisbobbe merged commit 11938c7 into zulip:main Sep 8, 2023
@gnprice gnprice deleted the pr-recipient-headers branch September 8, 2023 00:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Suppress repeated recipient headers
2 participants