Page MenuHomePhabricator

[native] Add component to display encrypted images
ClosedPublic

Authored by bartek on Mar 28 2023, 11:11 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Nov 8, 3:15 AM
Unknown Object (File)
Fri, Nov 8, 3:15 AM
Unknown Object (File)
Fri, Nov 8, 3:15 AM
Unknown Object (File)
Wed, Nov 6, 3:23 AM
Unknown Object (File)
Mon, Nov 4, 3:28 AM
Unknown Object (File)
Mon, Nov 4, 3:28 AM
Unknown Object (File)
Thu, Oct 31, 10:30 PM
Unknown Object (File)
Thu, Oct 31, 10:30 PM
Subscribers

Details

Summary

This diff adds a component that is able to decrypt and display encrypted images. It is similar to the <RemoteImage> component but I rewrote it to hooks.
The decryption is performed on every mount so this is very inefficient, caching will be added later.

Depends on D7224, D7232

Test Plan

The component itself is tested together with subsequent diffs where it is used.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

bartek held this revision as a draft.
bartek published this revision for review.Mar 28 2023, 12:06 PM
ashoat requested changes to this revision.Mar 28 2023, 12:40 PM
ashoat added a subscriber: ginsu.
ashoat added inline comments.
native/media/encrypted-image.react.js
33–39 ↗(On Diff #24302)

I am not necessarily opposed to setting state from the function body (I know this is an accepted pattern), but it's not clear to me if it's necessary here. Is there some reason this doesn't work?

const prevConnectionStatusRef = React.useRef(connectionStatus);
const attemptRef = React.useRef(0);

if (prevConnectionStatusRef.current !== connectionStatus) {
  if (!source && connectionStatus === 'connected') {
    attemptRef.current++;
  }
  prevConnectionStatusRef.current = connectionStatus;
}

Won't this have the same effect, without requiring additional executions of the function body?

66–99 ↗(On Diff #24302)

This part seems copy-pasted from Multimedia. Is it possible to instead factor out the part of Multimedia you want?

I think @ginsu also has a need for a component like Multimedia that takes a URI instead of a MediaInfo for his avatars work. Rather than have it copy-pasted in three places I think it would be good to factor out

This revision now requires changes to proceed.Mar 28 2023, 12:40 PM
native/media/encrypted-image.react.js
66–99 ↗(On Diff #24302)

Sorry, I meant to say RemoteImage, not Multimedia

native/media/encrypted-image.react.js
33–39 ↗(On Diff #24302)

This way is officially recommended. A very similar example in official React docs: https://react.dev/learn/you-might-not-need-an-effect#adjusting-some-state-when-a-prop-changes

native/media/encrypted-image.react.js
33–39 ↗(On Diff #24302)

I'm familiar with that example, but I don't think it's comparable. Let me know if you think I'm misunderstanding.

I think the link you shared explains that it is better to set state in the function body than to have it done in a useEffect.

In this case, neither setting state in the function body nor setting state in a useEffect is necessary. In fact, no setting of state should be necessary at all...

The difference between state and refs is that state changes trigger rerenders. We already have a state change to trigger the rerender (connectionStatus), so we don't need any more state at all.

native/media/encrypted-image.react.js
33–39 ↗(On Diff #24302)

Yeah, right, I misunderstood. We were talking about two different things.

66–99 ↗(On Diff #24302)

Yeah, makes sense.

I think @ginsu also has a need for a component like Multimedia that takes a URI instead of a MediaInfo for his avatars work

Do you mean D7187?

  • Replaced prevConnectionStatus state with ref. Kept attempt as state, because it needs to trigger hook and re-render when changed.
  • Extracted common logic to LoadableImage component - separate diff D7232
ashoat requested changes to this revision.Mar 29 2023, 10:16 AM

Sorry for requesting changes again, but I have a couple of questions. If you don't think any changes are necessary, please feel free to respond to my questions and re-request review!

native/media/encrypted-image.react.js
29 ↗(On Diff #24329)

Kept attempt as state, because it needs to trigger hook and re-render when changed.

Can you clarify further what you mean by this? My impression was that the rerender triggered by the connectionStatus change would update the attempt before rerendering, but I'm guessing you tested this and I'm missing something.

I'm not necessarily opposed to setting state from the function body if it's really necessary, but I'd like to understand why.

31–36 ↗(On Diff #24329)

Given that both RemoteImage and RemoteImage seem to need this functionality, I'm wondering why you didn't factor it out into LoadableImage

66–99 ↗(On Diff #24302)

Yes, although that is a draft diff not meant for review

This revision now requires changes to proceed.Mar 29 2023, 10:16 AM

Re-requesting review to verify my point of view

native/media/encrypted-image.react.js
29 ↗(On Diff #24329)

My impression was that the rerender triggered by the connectionStatus change would update the attempt before rerendering, but I'm guessing you tested this and I'm missing something.

Yes, but that attempt change isn't observed if it's a ref. Value change should trigger the effect to fire again. Ref is mutable and non-reactive so it cannot be an effect dependency.

Correct me if you think there's a better solution for this.

31–36 ↗(On Diff #24329)
ashoat requested changes to this revision.Mar 30 2023, 5:46 AM

Have you tried the code snippet I suggested in my first review on Tuesday? Does it work or no?

native/media/encrypted-image.react.js
29 ↗(On Diff #24329)

Ref is mutable and non-reactive so it cannot be an effect dependency.

I think it's fine to have a ref's value as a dependency for an effect, eg. attemptRef.current. I don't understand what you mean by "change isn't observed"... I guess you're thinking of passing attemptRef instead of attemptRef.current?

This revision now requires changes to proceed.Mar 30 2023, 5:46 AM
bartek added inline comments.
native/media/encrypted-image.react.js
29 ↗(On Diff #24329)

I think it's fine to have a ref's value as a dependency for an effect

I wish I agreed here, it would make things simple ;)

However React ESLint explicitly says this:

Screenshot 2023-03-30 at 15.15.25.png (181×940 px, 37 KB)

Also, I made a JSFiddle demonstrating the issue (look at the console at the bottom-right for console.logs)

According to React docs, "effects run as a result of rendering." so a re-render is needed to trigger the effect.

(Resigning to remove from my queue since it seems like @ashoat has got it, feel free to re-add me if there's anything specific)

Thanks for the detailed explanation and code example, @bartek – it looks like your approach is correct here

This revision is now accepted and ready to land.Mar 30 2023, 8:59 AM