From e77504c26fb9ee0263e7fc7e0d20dc5af7226f42 Mon Sep 17 00:00:00 2001 From: Andrea Maria Piana Date: Mon, 26 Mar 2018 10:03:43 +0100 Subject: [PATCH] Revert "Fix message ordering for 1-1 chats" This reverts commit 7efcdcb15046c6b6fdfa2d03965a3af00fb9c5a7. We decided to revert this commit as it introduced a couple of issues in message ordering. In order to release we revert back to the old behaviour. Signed-off-by: Julien Eluard --- src/status_im/chat/console.cljs | 36 ++--- src/status_im/chat/handlers.cljs | 8 +- src/status_im/chat/models.cljs | 16 +- src/status_im/chat/models/message.cljs | 153 ++++++++---------- 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, 108 insertions(+), 239 deletions(-) delete mode 100644 src/status_im/data_store/realm/schemas/account/v23/core.cljs delete 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 f873545e7c..10659f0435 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 constants] + [status-im.constants :as const] [status-im.i18n :as i18n] [clojure.string :as string])) @@ -9,31 +9,29 @@ :or {message-id (random/id)}}] {:message-id message-id :outgoing false - :chat-id constants/console-chat-id - :from constants/console-chat-id + :chat-id const/console-chat-id + :from const/console-chat-id :to "me" :content content :content-type content-type}) (def chat - {: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}) + {: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"}]}) (def contact - {:whisper-identity constants/console-chat-id - :name (string/capitalize constants/console-chat-id) - :photo-path constants/console-chat-id + {:whisper-identity const/console-chat-id + :name (string/capitalize const/console-chat-id) + :photo-path const/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 ee18dc8bd1..e1b75c6ce0 100644 --- a/src/status_im/chat/handlers.cljs +++ b/src/status_im/chat/handlers.cljs @@ -81,9 +81,7 @@ :group-chat true :public? true :is-active true - :timestamp now - :last-to-clock-value 0 - :last-from-clock-value 0}] + :timestamp now}] (merge (when-not exists? {:db (assoc-in db [:chats (:chat-id chat)] chat) @@ -142,9 +140,7 @@ :group-admin current-public-key :is-active true :timestamp timestamp - :contacts selected-contacts' - :last-to-clock-value 0 - :last-from-clock-value 0})) + :contacts selected-contacts'})) (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 a0866da56f..f9dd7d0911 100644 --- a/src/status_im/chat/models.cljs +++ b/src/status_im/chat/models.cljs @@ -15,15 +15,13 @@ (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}] - :last-to-clock-value 0 - :last-from-clock-value 0})) + {: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}]})) (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 15ad7ace6a..bc1947f527 100644 --- a/src/status_im/chat/models/message.cljs +++ b/src/status_im/chat/models/message.cljs @@ -5,7 +5,8 @@ [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 [{:accounts/keys [accounts current-account-id]}] @@ -25,7 +26,7 @@ (:ref (get available-commands-responses response-name)))) (defn add-message-to-db - [db chat-id {:keys [message-id from-clock-value to-clock-value] :as message} current-chat?] + [db chat-id {:keys [message-id clock-value] :as message} current-chat?] (let [prepared-message (cond-> (assoc message :chat-id chat-id :appearing? true) @@ -33,57 +34,43 @@ (assoc :appearing? false))] (cond-> (-> db (update-in [:chats chat-id :messages] assoc message-id prepared-message) - (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)) + (update-in [:chats chat-id :last-clock-value] (fnil max 0) 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 from-clock-value to-clock-value] + {:keys [from group-id chat-id content-type content message-id timestamp 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)) - {: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)))] + 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)))] (cond-> (-> fx (update :db add-message-to-db chat-identifier enriched-message current-chat?) (assoc :save-message (dissoc enriched-message :new?))) @@ -152,9 +139,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 :to-clock-value :timestamp :show?]) + :payload (cond-> (select-keys message [:content :content-type :clock-value :timestamp :show?]) (= :offline network-status) (assoc :show? false)))) @@ -172,17 +159,14 @@ (cond (and group-chat (not public?)) (let [{:keys [public-key private-key]} (get chats chat-id)] - {:send-group-message (-> options - (update-in [:message :payload] dissoc :to-clock-value) - (assoc :group-id chat-id - :keypair {:public public-key - :private private-key}))}) + {:send-group-message (assoc options + :group-id chat-id + :keypair {:public public-key + :private private-key})}) (and group-chat public?) - {: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])))} + {:send-public-group-message (assoc options :group-id chat-id + :username (get-in accounts [current-account-id :name]))} :else (merge {:send-message (assoc-in options [:message :to] chat-id)} @@ -192,17 +176,16 @@ (defn- prepare-message [params chat now random-id] (let [{:keys [chat-id identity message-text]} params - {: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}] + {: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}] (cond-> message (not group-chat) (assoc :message-type :user-message @@ -226,7 +209,7 @@ (merge fx (send cofx params')))) (defn- prepare-command - [identity chat-id now last-from-clock-value last-to-clock-value + [identity chat-id now clock-value {request-params :params request-command :command :keys [prefill prefillBotDb] @@ -249,22 +232,21 @@ :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) - :to-clock-value (inc last-to-clock-value) - :from-clock-value last-from-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) + :clock-value (clocks-utils/send clock-value) + :show? true})) (defn send-command [{{:keys [current-public-key chats]} :db :keys [now random-id-seq] :as cofx} params] @@ -273,12 +255,11 @@ :as content} :command chat-id :chat-id} params request (:request handler-data) - {:keys [last-to-clock-value - last-from-clock-value]} (get chats chat-id) + last-clock-value (get-in chats [chat-id :last-clock-value]) hidden-params (->> (:params command) (filter :hidden) (map :name)) - command' (prepare-command current-public-key chat-id now last-to-clock-value last-from-clock-value request content) + command' (prepare-command current-public-key chat-id now last-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 b68d8e38c8..c267e21d26 100644 --- a/src/status_im/chat/subs.cljs +++ b/src/status_im/chat/subs.cljs @@ -93,11 +93,10 @@ (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-values and - tuples themeselves are sorted according to the highest clock-values in the messages." + 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." [id->messages] - (let [clock-sorter (juxt :from-clock-value :to-clock-value) - datemark->messages (transduce (comp (map second) + (let [datemark->messages (transduce (comp (map second) (filter :show?) (map (fn [{:keys [timestamp] :as msg}] (assoc msg :datemark (time/day-relative timestamp))))) @@ -107,9 +106,8 @@ id->messages)] (->> datemark->messages (map (fn [[datemark messages]] - [datemark (->> messages (sort-by clock-sorter) reverse)])) - (sort-by (comp clock-sorter first second)) - reverse))) + [datemark (sort-by :clock-value > messages)])) + (sort-by (comp :clock-value first second) >)))) (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 f70568ba12..4e9da07f6f 100644 --- a/src/status_im/data_store/realm/chats.cljs +++ b/src/status_im/data_store/realm/chats.cljs @@ -7,12 +7,10 @@ (:refer-clojure :exclude [exists?])) (defn- normalize-chat [{:keys [chat-id] :as chat}] - (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)] + (let [last-message (messages/get-last-message chat-id)] (-> chat (realm/fix-map->vec :contacts) - (assoc :last-to-clock-value (or last-to-clock-value 0)) - (assoc :last-from-clock-value (or last-from-clock-value 0))))) + (assoc :last-clock-value (or (:clock-value last-message) 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 bb723c5dbc..a68857ced6 100644 --- a/src/status_im/data_store/realm/messages.cljs +++ b/src/status_im/data_store/realm/messages.cljs @@ -52,12 +52,11 @@ (realm/page from (+ from number-of-messages)) realm/js-object->clj)) -(defn get-last-clock-value - [chat-id clock-prop] +(defn get-last-message + [chat-id] (-> (realm/get-by-field @realm/account-realm :message :chat-id chat-id) - (realm/sorted clock-prop :desc) - (realm/single-clj) - (get clock-prop))) + (realm/sorted :clock-value :desc) + (realm/single-clj))) (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 981db10565..b490dceecb 100644 --- a/src/status_im/data_store/realm/schemas/account/core.cljs +++ b/src/status_im/data_store/realm/schemas/account/core.cljs @@ -20,8 +20,7 @@ [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.v23.core :as v23])) + [status-im.data-store.realm.schemas.account.v22.core :as v22])) ;; TODO(oskarth): Add failing test if directory vXX exists but isn't in schemas. @@ -92,7 +91,5 @@ :migration v21/migration} {:schema v22/schema :schemaVersion 22 - :migration v22/migration} - {:schema v23/schema - :schemaVersion 23 - :migration v23/migration}]) + :migration v22/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 deleted file mode 100644 index 0446a93394..0000000000 --- a/src/status_im/data_store/realm/schemas/account/v23/core.cljs +++ /dev/null @@ -1,64 +0,0 @@ -(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 deleted file mode 100644 index 312a7b3eae..0000000000 --- a/src/status_im/data_store/realm/schemas/account/v23/message.cljs +++ /dev/null @@ -1,32 +0,0 @@ -(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 c31e30b9f4..296340d866 100644 --- a/src/status_im/protocol/handlers.cljs +++ b/src/status_im/protocol/handlers.cljs @@ -473,7 +473,7 @@ route-fx (case type (:message :group-message - :public-group-message) {:dispatch [:chat-received-message/add (transform-protocol-message message)]} + :public-group-message) {:dispatch [:pre-received-message (transform-protocol-message message)]} :pending (cond-> {::pending-messages-save message} chat-message (assoc :dispatch