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.

Sign up for GitHub

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

Jump to bottom

Use Inbox to back TeamChannelSource #14849

Merged
merged 11 commits into from Jan 10, 2019
Merged

Use Inbox to back TeamChannelSource #14849

merged 11 commits into from Jan 10, 2019

Conversation

joshblum
Copy link
Member

Changes CachingTeamChannelSource to use the inbox to gather all conversations in a TLF to detect channel mentions in chat. depends on https://github.com/keybase/keybase/pull/3171 for server changes to send down all convs in the inbox. After a few client release cycles we can remove the server support types.PushTeamChannels

@joshblum joshblum requested a review from mmaxim November 27, 2018 22:20
mmaxim
mmaxim reviewed Dec 4, 2018
View reviewed changes
go/chat/search/indexer.go
@@ -454,6 +454,10 @@ func (idx *Indexer) allConvs(ctx context.Context, uid gregor1.UID) (map[string]t
chat1.ConversationStatus_FAVORITE,
chat1.ConversationStatus_MUTED,
},
MemberStatus: []chat1.ConversationMemberStatus{
Copy link
Contributor

Choose a reason for hiding this comment

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

Why bother with this?

Copy link
Member Author

Choose a reason for hiding this comment

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

RESET is included in the default inbox query, might speed things up

mmaxim
mmaxim reviewed Dec 4, 2018
View reviewed changes
go/chat/teamchannelsource.go

// Localize the conversations
res, _, err = c.G().InboxSource.Localize(ctx, uid, utils.RemoteConvs(convs),
convs, _, err := c.G().InboxSource.Localize(ctx, uid, inbox.ConvsUnverified,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can just call Read

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

Copy link
Member Author

@joshblum joshblum Dec 4, 2018 •
edited

Choose a reason for hiding this comment

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

reverted, using Read makes things more complex since we have to convert the TlfID to a name and would have to thread the MembersType through for that. (just to convert back to a tlfID when we change the local query to a remote one in the Read call). think it's simpler as is

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder why it is the case we can't pass TLFID here...

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 started threading it through and i think was hitting errors here (if i'm remember correctly) https://github.com/keybase/client/blob/master/go/chat/inboxsource.go#L27

i also prefer it as is since both GetChannelsTopicName and GetChannelsFull can use getTLFConversations instead of duplicating the query between Read and ReadUnverified (GetChannelsTopicName can't call Localize or we get into some unboxing cycle)

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure this is fine.

@joshblum joshblum force-pushed the joshblum/chanfix-CORE-9428 branch 4 times, most recently from 97dc590 to 57b7902 Compare December 4, 2018 18:46
@joshblum joshblum mentioned this pull request Dec 12, 2018
Merged
@joshblum
Copy link
Member Author

new test depends on https://github.com/keybase/keybase/pull/3245

@joshblum joshblum force-pushed the joshblum/chanfix-CORE-9428 branch 2 times, most recently from 5135140 to 551c2e8 Compare December 17, 2018 16:07
@mmaxim
Copy link
Contributor

mmaxim commented Dec 18, 2018

Maybe merge master into this branch, and I'll check it out again.

@joshblum
Copy link
Member Author

joshblum commented Dec 18, 2018 •
edited

merged in @mmaxim

mmaxim
mmaxim reviewed Dec 18, 2018
View reviewed changes
go/chat/storage/inbox.go
@@ -490,83 +489,78 @@ func (i *Inbox) applyQuery(ctx context.Context, query *chat1.GetInboxQuery, rcs
queryConvIDMap[c.String()] = true
}
}

queryMemberStatusMap := map[chat1.ConversationMemberStatus]bool{}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we just make this a static thing so we aren't creating it all the time?

Copy link
Member Author

Choose a reason for hiding this comment

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

but it's built from the query

Copy link
Contributor

Choose a reason for hiding this comment

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

Right but 99% of the time it will be the same values, I was thinking we could swap in a map with the defaults values in that case instead of creating it.

Copy link
Member Author

Choose a reason for hiding this comment

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

sgtm

mmaxim
mmaxim reviewed Dec 18, 2018
View reviewed changes
Copy link
Contributor

@mmaxim mmaxim left a comment

Choose a reason for hiding this comment

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

One comment inline.

@joshblum joshblum force-pushed the joshblum/chanfix-CORE-9428 branch 3 times, most recently from b772f09 to 7b89cfb Compare December 21, 2018 18:09
@joshblum joshblum force-pushed the joshblum/chanfix-CORE-9428 branch 5 times, most recently from 1247caa to 654d641 Compare January 8, 2019 15:08
@joshblum
Copy link
Member Author

joshblum commented Jan 9, 2019

@mmaxim this is ready for final pass

@joshblum joshblum merged commit 5f1a984 into master Jan 10, 2019
@joshblum joshblum deleted the joshblum/chanfix-CORE-9428 branch January 10, 2019 01:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mmaxim mmaxim mmaxim approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@joshblum @mmaxim