Page MenuHomePhabricator
Differential D14307

[native] Migrate recenter functionality in FullScreenViewModal
ClosedPublic
Actions

Authored by angelika on Feb 6 2025, 5:17 PM.
Tags
None
Referenced Files
F6240570: D14307.diff
Thu, Apr 24, 11:02 PM
Unknown Object (File)
Wed, Apr 16, 11:42 AM
Unknown Object (File)
Wed, Apr 16, 11:06 AM
Unknown Object (File)
Wed, Apr 16, 8:57 AM
Unknown Object (File)
Wed, Apr 16, 5:33 AM
Unknown Object (File)
Wed, Apr 16, 1:13 AM
Unknown Object (File)
Wed, Apr 16, 12:10 AM
Unknown Object (File)
Tue, Apr 15, 8:18 PM
View All 66 Files
Subscribers
ashoat

Details

Reviewers
tomek
ashoat
Commits
rCOMM87db6a222d12: [native] Migrate recenter functionality in FullScreenViewModal
Summary

https://linear.app/comm/issue/ENG-8150/convert-fullscreenviewmodal-to-reanimated-2-syntax

The recenter function triggers if there is no interaction (after some gesture ends and there is no animation). If the image is panned or zoomed out so there is some space left, the image is zoomed in to 1 (if necessary) and moved.

Depends on D14306

Test Plan

Tested after migration: verify the image is recentered after pan ended when zoomed out

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

angelika created this revision.Feb 6 2025, 5:17 PM
angelika held this revision as a draft.
Herald added a subscriber: ashoat. · View Herald TranscriptFeb 6 2025, 5:17 PM
angelika added a child revision: D14308: [native] Migrate fling functionality in FullScreenViewModal.Feb 6 2025, 5:39 PM
Harbormaster completed remote builds in B33422: Diff 46951.Feb 6 2025, 6:32 PM
angelika updated this revision to Diff 47085.Feb 12 2025, 8:52 PM

Rebase & bugfixes

angelika edited the summary of this revision. (Show Details)Feb 12 2025, 8:59 PM
angelika published this revision for review.Feb 12 2025, 9:09 PM
Harbormaster completed remote builds in B33501: Diff 47085.Feb 12 2025, 10:30 PM
tomek accepted this revision.Feb 17 2025, 3:52 AM
tomek added inline comments.
native/components/full-screen-view-modal.react.js
1179–1187 ↗(On Diff #47085)

It looks like in the original code this if was split into 3, for each value respectively. Should we do the same here?

This revision is now accepted and ready to land.Feb 17 2025, 3:52 AM
ashoat mentioned this in D14306: [native] Migrate backdropOpacityUpdate in FullScreenViewModal.Feb 19 2025, 5:51 PM
ashoat added inline comments.Feb 19 2025, 6:02 PM
native/components/full-screen-view-modal.react.js
1076 ↗(On Diff #47085)

I think "reset animation" is confusing naming here, as that could mean a lot of things. How about isRunningDoubleTapZoomAnimation?

1164 ↗(On Diff #47085)

In the old system, we would stop the recentering animations when activeInteraction flipped to true.

In the new system, when this happens due to pinchActive or isRunningResetAnimation, it looks like all three animations will be cancelled because new values are set for curScale, curX, and curY.

But in the case of panActive and isRunningDismissAnimation, it appears that a new value is not set for curScale. Does this mean the animation will continue? Do we need to cancel it some other way?

ashoat mentioned this in D14308: [native] Migrate fling functionality in FullScreenViewModal.Feb 19 2025, 8:36 PM
angelika updated this revision to Diff 47189.Feb 20 2025, 2:14 PM

Rebase & bugfixes

angelika marked 2 inline comments as done.Feb 20 2025, 3:06 PM
angelika planned changes to this revision.Feb 20 2025, 3:20 PM
angelika updated this revision to Diff 47201.Feb 21 2025, 8:07 PM

Rebase

This revision is now accepted and ready to land.Feb 21 2025, 8:07 PM
angelika added inline comments.Feb 21 2025, 8:25 PM
native/components/full-screen-view-modal.react.js
1164 ↗(On Diff #47085)

Copied the comment from other diff, because it also applies here:

I added cancelAnimation(curScale) when dismissing from pan. Now fling should be stopped when activeInteraction is true. In case activeInteraction is triggered by:

panActive turning to true: we explicitly cancel all animations in panStart
pinchActive turning to true: we explicitly cancel all animation in pinchStart
isRunningDismissAnimation: curX and curY are overridden and we explicitly cancel curScale
isRunningDoubleTapZoomAnimation: curX, curY and curScale are are overridden
This also applies to the recenter animation.

Harbormaster completed remote builds in B33581: Diff 47201.Feb 21 2025, 8:31 PM
ashoat accepted this revision.Feb 21 2025, 11:47 PM
angelika updated this revision to Diff 47223.Feb 24 2025, 3:52 AM

Rebase

Harbormaster completed remote builds in B33603: Diff 47223.Feb 24 2025, 6:08 AM
Closed by commit rCOMM87db6a222d12: [native] Migrate recenter functionality in FullScreenViewModal (authored by angelika). · Explain WhyFeb 25 2025, 2:48 AM
This revision was automatically updated to reflect the committed changes.
angelika added a commit: rCOMM87db6a222d12: [native] Migrate recenter functionality in FullScreenViewModal.

Revision Contents
Changeset List

PathSize
native/
components/
111 lines

Diff 47201

View Options

native/components/full-screen-view-modal.react.js

Show First 20 Lines • Show All 240 Lines • ▼ Show 20 Lines const messageDatas = userIDs.map(otherUserID => ({
creatorID: viewer.userID, creatorID: viewer.userID,
creatorFID: viewerFID, creatorFID: viewerFID,
targetID: otherUserID, targetID: otherUserID,
targetFID: request.userIDsToFID[otherUserID], targetFID: request.userIDsToFID[otherUserID],
time: now, time: now,
operation: 'farcaster_mutual', operation: 'farcaster_mutual',
})); }));
await createMessages(viewer, messageDatas, 'broadcast'); await createMessages(viewer, messageDatas, 'broadcast');
} else if (request.action === relationshipActions.ACKNOWLEDGE) {
updateIDs.push(...userIDs);
const insertRows = userIDs.map(userID => {
const [user1, user2] = sortUserIDs(viewer.userID, userID);
return { user1, user2, status: undirectedStatus.KNOW_OF };
});
const updateDatas = await updateChangedUndirectedRelationships(insertRows);
inkaAuthorUnsubmitted
Done
Inline Actions

updateChangedUndirectedRelationships only updates the relationship if there was none, or the relationship status provided it greater than the one in the db. This will not be the case here, because KNOW_OF is 0-the lowest possible value. So we will not override a more meaningful relationship.

inka: `updateChangedUndirectedRelationships` only updates the relationship if there was none, or the…
tomekUnsubmitted
Not Done
Inline Actions

Makes sense, but is there an easy way of checking if the relationship doesn't need to be updated?

tomek: Makes sense, but is there an easy way of checking if the relationship doesn't need to be…
inkaAuthorUnsubmitted
Done
Inline Actions

The only thing this function does is checking if the relationship needs to be updated, and if so updates it. So I don't think it makes sense to duplicate this logic.
We discussed this offline and agreed that this code makes sense and can be left as is.

inka: The only thing this function does is checking if the relationship needs to be updated, and if…
await createUpdates(updateDatas);
} else { } else {
invariant( invariant(
false, false,
`action ${request.action} is invalid or not supported currently`, `action ${request.action} is invalid or not supported currently`,
); );
} }
await createUpdates( await createUpdates(
▲ Show 20 Lines • Show All 169 Lines • Show Last 20 Lines