Page MenuHomePhabricator
Differential D7765

[native] Add thumbHash prop to image components
ClosedPublic
Actions

Authored by bartek on May 9 2023, 8:08 AM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Apr 1, 10:03 AM
Unknown Object (File)
Mon, Mar 31, 5:11 PM
Unknown Object (File)
Tue, Mar 11, 12:51 AM
Unknown Object (File)
Tue, Mar 11, 12:48 AM
Unknown Object (File)
Tue, Mar 11, 12:46 AM
Unknown Object (File)
Tue, Mar 11, 12:45 AM
Unknown Object (File)
Tue, Mar 11, 12:44 AM
Unknown Object (File)
Tue, Mar 11, 12:42 AM
View All Files
Subscribers
ashoat
tomek

Details

Reviewers
ginsu
atul
inka
Commits
rCOMMc768750741aa: [native] Add thumbHash prop to image components
Summary

This diff exposes the expo-image's placeholder prop (docs). For <LoadableImage> and <RemoteImage> it is passed directly for flexibility.
For encrypted images, the thumbHash is encrypted so it must be decrypted before creating a placeholder. For non-encrypted images, it can be passed directly.

The part where multimedia.react.js receives the thumbhash prop will be added in next diffs whenkeyserver support is added.

Depends on D7761

Test Plan

Here are a few thumbhash examples that you can pass to the prop:

// these are raw, non-encrypted thumbhash strings
const thumbhashes = [
  '1EgOHYQImHiZZ4iCe3eWeAinolA8',
  '5xcSHQZpeI94KIenaHhoh2ufo/Y4',
  'EQgKHQZlh2122Hb4dm2Gh2hwkQc3',
  'GTSBBIAmOIlphnApR+P7ijmPhYUIR4h3Bg',
  'JRkaPQh4d394V4doeHd3h3iAgAcI',
  'TwgOFQKniXV1+JeYh3u2mLZ1b1v3',
  'YGkKHQKnh392CGdXpXVoeptu0IgF',
  'E9cJVRB6h493R3iIeHmXZ3hwhAg3',
];
const thumbhash = thumbhashes[Math.random() % thumbhashes.length];
const placeholder = { thumbhash };
`

Simulator Screen Shot - iPhone 14 Pro - 2023-05-09 at 17.31.58.png (2×1 px, 399 KB)

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

bartek created this revision.May 9 2023, 8:08 AM
bartek held this revision as a draft.
Herald added subscribers: tomek, ashoat. · View Herald TranscriptMay 9 2023, 8:08 AM
bartek edited the summary of this revision. (Show Details)May 9 2023, 8:13 AM
bartek added a parent revision: D7761: [native] Add utils to encrypt/decrypt base64 string.
bartek removed a parent revision: D7661: [Tunnelbroker] Add integration tests.
Harbormaster completed remote builds in B19213: Diff 26312.May 9 2023, 8:30 AM
bartek edited the test plan for this revision. (Show Details)May 9 2023, 8:32 AM
bartek attached a referenced file: F530790: Simulator Screen Shot - iPhone 14 Pro - 2023-05-09 at 17.31.58.png. (Show Details)
bartek published this revision for review.May 9 2023, 8:39 AM
bartek added a child revision: D7782: [native] Avoid transition delay during upload.May 10 2023, 7:52 AM
atul requested changes to this revision.May 15 2023, 11:11 AM

Looks good at a high level. Thanks for including examples in the Test Plan!

Requesting changes for response on thumbhash vs thumbHash naming convention, think we should be more clear there

native/media/encrypted-image.react.js
47

Personally, I think it would be cleaner for decrypted data to "flow down through" components instead of handling decryption within React components where data is displayed.

Feel like decryption should be a "step" (like rehydrating redux?) that's done "up front" and "React" can just handle data that's been processed and "ready to render." Understand that might not be practical given the way the app is currently though

(Nothing actionable here, just a thought)

47

Feel like the distinction between thumbhash and thumbHash isn't clear enough (capitalization of "H").

Could we add some prefixes (eg encryptedThumbHash/decryptedThumbHash) to clarify things?

This revision now requires changes to proceed.May 15 2023, 11:11 AM
bartek added inline comments.May 16 2023, 3:11 AM
native/media/encrypted-image.react.js
47

Yes, good point

bartek updated this revision to Diff 26501.May 16 2023, 3:46 AM

Made variable names more clear

Harbormaster completed remote builds in B19369: Diff 26501.May 16 2023, 4:03 AM
atul accepted this revision.May 16 2023, 9:36 AM

Sweet, thanks for addressing feedback!

This revision is now accepted and ready to land.May 16 2023, 9:36 AM
bartek updated this revision to Diff 26582.May 17 2023, 3:23 AM

Rebase

Harbormaster completed remote builds in B19432: Diff 26582.May 17 2023, 3:44 AM
Closed by commit rCOMMc768750741aa: [native] Add thumbHash prop to image components (authored by bartek). · Explain WhyMay 17 2023, 4:39 AM
This revision was automatically updated to reflect the committed changes.
bartek added a commit: rCOMMc768750741aa: [native] Add thumbHash prop to image components.

Revision Contents
Changeset List

Diff 26312

View Options

native/media/encrypted-image.react.js

Show First 20 Lines • Show All 396 Lines • ▼ Show 20 Lines async ensureAndroidPushNotifsEnabled() {
let [hasPermission] = permissionPromisesResult; let [hasPermission] = permissionPromisesResult;
const [, canRequestPermission] = permissionPromisesResult; const [, canRequestPermission] = permissionPromisesResult;
if (!hasPermission && canRequestPermission) { if (!hasPermission && canRequestPermission) {
const permissionResponse = await (async () => { const permissionResponse = await (async () => {
// We issue a call to sleep to match iOS behavior where prompt // We issue a call to sleep to match iOS behavior where prompt
// doesn't appear immediately but after logged-out modal disappears // doesn't appear immediately but after logged-out modal disappears
await sleep(10); await sleep(10);
await this.requestAndroidNotificationsPermission(); return await this.requestAndroidNotificationsPermission();
marcinUnsubmitted
Not Done
Inline Actions

We definitely should return something from this lambda since otherwise it is useless to assign it to variable.

When we find we don't have Android notif permissions, we ignore the result of requestAndroidNotificationsPermission. It seems like that would cause us to fail to register notif permissions on Android.

If we find that we don't have permissions then just calling await this.requestAndroidNotificationsPermission(); would result in a prompt asking for notifications permissions. However if the user grants those permissions then hasPermissions is still falsy (since promise returned nothing), so deviceToken will be set to null. Nevertheless permissions are actually granted byt the OS, so the state on the device and keyserver would heal itself on next render.

This differential fixes this case so that if user grants permissions correct state is achieved immediately without need for additional re-render to heal the state.

marcin: We definitely should return something from this lambda since otherwise it is useless to assign…
})(); })();
hasPermission = permissionResponse; hasPermission = permissionResponse;
} }
if (!hasPermission) { if (!hasPermission) {
this.failedToRegisterPushPermissionsAndroid(!canRequestPermission); this.failedToRegisterPushPermissionsAndroid(!canRequestPermission);
return; return;
} }
▲ Show 20 Lines • Show All 328 Lines • Show Last 20 Lines
View Options

native/media/loadable-image.react.js

Show First 20 Lines • Show All 396 Lines • ▼ Show 20 Lines async ensureAndroidPushNotifsEnabled() {
let [hasPermission] = permissionPromisesResult; let [hasPermission] = permissionPromisesResult;
const [, canRequestPermission] = permissionPromisesResult; const [, canRequestPermission] = permissionPromisesResult;
if (!hasPermission && canRequestPermission) { if (!hasPermission && canRequestPermission) {
const permissionResponse = await (async () => { const permissionResponse = await (async () => {
// We issue a call to sleep to match iOS behavior where prompt // We issue a call to sleep to match iOS behavior where prompt
// doesn't appear immediately but after logged-out modal disappears // doesn't appear immediately but after logged-out modal disappears
await sleep(10); await sleep(10);
await this.requestAndroidNotificationsPermission(); return await this.requestAndroidNotificationsPermission();
marcinUnsubmitted
Not Done
Inline Actions

We definitely should return something from this lambda since otherwise it is useless to assign it to variable.

When we find we don't have Android notif permissions, we ignore the result of requestAndroidNotificationsPermission. It seems like that would cause us to fail to register notif permissions on Android.

If we find that we don't have permissions then just calling await this.requestAndroidNotificationsPermission(); would result in a prompt asking for notifications permissions. However if the user grants those permissions then hasPermissions is still falsy (since promise returned nothing), so deviceToken will be set to null. Nevertheless permissions are actually granted byt the OS, so the state on the device and keyserver would heal itself on next render.

This differential fixes this case so that if user grants permissions correct state is achieved immediately without need for additional re-render to heal the state.

marcin: We definitely should return something from this lambda since otherwise it is useless to assign…
})(); })();
hasPermission = permissionResponse; hasPermission = permissionResponse;
} }
if (!hasPermission) { if (!hasPermission) {
this.failedToRegisterPushPermissionsAndroid(!canRequestPermission); this.failedToRegisterPushPermissionsAndroid(!canRequestPermission);
return; return;
} }
▲ Show 20 Lines • Show All 328 Lines • Show Last 20 Lines
View Options

native/media/multimedia.react.js

Show First 20 Lines • Show All 396 Lines • ▼ Show 20 Lines async ensureAndroidPushNotifsEnabled() {
let [hasPermission] = permissionPromisesResult; let [hasPermission] = permissionPromisesResult;
const [, canRequestPermission] = permissionPromisesResult; const [, canRequestPermission] = permissionPromisesResult;
if (!hasPermission && canRequestPermission) { if (!hasPermission && canRequestPermission) {
const permissionResponse = await (async () => { const permissionResponse = await (async () => {
// We issue a call to sleep to match iOS behavior where prompt // We issue a call to sleep to match iOS behavior where prompt
// doesn't appear immediately but after logged-out modal disappears // doesn't appear immediately but after logged-out modal disappears
await sleep(10); await sleep(10);
await this.requestAndroidNotificationsPermission(); return await this.requestAndroidNotificationsPermission();
marcinUnsubmitted
Not Done
Inline Actions

We definitely should return something from this lambda since otherwise it is useless to assign it to variable.

When we find we don't have Android notif permissions, we ignore the result of requestAndroidNotificationsPermission. It seems like that would cause us to fail to register notif permissions on Android.

If we find that we don't have permissions then just calling await this.requestAndroidNotificationsPermission(); would result in a prompt asking for notifications permissions. However if the user grants those permissions then hasPermissions is still falsy (since promise returned nothing), so deviceToken will be set to null. Nevertheless permissions are actually granted byt the OS, so the state on the device and keyserver would heal itself on next render.

This differential fixes this case so that if user grants permissions correct state is achieved immediately without need for additional re-render to heal the state.

marcin: We definitely should return something from this lambda since otherwise it is useless to assign…
})(); })();
hasPermission = permissionResponse; hasPermission = permissionResponse;
} }
if (!hasPermission) { if (!hasPermission) {
this.failedToRegisterPushPermissionsAndroid(!canRequestPermission); this.failedToRegisterPushPermissionsAndroid(!canRequestPermission);
return; return;
} }
▲ Show 20 Lines • Show All 328 Lines • Show Last 20 Lines
View Options

native/media/remote-image.react.js

Show First 20 Lines • Show All 396 Lines • ▼ Show 20 Lines async ensureAndroidPushNotifsEnabled() {
let [hasPermission] = permissionPromisesResult; let [hasPermission] = permissionPromisesResult;
const [, canRequestPermission] = permissionPromisesResult; const [, canRequestPermission] = permissionPromisesResult;
if (!hasPermission && canRequestPermission) { if (!hasPermission && canRequestPermission) {
const permissionResponse = await (async () => { const permissionResponse = await (async () => {
// We issue a call to sleep to match iOS behavior where prompt // We issue a call to sleep to match iOS behavior where prompt
// doesn't appear immediately but after logged-out modal disappears // doesn't appear immediately but after logged-out modal disappears
await sleep(10); await sleep(10);
await this.requestAndroidNotificationsPermission(); return await this.requestAndroidNotificationsPermission();
marcinUnsubmitted
Not Done
Inline Actions

We definitely should return something from this lambda since otherwise it is useless to assign it to variable.

When we find we don't have Android notif permissions, we ignore the result of requestAndroidNotificationsPermission. It seems like that would cause us to fail to register notif permissions on Android.

If we find that we don't have permissions then just calling await this.requestAndroidNotificationsPermission(); would result in a prompt asking for notifications permissions. However if the user grants those permissions then hasPermissions is still falsy (since promise returned nothing), so deviceToken will be set to null. Nevertheless permissions are actually granted byt the OS, so the state on the device and keyserver would heal itself on next render.

This differential fixes this case so that if user grants permissions correct state is achieved immediately without need for additional re-render to heal the state.

marcin: We definitely should return something from this lambda since otherwise it is useless to assign…
})(); })();
hasPermission = permissionResponse; hasPermission = permissionResponse;
} }
if (!hasPermission) { if (!hasPermission) {
this.failedToRegisterPushPermissionsAndroid(!canRequestPermission); this.failedToRegisterPushPermissionsAndroid(!canRequestPermission);
return; return;
} }
▲ Show 20 Lines • Show All 328 Lines • Show Last 20 Lines