libCwtch-go changes for storage engine refactor and profile api changes #47

Merged
sarah merged 14 commits from cwtch_refactor into trunk 2021-12-08 00:44:03 +00:00
Owner

Significant refactor, see cwtch.im/cwtch#404

This removes a lot of functionality that has been moved to cwtch.

  • removal of manager.go all enhancement now takes place in the event Handler
  • removal of many utility methods - some of these have been moved into Cwtch, others have been inlined.
  • many api changes given the new "conversations" metaphor.
  • PeerCreated is now ContactCreated

Still a work in progress, but offered here for comments as it impacts the review of the cwtch pr

Significant refactor, see https://git.openprivacy.ca/cwtch.im/cwtch/pulls/404 This removes a lot of functionality that has been moved to cwtch. - removal of `manager.go` all enhancement now takes place in the event Handler - removal of many utility methods - some of these have been moved into Cwtch, others have been inlined. - many api changes given the new "conversations" metaphor. - `PeerCreated` is now `ContactCreated` Still a work in progress, but offered here for comments as it impacts the review of the cwtch pr
sarah added 4 commits 2021-11-19 19:57:29 +00:00
dan reviewed 2021-11-19 21:29:20 +00:00
go.mod Outdated
@ -12,2 +13,4 @@
golang.org/x/sys v0.0.0-20210823070655-63515b42dcdf // indirect
)
replace cwtch.im/cwtch => /home/sarah/workspace/src/cwtch.im/cwtch

not in final

not in final
dan marked this conversation as resolved
lib.go Outdated
@ -615,2 +520,2 @@
}))
}
messages, err := profile.GetMostRecentMessages(conversationID, 0, messageIndex, 1)
if err == nil && len(messages) == 1 {

this could have more than 1 message returned? then what do we do?

this could have more than 1 message returned? then what do we do?

seems like a missmatch always needing/only handing one message calling an interface that can return N?

seems like a missmatch always needing/only handing one message calling an interface that can return N?
Author
Owner

This is the future-proofed getNMessages API.

this could have more than 1 message returned? then what do we do?

It literally can't profile.GetMostRecentMessages(conversationID, 0, messageIndex, **1**)

This is the future-proofed getNMessages API. > this could have more than 1 message returned? then what do we do? It literally can't ` profile.GetMostRecentMessages(conversationID, 0, messageIndex, **1**)`
dan marked this conversation as resolved
lib.go Outdated
@ -650,0 +556,4 @@
if message.ID == id {
time, _ := time.Parse(time.RFC3339Nano, messages[0].Attr[constants2.AttrSentTimestamp])
msg := model.Message{
Message: messages[0].Body,

doesnt this make 100 coopies of message 0?
why fo we get messages back in a format we have to process and migrate data from?

doesnt this make 100 coopies of message 0? why fo we get messages back in a format we have to process and migrate data from?
Author
Owner

UI renders messages by row numberCwtch understands messages by message id.

For quoted messages we therefore need a mapping between the actual database row and the message ID. ContentHash gets us the ID. We don't want Row() to query the whole database and so we just look back over the most recent message - so 100 rows is chosen as a suitable limit.

I think ultimately we are going to have to push back quoted messages into Cwtch and just do the rendered there instead of in UI but I didn't want to mess too much with the API in this PR.

UI renders messages by row numberCwtch understands messages by message id. For quoted messages we therefore need a mapping between the actual database row and the message ID. ContentHash gets us the ID. We don't want Row() to query the whole database and so we just look back over the most recent message - so 100 rows is chosen as a suitable limit. I think ultimately we are going to have to push back quoted messages into Cwtch and just do the rendered there instead of in UI but I didn't want to mess too much with the API in this PR.
dan marked this conversation as resolved
@ -277,1 +272,4 @@
}
lastMessage, _ := profile.GetMostRecentMessages(conversationID, 0, 0, 1)
ev.Event.Data["unread"] = strconv.Itoa(1) // we've just created this contact so by definition this must be 1...

did we just copy the EnrichNewPeer function into place here?

did we just copy the EnrichNewPeer function into place here?
Author
Owner

yes

yes
dan marked this conversation as resolved
Member
Drone Build Status: failure https://build.openprivacy.ca/cwtch.im/libcwtch-go/148
sarah added 1 commit 2021-11-23 22:30:30 +00:00
continuous-integration/drone/pr Build is pending Details
edff87f77c
Updates to GetMessageByContentHash
dan reviewed 2021-11-23 23:28:35 +00:00
lib.go Outdated
@ -511,4 +466,0 @@
RejectInvite(C.GoStringN(profilePtr, profileLen), C.GoStringN(handlePtr, handleLen))
}
// RejectInvite rejects a group invite

How are we handeling rejecting invites?

How are we handeling rejecting invites?
dan marked this conversation as resolved
lib.go Outdated
@ -252,4 +254,0 @@
contactList := application.GetPeer(profileOnion).GetContacts()
for _, handle := range contactList {
totalMessages := application.GetPeer(profileOnion).GetContact(handle).Timeline.Len() + len(application.GetPeer(profileOnion).GetContact(handle).UnacknowledgedMessages)
eventHandler.Push(event.NewEvent(event.MessageCounterResync, map[event.Field]string{

this event is still defined in cwtch/event/common but seems unused. is the UI migrating off this functionality? or do we need to port it into Cwtch?

this event is still defined in cwtch/event/common but seems unused. is the UI migrating off this functionality? or do we need to port it into Cwtch?
dan marked this conversation as resolved
@ -269,1 +256,4 @@
for _, conversation := range conversations {
if (conversation.IsGroup() && groupHandler != nil) || conversation.IsServer() == false {
totalMessages, _ := profile.GetChannelMessageCount(conversation.ID, 0)
eventHandler.Push(event.NewEvent(event.MessageCounterResync, map[event.Field]string{

we're still doing it for groups

we're still doing it for groups
dan marked this conversation as resolved
lib.go Outdated
@ -647,3 +550,1 @@
indexedMessages, err = timeline.GetMessagesByHash(contentHash)
if err != nil {
indexedMessages = []model.LocallyIndexedMessage{}
id, err := profile.GetChannelMessageByContentHash(handle, 0, contentHash)

GetMessagesByContentHash why is it looping through the most recent 100 messages?
peer.GetChannelMessageByContentHash doesnt return the message? just hte id? and then we fetch only the most recent 100 messages and iteratively search through them for it? seems like a bad API? can't we push this way into Cwtch and storage? 

GetMessagesByContentHash why is it looping through the most recent 100 messages? peer.GetChannelMessageByContentHash doesnt return the message? just hte id? and then we fetch only the most recent 100 messages and iteratively search through them for it? seems like a bad API? can't we push this way into Cwtch and storage? 
sarah marked this conversation as resolved
@ -63,3 +64,2 @@
return eh.handleAppBusEvent(&e)
case ev := <-eh.profileEvents:
return eh.handleProfileEvent(&ev)
default:

why a default in the select with appChan with a nested select with appChan again and profileEvents?

why a default in the select with appChan with a nested select with appChan again and profileEvents?
@ -243,3 +236,3 @@
ph := NewPeerHelper(peer)
profile := eh.app.GetPeer(ev.Profile)
log.Debugf("New Profile Event to Handle: %v", ev)
switch ev.Event.EventType {

it's also not being handled by the UI. is the app responding to it?
options:

  • dont load the plugin then for now
  • again, handling logic should prolly be here or app, regardless either unsubscribe the queue from the event cus all events that fall through this event handler get json-ified and sent to the UI (or queued in an inf queue while suspended on android) or at least have this case handler just return so it doesnt get queued and sent to UI till we do handle it here
it's also not being handled by the UI. is the app responding to it? options: - dont load the plugin then for now - again, handling logic should prolly be here or app, regardless either unsubscribe the queue from the event cus all events that fall through this event handler get json-ified and sent to the UI (or queued in an inf queue while suspended on android) or at least have this `case` handler just `return` so it doesnt get queued and sent to UI till we do handle it here
@ -265,0 +246,4 @@
// TODO This Conversation May Not Exist Yet...But we are not in charge of creating it...
log.Errorf("todo wait for contact to be added before processing this event...")
}
ev.Event.Data["Nick"], _ = ci.GetAttribute(attr.PublicScope, attr.ProfileZone, constants.Name)

ph.GetNick checks Local.Name first and then falls back to Peer.Name so that we can locally rename a contact. the current code now always uses the remote peer's name ignoring local renames. this breaks existing functionality

ph.GetNick checks Local.Name first and then falls back to Peer.Name so that we can locally rename a contact. the current code now always uses the remote peer's name ignoring local renames. this breaks existing functionality
dan marked this conversation as resolved
@ -269,1 +254,3 @@
peer.SetGroupAttribute(ev.Event.Data[event.GroupID], attr.GetLocalScope(constants2.Archived), event.False)
ci, err := profile.FetchConversationInfo(ev.Event.Data["RemotePeer"])
if ci != nil && err == nil {
ev.Event.Data["Nick"], _ = ci.GetAttribute(attr.PublicScope, attr.ProfileZone, constants.Name)

same. ph.Nick regressoin

same. ph.Nick regressoin
dan marked this conversation as resolved
sarah added 1 commit 2021-11-26 22:26:44 +00:00
continuous-integration/drone/pr Build is pending Details
92d2925622
NewMessageByID, Suppress Network Tests
sarah added 1 commit 2021-11-26 23:07:30 +00:00
continuous-integration/drone/pr Build is pending Details
bd176f89cd
Prefer local overrides
dan reviewed 2021-11-27 01:34:15 +00:00
@ -265,0 +264,4 @@
var exists bool
ev.Event.Data["Nick"], exists = ci.GetAttribute(attr.LocalScope, attr.ProfileZone, constants.Name)
if !exists {
ev.Event.Data["Nick"], _ = ci.GetAttribute(attr.PublicScope, attr.ProfileZone, constants.Name)

fall back to handle if we don't have a public.profile.name?

fall back to handle if we don't have a public.profile.name?
dan marked this conversation as resolved
@ -270,0 +275,4 @@
var exists bool
ev.Event.Data["Nick"], exists = ci.GetAttribute(attr.LocalScope, attr.ProfileZone, constants.Name)
if !exists {
ev.Event.Data["Nick"], _ = ci.GetAttribute(attr.PublicScope, attr.ProfileZone, constants.Name)

fall back to handle if neither exist. in a group setting we are very likely not to be peered with everyone (there for unable to fetch names)

fall back to handle if neither exist. in a group setting we are very likely not to be peered with everyone (there for unable to fetch names)
dan marked this conversation as resolved
sarah added 2 commits 2021-12-01 12:17:07 +00:00
sarah added 1 commit 2021-12-06 20:23:06 +00:00
continuous-integration/drone/pr Build is pending Details
fee6c7e641
Fixup Event Handling for UI Cache
sarah added 1 commit 2021-12-06 20:24:06 +00:00
continuous-integration/drone/pr Build is pending Details
62f3d8878e
Reset to lib
sarah added 1 commit 2021-12-07 21:40:33 +00:00
continuous-integration/drone/pr Build is pending Details
1aea1c6e5f
Upgrade Cwtch and Server
sarah changed title from WIP: libCwtch-go changes for storage engine refactor and profile api changes to libCwtch-go changes for storage engine refactor and profile api changes 2021-12-07 21:40:59 +00:00
sarah added 1 commit 2021-12-07 21:43:15 +00:00
Member
Drone Build Status: failure https://build.openprivacy.ca/cwtch.im/libcwtch-go/158
sarah added 1 commit 2021-12-07 21:49:25 +00:00
continuous-integration/drone/pr Build is passing Details
a43c225267
Cleaning up group functionality after merge
Member
Drone Build Status: success https://build.openprivacy.ca/cwtch.im/libcwtch-go/159
dan approved these changes 2021-12-08 00:29:18 +00:00
Owner

lgtm just the question about GetNextEvent() having nested select statements? 

lgtm just the question about GetNextEvent() having nested select statements? 
sarah merged commit 79a87b78e0 into trunk 2021-12-08 00:44:03 +00:00
This repo is archived. You cannot comment on pull requests.
No reviewers
dan
No Label
No Milestone
No Assignees
3 Participants
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: cwtch.im/libcwtch-go#47
No description provided.