Skip to content
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

local echo (4/n): Migrate more fields to PerAccountStoreBase #1466

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

PIG208
Copy link
Member

@PIG208 PIG208 commented Apr 4, 2025

MessageStore will need to access queueId and selfUserId later for outbox message support.

@PIG208 PIG208 requested a review from chrisbobbe April 4, 2025 20:12
@PIG208 PIG208 added the maintainer review PR ready for review by Zulip maintainers label Apr 4, 2025
@gnprice
Copy link
Member

gnprice commented Apr 4, 2025

I see we were thinking in a similar direction today 🙂. I just sent #1467 which does the selfUserId portion of this (including some changes borrowed from this PR), as well as a number of other fields. How about you rebase the queueId commit atop that?

@PIG208
Copy link
Member Author

PIG208 commented Apr 4, 2025

Thanks! This has been rebased atop #1467.

@chrisbobbe
Copy link
Collaborator

#1467 has been merged, and GitHub says there are some conflicts; could you rebase again?

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.

Thanks! Small comment below.

@@ -384,6 +386,8 @@ abstract class PerAccountStoreBase {
////////////////////////////////
// Data attached to the self-account on the realm.

String get queueId => _core.queueId;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it intentional to move this from the section

  ////////////////////////////////
  // Where data comes from in the first place.

to the section

  ////////////////////////////////
  // Data attached to the self-account on the realm.

?

Copy link
Member Author

Choose a reason for hiding this comment

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

I want to position this after connection and before accountId, and I feel that it fits a bit better as it is associated with the self-account. But keeping it in the original section works too (proximity to UpdateMachine, queueId's old home, was a reason why it was there).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for explaining; otherwise it looks like an accident 🙂 perhaps Greg has thoughts about it.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I think up next to connection in the "Where data comes from in the first place" section would be better.

I see the queue ID as much more like the connection than it is like the account. It's part of "where data comes from" — each getEvents call passes it in order to get the right events.

Conversely it's not really "attached to" the self-account, in that there may be zero, one, or many queues at any given time for a given account and the queues are fairly ephemeral.

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes sense. Move it up to be right after connection.

@chrisbobbe chrisbobbe added integration review Added by maintainers when PR may be ready for integration and removed maintainer review PR ready for review by Zulip maintainers labels Apr 9, 2025
@chrisbobbe chrisbobbe assigned gnprice and unassigned chrisbobbe Apr 9, 2025
@chrisbobbe chrisbobbe requested a review from gnprice April 9, 2025 04:04
@gnprice
Copy link
Member

gnprice commented Apr 12, 2025

Thanks, and thanks @chrisbobbe for the review!

LGTM modulo the thread above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integration review Added by maintainers when PR may be ready for integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants