Why is the approach for constructing MessageData for notifs different than the approach for constructing RawMessageInfos above?

For instance, we use firstLine in one but not the other.

I strongly suspect it needs to be unified. RawMessageInfos can be constructed from MessageDatas. We should not maintain two duplicate codepaths.

In D13023#368342, @ashoat wrote:

Why is the approach for constructing MessageData for notifs different than the approach for constructing RawMessageInfos above?

For instance, we use firstLine in one but not the other.

I strongly suspect it needs to be unified. RawMessageInfos can be constructed from MessageDatas. We should not maintain two duplicate codepaths.

Explained in my comment: https://phab.comm.dev/D13023#368207

I saw that comment. All it does it link to some code on GitHub. I don't understand how it explains something, and especially don't understand how it responds to my comment above

In D13023#368350, @ashoat wrote:

I saw that comment. All it does it link to some code on GitHub on comment. I don't understand how it explains something, and especially don't understand how it responds to my comment above

It explicitly states that the logic in this diff was implemented in such a way to match to logic in the linked code. The logic in the linked code differs from the logic in the processDMOperation. Therefore this comment explains why the logic in notificationsCreationData differs from the logic in`processDMOperation`.

To expand on the feedback a little bit:

We should not have two separate codepaths for generating RawMessageInfo – one from notifs (where it starts as a MessageData) and one for content.

There should be only one version of a RawMessageInfo for a given message ID. The approach you've taken is problematic for many reasons, but the most obvious is that these messages get merged in the MessageStore both from notifs and from Tunnelbroker, but the way you're constructing them means it will be different from the two different sources.

In your recent work, I've noticed a pattern of "copy-paste" instead of "merge and factor". I understand you're trying to move fast, but you're currently not thinking nearly enough about the implications of your decisions to fork codepaths.

An ideal solution here would not only make sure that we have only one way to generate a MessageData / RawMessageInfo for thick threads, but would go further and also deduplicate it with the code you linked that handles thin threads.

If this feedback is confusing, I'd suggest syncing with @tomek first to try and understand it before responding further.

In D13023#368353, @ashoat wrote:

Need a deeper explanation. I think we should merge the codepaths

The code in processDMOperations constructs MessageData using name and color properties as they are. The keyserver code when handling change thread settings request first calls firstLine on name and toLower on color before constructing MessageData that are further used to send notifs. I wanted notificationsCreationData to match the keyserver behaviour.

I am not able to give deeper explanation than this since there is no additional logic there. I am fine with merging the codepaths if we don't require that notificationsCreationData doesn't match keyserver logic.

Ideally all three should match up. See my most recent comment

In D13023#368365, @ashoat wrote:

Ideally all three should match up. See my most recent comment

Noted. I will make sure that MessageData in each path are created according to what relevant keysever endpoint responder does.