Memmy-App / memmy

An Apollo inspired open-source iOS and Android client for Lemmy built with React-Native. Find us on the App Store and Google Play!
GNU Affero General Public License v3.0
548 stars 58 forks source link

Feature/video playback #869

Open robigan opened 1 year ago

robigan commented 1 year ago

PR Creator Checklist

Ensure you've checked the following before submitting your PR:

Summary

Please provide a summary of what your PR does

Adds initial video playback support

Screenshots

If the UI has been changed, include screenshots. We will not review your PR without screenshots if they are applicable.

It is currently very late at night, I'll do this tomorrow morning

Test Plan

Please documemnt the steps required to test your PR

Test any post you can find with a video link

robigan commented 1 year ago

Aw shit merge conflicts, will have to deal with those tomorrow

robigan commented 1 year ago

https://github.com/Memmy-App/memmy/assets/35210888/6136dd78-6f52-4d24-aa7d-936e34ecf67d

https://github.com/Memmy-App/memmy/assets/35210888/6df0b255-fbe6-435b-9b19-ea584300ba27

gkasdorf commented 1 year ago

Sorry for that conflict hell yea...it should just be a matter of fixing imports. Trying to get rid of these babel issues. I was going to fix everything up but I'll wait for now until you finish with this, and a few other branches people are working on. 🙏

robigan commented 1 year ago

So currently there are three major UX issues:

  1. Video thumbnails are blocking despite being called in an async function (I do not have time to debug this)
  2. There is no indication of a thumbnail being a video
  3. The underlying image viewer background gets pulled up (I seem to have a bug with iOS 17 where onFullscreenUpdate isn't called properly so I can't call onRequestOpenOrClose, but I have no idea really)
robigan commented 1 year ago

Sorry for that conflict hell yea...it should just be a matter of fixing imports. Trying to get rid of these babel issues. I was going to fix everything up but I'll wait for now until you finish with this, and a few other branches people are working on. pray

I would have fixed the merge conflicts myself but I've got to leave in 2h and need to pack so unfortunately I'll have to leave that up to you

gkasdorf commented 1 year ago

That's looking pretty good though!

gkasdorf commented 1 year ago

Sorry for that conflict hell yea...it should just be a matter of fixing imports. Trying to get rid of these babel issues. I was going to fix everything up but I'll wait for now until you finish with this, and a few other branches people are working on. pray

I would have fixed the merge conflicts myself but I've got to leave in 2h and need to pack so unfortunately I'll have to leave that up to you

Yea no problem. I can clean them up myself, just wanted to make sure you were taking a break from the work before I did that.

I'll take a look at those issues you were talking about as well 👍

robigan commented 1 year ago

That's looking pretty good though!

Thanks, I've started work on Media providers so that links from imgur, youtube, redgifs, etc. can also be embeded. I am mentioning redgifs because it's surprisingly the easiest to integrate cus they allow you to generate ephemeral api keys on the fly at runtime. In fact I knew Liftoff supported redgifs so I just used their same method.

robigan commented 1 year ago

@gkansdorf I am about to take off, I'm going to attempt to fix those 3 ux changes while on my flight but I am not sure how easy that is on a crappy intel macbook air. Heck I don't even know if this message will pass through

robigan commented 1 year ago

@gkansdorf I am about to take off, I'm going to attempt to fix those 3 ux changes while on my flight but I am not sure how easy that is on a crappy intel macbook air. Heck I don't even know if this message will pass through

Nvm I can't do this nor do I have time to over the coming couple of weeks. I'll have to revise this in two weeks, either way the groundwork has been laid and anyone is more than free to help with this, especially with the 3 UX problems I identified. @gkasdorf I tried quickly rebasing onto 0.5.6 tag but I quickly realized you've done a lot of changes in that area and I think you know what's best in terms of resolving the merge conflicts

gkasdorf commented 1 year ago

Fixed up the issues. I'll update the PR here in a bit.

robigan commented 1 year ago

Fixed up the issues. I'll update the PR here in a bit.

I'll save you some time and merge your changes into here, I forgot how much jetlag is a bitch (I travel a lot, yet I still forget how annoying it is) and tonight I find myself with some time on my hands. Or rather boredom of doing nothing practical

robigan commented 1 year ago

There are more merge conflicts? Oh ffs

robigan commented 1 year ago

Oh and great, according to my git client, there are no merge conflicts. Aye madre

robigan commented 1 year ago

Closes #399, although there are still some major improvements to be made tracked on another branch of mine

gkasdorf commented 1 year ago

Not sure why it's telling me there's conflicts whenever I pull and merge and it's fine...anyway checking it out now.

Edit: Merged in main

gkasdorf commented 1 year ago

I notice that MediaViewer doesn't seem to be displaying anything in the feed. Opening the post does work (since it seems PostContentView still is using ImageViewer as opposed to MediaViewer).

I can look more into why later as well.

Since it looks like (correct me if I'm wrong here?) that you're not directly modifying ImageViewer but instead just creating a separate component for VideoViewer that I could go ahead and make some changes in ImageViewer without affecting this? I have some stuff I want to clean up and fix in there but don't want to get in your way.

robigan commented 1 year ago

Yup, go full ahead. Strange that media isn't showing up in the feed viewer for you. I plan to work on these issues over the coming weeks (or days where possible) and optimize thumbnail loading. As far as maintainability goes I think for the long term ImageViewer and VideoViewer will eventually need to be merged but I think it's a good idea to keep them separate for the meantime because A) different devs are working on two different aspects of media viewing and B) cus there's yet a lot of improvements to be made and experimented with each until desirable results are achieved.