Add flags protocol message

[adambabik]

1. Upgraded our migration tool because it did not work with the second migration I added. It did not apply it even though it updated the table with the number of currently applied migration.
2. Refactored creating new streams in Messenger.
3. Stream supports multiple handlers now. There is a first handler that adds some UI-related flags (message unread flag) and the second one which stores the message in the database.
4. Added a method to the Database to get unread messages.

[dshulyak]

Stream supports multiple handlers now. There is a first handler that adds some UI-related flags (message unread flag) and the second one which stores the message in the database.

isn't it cleaner to have message to be unread by default? and update it by a client when it is read (e.g. revert flags in this implementation). this way you won't have to have chain of handlers that modify thing in the middle of the process

i am actually not against multple handlers, i think multiple handlers is much better for sending events (comparing to what i added before). but i would avoid changing object in handlers

[adambabik]

isn't it cleaner to have message to be unread by default? and update it by a client when it is read (e.g. revert flags in this implementation). this way you won't have to have chain of handlers that modify thing in the middle of the process

I am not sure that will be always a case. Imagine that you join a new public chat for the first time. The app will download the latest N messages and I don't think you want to mark all of them as unread. You want to mark as unread only those arriving after you joined.

this way you won't have to have chain of handlers that modify thing in the middle of the process

I don't see a better place to do it. I don't want to do it in protocol/v1 or protocol/adapters because it's Messenger's logic to manager unread messages.

but i would avoid changing object in handlers

Any better ideas? We need to have a possibility to adjust messages in Messenger. Also, in this case, it does not even make much sense to put Flags in protocol.Message because Flags is not defined in the protocol spec. But then we would need to create Message in the client/ and use composition with protocol.Message. Maybe that would not be bad...

[dshulyak]

I am not sure that will be always a case. Imagine that you join a new public chat for the first time. The app will download the latest N messages and I don't think you want to mark all of them as unread. You want to mark as unread only those arriving after you joined.

i still think all messages can be unread. and updated to read after the first view or not (looks like this is up to presentation layer). also, an application may query for messages after certain timestamp using clock/timestamp, right? so if you want to show only this new part of unread messages it is still possible but using query parameters. like you know select * from messages where timestamp > chat.joined and unread = true

We need to have a possibility to adjust messages in Messenger

do you have other use cases in mind? i like the composition idea more, a message can be wrapped before saving it with necessary defaults in the same stream. if you see it definitely necessary i would opt for that option

[adambabik]

i still think all messages can be unread. and updated to read after the first view or not

do you have other use cases in mind?

The other case is that you can silence a chat and then you want all messages from that chat to be marked as read by default. Having such a control in client/ will also reduce number of writes to the database because the final message state in many cases can be figured out before the message is inserted into the database.

i like the composition idea more, a message can be wrapped before saving it with necessary defaults in the same stream. if you see it definitely necessary i would opt for that option

Composition seems fine because we already have a few fields that are not in the protocol spec. However, it does not solve the problem which is to set the final state (or very close to being final) of a message before inserting it into the database.

Within Stream, I think it's totally fine to modify a message because its handlers are pretty much forEach with a side-effect or map operations. Alternatively, we can create a separate unit to be responsible for that but I don't see it necessary at this point.

[dshulyak]

The other case is that you can silence a chat and then you want all messages from that chat to be marked as read by default. Having such a control in client/ will also reduce number of writes to the database because the final message state in many cases can be figured out before the message is inserted into the database.

this case is also covered with sql query e.g. and chat.silenced != false. i don' see how it reduces number of writes cause with silenced chat you still want to store a message but don't show counter for new messages (btw telegram shows a counter but with a different color). if contact is blocked thats a different case, and probably message will be discarded and this is not relevant here

generally i would postpone any decision that affects how messages are represented, i don't see how making this decision sooner makes some use cases possible or easier to implement. but making it later gives more options how to implement representation

[adambabik]

i don' see how it reduces number of writes cause with silenced chat you still want to store a message but don't show counter for new messages

So you want to keep message as unread from silenced chats in the database and in the presentation layer write if m.Unread or chat.Silenced then display as read instead of just using m.Unread property?

We can postpone and I can make mark all message unread but I already presented a lot of examples that we need that control in client/.

If in the meanwhile you figure out a better idea that would be great but in my opinion, Stream is a good place to put more handlers that will modify message fields before the message is inserted into the database.

[dshulyak]

So you want to keep message as unread from silenced chats in the database and in the presentation layer write if m.Unread or chat.Silenced then display as read instead of just using m.Unread property?

i thought you are selecting subset of messages from database, so my proposal was to adjust query. but even if it is done later i don't see a problem. for instance what if in next release one more condition will be added, should it affect presentation for chats that already have data in local database?

so for example, in next release decided that instead of not showing unread messages for silenced chat we will show them in gray color. what we will have to do? upgrade messages in the database using migrations and change logic in one of the stream handlers? but instead we could just adjust query (or some filtering logic)

We can postpone and I can make mark all message unread but I already presented a lot of examples that we need that control in client/

you gave two examples. one - is marking message as read if it is sent after we joined a chat (e.g. comparing timestamps). second - marking message as read if chat is silenced (e.g. checking a bool on chat or contact) by postpone i meant that you can save messages as is in database and make a decision at the read time

If in the meanwhile you figure out a better idea that would be great but in my opinion, Stream is a good place to put more handlers that will modify message fields before the message is inserted into the database.

i don't want in some way "block" your decision, if i wasn't able to convince you and proposed solution doesn't seem a better option then i will be fine with a current one

[adambabik]

so for example, in next release decided that instead of not showing unread messages for silenced chat we will show them in gray color. what we will have to do? upgrade messages in the database using migrations and change logic in one of the stream handlers?

Just change the logic of a handler which equals the same amount of work as changing the query. And changing the query might have more severe consequences because it potentially can be used in many places so each place needs to be verified if the new result set is fine.

But that's a good point which does make me less confident whether marking message as read automatically is a good idea :P It seems like premature optimization at this point.

Anyway, original discussion was about where mark messages as unread. Please take a look https://github.com/status-im/status-console-client/pull/78#discussion_r290607578. Even if we use composition, we need to code it somewhere and still Stream handlers seems like the best place to do it to me.

[dshulyak]

Just change the logic of a handler which equals the same amount of work as changing the query. And changing the query might have more severe consequences because it potentially can be used in many places so each place needs to be verified if the new result set is fine.

data stored in local db will have to be changed too. i think changing query will have same consequences as changing result of the query. the difference is updating old data, otherwise experience will be different for chats that were pulled with "new" handler and for "old" handler

Anyway, original discussion was about where mark messages as unread. Please take a look #78 (comment). Even if we use composition, we need to code it somewhere and still Stream handlers seems like the best place to do it to me.

yes, in one of the original comments i meant the same handler, not in the stream before passing to handler

what i suggested in first comment is to invert flag and to have MsgRead instead of MsgUnread, hence all this query discussion

[adambabik]

what i suggested in first comment is to invert flag and to have MsgRead instead of MsgUnread, hence all this query discussion

Ah, that was not obvious :P It sounds good. Let's try this.