Move fullscreen to progress bar#58
Merged
omg-xtao merged 2 commits intoNextAlone:devfrom Nov 11, 2025
Merged
Conversation
I think moving from containerView to videoPlayerControlFrameLayout will make more changes, so I tried to dynamically set the icon. But the current seekbar length calculation isn't quite right, maybe it needs dynamic calculation too? I'm not sure. Please let NA take a look(.
Reviewer's GuideThis PR moves fullscreen control into the VideoPlayerControlFrameLayout by dynamically setting the fullscreen icon and visibility, refactors seekbar sizing offsets, and overhauls orientation toggling logic to rely on runtime display dimensions. Sequence diagram for fullscreen button visibility and orientation logicsequenceDiagram
participant PV as PhotoViewer
participant VPCL as VideoPlayerControlFrameLayout
participant Activity
participant WindowManager
PV->>VPCL: checkFullscreenButton()
VPCL->>PVCL: onMeasure()
PVCL->>PVCL: Set exitFullscreenButton visibility and icon
PVCL->>PVCL: Adjust seekbar size
PVCL->>PVCL: Set videoWidth/videoHeight if needed
PVCL->>PVCL: Hide fullscreenButton
PVCL->>Activity: setRequestedOrientation (based on display dimensions)
alt Landscape
Activity->>WindowManager: getDefaultDisplay().getRotation()
Activity->>Activity: setRequestedOrientation (LANDSCAPE/REVERSE_LANDSCAPE)
PVCL->>PVCL: toggleActionBar(false, false)
else Portrait
Activity->>Activity: setRequestedOrientation (PORTRAIT)
end
Class diagram for updated fullscreen and seekbar logic in PhotoViewerclassDiagram
class PhotoViewer {
- int videoWidth
- int videoHeight
- int parentWidth
- int parentHeight
- boolean wasRotated
- int fullscreenedByButton
- int prevOrientation
+ void checkFullscreenButton()
+ void setVisibility(int visibility)
}
class VideoPlayerControlFrameLayout {
+ VideoPlayerControlFrameLayout(Context context)
+ void onMeasure(int widthMeasureSpec, int heightMeasureSpec)
}
PhotoViewer "1" *-- "1" VideoPlayerControlFrameLayout : contains
PhotoViewer o-- "1" videoPlayerSeekbar
PhotoViewer o-- "1" exitFullscreenButton
PhotoViewer o-- "1" fullscreenButton
PhotoViewer o-- "1" videoPlayerTime
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Extract the repeated dp-based size calculations (e.g. dp(8), dp(48), dp(47), dp(12)) into named constants or a helper method instead of using magic numbers scattered throughout.
- Unify the orientation check logic (mixing parentWidth/parentHeight and videoWidth/videoHeight) into a single helper or state to avoid subtle inconsistencies and duplicated conditionals.
- Remove or replace the "Need further adjust" inline comments by finalizing the seekbar sizing logic or linking to a tracked issue so that the code doesn’t ship with unresolved FIXMEs.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Extract the repeated dp-based size calculations (e.g. dp(8), dp(48), dp(47), dp(12)) into named constants or a helper method instead of using magic numbers scattered throughout.
- Unify the orientation check logic (mixing parentWidth/parentHeight and videoWidth/videoHeight) into a single helper or state to avoid subtle inconsistencies and duplicated conditionals.
- Remove or replace the "Need further adjust" inline comments by finalizing the seekbar sizing logic or linking to a tracked issue so that the code doesn’t ship with unresolved FIXMEs.
## Individual Comments
### Comment 1
<location> `TMessagesProj/src/main/java/org/telegram/ui/PhotoViewer.java:3504` </location>
<code_context>
if (exitFullscreenButton.getVisibility() != VISIBLE) {
exitFullscreenButton.setVisibility(VISIBLE);
}
+ exitFullscreenButton.setImageResource(parentWidth > parentHeight ? R.drawable.msg_minvideo : R.drawable.msg_maxvideo);
extraWidth = dp(48);
layoutParams.rightMargin = dp(47);
</code_context>
<issue_to_address>
**question:** Image resource selection logic may be unclear for edge cases.
If parent and video aspect ratios can differ, please clarify which should determine the icon, or add documentation for the intended behavior in such cases.
</issue_to_address>
### Comment 2
<location> `TMessagesProj/src/main/java/org/telegram/ui/PhotoViewer.java:3542` </location>
<code_context>
timeSpring.start();
} else {
- videoPlayerSeekbar.setSize(getMeasuredWidth() - dp(2 + 14) - size - extraWidth, getMeasuredHeight());
+ videoPlayerSeekbar.setSize(getMeasuredWidth() - dp(8) - size - extraWidth, getMeasuredHeight()); //Need further adjust
timeValue.setValue(size);
}
</code_context>
<issue_to_address>
**suggestion:** Repeated use of dp(8) for seekbar sizing may need centralization.
Consider defining dp(8) as a constant or utility to maintain consistency and simplify future changes.
Suggested implementation:
```java
if (exitFullscreenButton.getVisibility() != GONE) { //see the video I sent in the group, I initially suspected the button wasn't fully hidden, so I made this change. Revert if necessary.
exitFullscreenButton.setVisibility(GONE);
}
extraWidth = 0;
layoutParams.rightMargin = dp(12);
timeSpring.getSpring().setFinalPosition(size);
timeSpring.start();
} else {
```
```java
videoPlayerSeekbar.setSize((int) (getMeasuredWidth() - dp(SEEKBAR_HORIZONTAL_PADDING) - value - extraWidth), getMeasuredHeight()); //Need further adjust
});
```
```java
// Centralized seekbar horizontal padding for sizing
private static final int SEEKBAR_HORIZONTAL_PADDING = 8;
public VideoPlayerControlFrameLayout(@NonNull Context context) {
int extraWidth;
ignoreLayout = true;
LayoutParams layoutParams = (LayoutParams) videoPlayerTime.getLayoutParams();
```
If there are other usages of `dp(8)` for seekbar sizing elsewhere in the file, they should also be replaced with `dp(SEEKBAR_HORIZONTAL_PADDING)` for consistency.
</issue_to_address>
### Comment 3
<location> `TMessagesProj/src/main/java/org/telegram/ui/PhotoViewer.java:9908-9910` </location>
<code_context>
+ if (AndroidUtilities.displaySize.x > AndroidUtilities.displaySize.y) {
+ parentActivity.setRequestedOrientation(ActivityInfo.SCREEN_ORIENTATION_PORTRAIT);
+ } else {
+ WindowManager manager = (WindowManager) parentActivity.getSystemService(Activity.WINDOW_SERVICE);
+ int displayRotation = manager.getDefaultDisplay().getRotation();
+ parentActivity.setRequestedOrientation(displayRotation == Surface.ROTATION_270 ?
+ ActivityInfo.SCREEN_ORIENTATION_REVERSE_LANDSCAPE : ActivityInfo.SCREEN_ORIENTATION_LANDSCAPE);
+ toggleActionBar(false, false);
</code_context>
<issue_to_address>
**question:** Orientation logic may not handle all rotation values.
Explicitly handle ROTATION_0 and ROTATION_90, or clarify why only ROTATION_270 is singled out.
</issue_to_address>
### Comment 4
<location> `TMessagesProj/src/main/java/org/telegram/ui/PhotoViewer.java:11008` </location>
<code_context>
- if (fullscreenButton[b].getVisibility() != View.GONE) {
- fullscreenButton[b].setVisibility(View.GONE);
- }
+ if (AndroidUtilities.displaySize.y > AndroidUtilities.displaySize.x && w > h && b == 0) {
+ videoWidth = w;
+ videoHeight = h;
</code_context>
<issue_to_address>
**question:** videoWidth and videoHeight assignment is limited to b == 0.
Please verify that only the first fullscreen button should update videoWidth and videoHeight, and that this behavior is correct for all use cases.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
omg-xtao
approved these changes
Nov 11, 2025
omg-xtao
pushed a commit
to PreviousAlone/Nnngram
that referenced
this pull request
Nov 11, 2025
seyrenus
pushed a commit
to seyrenus/Nnngram
that referenced
this pull request
Nov 19, 2025
…eo (NextAlone/Nagram#58) (cherry picked from commit 257f2ad)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I think moving from containerView to videoPlayerControlFrameLayout will make more changes, so I tried to dynamically set the icon. But the current seekbar length calculation isn't quite right, maybe it needs dynamic calculation too? I'm not sure. Please let NA take a look(.
Summary by Sourcery
Move the fullscreen toggle into the progress bar UI, dynamically display the appropriate icon, refine seekbar sizing, and overhaul orientation handling for fullscreen mode
New Features:
Enhancements: