Add timestamps to each message

Signed-off-by: Andrea Maria Piana <andrea.maria.piana@gmail.com>

src/status_im/chat/styles/message/datemark.cljs

@@ -7,7 +7,7 @@
 
 (def datemark
   {:opacity          0.5
-   :margin-top       20
+   :margin-top       16
    :height           20})
 
 (def datemark-text

src/status_im/chat/styles/message/message.cljs

@@ -20,12 +20,10 @@
    :height      16})
 
 (defn message-padding-top
-  [{:keys [first-in-date? same-author? same-direction?]}]
+  [{:keys [first-in-group?]}]
-  (cond
+  (if first-in-group?
-    first-in-date?  20
+    8
-    same-author?    8
+    4))
-    same-direction? 16
-    :else           24))
 
 (def message-empty-spacing
   {:height 16})
@@ -50,6 +48,14 @@
             :align-self     align
             :align-items    align})))
 
+(def message-timestamp
+  {:margin-left     5
+   :margin-right    5
+   :margin-bottom   -2
+   :color           colors/gray
+   :opacity         0.5
+   :align-self      :flex-end})
+
 (def selected-message
   {:margin-top  18
    :margin-left 40
@@ -60,6 +66,9 @@
   (merge {:flex-direction :column}
          (last-message-padding message)))
 
+(defn timestamp-content-wrapper [{:keys [outgoing]}]
+  {:flex-direction (if outgoing :row-reverse :row)})
+
 (defn group-message-view
   [outgoing]
   (let [align (if outgoing :flex-end :flex-start)]
@@ -71,11 +80,11 @@
 
 (def delivery-status
   {:align-self    :flex-end
-   :padding-right 56})
+   :padding-right 22})
 
 (def message-author
   {:width      photo-size
-   :align-self :flex-start})
+   :align-self :flex-end})
 
 (def photo
   {:border-radius (/ photo-size 2)

src/status_im/chat/subs.cljs

@@ -84,7 +84,7 @@
   (fn [{:keys [messages]} [_ message-id]]
     (get messages message-id)))
 
-(defn- partition-by-datemark
+(defn- intersperse-datemark
   "Reduce step which expects the input list of messages to be sorted by clock value.
   It makes best effort to group them by day.
   We cannot sort them by :timestamp, as that represents the clock of the sender
@@ -100,80 +100,120 @@
 
   so we bucket both in 1999-12-31"
   [{:keys [acc last-timestamp last-datemark]} {:keys [timestamp datemark] :as msg}]
-  (if (or (empty? acc)                                       ; initial element
+  (cond (empty? acc)                                     ; initial element
-          (and (not= last-datemark datemark)                 ; not the same day
-               (< timestamp last-timestamp)))                ; not out-of-order
         {:last-timestamp timestamp
          :last-datemark  datemark
-     :acc  (conj acc [datemark [msg]])}                      ; add new datemark group
+         :acc            (conj acc msg)}
-    {:last-timestamp (max timestamp last-timestamp)
-     :last-datemark last-datemark
-     :acc (conj (pop acc) (update (peek acc) 1 conj msg))})) ; conj to the last element
 
-(defn message-datemark-groups
+        (and (not= last-datemark datemark)               ; not the same day
-  "Transforms map of messages into sequence of `[datemark messages]` tuples, where
+             (< timestamp last-timestamp))               ; not out-of-order
-  messages with particular datemark are sorted according to their clock-values."
+        {:last-timestamp timestamp
+         :last-datemark  datemark
+         :acc            (conj acc {:value last-datemark ; intersperse datemark message
+                                    :type  :datemark}
+                               msg)}
+        :else
+        {:last-timestamp (max timestamp last-timestamp)  ; use last datemark
+         :last-datemark  last-datemark
+         :acc            (conj acc (assoc msg :datemark last-datemark))}))
+
+(defn sort-messages
+  "Remove hidden messages and sort by clock-value desc, breaking ties by message id"
   [id->messages]
-  (let [sorted-messages     (->> id->messages
+  (->> id->messages
        vals
-                                 (sort-by (juxt (comp unchecked-negate :clock-value) :message-id))) ; sort-by clock in reverse order, break ties by :message-id field
+      (filter :show?)
-        remove-hidden-xf    (filter :show?)
+      (sort-by (juxt (comp unchecked-negate :clock-value) :message-id))))
-        add-datemark-xf     (map (fn [{:keys [timestamp] :as msg}]
+
-                                   (assoc msg :datemark (time/day-relative timestamp))))]
+(defn- add-datemark [{:keys [timestamp] :as msg}]
-    (-> (transduce (comp remove-hidden-xf
+   (assoc msg :datemark (time/day-relative timestamp)))
-                         add-datemark-xf)
+
-                   (completing partition-by-datemark)
+(defn- add-timestamp [{:keys [timestamp] :as msg}]
+  (assoc msg :timestamp-str (time/timestamp->time timestamp)))
+
+(defn intersperse-datemarks
+  "Add a datemark in between an ordered seq of messages when two datemarks are not
+  the same. Ignore messages with out-of-order timestamps"
+  [messages]
+  (when (seq messages)
+    (let [messages-with-datemarks (transduce (comp
+                                               (map add-datemark)
+                                               (map add-timestamp))
+                                             (completing intersperse-datemark :acc)
                                              {:acc []}
-                   sorted-messages)
+                                             messages)]
-        :acc)))
+      ; Append last datemark
+      (conj messages-with-datemarks {:value (:datemark (peek messages-with-datemarks))
+                                     :type  :datemark}))))
+
+(defn- set-previous-message-first-in-group [stream]
+  (conj (pop stream) (assoc (peek stream) :first-in-group? true)))
+
+; any message that comes after this amount of ms will be grouped separately
+(def ^:private group-ms 60000)
+
+(defn add-positional-metadata
+  "Reduce step which adds positional metadata to a message and conditionally
+  update the previous message with :first-in-group?."
+  [{:keys [stream last-outgoing-seen]}
+   {:keys [type from datemark outgoing timestamp] :as message}]
+  (let [previous-message         (peek stream)
+        ; Was the previous message from a different author or this message
+        ; comes after x ms
+        last-in-group?           (or (not= from (:from previous-message))
+                                     (> (- (:timestamp previous-message) timestamp) group-ms))
+        same-direction?          (= outgoing (:outgoing previous-message))
+        ; Have we seen an outgoing message already?
+        last-outgoing?           (and (not last-outgoing-seen)
+                                      outgoing)
+        datemark?                (= :datemark (:type message))
+        ; If this is a datemark or this is the last-message of a group,
+        ; then the previous message was the first
+        previous-first-in-group? (or datemark?
+                                     last-in-group?)
+        new-message              (assoc message
+                                        :same-direction? same-direction?
+                                        :last-in-group? last-in-group?
+                                        :last-outgoing? last-outgoing?)]
+    {:stream             (cond-> stream
+                           previous-first-in-group?
+                           ; update previuous message if necessary
+                           set-previous-message-first-in-group
+
+                           :always
+                           (conj new-message))
+     ; mark the last message sent by the user
+     :last-outgoing-seen (or last-outgoing-seen last-outgoing?)}))
+
+(defn messages-stream
+  "Enhances the messages in message sequence interspersed with datemarks
+  with derived stream context information, like:
+  `:first-in-group?`, `last-in-group?`, `:same-direction?`, `:last?` and `:last-outgoing?` flags."
+  [ordered-messages]
+  (when (seq ordered-messages)
+    (let [initial-message (first ordered-messages)
+          message-with-metadata (assoc initial-message
+                                       :last-in-group? true
+                                       :last? true
+                                       :last-outgoing? (:outgoing initial-message))]
+      (->> (rest ordered-messages)
+           (reduce add-positional-metadata
+                   {:stream             [message-with-metadata]
+                    :last-outgoing-seen (:last-outgoing? message-with-metadata)})
+           :stream))))
 
 (reg-sub
-  :get-chat-message-datemark-groups
+  :get-ordered-chat-messages
   (fn [[_ chat-id]]
     (subscribe [:get-chat chat-id]))
   (fn [{:keys [messages]}]
-    (message-datemark-groups messages)))
+    (sort-messages messages)))
-
-(defn messages-stream
-  "Transforms message-datemark-groups into flat sequence of messages interspersed with
-  datemark messages.
-  Additionaly enhances the messages in message sequence with derived stream context information,
-  like `:same-author?`, `:same-direction?`, `:last?` and `:last-outgoing?` flags. "
-  [message-datemark-groups]
-  (if (seq message-datemark-groups)
-    (let [messages-seq (mapcat second message-datemark-groups)
-          {last-message-id :message-id} (first messages-seq)
-          {last-outgoing-message-id :message-id} (->> messages-seq
-                                                      (filter :outgoing)
-                                                      first)]
-      (->> message-datemark-groups
-           (mapcat (fn [[datemark messages]]
-                     (let [prepared-messages (into []
-                                                   (map (fn [previous-message
-                                                             {:keys [message-id] :as message}
-                                                             next-message]
-                                                          (assoc message
-                                                                 :same-author?            (= (:from message)
-                                                                                             (:from previous-message))
-                                                                 :same-direction?         (= (:outgoing message)
-                                                                                             (:outgoing previous-message))
-                                                                 :last-by-same-author?    (not= (:from message)
-                                                                                                (:from next-message))
-                                                                 :last?                   (= message-id
-                                                                                             last-message-id)
-                                                                 :last-outgoing?          (= message-id
-                                                                                             last-outgoing-message-id)))
-                                                        (concat (rest messages) '(nil))
-                                                        messages
-                                                        (concat '(nil) (butlast messages))))]
-                       (conj prepared-messages {:type :datemark
-                                                :value datemark}))))))))
 
 (reg-sub
   :get-current-chat-messages
   :<- [:get-current-chat]
   (fn [{:keys [messages]}]
-    (-> messages message-datemark-groups messages-stream)))
+    (-> messages sort-messages intersperse-datemarks messages-stream)))
 
 (reg-sub
   :get-commands-for-chat
@@ -335,8 +375,8 @@
 (reg-sub
   :get-last-message
   (fn [[_ chat-id]]
-    (subscribe [:get-chat-message-datemark-groups chat-id]))
+    (subscribe [:get-ordered-chat-messages chat-id]))
-  (comp first second first))
+  first)
 
 (reg-sub
   :chat-animations

src/status_im/chat/views/message/message.cljs

@@ -274,6 +274,9 @@
   (letsubs [{:keys [photo-path]} [:get-current-account]]
     (photo from photo-path)))
 
+(defview message-timestamp [t]
+  [react/text {:style style/message-timestamp} t])
+
 (defview message-author-name [from message-username]
   (letsubs [username [:get-contact-name-by-identity from]]
     [react/text {:style style/message-author-name} (or username
@@ -281,20 +284,21 @@
                                                        (gfycat/generate-gfy from))])) ; TODO: We defensively generate the name for now, to be revisited when new protocol is defined
 
 (defn message-body
-  [{:keys [last-outgoing? last-by-same-author? message-type same-author? from outgoing group-chat username] :as message} content]
+  [{:keys [timestamp-str last-outgoing? last-in-group? message-type first-in-group? from outgoing group-chat username] :as message} content]
   [react/view (style/group-message-wrapper message)
    [react/view (style/message-body message)
+    (when (not outgoing)
       [react/view style/message-author
-     (when last-by-same-author?
+       (when last-in-group?
-       (if outgoing
-         [my-photo from]
          [react/touchable-highlight {:on-press #(re-frame/dispatch [:show-profile from])}
           [react/view
-           [member-photo from]]]))]
+           [member-photo from]]])])
     [react/view (style/group-message-view outgoing)
-     (when-not same-author?
+     (when first-in-group?
        [message-author-name from username])
-     content]]
+     [react/view {:style (style/timestamp-content-wrapper message)}
+       content
+       [message-timestamp timestamp-str]]]]
    (when last-outgoing?
      [react/view style/delivery-status
       (if (= message-type :group-user-message)

src/status_im/utils/datetime.cljs

@@ -32,21 +32,23 @@
           loc (get goog.i18n (str "DateTimeSymbols_" name-first))]
       (or loc goog.i18n.DateTimeSymbols_en))))
 
-;; Closure does not have an enum for datetime formats
+(def medium-date-time-format (.-MEDIUM_DATETIME goog.i18n.DateTimeFormat.Format))
-(def short-date-time-format 10)
+(def medium-date-format (.-MEDIUM_DATE goog.i18n.DateTimeFormat.Format))
-(def short-date-format 2)
+(def short-time-format (.-SHORT_TIME goog.i18n.DateTimeFormat.Format))
 
 (defn mk-fmt [locale format]
   (goog.i18n.DateTimeFormat. format (locale-symbols locale)))
 
 (def date-time-fmt
-  (mk-fmt status-im.i18n/locale short-date-time-format))
+  (mk-fmt status-im.i18n/locale medium-date-time-format))
 (def date-fmt
-  (mk-fmt status-im.i18n/locale short-date-format))
+  (mk-fmt status-im.i18n/locale medium-date-format))
+(def time-fmt
+  (mk-fmt status-im.i18n/locale short-time-format))
 
 (defn- to-str [ms old-fmt-fn yesterday-fmt-fn today-fmt-fn]
   (let [date (from-long ms)
-        local (plus date time-zone-offset)
+        local (plus date time-zone-offset) ; this is wrong, it uses the current timezone offset, regardless of DST
         today (t/today-at-midnight)
         yesterday (plus today (days -1))]
     (cond
@@ -58,7 +60,7 @@
   (to-str ms
           #(.format date-fmt %)
           #(label :t/datetime-yesterday)
-          #(unparse (formatters :hour-minute) %)))
+          #(.format time-fmt %)))
 
 (defn day-relative [ms]
   (to-str ms
@@ -72,7 +74,7 @@
                                     (plus time-zone-offset))))
 
 (defn timestamp->time [ms]
-  (unparse (formatter "HH:mm") (-> ms
+  (.format time-fmt (-> ms
                         from-long
                         (plus time-zone-offset))))
 

test/cljs/status_im/test/chat/subs.cljs

@@ -2,56 +2,135 @@
   (:require [cljs.test :refer-macros [deftest is testing]]
             [status-im.chat.subs :as s]))
 
-
-(defn messages-ordered? [messages]
-  (let [clock-values (map :clock-value messages)]
-    (= (-> clock-values sort reverse) clock-values)))
-
 (deftest test-message-datemark-groups
-  (testing "it orders a map of messages by clock-values when all on the same day (by sender timestamp)"
+  (testing "it orders a map of messages by clock-values desc, breaking ties by message-id asc and removing hidden messages"
-    (let [datemark "Jan 1, 1970"
+    (let [message-1 {:show? true
-          message-1 {:show? true
+                     :message-id "doesn't matter 1"
-                     :timestamp 0
                      :clock-value 1}
           message-2 {:show? true
-                     :timestamp 0
+                     :message-id "doesn't matter 2"
                      :clock-value 2}
           message-3 {:show? true
-                     :timestamp 0
+                     :message-id "does matter 2"
-                     :clock-value 3}
-          unordered-messages {2 message-2
-                              1 message-1
-                              3 message-3}
-          [[actual-datemark actual-messages]] (s/message-datemark-groups unordered-messages)]
-
-      (is (= datemark actual-datemark))
-      (is (= 3 (count actual-messages)))
-      (is (messages-ordered? actual-messages))))
-
-  (testing "it mantains the order even when timestamps are across days"
-    (let [datemark-day-1 "Jan 1, 2000"
-          datemark-day-2 "Dec 31, 1999"
-          message-1 {:show? true
-                     :timestamp 946641600000 ; 1999
-                     :clock-value 1}
-          message-2 {:show? true
-                     :timestamp 946728000000 ; 2000 this will displayed in 1999
-                     :clock-value 2}
-          message-3 {:show? true
-                     :timestamp 946641600000 ; 1999
                      :clock-value 3}
           message-4 {:show? true
-                     :timestamp 946728000000 ; 2000
+                     :message-id "does matter 1"
-                     :clock-value 4}
+                     :clock-value 3}
-          unordered-messages {2 message-2
+          hidden-message {:show? false
-                              1 message-1
+                          :clock-value 1}
-                              4 message-4
+          unordered-messages (->> [message-1
-                              3 message-3}
+                                   message-2
-          [[actual-dm-1 actual-msg-1]
+                                   message-3
-           [actual-dm-2 actual-msg-2]] (s/message-datemark-groups unordered-messages)]
+                                   message-4
+                                   hidden-message]
+                                  (map (juxt :message-id identity))
+                                  shuffle ; clojure maps are sorted for n <= 32
+                                  (into {}))]
+      (is (= [message-4
+              message-3
+              message-2
+              message-1] (s/sort-messages unordered-messages))))))
 
-      (is (= datemark-day-1 actual-dm-1))
+(deftest intersperse-datemarks
-      (is (= datemark-day-2 actual-dm-2))
+  (testing "it mantains the order even when timestamps are across days"
-      (is (= 1 (count actual-msg-1)))
+    (let [message-1 {:timestamp 946641600000} ; 1999}
-      (is (= 3 (count actual-msg-2)))
+          message-2 {:timestamp 946728000000} ; 2000 this will displayed in 1999
-      (is (messages-ordered? (concat actual-msg-1 actual-msg-2))))))
+          message-3 {:timestamp 946641600000} ; 1999
+          message-4 {:timestamp 946728000000} ; 2000
+          ordered-messages [message-4
+                            message-3
+                            message-2
+                            message-1]
+          [m1 d1 m2 m3 m4 d2] (s/intersperse-datemarks ordered-messages)]
+      (is (= "Jan 1, 2000"
+             (:datemark m1)))
+      (is (= {:type :datemark
+              :value "Jan 1, 2000"} d1))
+      (is (= "Dec 31, 1999"
+             (:datemark m2)
+             (:datemark m3)
+             (:datemark m4)))
+      (is (= {:type :datemark
+              :value "Dec 31, 1999"} d2)))))
+
+(deftest message-stream-tests
+  (testing "messages with no interspersed datemarks"
+    (let [m1 {:from "1"
+              :datemark "a"
+              :outgoing false}
+          m2   {:from "2"
+                :datemark "a"
+                :outgoing true}
+          m3    {:from "2"
+                 :datemark "a"
+                 :outgoing true}
+          dm1  {:type :datemark
+                :value "a"}
+          messages [m1 m2 m3 dm1]
+          [actual-m1
+           actual-m2
+           actual-m3] (s/messages-stream messages)]
+      (testing "it marks only the first message as :last?"
+        (is (:last? actual-m1))
+        (is (not (:last? actual-m2)))
+        (is (not (:last? actual-m3))))
+      (testing "it marks the first outgoing message as :last-outgoing?"
+        (is (not (:last-outgoing? actual-m1)))
+        (is (:last-outgoing? actual-m2))
+        (is (not (:last-outgoing? actual-m3))))
+      (testing "it marks the message with :same-direction? when from the same author"
+        (is (not (:same-direction? actual-m1)))
+        (is (not (:same-direction? actual-m2))
+            (is (:same-direction? actual-m3))))
+      (testing "it marks messages from the same author next to another with :first-in-group?"
+        (is (:first-in-group? actual-m1))
+        (is (not (:first-in-group? actual-m2)))
+        (is (:first-in-group? actual-m3)))
+      (testing "it marks the last message from the same author with :last-in-group?"
+        (is (:last-in-group? actual-m1))
+        (is (:last-in-group? actual-m2))
+        (is (not (:last-in-group? actual-m3))))))
+  (testing "messages with interspersed datemarks"
+    (let [m1 {:from "2"          ; first & last in group
+              :timestamp 63000
+              :outgoing true}
+          dm1 {:type :datemark
+               :value "a"}
+          m2  {:from "2"         ; first & last in group as more than 1 minute after previous message
+               :timestamp 62000
+               :outgoing false}
+          m3  {:from "2"         ; last in group
+               :timestamp 1
+               :outgoing false}
+          m4  {:from "2"         ; first in group
+               :timestamp 0
+               :outgoing false}
+          dm2 {:type :datemark
+               :value "b"}
+          messages [m1 dm1 m2 m3 m4 dm2]
+          [actual-m1
+           _
+           actual-m2
+           actual-m3
+           actual-m4
+           _] (s/messages-stream messages)]
+      (testing "it marks the first outgoing message as :last-outgoing?"
+        (is (:last-outgoing? actual-m1))
+        (is (not (:last-outgoing? actual-m2)))
+        (is (not (:last-outgoing? actual-m3)))
+        (is (not (:last-outgoing? actual-m4))))
+      (testing "it resets :same-direction? after a datemark"
+        (is (not (:same-direction? actual-m1)))
+        (is (not (:same-direction? actual-m2))
+            (is (:same-direction? actual-m3)))
+        (is (:same-direction? actual-m4)))
+      (testing "it sets :first-in-group? after a datemark"
+        (is (:first-in-group? actual-m1))
+        (is (:first-in-group? actual-m4)))
+      (testing "it sets :first-in-group? if more than 60s have passed since last message"
+        (is (:first-in-group? actual-m2)))
+      (testing "it sets :last-in-group? after a datemark"
+        (is (:last-in-group? actual-m1))
+        (is (:last-in-group? actual-m2))
+        (is (:last-in-group? actual-m3))
+        (is (not (:last-in-group? actual-m4)))))))

test/cljs/status_im/test/runner.cljs

@@ -9,6 +9,7 @@
             [status-im.test.profile.events]
             [status-im.test.bots.events]
             [status-im.test.chat.models.input]
+            [status-im.test.chat.subs]
             [status-im.test.chat.views.message]
             [status-im.test.i18n]
             [status-im.test.protocol.web3.inbox]
@@ -37,6 +38,7 @@
 (doo-tests
  'status-im.test.utils.async
  'status-im.test.chat.events
+ 'status-im.test.chat.subs
  'status-im.test.accounts.events
  'status-im.test.contacts.events
  'status-im.test.profile.events

test/cljs/status_im/test/utils/datetime.cljs

@@ -4,7 +4,7 @@
             [cljs-time.core :as t]))
 
 (defn match [name symbols]
-  (is (identical? (.-dateTimeSymbols_ (d/mk-fmt name d/short-date-format))
+  (is (identical? (.-dateTimeSymbols_ (d/mk-fmt name d/medium-date-format))
                   symbols)))
 
 (deftest date-time-formatter-test
@@ -23,29 +23,30 @@
 
 (deftest to-short-str-today-test
   (with-redefs [t/*ms-fn* (constantly epoch-plus-3d)
+                d/time-fmt (d/mk-fmt "us" d/short-time-format)
                 d/time-zone-offset (t/period :hours 0)]
-   (is (= (d/to-short-str epoch-plus-3d) "00:00"))))
+   (is (= (d/to-short-str epoch-plus-3d) "12:00 AM"))))
 
 (deftest to-short-str-before-yesterday-us-test
   (with-redefs [t/*ms-fn* (constantly epoch-plus-3d)
                 d/time-zone-offset (t/period :hours 0)
-                d/date-fmt (d/mk-fmt "us" d/short-date-format)]
+                d/date-fmt (d/mk-fmt "us" d/medium-date-format)]
     (is (= (d/to-short-str epoch) "Jan 1, 1970"))))
 
 (deftest to-short-str-before-yesterday-nb-test
   (with-redefs [d/time-zone-offset (t/period :hours 0)
-                d/date-fmt (d/mk-fmt "nb-NO" d/short-date-format)
+                d/date-fmt (d/mk-fmt "nb-NO" d/medium-date-format)
                 t/*ms-fn* (constantly epoch-plus-3d)]
     (is (= (d/to-short-str epoch) "1. jan. 1970"))))
 
 (deftest day-relative-before-yesterday-us-test
   (with-redefs [t/*ms-fn* (constantly epoch-plus-3d)
                 d/time-zone-offset (t/period :hours 0)
-                d/date-fmt (d/mk-fmt "us" d/short-date-time-format)]
+                d/date-fmt (d/mk-fmt "us" d/medium-date-time-format)]
     (is (= (d/day-relative epoch) "Jan 1, 1970, 12:00:00 AM"))))
 
 (deftest day-relative-before-yesterday-nb-test
   (with-redefs [t/*ms-fn* (constantly epoch-plus-3d)
                 d/time-zone-offset (t/period :hours 0)
-                d/date-fmt (d/mk-fmt "nb-NO" d/short-date-time-format)]
+                d/date-fmt (d/mk-fmt "nb-NO" d/medium-date-time-format)]
     (is (= (d/day-relative epoch) "1. jan. 1970, 00:00:00"))))