Skip to content

refactor: simplify video zooming#26527

Merged
alextran1502 merged 1 commit intomainfrom
fix/video-zooming
Mar 2, 2026
Merged

refactor: simplify video zooming#26527
alextran1502 merged 1 commit intomainfrom
fix/video-zooming

Conversation

@shenlong-tanwen
Copy link
Member

No description provided.

@goalie2002
Copy link
Contributor

Sorry for creating a bunch of refactoring work for you guys. I did briefly consider trying to implement it with just the photoView in the asset_page, but assumed there was a reason for video_viewer being architected this way, so I tried to stick with it. I never thought to ask about that, my bad!

@uhthomas
Copy link
Collaborator

@shenlong-tanwen thank you! The nested photo view is causing strange subtle issues. I am not sure why this wasn't caught during the review but /shrug.

@goalie2002 No need to apologise, your contributions are hugely appreciated.

onPageBuild: _onPageBuild,
enablePanAlways: true,
backgroundDecoration: backgroundDecoration,
child: SizedBox(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with #26545, the sized box should be removed

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've left it in for now to be removed in your PR

# Conflicts:
#	mobile/lib/presentation/widgets/asset_viewer/asset_page.widget.dart
#	mobile/lib/presentation/widgets/asset_viewer/video_viewer.widget.dart
@shenlong-tanwen shenlong-tanwen marked this pull request as ready for review February 27, 2026 15:41
@goalie2002
Copy link
Contributor

goalie2002 commented Feb 27, 2026

I ran a quick test on this and I noticed that it lets you zoom into the black bars around the video; essentially the zoomable area is the entire screen instead of just the video itself. This differs from the way it works on photos and the previous implementation of video zooming on main

@shenlong-tanwen
Copy link
Member Author

I ran a quick test on this and I noticed that it lets you zoom into the black bars around the video; essentially the zoomable area is the entire screen instead of just the video itself. This differs from the way it works on photos and the previous implementation of video zooming on main

I don't see this. Do you have a sample asset where I can reproduce this or a recording of the issue?

@alextran1502 alextran1502 enabled auto-merge (squash) March 2, 2026 16:30
@alextran1502 alextran1502 merged commit f54924d into main Mar 2, 2026
55 checks passed
@alextran1502 alextran1502 deleted the fix/video-zooming branch March 2, 2026 17:53
@goalie2002
Copy link
Contributor

I don't see this. Do you have a sample asset where I can reproduce this or a recording of the issue?

screen-20260302-100551.2.-.Trim.mp4

IIRC it's caused by the Center widget expanding to the parent's size, and my fix previously was to calculate the required size myself

@uhthomas
Copy link
Collaborator

uhthomas commented Mar 2, 2026

@goalie2002 explicit sizing in #26545 should resolve that.

@goalie2002
Copy link
Contributor

@goalie2002 explicit sizing in #26545 should resolve that.

Awesome, I'll give that one a test!

@uhthomas
Copy link
Collaborator

uhthomas commented Mar 2, 2026

Sounds good! It needs rebasing, which I'll do in a couple of hours.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants