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

Closes nextcloud#1702

Signed-off-by: Daniel Standfest <[email protected]>
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

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