Add click to play/pause feature on previews for audio/video
Needs ReviewPublic

Authored by meven on Jul 1 2019, 10:14 AM.

Details

Reviewers
elvisangelaccio
ngraham
Group Reviewers
Dolphin
Test Plan

In dolphin, click on a video or audio preview in the information panel.
The video or audio preview starts

Diff Detail

Repository
R318 Dolphin
Branch
arcpatch-D22183
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 15075
Build 15093: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes

+1 for this feature. However, I feel like as implemented, it won't be very discoverable. To improve this, how about doing one of the following:

  • Use a pointing hand cursor when hovering over the preview, to show that something will happen on click
  • Show a play button in the center of the preview on hover, and make it look clicked and then disappear when the preview is clicked
  • A combination of both
  • Something else?
  • etc.
anthonyfieroni added inline comments.
src/panels/information/phononwidget.cpp
228–229

Why 2nd time ?

meven updated this revision to Diff 61432.Jul 9 2019, 5:08 PM

Avoid a double installEventFilter

meven marked an inline comment as done.Jul 9 2019, 5:09 PM
meven added inline comments.
src/panels/information/phononwidget.cpp
228–229

Good catch, thanks
The first line was added by mistake in a previous diff, making the one added here useless.

meven marked 2 inline comments as done.Jul 9 2019, 5:09 PM
meven added a comment.Jul 15 2019, 7:19 AM

+1 for this feature. However, I feel like as implemented, it won't be very discoverable. To improve this, how about doing one of the following:

  • Use a pointing hand cursor when hovering over the preview, to show that something will happen on click
  • Show a play button in the center of the preview on hover, and make it look clicked and then disappear when the preview is clicked
  • A combination of both
  • Something else?
  • etc.

Great point, discoverability !

In a previous version it had a tooltip text saying "click to play" on the preview on hover.

I would favor the play arrow on the preview and maybe the click mouse cursor on hover.
Users should be familiar with this behavior.

I plan to remove the video preview film borders around the video.

meven updated this revision to Diff 61779.Jul 15 2019, 8:07 AM

Add an arrow on videos to let the user know he can click to preview it, add a Pointing Hand cursor when hover the preview for audio and video

meven updated this revision to Diff 61780.Jul 15 2019, 8:12 AM

Let the video player use the pointing hand cursor

shubham added inline comments.
src/panels/information/phononwidget.cpp
101

use qobjectcast

Much better! UI-wide, I think this is almost there. As a final thing, can you add a white outline around the play icon that's on top of the preview image? Without this, it barely shows up against dark backgrounds.

Also maybe consider making it a bit smaller so it doesn't obscure so much of the preview.

meven updated this revision to Diff 61843.Jul 16 2019, 9:08 AM
meven marked an inline comment as done.

Reduce the play icon to 48px, add a border around the arrow

ngraham accepted this revision.Jul 16 2019, 1:04 PM

Thanks!

This revision is now accepted and ready to land.Jul 16 2019, 1:04 PM
meven updated this revision to Diff 61892.Jul 17 2019, 9:18 AM

Fix play arrow position when devicePixelRatio > 1

meven added a comment.Jul 17 2019, 9:27 AM

I have update the code to reflect an issue with the positioning in Hdpi cases.

I feel the arrow is not very satisfying eye-candy wise, but I have little QPainter experience to make it look better.
So I feel it can land but we should improve on the situation later.

Btw should it land in 19.08 ?

src/panels/information/phononwidget.cpp
101

QEvent does not inherit QObject : qobject_cast can't be used to cast its instances.

Hmm, the triangle looks kinda janky with a 2x scale factor:

It's worse with a factional scale factor (1.5 here):

The new feature deadline is tomorrow; I say we polish this up and land it for 19.12 instead of rushing it into 19.08.

meven added a comment.Jul 17 2019, 6:58 PM

Hmm, the triangle looks kinda janky with a 2x scale factor:

It's worse with a factional scale factor (1.5 here):

The janky-ness is what I meant with not satisfying.
In the 1.5 scale factor, the positioning of the arrow is off as well, smells like a rounding issue, I will look into.

The new feature deadline is tomorrow; I say we polish this up and land it for 19.12 instead of rushing it into 19.08.

I agree no rush.

elvisangelaccio requested changes to this revision.Jul 17 2019, 7:43 PM
elvisangelaccio added inline comments.
src/panels/information/informationpanelcontent.cpp
372–374

Is it possible to not hardcode these numbers?

379–381

Coding style: opening brace should go to the end of previous line.

src/panels/information/phononwidget.cpp
98

Coding style: opening brace should go the next line for functions.

100

Missing space before brace

This revision now requires changes to proceed.Jul 17 2019, 7:43 PM
meven updated this revision to Diff 61945.Jul 18 2019, 6:35 AM

Codying style, use constants, try to fix fractionnal scaling hdpi issue

meven updated this revision to Diff 61946.Jul 18 2019, 6:36 AM
meven marked 2 inline comments as done.

Coding style

meven marked 3 inline comments as done.Jul 18 2019, 6:37 AM
meven updated this revision to Diff 61964.Jul 18 2019, 9:41 AM

Draw a polygone arrow with gradient

meven updated this revision to Diff 61965.Jul 18 2019, 9:56 AM

Add a border around the play arrow, tweak colors

meven added a comment.Jul 18 2019, 9:56 AM

With the latest patch, I have reduced the arrow size and used a gradient to fill it to avoid low contrast with the video :

With dark breeze:

With breeze:

Screenshots are made with the video filmstrip preview option turned off. I really like this option, the current film strip don't look nice.

Some color feedback is welcome as well.

With breeze:

+1, and let's use this style unconditionally. The lighting model itself doesn't change with the color scheme (only the colors themselves). So if there's going ot be a gradient like this, the top part of it should always have the lighter color.

meven updated this revision to Diff 62004.Jul 18 2019, 7:48 PM

Use white to black gradient unconditionally

meven updated this revision to Diff 62028.Jul 19 2019, 7:21 AM

Improve variable naming

meven updated this revision to Diff 62340.Jul 22 2019, 6:37 PM

Check the video is not already playing the current file before resetting the video preview state

meven updated this revision to Diff 62369.Jul 23 2019, 7:46 AM

Avoid playing a paused video when autoplay is on and the panel is resized

meven updated this revision to Diff 62640.Sat, Jul 27, 11:39 AM

Refresh the preview when a video is hidden, in auto-play mode avoid restarting a video that has been stopped when resizing the panel

meven updated this revision to Diff 62712.Mon, Jul 29, 7:07 AM

On autoplay mode when a media file has already been previewed once, a none media file was previewed, and the media file is being previewed again, play it automatically as the autoplay implies

shubham removed a subscriber: shubham.Tue, Jul 30, 5:41 AM
meven retitled this revision from Add click to play/play feature on previews for audio/video to Add click to play/pause feature on previews for audio/video.Wed, Aug 7, 7:54 AM
elvisangelaccio requested changes to this revision.Sun, Aug 11, 3:05 PM
elvisangelaccio added inline comments.
src/panels/information/informationpanelcontent.cpp
371–372

Please always use camelCase.

src/panels/information/phononwidget.cpp
103–119

Have you tried to override mousePressEvent() instead of using an event filter?

src/panels/information/phononwidget.h
78

Why a slot? This is just a getter, isn't it?

This revision now requires changes to proceed.Sun, Aug 11, 3:05 PM
meven updated this revision to Diff 63565.Sun, Aug 11, 6:52 PM

CamelCase two variables, correct a getter

meven marked 2 inline comments as done.Sun, Aug 11, 6:53 PM
meven added inline comments.
src/panels/information/phononwidget.cpp
103–119

I did.
I needed to have an eventFilter so that I can reuse this code directly, informationpanelcontent.cpp line 244 :

m_preview->installEventFilter(m_phononWidget);
meven marked 2 inline comments as done.Sun, Aug 11, 6:54 PM

Btw I think I spotted a regression:

  1. Start playback
  2. Right-click on info panel -> uncheck "Preview"
  3. Right-click again -> check "Preview"

The phonon widget does not show up again.

src/panels/information/phononwidget.cpp
103–119

Why would we need that? I just tried to move this code from eventFilter() to mousePressEvent() and it seems to work fine.

meven updated this revision to Diff 63709.Wed, Aug 14, 9:21 AM
  • for video previews, hide player completely when playback has finished or is stopped- better enforce video size- display the play arrow only for video files
meven added a comment.Wed, Aug 14, 9:23 AM

Btw I think I spotted a regression:

  1. Start playback
  2. Right-click on info panel -> uncheck "Preview"
  3. Right-click again -> check "Preview"

    The phonon widget does not show up again.

Thank you for testing the patch.
This issue is fixed now, and was even more important than your reproducing steps suggests.

meven marked an inline comment as done.Wed, Aug 14, 9:29 AM
meven added inline comments.
src/panels/information/phononwidget.cpp
103–119

It is so that the preview widget can be clicked to start video playback, and thanks to this eventFilter I can reuse directly the logic for both m_phononWidget and m_preview, just use installEventFilter / removeEventFilter.
Using mousePressEvent would force me to add two mousePressEvent in PhononWidget and PixmapViewer and add a code path to call PhononWidget::play() from PixmapViewer::mousePressEvent().

Btw, my latest changes completely hide the video controls as long as the video is not playing or paused.

meven marked 2 inline comments as done.Wed, Aug 14, 9:29 AM