Skip to content

Conversation

@Stypox
Copy link
Member

@Stypox Stypox commented Oct 5, 2020

What is it?

  • Bug fix (user facing)
  • Feature (user facing)
  • Code base improvement (dev facing)
  • Meta improvement to the project (dev facing)

Description of the changes in your PR

Me and apparently also some other people (see #4414 and some discussion in Release 0.20.0) miss the buttons to switch to popup and background directly from the landscape player. This PR adds them back. Note that there are some issues with vertical videos, please just ignore them, as this is just a prototype. I will develop further this PR if you think it is a good idea to add those buttons back, otherwise it can as well be closed @opusforlife2 @TobiGr @wb9688 @avently @Redirion

Fixes the following issue(s)

Closes #4414

Testing apk

Would this suit you @ferrero11?
app-debug.zip

Agreement

@opusforlife2
Copy link
Collaborator

Only going by the description: Just landscape? Not full screen? What about vertical full screen?

@Kromppo
Copy link

Kromppo commented Oct 5, 2020

yes this would work perfectly for me

@avently
Copy link
Contributor

avently commented Oct 5, 2020

Oh, man. Why do you do it with me? I thought we are friends...

@Stypox
Copy link
Member Author

Stypox commented Oct 5, 2020

What about vertical full screen?

That's what I said: I did not polish it, I only implemented it as fast as I could

@Kromppo
Copy link

Kromppo commented Oct 5, 2020

@opusforlife2 I was going for both vertical and landscape full screen but if it can only be implemented in landscape ill settle for that.

Note that there are some issues with vertical videos, please just ignore them, as this is just a prototype. I will develop further this PR if you think it is a good idea to add those buttons back.

I assumed by that part he meant this feature will be added for both vertical and landscape

@opusforlife2
Copy link
Collaborator

@Stypox Oh. I thought you meant that vertical videos are somewhat buggy with just the landscape code. If you have implemented it for full screen in general, then change your PR title to say 'full screen', not 'landscape'.

@avently
Copy link
Contributor

avently commented Oct 5, 2020

@Stypox you already have a possibility to view description without closing a video. So you can make a more useful way to get to description via a swipe like TobiGr proposed (central part of a video allows to scroll to the description). This methods makes possible to get to popup/background buttons as well without the need to add useless buttons into saint place

@opusforlife2
Copy link
Collaborator

It happened again, a second time! ಠ▃ಠ
I had posted a comment and now it isn't there! What is happening?

What I had (approximately) written in the comment:

@Stypox You asked for my opinion:

Personally, I don't care about these buttons because I rarely switch players, so the swipe on full screen button is enough for me. But I can see the use for users who use them.

Something to keep in mind: There shouldn't be too many buttons in the drop down button row. Right now there are 3, and this PR restores them to 5. 5 buttons look fine, but there will be some upper limit. Whatever buttons make it to the player controls should be those which need to be used without exiting full screen. So from that perspective, these two buttons, the mute button and the timestamped share button seem fine. The open in browser button doesn't seem to fit in, here.

@Kromppo
Copy link

Kromppo commented Oct 5, 2020

The problem with the swipe is it is hard to access without accidentally triggering a navigation gesture. If it was changed to you could access the video description with a middle of the player swipe the that would work. However, that's only preferable to the drop down menu if you can continue watching the video when you access the button like you can with the mute sound and share buttons now.

@Kromppo
Copy link

Kromppo commented Oct 5, 2020

maybe move the open in browser button to the video description and move the two player buttons to the drop down menu, and then have a share button in both places so you can share timestamp but also just share the link without opening the video.

@MD77MD
Copy link

MD77MD commented Oct 5, 2020

@Stypox you already have a possibility to view description without closing a video. So you can make a more useful way to get to description via a swipe like TobiGr proposed (central part of a video allows to scroll to the description). This methods makes possible to get to popup/background buttons as well without the need to add useless buttons into saint place

I like this suggestion better... I think we should implement the central swipe first then see if we still feel the same about adding those buttons.

@Kromppo
Copy link

Kromppo commented Oct 5, 2020

Would you still be able to see the video with the central swipe just slightly dimmer like you can now with the drop down overlay? My reasoning is that you are more likely to want to use the pop up player and background player buttons while watching the video compared to the open in browser button. I think switching their places would make more sense but maybe the central swipe gesture can be added for convenience like the volume and brightness adjusters are currently used.

@opusforlife2
Copy link
Collaborator

Oh, by the way, I never understood why the popup button in the arrow drop down row wasn't the same one as the icon below the thumbnail. The headphone icon for background playback is the same. The popup icon should also be the same as the button below the thumbnail.

@Kromppo
Copy link

Kromppo commented Oct 6, 2020

I think it the PR he gave me the two icons are the same but yeah the one in the video description is better than the one that was in the drop down menu

@Kromppo
Copy link

Kromppo commented Oct 6, 2020

i also got some supporters on the original feature request but thank you for the changes here

@Stypox
Copy link
Member Author

Stypox commented Oct 6, 2020

Off topic:
@ferrero11 may I ask you if you are italian? I am, and your nickname is that of an italian factory, sooo I'm curious ;-)

@Kromppo
Copy link

Kromppo commented Oct 6, 2020

No, I am not Italian, but my nickname is based of the company. I took it from the Ferrero Rocher Chocolates they make

@Kromppo
Copy link

Kromppo commented Oct 7, 2020

What if you made the buttons optional like you did with the buttons in the notification tab player, so that we could select which of the 5 buttons we wanted in our fullscreen player, and those who dint want then could stick with the ones in the video description

@TobiGr TobiGr force-pushed the dev branch 2 times, most recently from 679bc75 to 2aeccc0 Compare March 16, 2021 08:24
@triallax triallax changed the title Readd background and popup button to landscape player Read background and popup button to landscape player Jun 13, 2021
@skyGtm
Copy link

skyGtm commented Jun 13, 2021

@mhmdanas As per the description

This PR adds them back

The title should be Re-add not Read.

@triallax triallax changed the title Read background and popup button to landscape player Re-add background and popup button to landscape player Jun 13, 2021
@triallax
Copy link
Contributor

@skyGtm thanks for the heads-up; I had assumed that "Readd" was a typo of "Read" (even though I actually read the PR's description :/ ). I fixed it now.

@litetex
Copy link
Member

litetex commented Oct 1, 2021

@Stypox
At first - before doing anything else: Is this PR required anymore? There doesn't seem to be a lot of activity and requests recently here...

Anyway this PR has:

  • no progress
  • no GitHub actions build
  • merge conflicts

Converted it to a draft, will close it when there is no further progress.

@litetex litetex marked this pull request as draft October 1, 2021 17:43
@litetex litetex added the waiting for author If the author doesn't respond, the issue will be auto-closed. Otherwise the label will be removed. label Oct 1, 2021
@Stypox Stypox closed this Oct 6, 2021
@AudricV AudricV removed the waiting for author If the author doesn't respond, the issue will be auto-closed. Otherwise the label will be removed. label Oct 6, 2021
@litetex litetex mentioned this pull request Oct 10, 2021
2 tasks
@Stypox Stypox deleted the player-switches branch August 4, 2022 09:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add the Switch to Pop-up/Background player buttons back

9 participants