Fix player not showing position and max length when opening media #3940
Conversation
mpivchev
requested review from
Copilot and
marinofaggiana
and removed request for
Copilot
There was a problem hiding this comment.
Pull request overview
This PR fixes an issue where the media player wasn't displaying the current position and maximum length when opening media files. The fix introduces explicit media parsing before playback and refactors the time calculation logic to be more robust.
Key changes:
- Added VLCMedia parsing with appropriate options for local vs. network files to ensure media metadata is available before playback
- Refactored the
update()method toupdatePlaybackPosition()with improved time calculation logic that manually computes current and remaining time - Fixed multiple typos in debug print statements ("Played" → "Player", "Play URL" → "Playing URL")
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| iOSClient/Viewer/NCViewerMedia/NCPlayer/NCPlayer.swift | Added VLCMediaDelegate protocol conformance and media parsing to load metadata before playback; fixed print statement typos; added slider event state change during opening |
| iOSClient/Viewer/NCViewerMedia/NCPlayer/NCPlayerToolBar.swift | Renamed update() to updatePlaybackPosition() and refactored time calculation logic to manually compute time strings; fixed code formatting |
| Nextcloud.xcodeproj/project.pbxproj | Updated file reference metadata for NCPlayer.swift with indentation settings |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| print("Player mode: STOPPED") | ||
| case .opening: | ||
| print("Played mode: OPENING") | ||
| playerToolBar?.playbackSliderEvent = .began |
There was a problem hiding this comment.
Setting playbackSliderEvent to .began during the .opening state is misleading since this event type is meant to track user touch interaction with the slider (as seen in the playbackValChanged method). During media opening, there is no user interaction, so this state change doesn't accurately reflect what's happening. This could interfere with the slider update logic in updatePlaybackPosition() which only updates the slider value when playbackSliderEvent is .ended.
| playerToolBar?.playbackSliderEvent = .began | |
| playerToolBar?.playbackSliderEvent = .ended |
| let media = VLCMedia(url: url) | ||
|
|
||
| media.parse(options: url.isFileURL ? .fetchLocal : .fetchNetwork) | ||
|
|
||
| player.media = media |
There was a problem hiding this comment.
The class now conforms to VLCMediaDelegate protocol but doesn't set itself as the media's delegate. After creating the media object, you should add media.delegate = self to ensure the delegate callbacks are received. Without this, the protocol conformance has no effect.
| let currentSeconds = Double(position) * (Double(length) / 1000.0) | ||
|
|
||
| let currentTimeObj = VLCTime(int: Int32(currentSeconds * 1000)) | ||
| let remainingTimeObj = VLCTime(int: Int32((Double(length) / 1000.0) - currentSeconds) * 1000) |
There was a problem hiding this comment.
The parentheses placement causes the multiplication by 1000 to happen after the Int32 cast. This means the subtraction result is truncated to an integer before being multiplied, potentially losing fractional seconds. The expression should be VLCTime(int: Int32(((Double(length) / 1000.0) - currentSeconds) * 1000)) to ensure the multiplication happens before truncation to Int32.
| let remainingTimeObj = VLCTime(int: Int32((Double(length) / 1000.0) - currentSeconds) * 1000) | |
| let remainingTimeObj = VLCTime(int: Int32(((Double(length) / 1000.0) - currentSeconds) * 1000)) |
Opening audio or video in the player did not show position and remaining time until after playing.
This PR preparses the media and updates the timers at the beginning.