tinode / ios

Tinodios: Tinode Messaging Client for iOS
Apache License 2.0
243 stars 107 forks source link

Remove EditHistoryDb #180

Closed aforge closed 2 years ago

aforge commented 2 years ago

This PR changes the logic of message replacement as follows:

or-else commented 2 years ago

BTW, SQLite has a thing called Partial Index. It can be used on effective_seq to enforce unique values when they are not NULL: CREATE UNIQUE INDEX messages_topic_id_effective_seq ON messages(table_id, effective_seq) WHERE effective_seq IS NOT NULL

or-else commented 2 years ago

This approach (and I think your approach too) won't work correctly in case of "A replaced by B replaced by C, when only A and C are downloaded". I don't think it's a big deal though. We can consider such chained replacements a client bug.

aforge commented 2 years ago

Yeah, I think effective_seq is a reasonable solution here. It can be used to avoid incorrectly rewriting head and content when replacement messages arrive in reverse chronologic order.

Additionally, we can also add effective_head and effective_content (that come from replacement messages) in order to keep head and content intact.

Upsides:

LMK what you think.

This approach (and I think your approach too) won't work correctly in case of "A replaced by B replaced by C, when only A > and C are downloaded". I don't think it's a big deal though. We can consider such chained replacements a client bug.

What is the problem here? We don't send replacements for replacements. C is supposed to replace A in this scheme so the new content of A should be fine.

If we didn't download B, it just won't show up in A's edit history.

or-else commented 2 years ago

Additionally, we can also add effective_head and effective_content (that come from replacement messages) in order to keep head and content intact

I don't understand this. With effective_seq you don't need to rewrite anything but effective_seq and replaced_with. I think there is still some misunderstanding. Should we voice chat?

We don't send replacements for replacements.

Right. But someone might create a client which does it. We can just treat such client as buggy.

aforge commented 2 years ago

Additionally, we can also add effective_head and effective_content (that come from replacement messages) in order to keep head and content intact

I don't understand this. With effective_seq you don't need to rewrite anything but effective_seq and replaced_with. I think there is still some misunderstanding. Should we voice chat?

We don't send replacements for replacements.

Right. But someone might create a client which does it. We can just treat such client as buggy.

Changed to effective_seq. PTAL

or-else commented 2 years ago

BTW, I think a non-unique index on topicId + effectiveSeq would be useful. It would be better to create a partial unique index on it, but SQLite.swift does not support partial indexes. It can be created with plain SQL though.

aforge commented 2 years ago

BTW, I think a non-unique index on topicId + effectiveSeq would be useful. It would be better to create a partial unique index on it, but SQLite.swift does not support partial indexes. It can be created with plain SQL though.

Added.