From 7efcdcb15046c6b6fdfa2d03965a3af00fb9c5a7 Mon Sep 17 00:00:00 2001 From: janherich Date: Fri, 2 Mar 2018 00:35:59 +0100 Subject: [PATCH] Fix message ordering for 1-1 chats --- src/status_im/chat/console.cljs | 36 +++-- src/status_im/chat/handlers.cljs | 38 +++-- src/status_im/chat/models.cljs | 16 +- src/status_im/chat/models/message.cljs | 152 ++++++++++-------- src/status_im/chat/subs.cljs | 12 +- src/status_im/data_store/realm/chats.cljs | 6 +- src/status_im/data_store/realm/messages.cljs | 9 +- .../realm/schemas/account/core.cljs | 9 +- .../realm/schemas/account/v23/core.cljs | 64 ++++++++ .../realm/schemas/account/v23/message.cljs | 32 ++++ src/status_im/protocol/handlers.cljs | 2 +- 11 files changed, 254 insertions(+), 122 deletions(-) create mode 100644 src/status_im/data_store/realm/schemas/account/v23/core.cljs create mode 100644 src/status_im/data_store/realm/schemas/account/v23/message.cljs diff --git a/src/status_im/chat/console.cljs b/src/status_im/chat/console.cljs index 10659f0435..f873545e7c 100644 --- a/src/status_im/chat/console.cljs +++ b/src/status_im/chat/console.cljs @@ -1,7 +1,7 @@ (ns status-im.chat.console (:require [status-im.ui.components.styles :refer [default-chat-color]] [status-im.utils.random :as random] - [status-im.constants :as const] + [status-im.constants :as constants] [status-im.i18n :as i18n] [clojure.string :as string])) @@ -9,29 +9,31 @@ :or {message-id (random/id)}}] {:message-id message-id :outgoing false - :chat-id const/console-chat-id - :from const/console-chat-id + :chat-id constants/console-chat-id + :from constants/console-chat-id :to "me" :content content :content-type content-type}) (def chat - {:chat-id const/console-chat-id - :name (string/capitalize const/console-chat-id) - :color default-chat-color - :group-chat false - :is-active true - :unremovable? true - :timestamp (.getTime (js/Date.)) - :photo-path const/console-chat-id - :contacts [{:identity const/console-chat-id - :text-color "#FFFFFF" - :background-color "#AB7967"}]}) + {:chat-id constants/console-chat-id + :name (string/capitalize constants/console-chat-id) + :color default-chat-color + :group-chat false + :is-active true + :unremovable? true + :timestamp (.getTime (js/Date.)) + :photo-path constants/console-chat-id + :contacts [{:identity constants/console-chat-id + :text-color "#FFFFFF" + :background-color "#AB7967"}] + :last-to-clock-value 0 + :last-from-clock-value 0}) (def contact - {:whisper-identity const/console-chat-id - :name (string/capitalize const/console-chat-id) - :photo-path const/console-chat-id + {:whisper-identity constants/console-chat-id + :name (string/capitalize constants/console-chat-id) + :photo-path constants/console-chat-id :dapp? true :unremovable? true :bot-url "local://console-bot" diff --git a/src/status_im/chat/handlers.cljs b/src/status_im/chat/handlers.cljs index 46f6b50da2..c1c69cbaf4 100644 --- a/src/status_im/chat/handlers.cljs +++ b/src/status_im/chat/handlers.cljs @@ -73,13 +73,15 @@ :create-new-public-chat (fn [{:keys [db]} [_ topic]] (let [exists? (boolean (get-in db [:chats topic])) - chat {:chat-id topic - :name topic - :color components.styles/default-chat-color - :group-chat true - :public? true - :is-active true - :timestamp (random/timestamp)}] + chat {:chat-id topic + :name topic + :color components.styles/default-chat-color + :group-chat true + :public? true + :is-active true + :timestamp (random/timestamp) + :last-to-clock-value 0 + :last-from-clock-value 0}] (merge (when-not exists? {:db (assoc-in db [:chats (:chat-id chat)] chat) @@ -129,16 +131,18 @@ group-name (group-name-from-contacts contacts selected-contacts username)) {:keys [public private]} (protocol/new-keypair!)] - {:chat-id (random/id) - :public-key public - :private-key private - :name chat-name - :color components.styles/default-chat-color - :group-chat true - :group-admin current-public-key - :is-active true - :timestamp (random/timestamp) - :contacts selected-contacts'})) + {:chat-id (random/id) + :public-key public + :private-key private + :name chat-name + :color components.styles/default-chat-color + :group-chat true + :group-admin current-public-key + :is-active true + :timestamp (random/timestamp) + :contacts selected-contacts' + :last-to-clock-value 0 + :last-from-clock-value 0})) (handlers/register-handler-fx :create-new-group-chat-and-open diff --git a/src/status_im/chat/models.cljs b/src/status_im/chat/models.cljs index f9dd7d0911..a0866da56f 100644 --- a/src/status_im/chat/models.cljs +++ b/src/status_im/chat/models.cljs @@ -15,13 +15,15 @@ (defn- create-new-chat [{:keys [db now]} chat-id] (let [name (get-in db [:contacts/contacts chat-id :name])] - {:chat-id chat-id - :name (or name (gfycat/generate-gfy chat-id)) - :color styles/default-chat-color - :group-chat false - :is-active true - :timestamp now - :contacts [{:identity chat-id}]})) + {:chat-id chat-id + :name (or name (gfycat/generate-gfy chat-id)) + :color styles/default-chat-color + :group-chat false + :is-active true + :timestamp now + :contacts [{:identity chat-id}] + :last-to-clock-value 0 + :last-from-clock-value 0})) (defn add-chat "Adds new chat to db & realm, if the chat with same id already exists, justs restores it" diff --git a/src/status_im/chat/models/message.cljs b/src/status_im/chat/models/message.cljs index 2745ac29df..0018d1f8b0 100644 --- a/src/status_im/chat/models/message.cljs +++ b/src/status_im/chat/models/message.cljs @@ -4,7 +4,7 @@ [status-im.chat.events.console :as console-events] [status-im.chat.events.requests :as requests-events] [status-im.chat.models :as chat-model] - [status-im.chat.models.commands :as commands-model] + [status-im.chat.models.commands :as commands-model] [status-im.utils.clocks :as clocks-utils])) (defn- get-current-account @@ -25,7 +25,7 @@ (:ref (get available-commands-responses response-name)))) (defn add-message-to-db - [db chat-id {:keys [message-id clock-value] :as message} current-chat?] + [db chat-id {:keys [message-id from-clock-value to-clock-value] :as message} current-chat?] (let [prepared-message (cond-> (assoc message :chat-id chat-id :appearing? true) @@ -33,43 +33,57 @@ (assoc :appearing? false))] (cond-> (-> db (update-in [:chats chat-id :messages] assoc message-id prepared-message) - (update-in [:chats chat-id :last-clock-value] (fnil max 0) clock-value)) + (update-in [:chats chat-id :last-from-clock-value] max from-clock-value) + (update-in [:chats chat-id :last-to-clock-value] max to-clock-value)) (not current-chat?) (update-in [:chats chat-id :unviewed-messages] (fnil conj #{}) message-id)))) +;; We start with [0 0] ([from-clock-value to-clock-value]) for each participant of 1-1 chat (local perspective on each device). +;; Now for sending, we always increment the to-clock-value and include it in message payload being sent (so only to-clock-value is present in network message). +;; Locally, the sent message always replicates the latest-from-clock-value of the chat. +;; Upon receiving message, receiver reads the to-clock-value of the received message and sets that to be the from-clock-value locally +;; (this will be also the new latest-from-clock-value of the chat), to-clock-value for the message is the latest-to-clock-value of the 1-1 chat`. + +;; All this ensures, that there will be no [from-clock-value to-clock-value] duplicate in chat message on each device + the local order will appear consistent, +;; even if it’s possible it won’t be the same on both devices (for example user A sends 5 messages, during the sending, +;; he receives the message from user B, so his local order will be A1, A2, B, A3, A4, A5, but his messages will take a long time to reach user B, +;; for some reason, so user B will see it as B, A1, A2, A3, A4, A5). +;; I don’t think that’s very problematic and I don’t think we can do much about it without single source of truth where order received messages are serialised +;; and definite order is established (server), it is the case even in the current implementation. + (defn receive [{:keys [db now] :as cofx} - {:keys [from group-id chat-id content-type content message-id timestamp clock-value] + {:keys [from group-id chat-id content-type content message-id timestamp from-clock-value to-clock-value] :as message}] (let [{:keys [current-chat-id view-id access-scope->commands-responses] :contacts/keys [contacts]} db {:keys [public-key] :as current-account} (get-current-account db) - chat-identifier (or group-id chat-id from) - current-chat? (and (= :chat view-id) - (= current-chat-id chat-identifier)) - fx (if (get-in db [:chats chat-identifier]) - (chat-model/upsert-chat cofx {:chat-id chat-identifier - :group-chat (boolean group-id)}) - (chat-model/add-chat cofx chat-identifier)) - chat (get-in fx [:db :chats chat-identifier]) - command-request? (= content-type constants/content-type-command-request) - command (:command content) - enriched-message (cond-> (assoc message - :chat-id chat-identifier - :timestamp (or timestamp now) - :show? true - :clock-value (clocks-utils/receive - clock-value - (:last-clock-value chat))) - public-key - (assoc :user-statuses {public-key (if current-chat? :seen :received)}) - (and command command-request?) - (assoc-in [:content :content-command-ref] - (lookup-response-ref access-scope->commands-responses - current-account - (get-in fx [:db :chats chat-identifier]) - contacts - command)))] + chat-identifier (or group-id chat-id from) + current-chat? (and (= :chat view-id) + (= current-chat-id chat-identifier)) + fx (if (get-in db [:chats chat-identifier]) + (chat-model/upsert-chat cofx {:chat-id chat-identifier + :group-chat (boolean group-id)}) + (chat-model/add-chat cofx chat-identifier)) + {:keys [last-from-clock-value + last-to-clock-value]} (get-in fx [:db :chats chat-identifier]) + command-request? (= content-type constants/content-type-command-request) + command (:command content) + enriched-message (cond-> (assoc message + :chat-id chat-identifier + :timestamp (or timestamp now) + :show? true + :to-clock-value last-to-clock-value + :from-clock-value (or to-clock-value (inc last-from-clock-value))) + public-key + (assoc :user-statuses {public-key (if current-chat? :seen :received)}) + (and command command-request?) + (assoc-in [:content :content-command-ref] + (lookup-response-ref access-scope->commands-responses + current-account + (get-in fx [:db :chats chat-identifier]) + contacts + command)))] (cond-> (-> fx (update :db add-message-to-db chat-identifier enriched-message current-chat?) (assoc :save-message (dissoc enriched-message :new?))) @@ -138,9 +152,9 @@ :from current-account-id}}}))) (defn- generate-message - [{:keys [network-status]} chat-id message] + [{:keys [network-status]} chat-id message] (assoc (select-keys message [:from :message-id]) - :payload (cond-> (select-keys message [:content :content-type :clock-value :timestamp :show?]) + :payload (cond-> (select-keys message [:content :content-type :to-clock-value :timestamp :show?]) (= :offline network-status) (assoc :show? false)))) @@ -158,14 +172,17 @@ (cond (and group-chat (not public?)) (let [{:keys [public-key private-key]} (get chats chat-id)] - {:send-group-message (assoc options - :group-id chat-id - :keypair {:public public-key - :private private-key})}) + {:send-group-message (-> options + (update-in [:message :payload] dissoc :to-clock-value) + (assoc :group-id chat-id + :keypair {:public public-key + :private private-key}))}) (and group-chat public?) - {:send-public-group-message (assoc options :group-id chat-id - :username (get-in accounts [current-account-id :name]))} + {:send-public-group-message (-> options + (update-in [:message :payload] dissoc :to-clock-value) + (assoc :group-id chat-id + :username (get-in accounts [current-account-id :name])))} :else (merge {:send-message (assoc-in options [:message :to] chat-id)} @@ -175,16 +192,17 @@ (defn- prepare-message [params chat now random-id] (let [{:keys [chat-id identity message-text]} params - {:keys [group-chat public? last-clock-value]} chat - message {:message-id random-id - :chat-id chat-id - :content message-text - :from identity - :content-type constants/text-content-type - :outgoing true - :timestamp now - :clock-value (clocks-utils/send last-clock-value) - :show? true}] + {:keys [group-chat public? last-from-clock-value last-to-clock-value]} chat + message {:message-id random-id + :chat-id chat-id + :content message-text + :from identity + :content-type constants/text-content-type + :outgoing true + :timestamp now + :from-clock-value last-from-clock-value + :to-clock-value (inc last-to-clock-value) + :show? true}] (cond-> message (not group-chat) (assoc :message-type :user-message @@ -208,7 +226,7 @@ (merge fx (send cofx params')))) (defn- prepare-command - [identity chat-id now clock-value + [identity chat-id now last-from-clock-value last-to-clock-value {request-params :params request-command :command :keys [prefill prefillBotDb] @@ -231,21 +249,22 @@ :short-preview (:short-preview command) :bot (or (:bot command) (:owner-id command)))] - {:message-id id - :from identity - :to chat-id - :timestamp now - :content content' - :content-type (or content-type - (if request - constants/content-type-command-request - constants/content-type-command)) - :outgoing true - :to-message to-message - :type (:type command) - :has-handler (:has-handler command) - :clock-value (clocks-utils/send clock-value) - :show? true})) + {:message-id id + :from identity + :to chat-id + :timestamp now + :content content' + :content-type (or content-type + (if request + constants/content-type-command-request + constants/content-type-command)) + :outgoing true + :to-message to-message + :type (:type command) + :has-handler (:has-handler command) + :to-clock-value (inc last-to-clock-value) + :from-clock-value last-from-clock-value + :show? true})) (defn send-command [{{:keys [current-public-key chats] :as db} :db :keys [now random-id-seq] :as cofx} add-to-chat-id params] @@ -254,11 +273,12 @@ :as content} :command chat-id :chat-id} params request (:request handler-data) - last-clock-value (get-in chats [chat-id :last-clock-value]) + {:keys [last-to-clock-value + last-from-clock-value]} (get chats chat-id) hidden-params (->> (:params command) (filter :hidden) (map :name)) - command' (prepare-command current-public-key chat-id now last-clock-value request content) + command' (prepare-command current-public-key chat-id now last-to-clock-value last-from-clock-value request content) params' (assoc params :command command') fx (-> (chat-model/upsert-chat cofx {:chat-id chat-id}) (update :db add-message-to-db chat-id command' true) diff --git a/src/status_im/chat/subs.cljs b/src/status_im/chat/subs.cljs index 2ef01424b7..d56c2ce8db 100644 --- a/src/status_im/chat/subs.cljs +++ b/src/status_im/chat/subs.cljs @@ -90,10 +90,11 @@ (defn message-datemark-groups "Transforms map of messages into sequence of `[datemark messages]` tuples, where - messages with particular datemark are sorted according to their `:clock-value` and - tuples themeselves are sorted according to the highest `:clock-value` in the messages." + messages with particular datemark are sorted according to their clock-values and + tuples themeselves are sorted according to the highest clock-values in the messages." [id->messages] - (let [datemark->messages (transduce (comp (map second) + (let [clock-sorter (juxt :from-clock-value :to-clock-value) + datemark->messages (transduce (comp (map second) (filter :show?) (map (fn [{:keys [timestamp] :as msg}] (assoc msg :datemark (time/day-relative timestamp))))) @@ -103,8 +104,9 @@ id->messages)] (->> datemark->messages (map (fn [[datemark messages]] - [datemark (sort-by :clock-value > messages)])) - (sort-by (comp :clock-value first second) >)))) + [datemark (->> messages (sort-by clock-sorter) reverse)])) + (sort-by (comp clock-sorter first second)) + reverse))) (reg-sub :get-chat-message-datemark-groups diff --git a/src/status_im/data_store/realm/chats.cljs b/src/status_im/data_store/realm/chats.cljs index beae90cfda..e27e7f727f 100644 --- a/src/status_im/data_store/realm/chats.cljs +++ b/src/status_im/data_store/realm/chats.cljs @@ -7,10 +7,12 @@ (:refer-clojure :exclude [exists?])) (defn- normalize-chat [{:keys [chat-id] :as chat}] - (let [last-message (messages/get-last-message chat-id)] + (let [last-to-clock-value (messages/get-last-clock-value chat-id :to-clock-value) + last-from-clock-value (messages/get-last-clock-value chat-id :from-clock-value)] (-> chat (realm/fix-map->vec :contacts) - (assoc :last-clock-value (or (:clock-value last-message) 0))))) + (assoc :last-to-clock-value (or last-to-clock-value 0)) + (assoc :last-from-clock-value (or last-from-clock-value 0))))) (defn get-all-active [] diff --git a/src/status_im/data_store/realm/messages.cljs b/src/status_im/data_store/realm/messages.cljs index a68857ced6..bb723c5dbc 100644 --- a/src/status_im/data_store/realm/messages.cljs +++ b/src/status_im/data_store/realm/messages.cljs @@ -52,11 +52,12 @@ (realm/page from (+ from number-of-messages)) realm/js-object->clj)) -(defn get-last-message - [chat-id] +(defn get-last-clock-value + [chat-id clock-prop] (-> (realm/get-by-field @realm/account-realm :message :chat-id chat-id) - (realm/sorted :clock-value :desc) - (realm/single-clj))) + (realm/sorted clock-prop :desc) + (realm/single-clj) + (get clock-prop))) (defn get-unviewed [current-public-key] diff --git a/src/status_im/data_store/realm/schemas/account/core.cljs b/src/status_im/data_store/realm/schemas/account/core.cljs index b490dceecb..981db10565 100644 --- a/src/status_im/data_store/realm/schemas/account/core.cljs +++ b/src/status_im/data_store/realm/schemas/account/core.cljs @@ -20,7 +20,8 @@ [status-im.data-store.realm.schemas.account.v19.core :as v19] [status-im.data-store.realm.schemas.account.v20.core :as v20] [status-im.data-store.realm.schemas.account.v21.core :as v21] - [status-im.data-store.realm.schemas.account.v22.core :as v22])) + [status-im.data-store.realm.schemas.account.v22.core :as v22] + [status-im.data-store.realm.schemas.account.v23.core :as v23])) ;; TODO(oskarth): Add failing test if directory vXX exists but isn't in schemas. @@ -91,5 +92,7 @@ :migration v21/migration} {:schema v22/schema :schemaVersion 22 - :migration v22/migration}]) - + :migration v22/migration} + {:schema v23/schema + :schemaVersion 23 + :migration v23/migration}]) diff --git a/src/status_im/data_store/realm/schemas/account/v23/core.cljs b/src/status_im/data_store/realm/schemas/account/v23/core.cljs new file mode 100644 index 0000000000..0446a93394 --- /dev/null +++ b/src/status_im/data_store/realm/schemas/account/v23/core.cljs @@ -0,0 +1,64 @@ +(ns status-im.data-store.realm.schemas.account.v23.core + (:require [status-im.data-store.realm.schemas.account.v22.chat :as chat] + [status-im.data-store.realm.schemas.account.v1.chat-contact :as chat-contact] + [status-im.data-store.realm.schemas.account.v19.contact :as contact] + [status-im.data-store.realm.schemas.account.v20.discover :as discover] + [status-im.data-store.realm.schemas.account.v23.message :as message] + [status-im.data-store.realm.schemas.account.v12.pending-message :as pending-message] + [status-im.data-store.realm.schemas.account.v1.processed-message :as processed-message] + [status-im.data-store.realm.schemas.account.v19.request :as request] + [status-im.data-store.realm.schemas.account.v19.user-status :as user-status] + [status-im.data-store.realm.schemas.account.v5.contact-group :as contact-group] + [status-im.data-store.realm.schemas.account.v5.group-contact :as group-contact] + [status-im.data-store.realm.schemas.account.v8.local-storage :as local-storage] + [status-im.data-store.realm.schemas.account.v21.browser :as browser] + [goog.object :as object] + [taoensso.timbre :as log] + [cljs.reader :as reader] + [clojure.string :as string])) + +(def schema [chat/schema + chat-contact/schema + contact/schema + discover/schema + message/schema + pending-message/schema + processed-message/schema + request/schema + user-status/schema + contact-group/schema + group-contact/schema + local-storage/schema + browser/schema]) + +(defn update-new-message [new-realm message-id to-clock-value from-clock-value] + (when-let [message (some-> new-realm + (.objects "message") + (.filtered (str "message-id = \"" message-id "\"")) + (aget 0))] + (aset message "to-clock-value" to-clock-value) + (aset message "from-clock-value" from-clock-value))) + +(defn update-chat-messages [old-realm new-realm chat-id] + (let [from-clock-value (atom 0) + to-clock-value (atom 0)] + (some-> old-realm + (.objects "message") + (.filtered (str "chat-id = \"" chat-id "\"")) + (.sorted "clock-value" false) + (.map (fn [message _ _] + (let [message-id (object/get message "message-id") + outgoing? (boolean (object/get message "outgoing"))] + (if outgoing? + (update-new-message new-realm message-id (swap! to-clock-value inc) @from-clock-value) + (update-new-message new-realm message-id @to-clock-value (swap! from-clock-value inc))))))))) + +(defn update-chats [old-realm new-realm] + (some-> new-realm + (.objects "chat") + (.map (fn [chat _ _] + (update-chat-messages old-realm new-realm (object/get chat "chat-id")))))) + +(defn migration [old-realm new-realm] + (log/debug "migrating v23 account database: " old-realm new-realm) + (update-chats old-realm new-realm)) diff --git a/src/status_im/data_store/realm/schemas/account/v23/message.cljs b/src/status_im/data_store/realm/schemas/account/v23/message.cljs new file mode 100644 index 0000000000..312a7b3eae --- /dev/null +++ b/src/status_im/data_store/realm/schemas/account/v23/message.cljs @@ -0,0 +1,32 @@ +(ns status-im.data-store.realm.schemas.account.v23.message) + +(def schema {:name :message + :primaryKey :message-id + :properties {:message-id :string + :from :string + :to {:type :string + :optional true} + :group-id {:type :string + :optional true} + :content :string ; TODO make it ArrayBuffer + :content-type :string + :username {:type :string + :optional true} + :timestamp :int + :chat-id {:type :string + :indexed true} + :outgoing :bool + :retry-count {:type :int + :default 0} + :message-type {:type :string + :optional true} + :message-status {:type :string + :optional true} + :user-statuses {:type :list + :objectType :user-status} + :from-clock-value {:type :int + :default 0} + :to-clock-value {:type :int + :default 0} + :show? {:type :bool + :default true}}}) diff --git a/src/status_im/protocol/handlers.cljs b/src/status_im/protocol/handlers.cljs index a1923bcbd3..0fc5427544 100644 --- a/src/status_im/protocol/handlers.cljs +++ b/src/status_im/protocol/handlers.cljs @@ -470,7 +470,7 @@ route-fx (case type (:message :group-message - :public-group-message) {:dispatch [:pre-received-message (transform-protocol-message message)]} + :public-group-message) {:dispatch [:chat-received-message/add (transform-protocol-message message)]} :pending (cond-> {::pending-messages-save message} chat-message (assoc :dispatch