From 00e058d392350da9e279cbd510c89511d6efd928 Mon Sep 17 00:00:00 2001 From: Lyubo Marinov Date: Thu, 3 Aug 2017 12:08:34 -0500 Subject: [PATCH] [RN] Cache avatars and provide a default in case load fails (2) Refactors the previous "[RN] Cache avatars and provide a default in case load fails" for the purposes of simplification but also modifies its functionality at the same time. For example: - Always displays the default avatar immediately which may be seen if the remote avatar needs to be downloaded. - Does not use random colors. - Uses a default avatar image which is not transparent and ugly but at least it's the same image that's used on Web. I've started talks to have images/avatar2.png replaced with a transparent and beautiful so that will land later on and we'll see the automagic colors in all their glory then. --- .../participants/components/Avatar.native.js | 186 ++++++++++++++-- .../components/AvatarImage.native.js | 208 ------------------ .../components/ParticipantView.native.js | 11 + .../participants/components/defaultAvatar.png | Bin 5443 -> 0 bytes 4 files changed, 183 insertions(+), 222 deletions(-) delete mode 100644 react/features/base/participants/components/AvatarImage.native.js delete mode 100644 react/features/base/participants/components/defaultAvatar.png diff --git a/react/features/base/participants/components/Avatar.native.js b/react/features/base/participants/components/Avatar.native.js index 845326ce8..19c619e46 100644 --- a/react/features/base/participants/components/Avatar.native.js +++ b/react/features/base/participants/components/Avatar.native.js @@ -1,7 +1,22 @@ import React, { Component } from 'react'; -import { CustomCachedImage } from 'react-native-img-cache'; +import { View } from 'react-native'; +import { CachedImage, ImageCache } from 'react-native-img-cache'; -import AvatarImage from './AvatarImage'; +import { Platform } from '../../react'; +import { ColorPalette } from '../../styles'; + +// FIXME @lyubomir: The string images/avatar2.png appears three times in our +// source code at the time of this writing. Firstly, it presents a maintenance +// obstacle which increases the risks of inconsistency. Secondly, it is +// repulsive (when enlarged, especially, on mobile/React Native, for example). +/** + * The default image/source to be used in case none is specified or the + * specified one fails to load. + * + * @private + * @type {string} + */ +const _DEFAULT_SOURCE = require('../../../../../images/avatar2.png'); /** * Implements an avatar as a React Native/mobile {@link Component}. @@ -60,6 +75,8 @@ export default class Avatar extends Component { if (prevURI !== nextURI || !this.state) { const nextState = { + backgroundColor: this._getBackgroundColor(nextProps), + /** * The source of the {@link Image} which is the actual * representation of this {@link Avatar}. The state @@ -70,9 +87,7 @@ export default class Avatar extends Component { * uri: string * }} */ - source: { - uri: nextURI - } + source: _DEFAULT_SOURCE }; if (this.state) { @@ -80,9 +95,95 @@ export default class Avatar extends Component { } else { this.state = nextState; } + + // XXX @lyubomir: My logic for the character # bellow is as follows: + // - Technically, URI is supposed to start with a scheme and scheme + // cannot contain the character #. + // - Technically, the character # in URI signals the start of the + // fragment/hash. + // - Technically, the fragment/hash does not imply a retrieval + // action. + // - Practically, the fragment/hash does not always mandate a + // retrieval action. For example, an HTML anchor with an href that + // starts with the character # does not cause a Web browser to + // initiate a retrieval action. + // So I'll use the character # at the start of URI to not initiate + // an image retrieval action. + if (nextURI && !nextURI.startsWith('#')) { + const nextSource = { uri: nextURI }; + + // Wait for the source/URI to load. + ImageCache.get().on( + nextSource, + /* observer */ () => { + this._unmounted || this.setState((prevState, props) => { + if (props.uri === nextURI + && (!prevState.source + || prevState.source.uri !== nextURI)) { + return { source: nextSource }; + } + + return {}; + }); + }, + /* immutable */ true); + } } } + /** + * Notifies this Component that it will be unmounted and destroyed + * and, most importantly, that it should no longer call + * {@link #setState(Object)}. Avatar needs it because it downloads + * images via {@link ImageCache} which will asynchronously notify about + * success. + * + * @inheritdoc + * @returns {void} + */ + componentWillUnmount() { + this._unmounted = true; + } + + /** + * Computes a hash over the URI and returns a HSL background color. We use + * 75% as lightness, for nice pastel style colors. + * + * @param {Object} props - The read-only React Component props from + * which the background color is to be generated. + * @private + * @returns {string} - The HSL CSS property. + */ + _getBackgroundColor({ uri }) { + if (!uri) { + // @lyubomir: I'm leaving @saghul's implementation which picks up a + // random color bellow so that we have it in the source code in + // case we decide to use it in the future. However, I think at the + // time of this writing that the randomness reduces the + // predictability which React is supposed to bring to our app. + return ColorPalette.white; + } + + let hash = 0; + + if (typeof uri === 'string') { + /* eslint-disable no-bitwise */ + + for (let i = 0; i < uri.length; i++) { + hash = uri.charCodeAt(i) + ((hash << 5) - hash); + hash |= 0; // Convert to 32-bit integer + } + + /* eslint-enable no-bitwise */ + } else { + // @saghul: If we have no URI yet, we have no data to hash from. So + // use a random value. + hash = Math.floor(Math.random() * 360); + } + + return `hsl(${hash % 360}, 100%, 75%)`; + } + /** * Implements React's {@link Component#render()}. * @@ -91,16 +192,73 @@ export default class Avatar extends Component { render() { // Propagate all props of this Avatar but the ones consumed by this // Avatar to the Image it renders. + const { + /* eslint-disable no-unused-vars */ - // eslint-disable-next-line no-unused-vars - const { uri, ...props } = this.props; + // The following are forked in state: + uri: forked0, - return ( - - ); + /* eslint-enable no-unused-vars */ + + style, + ...props + } = this.props; + const { + backgroundColor, + source + } = this.state; + + // If we're rendering the _DEFAULT_SOURCE, then we want to do some + // additional fu like having automagical colors generated per + // participant, transparency to make the intermediate state while + // downloading the remote image a little less "in your face", etc. + let styleWithBackgroundColor; + + if (source === _DEFAULT_SOURCE && backgroundColor) { + styleWithBackgroundColor = { + ...style, + + backgroundColor, + + // FIXME @lyubomir: Without the opacity bellow I feel like the + // avatar colors are too strong. Besides, we use opacity for the + // ToolbarButtons. That's where I copied the value from and we + // may want to think about "standardizing" the opacity in the + // app in a way similar to ColorPalette. + opacity: 0.1, + overflow: 'hidden' + }; + } + + // If we're styling with backgroundColor, we need to wrap the Image in a + // View because of a bug in React Native for Android: + // https://github.com/facebook/react-native/issues/3198 + let imageStyle; + let viewStyle; + + if (styleWithBackgroundColor) { + if (Platform.OS === 'android') { + imageStyle = style; + viewStyle = styleWithBackgroundColor; + } else { + imageStyle = styleWithBackgroundColor; + } + } else { + imageStyle = style; + } + + let element = React.createElement(CachedImage, { + ...props, + + resizeMode: 'contain', + source, + style: imageStyle + }); + + if (viewStyle) { + element = React.createElement(View, { style: viewStyle }, element); + } + + return element; } } diff --git a/react/features/base/participants/components/AvatarImage.native.js b/react/features/base/participants/components/AvatarImage.native.js deleted file mode 100644 index 26ff67e6d..000000000 --- a/react/features/base/participants/components/AvatarImage.native.js +++ /dev/null @@ -1,208 +0,0 @@ -import React, { Component } from 'react'; -import { Image, View } from 'react-native'; - -import { Platform } from '../../react'; - -/** - * The default avatar to be used, in case the requested URI is not available - * or fails to load. It is an inline version of images/avatar2.png. - * - * @type {string} - */ -const DEFAULT_AVATAR = require('./defaultAvatar.png'); - -/** - * The number of milliseconds to wait when the avatar URI is undefined before we - * start showing a default locally generated one. Note that since we have no - * URI, we have nothing we can cache, so the color will be random. - * - * @type {number} - */ -const UNDEFINED_AVATAR_TIMEOUT = 1000; - -/** - * Implements an Image component wrapper, which returns a default image if the - * requested one fails to load. The default image background is chosen by - * hashing the URL of the image. - */ -export default class AvatarImage extends Component { - /** - * AvatarImage component's property types. - * - * @static - */ - static propTypes = { - /** - * If set to true it will not load the URL, but will use the - * default instead. - */ - forceDefault: React.PropTypes.bool, - - /** - * The source the {@link Image}. - */ - source: React.PropTypes.object, - - /** - * The optional style to add to the {@link Image} in order to customize - * its base look (and feel). - */ - style: React.PropTypes.object - }; - - /** - * Initializes new AvatarImage component. - * - * @param {Object} props - Component props. - */ - constructor(props) { - super(props); - - this.state = { - failed: false, - showDefault: false - }; - - this.componentWillReceiveProps(props); - - this._onError = this._onError.bind(this); - } - - /** - * Notifies this mounted React Component that it will receive new props. - * If the URI is undefined, wait {@code UNDEFINED_AVATAR_TIMEOUT} ms and - * start showing a default locally generated avatar afterwards. - * - * Once a URI is passed, it will be rendered instead, except if loading it - * fails, in which case we fallback to a locally generated avatar again. - * - * @inheritdoc - * @param {Object} nextProps - The read-only React Component props that this - * instance will receive. - * @returns {void} - */ - componentWillReceiveProps(nextProps) { - const prevSource = this.props.source; - const prevURI = prevSource && prevSource.uri; - const nextSource = nextProps.source; - const nextURI = nextSource && nextSource.uri; - - if (typeof prevURI === 'undefined') { - clearTimeout(this._timeout); - if (typeof nextURI === 'undefined') { - this._timeout - = setTimeout( - () => this.setState({ showDefault: true }), - UNDEFINED_AVATAR_TIMEOUT); - } else { - this.setState({ showDefault: nextProps.forceDefault }); - } - } - } - - /** - * Clear the timer just in case. See {@code componentWillReceiveProps} for - * details. - * - * @inheritdoc - */ - componentWillUnmount() { - clearTimeout(this._timeout); - } - - /** - * Computes a hash over the URI and returns a HSL background color. We use - * 75% as lightness, for nice pastel style colors. - * - * @private - * @returns {string} - The HSL CSS property. - */ - _getBackgroundColor() { - const uri = this.props.source.uri; - let hash = 0; - - // If we have no URI yet we have no data to hash from, so use a random - // value. - if (typeof uri === 'undefined') { - hash = Math.floor(Math.random() * 360); - } else { - /* eslint-disable no-bitwise */ - - for (let i = 0; i < uri.length; i++) { - hash = uri.charCodeAt(i) + ((hash << 5) - hash); - hash |= 0; // Convert to 32bit integer - } - - /* eslint-enable no-bitwise */ - } - - return `hsl(${hash % 360}, 100%, 75%)`; - } - - /** - * Error handler for image loading. When an image fails to load we'll mark - * it as failed and load the default URI instead. - * - * @private - * @returns {void} - */ - _onError() { - this.setState({ failed: true }); - } - - /** - * Implements React's {@link Component#render()}. - * - * @inheritdoc - */ - render() { - const { failed, showDefault } = this.state; - const { - // The following is/are forked in state: - forceDefault, // eslint-disable-line no-unused-vars - - source, - style, - ...props - } = this.props; - - if (failed || showDefault) { - const coloredBackground = { - ...style, - backgroundColor: this._getBackgroundColor(), - overflow: 'hidden' - }; - - // We need to wrap the Image in a View because of a bug in React - // Native for Android: - // https://github.com/facebook/react-native/issues/3198 - const workaround3198 = Platform.OS === 'android'; - let element = React.createElement(Image, { - ...props, - source: DEFAULT_AVATAR, - style: workaround3198 ? style : coloredBackground - }); - - if (workaround3198) { - element - = React.createElement( - View, - { style: coloredBackground }, - element); - } - - return element; - } else if (typeof source.uri === 'undefined') { - return null; - } - - // We have a URI and it's time to render it. - return ( - - ); - } -} diff --git a/react/features/base/participants/components/ParticipantView.native.js b/react/features/base/participants/components/ParticipantView.native.js index cefea018a..8369b4630 100644 --- a/react/features/base/participants/components/ParticipantView.native.js +++ b/react/features/base/participants/components/ParticipantView.native.js @@ -198,6 +198,17 @@ function _mapStateToProps(state, ownProps) { if (participant) { avatar = getAvatarURL(participant); connectionStatus = participant.connectionStatus; + + // Avatar (on React Native) now has the ability to generate an + // automatically-colored default image when no URI/URL is specified or + // when it fails to load. In order to make the coloring permanent(ish) + // per participant, Avatar will need something permanent(ish) per + // perticipant, obviously. A participant's ID is such a piece of data. + // But the local participant changes her ID as she joins, leaves. + // TODO @lyubomir: The participants may change their avatar URLs at + // runtime which means that, if their old and new avatar URLs fail to + // download, Avatar will change their automatically-generated colors. + avatar || participant.local || (avatar = `#${participant.id}`); } return { diff --git a/react/features/base/participants/components/defaultAvatar.png b/react/features/base/participants/components/defaultAvatar.png deleted file mode 100644 index b5b04608a9c70cd936c123024ac9543d3b4b9bcb..0000000000000000000000000000000000000000 GIT binary patch literal 0 HcmV?d00001 literal 5443 zcmeHLXHyeg(^iUxP(+Fp5yT+Ui}V13L6RUXltcsrB1nt0(2E60^dTld5J8b%Luk^g zR1p;Ep|=}|Kw=}albC!vjh4tKd02`2< zgOlq5_eCCFKK@Grf=%;>JMiK&@6!ot!TiMnTV-|hk0-of#ale3Gfn>*%-r?-!wMP+quT|;AYOIv$K=l35y zz5RnjBV*%})BnuT7nT?+Ya73|clQsEPYUFTSI?ZexNUUjwzZ(pT1!;qNI*)*uaSL? zrdmOtGdHxpu-)cXG9+E<<>PF)Wc@?U7_!}Hi&pO6b(WWt;5ln5!DkU`7k&2ChmaGD z-2_Usd9rC{ar>#B*oCvF6#X~kYWCfUibIX*lLBPZ)K+3j7}pi;p|H8lMs%7N`tOYS z$!Y>#!1M_p!eA&`tT@^(X9sEG8GQ zh$pQ)!f^m#SLv(5ReE2?%sd&Lp``h|*3@aS$C)u`FsT zu4nHD|0d^~I`ngI4fcaPU%O-+?+E)r{iBc)tI|@_W`~sWDK-XLPG4hBQ+`7g*n*r1 z#B%j=LsoR?e7Y!r@6^vbPx~ZTkRe^)z&eHi_f=Ojn;CQ53zsyXaeEn@o0s2~`aQf& zs?WJV(nTieVNG^eFK;X5*^6@_dxnZ2Mrs?rjl}|Y^Bwd20`n1&cBpHSYaHTM z3D~b=99MRqvAR|a{E5vn{wES5_0}x06$6Q?5DCzl6px4gD(%X&l6oV67KYj3ur=SV zmsN6q|7Y@6;`xt$CQu zz3yAY8t=ePI_MPYKp395iXn1}tOTP5^ie*WGS}zDXGxpuTj#Z;t@ovJTdKBae7=_L zmSA9aBik*)U_vr`=?2%@i`dLC5!;g3%rtFvaYNZFA7e9NSM03$ zog%XYZ#X;UMh3$=I~SuJlVZt-T#j#J-}Q>LsEW&Oi!`Ud=G*13p z#Gmm*)Nf%=9r$xQp*%1=`E+o&xEAh*XT%HN3amPEVY`l;ntgzlj7QBYEV^eP9(9MM zWEn#8E7$#=dGX&Zew^s%7l{ZZX(uPLLe30%_P}SZLjat1$)wZ|5bpMgo-h4I5D{}Q zd;J6szV36b${KI;tZ$z7`97fEU&LqpnK%pWAeA=|Tg*M^WH#JW3bEE(^-UVK#?|y3 z(%6|~G@Le;lo_YE=QXX9YH7Ge2*uBa9qeb06iOfzzF{~ZQghc9c2m7C-5_O%@rDG?cIQpH@8wb6) z4KUc^#4UWtXjH@CL~42`XxG!;*Y{0qx{=<*vWk{6^5jE6^X`7I;R&T-Mdffg=MX6_ zK4k0wUN2{S$p2%F+No=-@~7x=+J&R+)&&(MGT!gHq#X|ihZ>yLL2 zJ-Z>FO~Vh~pQq<7&#gRS z5wOI2_#>&}v7ar$CAUvxi>Ehi!-|p2pGG75x2-=S!rHrGx^RaI_cMf*3v~%7Qop_q zqb0P+lu~*arW4!&R^x7eH-QG-cs@jpa0~8;9NE8Fo;UNB{^_maolD_+r8=r!T=xwrsPwe#!l zLudG#_Iw(~B#3pt#6pA3g@LvgFmJwh{XQwsOsnztMT)wW~8a5@M# zgp9zgo!Zw7`sI-dcc0w#ZNPo>MX2GgWLVE|sc_+fmaO`2sM}s~-LuSd1qV5>p2T^) zMc?lXm}PGh20)0qr`Pxkciw4Hwkub|1JJl11sllk!e7+b{RA7`!158#p`uqEB7yD=PTdy601G>j|H=ZB#FK|8& zJ|+#B&pcmg)fyZx5Kb67ud}7D3IMs|VeNnJ-?be4D9W#$gePxj!n(BQ* z7XhH`t@AONH>h{JexHcycH{v;nPu%+%Lj1~hUNxy40W0ve-SY@*IyOZ$fLZDjHyax zw0=UW*Pjct&t)N=e!qVKBxH(tOG*op+A7m-PO;4M!aX%rDPgqF) zUSlxro6R+jo{g0FX;aySDi0eOE3c4~Pj3fQ%mj&-lhd6r3B8ySw@q8HfNp&o+c*lq ztg#UPwsNuiZw+>QlyENHZT)>cUGMMYyz`VI$7H>k{l3F==>??lu8v8CVqerLjrhqtT;aO01HfEa7DCJ!NE+S-@@}Zcj#C-+IsjJBG zMKUWv5^(a-l}0<4;m|IcRpr8dQaeQ{MdN(}Id2ec>u+n;ACr=_tpTosqn@;Bp4lg& zH<$l$Gt?~470nV&0J`pdvJgP?&zey4h(|VZ$=}RLU@aR!-aW?6<;NL5p?b>83+%Hy zT!OoPr9R+oB6wUXxkoZ9^(*^~T0f=06}J)ELn*N0Y^_Fyk&bNT!Y3YO^#rod?A2O$ zTj9LHy_9+ZocB?sMUXCM>j<)rbX0G9P$wqDHYyW71qHFr#$N9u;D!w5}giHVK zav-WE%l>ZD1YNuU{QGGV{^ee)Ubt8Gqq5B)Cz5j^-=B4LkkB!%>9`Z#!7}TeM8e4l z{8<2l#I3m?>MNA3u_A7#uXJ(O6MZV#v=Q?3wJLO@7PAf}p3-R4IQ z!H3rf%i*B=CDPP(Xc?CjbqHEV1uh$d(2KIU=Rl5c_y;9Gj*8N`D@5GW;`gaamME!g zQ|R|`e#>fdnK7Tm8*)V$zeOIoqMEOt1RdCu$?PCi@B`gGldDRBOP|P9LWI0W66-GQ zD%0@Ew*=QJa*du$YCCj}OC}XYr11k6JwX9QK-WBS-8W#~2z=@%A-5ksebaEzOJc*w zaL`@Co0M7G3ZI4%7EquN7+<#m^a#khO&~WyS+^U>je)G?z3>@UVD2gYB;>SoHp>%o zx;vXA36IOj5sCa=Iw0l^>4X9jM|iG{m&Q#WlTw|Bbp28t3_U#)VXuMh%SNaRY6<2c zZYg7ZQXz-mJ6rI;oZilsa6?PwvP~z$0XOGa1;YUs=UH*X0Y~TACcbZn&`b9BBvgoU z4O#F7q5Gp0c(H2PX6d!s;m%e8f^#VuZ$O}%f!L=A^k<+81iB;W0-h;kxYNKeqQRM- zVG&WNyw#VfKdrXam#aUmvDH_se@kgg>2^f8Gu<(hY0QP5VGt4SL7&xOEgS4?b7n2; z>TI)SE$irPQ)R_AcN*j|oxZcppA}2%Y%^iSPIk6EVIA4pZR_tkv z29C0W8pXO5M}AFZ3ZZoxWauXd-Cwel zU?<5vIZcrySBR8Y6uuvfBC87Dx1l0Ji6RfEh^ot4O9-hFVJ$aHsV)*nGD6Oe$ngXr zH&5jFeYceFDfH-%Y0K|1_zVl{b}YGJ8q)IMk%V_vrn_3)z8~JN6F$X(-)5z$&6X;UH`wT!X0S-c!|fctc+x(j=9AzW_dk_TW;Zg_uwGQ{lPPaNp`t=!lqAL^L#*?*P#owy& z*^xH(3pHye;~==J20Mt}^U6v^0rPxIEnwd4d6hpXAJ_P2cThqBTgW#)l1(bha|gk* zM1m>w%8tp!p3GTAX!maSr5I>u3!*1{OWlJSRMl7VsvF9gk|fTd|EtPE1*VOQ zIIOe)g^lAPY@g7+h!wOpn`(Oy8#{2f-N&4XWxR>(kpU&j)T#<#WYDyk6K!W&p2Nu3 zJ53p^9?k#CW7XD)PvkdQNhD>0cyah?kT}O+yVzyW(wC=5M-~$YS7Pi*kTve&&4q}p zubV|F(nN}TufI^+ue^fGc=AZ8zf+nrUszg_6+hHid6wESBBp+>&~;)tCxz6yR6u0B z_dP`F*@w&aEF~C@$i3IzO@{urb}H$^e+0&5<&