Initial reaction support #1

Since there is the client.isMyNick() check this means we prevent some of the typing TAGMSGs from reaching _handleChatMessages() but not all. I think I'd prefer to not have this special case and always pass all of the TAGMSGs to _handleChatMessages().

We could directly build a list of MessageEntry here to remove the need for the privmsgs.map() call below.

Can we move this variable declaration right before we need it, ie. right before the loop on the message entries?

Also I don't think we need to take into account reacts here: reacts don't mark buffers as unread, right?

Could we maybe use the same signature as the MessageEntry constructor here for consistency?

(Was there a reason for not doing this? For instance, constructing this object requires some tags?)

my rationale is that only messages of a particular format can be a ReactionEntry (and we don't really care about the raw message anyway). We already have to do the work of determining whether a message has that format in client.dart, and we would need to duplicate it all here (with a lot of assertions) to get the relevant fields out of an IrcMessage

Hm, msgids will typically contain 1000 entries. This sounds a bit much... What's the performance impact of this function?

This function has almost the same code, and is called in the same context with the same values as fetchMessageSetByNetworkMsgid above, so I was expecting similar performance to existing code

as discussed on IRC, this is called with the whole buffer while fetchMessageSetByNetworkMsgid is only called with reply parents

I restructured the query to only take two message parameters. This should be a safer way to build the sql query. I'm not sure if it's so much more performant (I didn't check that the query plan is optimized with indices) but it should be better since it's all in the sql engine now.

In Goguma "store" usually means "upsert", see for instance storeBuffer() and storeMessage(). So I think we can drop OrUpdate here.

Nit: I slightly prefer for loops over forEach() callbacks.

this isn't actually forEach from Iterable, since Map doesn't implement Iterable (which is why this function takes two values instead of one). There's a clean forEach version of this which uses dart > v3.0.0 and patterns, which I will change to and we'll see which one is better.

This could maybe be simplified with Map.keys and Map.values. But the current logic is fine by me as well.

I personally don't think we should explain how SQLite works here. Readers unfamiliar with ON CONFLICT … DO can easily read the docs.

I left the link to the docs. I agree that everything else is superfluous.

Nit: missing spaces around =

Could we maybe name this _messagesByNetworkMsgid just to make it clear it's different from database IDs?

Or should we use database IDs as keys instead?

Nit: stray empty line.

I'm not sure I understand. Is the concern that msgs might yield different objects when it's iterated over twice?

Yes, this happened in practice and caused some bugs. In particular, some iterables are "consumed" once you iterate over them (with a for loop) while others aren't. If this iterable is consumed in the first call, then it will be empty in the next call.

Would be nice to extract this in a helper function, e.g. _populateMessagesById.

Nit: missing space before }

This should probably be private, since it doesn't notify any listener.

We might have to use it later to when sending a reaction, but I'm not entirely sure. I can always make it unprivate at that point if need-be.

Do we only allow a single reaction per nick?

Yes, each nick can only react once.

Nit: missing space before (

Nit: where is this file coming from?

It think it's autogenerated by the flutter debugger. That or some portion of the dart tooling that I'm using. See https://docs.flutter.dev/tools/devtools/extensions#configure-extension-enablement-states

Either way since it's mentioned in the official flutter docs, I thought it'd be fine to add to the .gitignore

I think the 3 lines above can be reverted to be the same as the previous logic.

Hm, not a fan of whereType() because it's dangerous wrt. type safety. If this doesn't match what's in the entries then all entries are silently dropped?

Nit: this is a map, not a set (ditto for parentSet)

Nit: [] can be used instead of List.empty().

I'm kind of wondering if we should really store the nickname as a field, or if we should take a more Message-like approach where the whole raw IRC message is stored in the DB. The reason is that we may want to access other message tags, e.g. account. I'm unsure we'll want to dedup by nickname on the long run.

Style nit: we do not align SQL keywords (we don't align anything in general)

Nit: maybe reactions could be optional like replyTo

Hm, what caused this?

Seems like this is unused.