Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: implement audio message component #15594

Merged
merged 1 commit into from
May 4, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Binary file added resources/images/icons2/20x20/[email protected]
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added resources/images/icons2/20x20/[email protected]
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added resources/images/icons2/20x20/[email protected]
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added resources/images/icons2/20x20/[email protected]
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
2 changes: 1 addition & 1 deletion src/quo2/components/record_audio/record_audio/view.cljs
Original file line number Diff line number Diff line change
Expand Up @@ -522,7 +522,7 @@
seeking-audio?]
[:f> soundtrack/f-soundtrack
{:audio-current-time-ms audio-current-time-ms
:player-ref player-ref
:player-ref @player-ref
:seeking-audio? seeking-audio?}]])
(when (or @recording? @reviewing-audio?)
[:f> f-time-counter @recording? @recording-length-ms @ready-to-delete? @reviewing-audio?
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
(let [player-ref (reagent/atom {})
audio-current-time-ms (reagent/atom 0)]
(h/render [:f> soundtrack/f-soundtrack
{:player-ref player-ref
{:player-ref @player-ref
:audio-current-time-ms audio-current-time-ms}])
(-> (h/expect (h/get-by-test-id "soundtrack"))
(.toBeTruthy)))))
Expand All @@ -31,7 +31,7 @@
audio-current-time-ms (reagent/atom 0)]
(h/render [:f> soundtrack/f-soundtrack
{:seeking-audio? seeking-audio?
:player-ref player-ref
:player-ref @player-ref
:audio-current-time-ms audio-current-time-ms}])
(h/fire-event
:on-sliding-start
Expand All @@ -47,7 +47,7 @@
audio-current-time-ms (reagent/atom 0)]
(h/render [:f> soundtrack/f-soundtrack
{:seeking-audio? seeking-audio?
:player-ref player-ref
:player-ref @player-ref
:audio-current-time-ms audio-current-time-ms}])
(h/fire-event
:on-sliding-start
Expand All @@ -69,7 +69,7 @@
audio-current-time-ms (reagent/atom 0)]
(h/render [:f> soundtrack/f-soundtrack
{:seeking-audio? seeking-audio?
:player-ref player-ref
:player-ref @player-ref
:audio-current-time-ms audio-current-time-ms}])
(h/fire-event
:on-sliding-start
Expand Down
10 changes: 6 additions & 4 deletions src/quo2/components/record_audio/soundtrack/view.cljs
Original file line number Diff line number Diff line change
Expand Up @@ -10,20 +10,22 @@
(def ^:private thumb-dark (js/require "../resources/images/icons2/12x12/thumb-dark.png"))

(defn f-soundtrack
[{:keys [audio-current-time-ms player-ref seeking-audio?]}]
(let [audio-duration-ms (audio/get-player-duration @player-ref)]
[{:keys [audio-current-time-ms player-ref style seeking-audio?]}]
(let [audio-duration-ms (audio/get-player-duration player-ref)]
[:<>
[slider/slider
{:test-ID "soundtrack"
:style (style/player-slider-container)
:style (merge
(style/player-slider-container)
(or style {}))
:minimum-value 0
:maximum-value audio-duration-ms
:value @audio-current-time-ms
:on-sliding-start #(reset! seeking-audio? true)
:on-sliding-complete (fn [seek-time]
(reset! seeking-audio? false)
(audio/seek-player
@player-ref
player-ref
seek-time
#(log/debug "[record-audio] on seek - seek time: " seek-time)
#(log/error "[record-audio] on seek - error: " %)))
Expand Down
2 changes: 2 additions & 0 deletions src/quo2/core.cljs
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@
quo2.components.profile.select-profile.view
quo2.components.reactions.reaction
quo2.components.record-audio.record-audio.view
quo2.components.record-audio.soundtrack.view
quo2.components.selectors.disclaimer.view
quo2.components.selectors.filter.view
quo2.components.selectors.selectors.view
Expand Down Expand Up @@ -186,6 +187,7 @@

;;;; RECORD AUDIO
(def record-audio quo2.components.record-audio.record-audio.view/record-audio)
(def soundtrack quo2.components.record-audio.soundtrack.view/f-soundtrack)

;;;; SETTINGS
(def privacy-option quo2.components.settings.privacy-option/card)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
(ns status-im2.contexts.chat.messages.content.audio.component-spec
(:require [status-im2.contexts.chat.messages.content.audio.view :as audio-message]
[test-helpers.component :as h]
[react-native.audio-toolkit :as audio]
[re-frame.core :as re-frame]))

(def message
{:audio-duration-ms 5000
:message-id "message-id"})

(def context
{:in-pinned-view? false})

(defn setup-subs
[subs]
(doseq [keyval subs]
(re-frame/reg-sub
(key keyval)
(fn [_] (val keyval)))))

(h/describe "audio message"
(h/before-each
#(setup-subs {:mediaserver/port 1000}))

(h/test "renders correctly"
(h/render [audio-message/audio-message message context])
(h/is-truthy (h/get-by-label-text :audio-message-container)))

(h/test "press play calls audio/toggle-playpause-player"
(with-redefs [audio/toggle-playpause-player (js/jest.fn)
audio/new-player (fn [_ _ _] {})
audio/destroy-player #()
audio/prepare-player (fn [_ on-success _] (on-success))
audio-message/download-audio-http (fn [_ on-success] (on-success "audio-uri"))]
(h/render [audio-message/audio-message message context])
(h/fire-event
:on-press
(h/get-by-label-text :play-pause-audio-message-button))
(-> (h/expect audio/toggle-playpause-player)
(.toHaveBeenCalledTimes 1))))

(h/test "press play renders error"
(h/render [audio-message/audio-message message context])
(with-redefs [audio/toggle-playpause-player (fn [_ _ _ on-error] (on-error))
audio/new-player (fn [_ _ _] {})
audio/destroy-player #()
audio/prepare-player (fn [_ on-success _] (on-success))
audio-message/download-audio-http (fn [_ on-success] (on-success "audio-uri"))]
(h/fire-event
:on-press
(h/get-by-label-text :play-pause-audio-message-button))
(h/wait-for #(h/is-truthy (h/get-by-label-text :audio-error-label))))))
41 changes: 41 additions & 0 deletions src/status_im2/contexts/chat/messages/content/audio/style.cljs
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
(ns status-im2.contexts.chat.messages.content.audio.style
(:require [quo2.foundations.colors :as colors]
[quo2.theme :as theme]))

(defn container
[]
{:width 295
:height 56
:border-radius 12
:border-width 1
:padding 12
:flex-direction :row
:align-items :center
:justify-content :space-between
:border-color (colors/theme-colors colors/neutral-20 colors/neutral-80)
:background-color (colors/theme-colors colors/neutral-5 colors/neutral-80-opa-40)})

(def play-pause-slider-container
{:flex-direction :row
:align-items :center})

(def slider-container
{:position :absolute
:left 60
:right 71
:bottom nil})

(defn play-pause-container
[]
{:background-color (get-in colors/customization [:blue (if (theme/dark?) 60 50)])
:width 32
:height 32
:border-radius 16
:align-items :center
:justify-content :center})

(def timestamp
{:margin-left 4})

(def error-label
{:margin-bottom 16})
204 changes: 204 additions & 0 deletions src/status_im2/contexts/chat/messages/content/audio/view.cljs
Original file line number Diff line number Diff line change
@@ -0,0 +1,204 @@
(ns status-im2.contexts.chat.messages.content.audio.view
(:require ["react-native-blob-util" :default ReactNativeBlobUtil]
[goog.string :as gstring]
[reagent.core :as reagent]
[react-native.audio-toolkit :as audio]
[status-im2.contexts.chat.messages.content.audio.style :as style]
[react-native.platform :as platform]
[taoensso.timbre :as log]
[quo2.foundations.colors :as colors]
[quo2.core :as quo]
[react-native.core :as rn]
[utils.re-frame :as rf]
[utils.i18n :as i18n]))

(def ^:const media-server-uri-prefix "https://localhost:")
(def ^:const audio-path "/messages/audio")
(def ^:const uri-param "?messageId=")

(defonce active-players (atom {}))
(defonce audio-uris (atom {}))
(defonce progress-timer (atom nil))
(defonce current-player-key (reagent/atom nil))

(defn get-player-key
[message-id in-pinned-view?]
(str in-pinned-view? message-id))

(defn destroy-player
[player-key]
(when-let [player (@active-players player-key)]
(audio/destroy-player player)
(swap! active-players dissoc player-key)))

(defn update-state
[state new-state]
(when-not (= @state new-state)
(reset! state new-state)))

(defn seek-player
[player-key player-state value on-success]
(when-let [player (@active-players player-key)]
(audio/seek-player
player
value
#(when on-success (on-success))
#(update-state player-state :error))
(update-state player-state :seeking)))

(defn download-audio-http
[base64-uri on-success]
(-> (.config ReactNativeBlobUtil (clj->js {:trusty platform/ios?}))
(.fetch "GET" (str base64-uri))
(.then #(on-success (.base64 ^js %)))
(.catch #(log/error "could not fetch audio " base64-uri))))

(defn create-player
[{:keys [progress player-state player-key]} audio-url on-success]
(download-audio-http
audio-url
(fn [base64-data]
(let [player (audio/new-player
(str "data:audio/acc;base64," base64-data)
{:autoDestroy false
:continuesToPlayInBackground false}
(fn []
(update-state player-state :ready-to-play)
(reset! progress 0)
(when (and @progress-timer (= @current-player-key player-key))
(js/clearInterval @progress-timer)
(reset! progress-timer nil))))]
(swap! active-players assoc player-key player)
(audio/prepare-player
player
#(when on-success (on-success))
#(update-state player-state :error)))))
(update-state player-state :preparing))

(defn play-pause-player
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see you tried to separate some of the state management outside the f-audio-message component, but still @briansztamfater, I'd just like to share some thoughts or suggestions.

  1. A very good chunk of this namespace is not concerned or doesn't need to know about hiccup, reagent, etc, it's just raw timers and manipulation of atoms. In my experience in other React projects, this is a perfect candidate for a separate hook with good unit tests.
  2. HTTP effects should happen at re-frame's event layer, ideally.
  3. If we so desire to reduce usages of use-effect, we can also extract the whole state management into re-frame. Of course this is not a trivial task since the code is already written in the style of hooks, but it could lead to the best results.

But to summarize, I think the effectful part should exist as a separate hook, even outside this file, for example in content/audio/hooks.cljs and unit tested.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ilmotta As always, thanks for your valuable feedback, and agree with them. As I have another ongoing time consuming task and given that manual testing went fine, I wanted to merge this PR, and created an issue to keep track of this suggestion 👍 #15816
Will make sure to address it as soon as possible!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for taking the time to create the issue @briansztamfater. I would have done the same, priorities first :)

[{:keys [player-key player-state progress message-id audio-duration-ms seeking-audio?
user-interaction?]
:as params}]
(let [mediaserver-port (rf/sub [:mediaserver/port])
audio-uri (str media-server-uri-prefix
mediaserver-port
audio-path
uri-param
message-id)
player (@active-players player-key)
playing? (= @player-state :playing)]
(when-not playing?
(reset! current-player-key player-key))
(if (and player
(= (@audio-uris player-key) audio-uri))
(audio/toggle-playpause-player
player
(fn []
(update-state player-state :playing)
(when @progress-timer
(js/clearInterval @progress-timer))
(reset! progress-timer
(js/setInterval
(fn []
(let [player (@active-players player-key)
current-time (audio/get-player-current-time player)
playing? (= @player-state :playing)]
(when (and playing? (not @seeking-audio?) (> current-time 0))
(reset! progress current-time))))
100)))
(fn []
(update-state player-state :ready-to-play)
(when (and @progress-timer user-interaction?)
(js/clearInterval @progress-timer)
(reset! progress-timer nil)))
#(update-state player-state :error))
(do
(swap! audio-uris assoc player-key audio-uri)
(destroy-player player-key)
(create-player params
audio-uri
(fn []
(reset! seeking-audio? false)
(if (> @progress 0)
(let [seek-time (* audio-duration-ms @progress)
checked-seek-time (min audio-duration-ms seek-time)]
(seek-player
player-key
player-state
checked-seek-time
#(play-pause-player params)))
(play-pause-player params))))))))

(defn f-audio-message
[player-state progress seeking-audio? {:keys [audio-duration-ms message-id]}
{:keys [in-pinned-view?]}]
(let [player-key (get-player-key message-id in-pinned-view?)
player (@active-players player-key)
duration (if (and player (not (#{:preparing :not-loaded :error} @player-state)))
(audio/get-player-duration player)
audio-duration-ms)
time-secs (quot
(if (or @seeking-audio? (#{:playing :seeking} @player-state))
(if (<= @progress 1) (* duration @progress) @progress)
duration)
1000)]
(rn/use-effect (fn [] #(destroy-player player-key)))
(rn/use-effect
(fn []
(when (and (some? @current-player-key)
(not= @current-player-key player-key)
(= @player-state :playing))
(play-pause-player {:player-key player-key
:player-state player-state
:progress progress
:message-id message-id
:audio-duration-ms duration
:seeking-audio? seeking-audio?
:user-interaction? false})))
[@current-player-key])
(if (= @player-state :error)
[quo/text
{:style style/error-label
:accessibility-label :audio-error-label
:weight :medium
:size :paragraph-2}
(i18n/label :error-loading-audio)]
[rn/view
{:accessibility-label :audio-message-container
:style (style/container)}
[rn/touchable-opacity
{:accessibility-label :play-pause-audio-message-button
:on-press #(play-pause-player {:player-key player-key
:player-state player-state
:progress progress
:message-id message-id
:audio-duration-ms duration
:seeking-audio? seeking-audio?
:user-interaction? true})
:style (style/play-pause-container)}
[quo/icon
(case @player-state
:preparing :i/loading
:playing :i/pause-audio
:i/play-audio)
{:size 20
:color colors/white}]]
[:f> quo/soundtrack
{:style style/slider-container
:audio-current-time-ms progress
:player-ref (@active-players player-key)
:seeking-audio? seeking-audio?}]
[quo/text
{:style style/timestamp
:accessibility-label :audio-duration-label
:weight :medium
:size :paragraph-2}
(gstring/format "%02d:%02d" (quot time-secs 60) (mod time-secs 60))]])))

(defn audio-message
[message context]
(let [player-state (reagent/atom :not-loaded)
progress (reagent/atom 0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@briansztamfater, I see progress is passed along in this file to other functions and it's named as progress-ref, even though it's not a ref. Refs are non-reactive and usually start as (atom nil). If that makes sense, could you rename all instances of progress-ref to just progress, for example? I was confused by the suffix ref when it's a reactive state :)

seeking-audio? (reagent/atom false)]
(fn []
[:f> f-audio-message player-state progress seeking-audio? message context])))
Loading