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

Add video UI controls [100k sats] #1530

Closed
alltheseas opened this issue Sep 2, 2023 · 18 comments
Closed

Add video UI controls [100k sats] #1530

alltheseas opened this issue Sep 2, 2023 · 18 comments
Assignees
Labels
bounty Sats enhancement Improvement

Comments

@alltheseas
Copy link
Collaborator

alltheseas commented Sep 2, 2023

user story

As a Damus user who plays videos, I would like video player UI controls - pause, play, volume, full screen, play speed - so that I can better control videos to my preference.

acceptance criteria

  1. Can play & pause
  2. Can change volume visually w/ vertical bar
  3. Can change playback settings: 0.5, 0.75, 1.0, 1.25, 1.5, 2.0 playback speed
  4. Can enter and exit fullscreen
Screenshot 2023-09-02 at 5 08 08 PM Screenshot 2023-09-02 at 5 08 21 PM Screenshot 2023-09-02 at 5 08 16 PM

100k sats bounty by @dmnyc

#1386 (comment)

@kroucis
Copy link
Contributor

kroucis commented Sep 5, 2023

I am working on this feature now, is the bounty still available?

@alltheseas
Copy link
Collaborator Author

Yes

kroucis added a commit to kroucis/damus-ios that referenced this issue Sep 5, 2023
…er, providing play/pause, volume control, content scale, playback rate controls, as well as a seek/timecode bar.
@kroucis
Copy link
Contributor

kroucis commented Sep 5, 2023

I have opened a PR with the implementation of these UI controls. I made some executive decisions on certain functionality and color/theming as I tested. I am, of course, open to requests on how these controls should be improved for a more intuitive user interaction model.
Somewhat related: I am 90% certain I have a fix for #1386 that can be implemented without this PR, but I think merging the video control changes in first makes sense. What do you all think: should I wait on the fix until after the UI controls have been reviewed/merged, or try to craft a fix before the UI controls that might need to be changed because of the merge?

@alltheseas
Copy link
Collaborator Author

What do you all think: should I wait on the fix until after the UI controls have been reviewed/merged, or try to craft a fix before the UI controls that might need to be changed because of the merge?

This is a @jb55 question.

@bryanmontz is fixing the video player double playback bug, so I would recommend waiting until playback bug is fixed.

@alltheseas
Copy link
Collaborator Author

alltheseas commented Nov 18, 2023

https://damus.io/note150m02jqsltz2la264k6d58saf8qaw2cz95x6yeayt6ugpwsgfalqf6m75e

Video controls have issue where they render only if a video was added after an image

@danieldaquino danieldaquino self-assigned this Mar 11, 2024
@danieldaquino
Copy link
Contributor

@jb55 @ericholguin @alltheseas following up on today's standup meeting, I will try to tackle this ticket first, and then work on other video support tickets.

Please let me know if there is another particular ticket that you think I should tackle before this one.

@alltheseas alltheseas added this to the 1.8 post Madeira milestone Mar 12, 2024
@danieldaquino danieldaquino moved this to In Progress 🏗️ in Damus Roadmap 🛣️ Mar 12, 2024
@danieldaquino
Copy link
Contributor

@jb55 @alltheseas, I believe I am getting close to completing this one. I have a working draft which is mostly ready, but I still need to fix some minor details here and there.

This issue involves getting around some SwiftUI quirks, so I tried to make the best UX I could within SwiftUI's quirks. It is also familiar UX for those coming from Twitter or other social apps. Here is how it works:

  1. Videos on the feed only have a mute/unmute button
  2. When the user clicks on the video, they are taken to a full screen carousel view (similar to when you click on an image)
  3. The full-screen carousel view now includes a short version of the note, for context and reading.
  4. The full-screen carousel view shows all video playback controls.
  5. If the carousel has multiple videos/images, the user can swipe between them normally as expected

Here is a short demo: https://drive.google.com/file/d/1r3JWZFljrzn8eriaZliQzX4fY0iYzecL/view?usp=sharing
I also sent a draft patch series: https://groups.google.com/a/damus.io/g/patches/c/6LQ5xVuJC_0

Please let me know if you think this is going in the right direction, or if you have any requests, suggestions, feedback, etc.

Thanks!

@jb55
Copy link
Collaborator

jb55 commented Mar 14, 2024 via email

@alltheseas
Copy link
Collaborator Author

🙏

@dmnyc
Copy link

dmnyc commented Mar 15, 2024 via email

@jb55 jb55 moved this from In Progress 🏗️ to In Review 🔬 in Damus Roadmap 🛣️ Mar 17, 2024
@danieldaquino
Copy link
Contributor

@jb55, I fixed several minor issues I was seeing with the changes, did more thorough testing, and sent you the official (non-draft) patch series (https://groups.google.com/a/damus.io/g/patches/c/H0IYJJSgzX4)

Please let me know if you have any suggestions, concerns, or questions!

P.S. If the bounty is still up and these changes fulfill all criteria for the bounty, I believe it should be paid to Damus itself and not directly to me (Since I made these changes during my usual work hours).

Thanks!

@jb55
Copy link
Collaborator

jb55 commented Mar 23, 2024

P.S. If the bounty is still up and these changes fulfill all criteria for the bounty, I believe it should be paid to Damus itself and not directly to me (Since I made these changes during my usual work hours).

it's fine if you collect the bounty, you did fix it after all.

@danieldaquino
Copy link
Contributor

P.S. If the bounty is still up and these changes fulfill all criteria for the bounty, I believe it should be paid to Damus itself and not directly to me (Since I made these changes during my usual work hours).

it's fine if you collect the bounty, you did fix it after all.

@dmnyc, if the changes made on the App Store 1.8 satisfy your criteria for the bounty, can you please redirect the bounty to @alltheseas instead (not me)?

I think he has his zaps redirected somewhere else though, it might be a good idea to double-check with him about the LN address.

Thank you very much 🙏

@dmnyc
Copy link

dmnyc commented May 15, 2024

Sure thing! @alltheseas Let me know where you want me to send it. 🙌⚡️

@alltheseas
Copy link
Collaborator Author

Zap Daniel

@jb55
Copy link
Collaborator

jb55 commented May 15, 2024 via email

@dmnyc
Copy link

dmnyc commented May 15, 2024

bounty

@danieldaquino
Copy link
Contributor

Thank you @dmnyc!! Also thank you @alltheseas for mirroring the zap back to me ⚡️🪞😜

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bounty Sats enhancement Improvement
Projects
Archived in project
Development

No branches or pull requests

5 participants