Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support videos in media view #1844

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

DanielStandfest
Copy link
Contributor

added support for videos (and playback) when swiping in media view.

Closes #1702

@SystemKeeper

@SystemKeeper
Copy link
Collaborator

Very cool @DanielStandfest, thanks a lot for working on this 👍
We are going to take a look at this in the upcoming week.

Btw, we can add you to the Nextcloud org if you like? Then you can directly push here. We also have a developer chat where we can create a guest account for you, if you like. Just let me know :)

@DanielStandfest
Copy link
Contributor Author

Thank you @SystemKeeper.
Sure, would be nice to be added :)

Appreciate it.

@SystemKeeper
Copy link
Collaborator

The warning about the branch being out of date is usually not an issue, it's just a warning. If merging would not be possible
you would see a message like:
image

Since we pull in latest translation changes at least once a day, it's very likely that your branch becomes outdated quite quickly.

If you wanna update the branch to be inline with latest master, you can use the rebase option to prevent unnecessary merge commits btw:
image

Copy link

github-actions bot commented Nov 2, 2024

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

(If you believe you should not receive this message, you can add yourself to the blocklist.)

@DanielStandfest
Copy link
Contributor Author

Any news on this one?

@SystemKeeper

@Ivansss
Copy link
Member

Ivansss commented Nov 12, 2024

Hello @DanielStandfest ,

Thanks a lot for your PR :)

Sorry for our late reply, @SystemKeeper and I did take a look to your PR last week but forgot to write a reply here.

We noticed a few things that could be addressed before merging your PR:

  1. It would be nice if the NCMediaViewerViewController is opened directly when tapping on a video file too:

    if NCUtils.isImage(fileType: fileParameter.mimetype) {
    let mediaViewController = NCMediaViewerViewController(initialMessage: message)
    let navController = CustomPresentableNavigationController(rootViewController: mediaViewController)
    self.present(navController, interactiveDismissalType: .standard)
    return
    }

  2. We could skip for now "webm" and "mkv" videos and do not try to present them in the NCMediaViewerViewController and just present them using VLC:

    if VLCKitVideoViewController.supportedFileExtensions.contains(fileExtension) {

  3. (Optional for this PR, could be done in a follow-up PR by us or you)
    Instead of directly downloading videos when presenting them in the NCMediaViewerViewController, we could show a play button and the preview of the video (if available) and only when tapping on the play button we start downloading the video

@DanielStandfest DanielStandfest force-pushed the support-videos-in-media-view branch 4 times, most recently from 5915f89 to f822950 Compare November 14, 2024 00:30
Closes nextcloud#1702

Signed-off-by: Daniel Standfest <daniel@standfest.io>
@DanielStandfest
Copy link
Contributor Author

Hi @Ivansss,

thank you for your valuable feedback!

Points 1 and 2 have now been implemented. Additionally, I noticed the issue with unsupported video formats when swiping through webm and mkv videos. I’ve adjusted the getPreviousFileMessage and getNextFileMessage methods to address this.

Regarding point 3, I’d suggest handling it as a separate feature in a future update, and I recommend creating a dedicated issue to track it.

Thanks again for your insights!

Comment on lines +3321 to +3322
let fileLocalPath = fileParameter.fileStatus?.fileLocalPath ?? ""
let fileExtension = URL(fileURLWithPath: fileLocalPath).pathExtension.lowercased()
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
let fileLocalPath = fileParameter.fileStatus?.fileLocalPath ?? ""
let fileExtension = URL(fileURLWithPath: fileLocalPath).pathExtension.lowercased()
let filePath = fileParameter.path ?? ""
let fileExtension = URL(fileURLWithPath: filePath).pathExtension.lowercased()

fileStatus is only available when we start downloading a file, that's why at the moment when you tap on a video the first time, it will start downloading the video file and it will open it in the QLPreviewController. Then the next times it will open it directly in the NCMediaViewer, since it can get the local path from the fileStatus.

If we get the path directly from the fileParameter, we are able to check its extension and the video file will be opened in the NCMediaViewer directly.

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.

Support videos in media view
3 participants