[#21658] - Avatars blinking #21782
[#21658] - Avatars blinking #21782
Conversation
ulisesmac
requested review from
ilmotta,
alwx,
seanstrom,
flexsurfer and
mohsen-ghafouri
src/react_native/fast_image.cljs
Outdated
| on-image-loaded (fn [event on-load source] | ||
| (when (fn? on-load) (on-load event)) | ||
| (reset! loaded? true) | ||
| (reset! error? false) | ||
| (reset! previous-source source))] |
| (defn base-list-props | ||
| [{:keys [key-fn render-fn empty-component header footer separator data render-data on-drag-end-fn] | ||
| [{:keys [key-fn data render-fn empty-component header footer separator render-data on-drag-end-fn] | ||
| :as props}] | ||
| (merge | ||
| {:data (to-array data)} | ||
| (when key-fn {:keyExtractor (wrap-key-fn key-fn)}) | ||
| (when render-fn {:renderItem (wrap-render-fn render-fn render-data)}) | ||
| (when separator {:ItemSeparatorComponent (fn [] (reagent/as-element separator))}) | ||
| (when empty-component {:ListEmptyComponent (fn [] (reagent/as-element empty-component))}) | ||
| (when header {:ListHeaderComponent (reagent/as-element header)}) | ||
| (when footer {:ListFooterComponent (reagent/as-element footer)}) | ||
| (when on-drag-end-fn {:onDragEnd (wrap-on-drag-end-fn on-drag-end-fn)}) | ||
| (dissoc props :data :header :footer :empty-component :separator :render-fn :key-fn :on-drag-end-fn))) | ||
| (cond-> {:data (to-array data)} | ||
| key-fn (assoc :keyExtractor (wrap-key-fn key-fn)) | ||
| render-fn (assoc :renderItem (wrap-render-fn render-fn render-data)) | ||
| separator (assoc :ItemSeparatorComponent (fn [] (reagent/as-element separator))) | ||
| empty-component (assoc :ListEmptyComponent (fn [] (reagent/as-element empty-component))) | ||
| header (assoc :ListHeaderComponent (reagent/as-element header)) | ||
| footer (assoc :ListFooterComponent (reagent/as-element footer)) | ||
| on-drag-end-fn (assoc :onDragEnd (wrap-on-drag-end-fn on-drag-end-fn)) | ||
| :always (merge (dissoc-custom-props props)))) | ||
|
|
There was a problem hiding this comment.
multiple merge -> multiple assoc
| (defn f-avatar | ||
| (def scroll-range #js [0 48]) | ||
| (def scale-range #js [1 0.4]) | ||
|
|
||
| (defn view | ||
| [{:keys [scroll-y full-name online? profile-picture customization-color border-color]}] | ||
| (let [image-scale-animation (reanimated/interpolate scroll-y | ||
| scroll-animation-input-range | ||
| [1 0.4] | ||
| header-extrapolation-option) | ||
| image-top-margin-animation (reanimated/interpolate scroll-y | ||
| scroll-animation-input-range | ||
| [0 20] | ||
| header-extrapolation-option) | ||
| image-side-margin-animation (reanimated/interpolate scroll-y | ||
| scroll-animation-input-range | ||
| [-4 -20] | ||
| header-extrapolation-option)] | ||
| [reanimated/view | ||
| {:style (style/wrapper {:scale image-scale-animation | ||
| :margin-top image-top-margin-animation | ||
| :margin image-side-margin-animation | ||
| :border-color border-color})} | ||
| (let [image-scale (reanimated/interpolate scroll-y scroll-range scale-range :clamp)] | ||
| [reanimated/view {:style (style/wrapper border-color image-scale)} |
There was a problem hiding this comment.
Avatar animation fix
| :render-fn (fn [item] | ||
| (chat-list-item/chat-list-item item theme)) | ||
| :render-data {:theme theme} | ||
| :render-fn chat-list-item/chat-list-item |
There was a problem hiding this comment.
Fix rerenders on items for this flat-list
| (defn avatar-border-color | ||
| [theme] | ||
| (if platform/android? | ||
| colors/neutral-80-opa-80 ;; Fix is not needed because Android doesn't use blur | ||
| (colors/theme-colors colors/border-avatar-light colors/neutral-80-opa-80 theme))) |
There was a problem hiding this comment.
Avatar's ring fix
There was a problem hiding this comment.
Worth noting that we have an issue that we can play in 2.33 to remove identity rings #21743
There was a problem hiding this comment.
This ring I'm referring to is the one showed in PR's video:
avatar-circle-not-matching.mp4
different to the identity ring
Jenkins BuildsClick to see older builds (9)
|
|
hey @ulisesmac do you know why we didn't have |
| (defn avatar-border-color | ||
| [theme] | ||
| (if platform/android? | ||
| colors/neutral-80-opa-80 ;; Fix is not needed because Android doesn't use blur | ||
| (colors/theme-colors colors/border-avatar-light colors/neutral-80-opa-80 theme))) |
There was a problem hiding this comment.
Worth noting that we have an issue that we can play in 2.33 to remove identity rings #21743
src/react_native/fast_image.cljs
Outdated
| :priority :high} | ||
| source)) | ||
|
|
||
| ;; NOTE: We need to use ratoms to avoid the flickering since their state is updated |
There was a problem hiding this comment.
@ulisesmac I thought all image flickers were caused by buggy implementation (e.g. components receiving unstable function references as we both identified in #21658).
If the new props passed to fast-image are equal to previous props, Reagent would skip processing fast-image altogether and there would be no flicker. If you could pick one example where this workaround is needed, could you explain what's changing between render passes of fast-image that is causing Reagent to reprocess it?
|
Thank you so much for your review!
Yeah, that's true for unnecessary rerenders, but not all image flickerings are due to this.
This is a side effect of the image server. I added videos to the description, here's an example: We use an URL And now, you may wonder
The answer is that our wrapper for What I did in this PR is implementing a second bg image that avoids that visual blank state. It's just a visual fix. This fix also improved the appearance of necessary re-renders. e.g. When the user `updates their profile name, the avatar blinks on develop, in this PR this no longer happens. LMK if you want me to record videos comparing the differences. IMO this fix is relatively cheap and provides a lot the looking of the app.
The image server dynamic port is a reason, another reason is when we just update its props, so I'd say the issue has been there all the time. Maybe on some fast iOS devices the rerender is too fast to be easily perceived |
|
Thanks @ulisesmac for the detailed answer. The fast image cache strategy we use is the default, The best possible implementation I think is if we could leverage the A change of port shouldn't be considered a change of resource between the client & status-go communication. But I think fast-image doesn't provide a way to customize the logic to decide when the cache is stale or not, at least as far as the official docs go. What if we provide a custom React key that's stable between re-renders by ignoring the port change? Wouldn't that solve the flashing?
For loading, we should ideally differenciate between fetching local resources and actual remote resources. The loading spinner should only be used for non-local resources, similar to how we avoid in UX to show spinners for resources loading in milliseconds. Images coming from the media-server are local as far as I understand. For error states I'm not sure what to say, but it should be a rare problem that wouldn't justify flashing side-effects. |
I agree, or explore better solutions for the image-server.
The problem is: if we skip the rerender, how do we know when the image should have been updated? E.g. the user actually had their profile picture updated but we ignored the rerender.
For local resources we shouldn't use fast-image, I agree. But even if no spinners or error states were displayed, the fast image still shows a blink (since it throws the previous image and loads the new one) |
The idea is that the port number shouldn't be considered part of the cache key for FastImage or any other mechanism. If we use Don't judge too much the following snippet @ulisesmac, but it's a starting point and seems to work well. I wouldn't be surprised if this idea is flawed. For simplification purposes I ignored loading and error conditions. (defn clean-port
[uri]
(string/replace-first uri #":\d+" ""))
(def fast-image
(reagent/adapt-react-class
(rn/memo
(fn [^js props]
(let [source (oops/oget props :source)]
(react/createElement
FastImage
(js/Object.assign
#js {}
props
(when (and props source)
#js {:source (if (string? source)
#js {:uri source
:priority "high"}
#js {:uri (oops/oget source :uri)
:priority "high"})})))))
(fn [prev-props next-props]
(let [prev-uri (oops/oget prev-props :source :uri)
next-uri (oops/oget next-props :source :uri)]
(and prev-uri
next-uri
(= (remove-port prev-uri) (remove-port next-uri))))))))In any case, it's not my intention to suggest this is the only way, I leave it up to you and other CCs to judge. It's good to know we have other options in the future to better fix blinking images. |
i'm not sure i understand how that happen, could you elaborate? why does it switch to a background state in that case? |
|
@flexsurfer I think when an iOS share-sheet opens, the OS will sorta transition the the app into an inactive/background state because share-sheet is native OS menu and outside of the app's control. I think also might happen when someone pulls down the iOS control-center from the top-right while running the app. |
|
@ulisesmac I like the idea of having a component that maintains a background image and foreground image, that could be pretty useful for transitioning between two images! Though I was thinking that @ilmotta's approach with using a component to ignore changes with Also, we still have a |
There was a problem hiding this comment.
Even though I think there are better solutions to the blinking problem, I think your solution is practical and solves the problem without introducing performance regressions. Approved! Thanks @ulisesmac for solving this old problem!
It's switched to a background state because, as @seanstrom said, the share sheet comes from the OS, so it swithces our app to bg, after the sheet is closed our app returns to foreground. Because of this, the image server changes its ports and a re-render happens for all the images provided by the server. |
|
@ilmotta Thanks for remembering me The solution you provided looks better to me since we aren't duplicating the images shown. However, although it solves the unnecessary re-render problem, it doesn't solve the issue about swapping the image without a blink. Thanks for approving, but I'll test your suggestion to see how it behaves 👍 Thanks again |
- On Android, the border was inconsistent depending on the theme. - Fix animation, simplify the calculation, and now it matches designs
ulisesmac
force-pushed
the
21658-avatars-blinking
branch
from
7764e2c to
0bd6156
Compare
src/react_native/fast_image.cljs
Outdated
| (defn- placeholder | ||
| [{:keys [style fallback-content error? loaded?]}] | ||
| [rn/view | ||
| {:style (assoc style | ||
| :flex 1 | ||
| :justify-content :center | ||
| :align-items :center)} | ||
| (cond | ||
| (and error? fallback-content) fallback-content | ||
| error? [rn/text "X"] | ||
| (not loaded?) [rn/activity-indicator {:animating true}])]) | ||
|
|
||
| ;; We cannot use hooks since `reactify-component` seems to ignore the functional compiler | ||
| (defn- internal-fast-image | ||
| [_] | ||
| (let [loaded? (reagent/atom false) | ||
| error? (reagent/atom false)] | ||
| (fn [{:keys [source fallback-content] :as props}] | ||
| [fast-image-class | ||
| (merge | ||
| props | ||
| {:source (if (string? source) | ||
| {:uri source | ||
| :priority :high} | ||
| source) | ||
| :on-error (fn [e] | ||
| (when-let [on-error (:on-error props)] | ||
| (on-error e)) | ||
| (reset! error? true)) | ||
| :on-load (fn [e] | ||
| (when-let [on-load (:on-load props)] | ||
| (on-load e)) | ||
| (reset! loaded? true) | ||
| (reset! error? false))}) | ||
| (let [loaded? (reagent/atom false) | ||
| error? (reagent/atom false) | ||
| on-image-error (fn [event on-error] | ||
| (when (fn? on-error) (on-error event)) | ||
| (reset! error? true)) | ||
| on-image-loaded (fn [event on-load] | ||
| (when (fn? on-load) (on-load event)) | ||
| (reset! loaded? true) | ||
| (reset! error? false))] | ||
| (fn [{:keys [source fallback-content on-error on-load] :as props}] | ||
| [:> FastImage | ||
| (assoc props | ||
| :source (build-source source) | ||
| :on-error #(on-image-error % on-error) | ||
| :on-load #(on-image-loaded % on-load)) | ||
| (when (or @error? (not @loaded?)) | ||
| [placeholder (:style props) | ||
| (if @error? | ||
| (or fallback-content [rn/text "X"]) | ||
| (when-not @loaded? | ||
| [rn/activity-indicator {:animating true}]))])]))) | ||
| [placeholder | ||
| {:style (js->clj (:style props)) | ||
| :fallback-content fallback-content | ||
| :error? @error? | ||
| :loaded? @loaded?}])]))) | ||
|
|
||
| (defn- compare-sources | ||
| [old-props new-props] | ||
| (let [old-source (oops/oget old-props :source) | ||
| new-source (oops/oget new-props :source)] | ||
| (and old-source | ||
| new-source | ||
| (= (remove-port old-source) (remove-port new-source))))) | ||
|
|
||
| (def fast-image | ||
| (-> internal-fast-image | ||
| (reagent/reactify-component) | ||
| (rn/memo compare-sources) | ||
| (reagent/adapt-react-class))) |
There was a problem hiding this comment.
@ilmotta took your suggestion here, it actually fixes the problem. Thanks!
I prefer this solution since we aren't duplicating a component. mantainability isn't hurt since reactify-component was used.
BTW, for some reason, reactify-component seems to ignore the functional compiler, so no hooks were used (the comment on the code has been already added).
There was a problem hiding this comment.
The way you used reactify-component made things much cleaner 👍🏼
| (defn- remove-port | ||
| [source] | ||
| (if (string? source) | ||
| (string/replace-first source #":\d+" "") | ||
| (string/replace-first (oops/oget source :uri) #":\d+" ""))) |
There was a problem hiding this comment.
I'm not sure, but does oops/get throw an exception if it doesn't find :uri?
If so, it might be worth using goog.object/get with a default value like: (goog.object/get source :uri "")
There was a problem hiding this comment.
Good question @seanstrom. Some time ago I changed the default behavior of oops during development, which is to instrument calls and throw on missing keys. Now it just warns. In advanced compilation instrumentation in oops is disabled by default and we don't have to configure anything special.
status-mobile/src/status_im/setup/oops.cljs
Lines 5 to 12 in eae887e
|
Hey @ulisesmac ! Will it be needed to test this PR? |
fixes #21658
fixes #21215
Summary
This PR fixes some issues found related to the rendering of some components. The code fixes are listed in the self-review.
The main fixes are:
1. fast-image usages blinking
A background image for the image was added, this fix might be polemic because we are duplicating the images rendered, so feel free to share your thoughts about it. (btw, the
no-flicker-imagein the repo already does this forrn/image). The comparison:Before:
avatars-blinking.mp4
After:
avatars-fixed.mp4
2. Avatar not being updated in profile screen:
Before:
Avatar-not-updated.mp4
After:
avatar-not-updated-fixed.mp4
3. Fixed the avatar styles and animation.
Reported in:
Now it follows designs and the calculations are simpler. You can see the animation also looks more stable:
Before:
avatar-animation.mp4
After:
avatar-animation-fixed.mp4
4. Fixed the ring around the avatar on Android, it changes its color. This feature still works on iOS consistently.
Before:
avatar-circle-not-matching.mp4
After:
avatar.circle.mp4
Platforms
status: ready