Differential D22183

Add click to play/pause feature on previews for audio/video
ClosedPublic
Actions

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

Details

Reviewers
elvisangelaccio
ngraham
Group Reviewers
Dolphin
Commits
R318:eaec53a7af4e: Add click to play/pause feature on previews for audio/video
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
mouse-click-phono
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 13741
Build 13759: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes
elvisangelaccio added inline comments.Jul 17 2019, 7:43 PM
src/panels/information/informationpanelcontent.cpp
334–336 ↗(On Diff #60911)

Is it possible to not hardcode these numbers?

src/panels/information/phononwidget.cpp
98 ↗(On Diff #60911)

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

100 ↗(On Diff #60911)

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.

ngraham added a comment.Jul 18 2019, 7:29 PM
In D22183#497233, @meven wrote:

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.Jul 27 2019, 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.Jul 29 2019, 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.Jul 30 2019, 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.Aug 7 2019, 7:54 AM
elvisangelaccio requested changes to this revision.Aug 11 2019, 3:05 PM
elvisangelaccio added inline comments.
src/panels/information/informationpanelcontent.cpp
333–334 ↗(On Diff #60911)

Please always use camelCase.

src/panels/information/phononwidget.cpp
103–119 ↗(On Diff #60911)

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

src/panels/information/phononwidget.h
76 ↗(On Diff #60911)

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

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

CamelCase two variables, correct a getter

meven marked 2 inline comments as done.Aug 11 2019, 6:53 PM
meven added inline comments.
src/panels/information/phononwidget.cpp
103–119 ↗(On Diff #60911)

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.Aug 11 2019, 6:54 PM
elvisangelaccio added a comment.Aug 13 2019, 8:34 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 ↗(On Diff #60911)

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.Aug 14 2019, 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.Aug 14 2019, 9:23 AM
In D22183#511449, @elvisangelaccio wrote:

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.Aug 14 2019, 9:29 AM
meven added inline comments.
src/panels/information/phononwidget.cpp
103–119 ↗(On Diff #60911)

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.Aug 14 2019, 9:29 AM
meven updated this revision to Diff 64533.Aug 25 2019, 7:19 AM

Make state() a simple accessor, not a slot, camelcase some variable names

alexde added a subscriber: alexde.Aug 25 2019, 8:50 AM

Is the triangle set? How about such a triangle:

https://www.netclipart.com/pp/m/120-1201455_play-button-png-video-youtube-youtube-video-play.png

I can imagine that it can be discerned better due to the dark background casted on the image preview within the outer white ring.
For Breeze Dark, the colors could become inverted.

meven added a comment.Aug 25 2019, 9:37 AM
In D22183#518504, @alexde wrote:

Is the triangle set? How about such a triangle:

https://www.netclipart.com/pp/m/120-1201455_play-button-png-video-youtube-youtube-video-play.png

I can imagine that it can be discerned better due to the dark background casted on the image preview within the outer white ring.
For Breeze Dark, the colors could become inverted.

I am not against a better version, but the culprit is that I am no designer.
For anything more complicated we would need an icon/svg.

alexde added a comment.EditedAug 25 2019, 11:27 AM

I am not against a better version, but the culprit is that I am no designer.
For anything more complicated we would need an icon/svg.

Me neither but I created a SVG for you with Inkscape:

Play_white.svg2 KBDownload

Play_black.svg2 KBDownload

You may use them for testing or change them the way you like.

elvisangelaccio added a comment.Aug 25 2019, 2:47 PM
In D22183#511449, @elvisangelaccio wrote:

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.

Hmm, I can still reproduce this with the latest version of the patch

src/panels/information/informationpanelcontent.cpp
171–172

Why remove this comment? We are still killing the preview job here, aren't we?

src/panels/information/phononwidget.cpp
105 ↗(On Diff #60911)

Unnecessary semicolon

meven updated this revision to Diff 64642.Aug 26 2019, 8:09 AM
meven marked an inline comment as done.

Fix issue when showing/adding preview, re-add comment, formatting

meven added a comment.Aug 26 2019, 8:33 AM
In D22183#518560, @alexde wrote:

I am not against a better version, but the culprit is that I am no designer.
For anything more complicated we would need an icon/svg.

Me neither but I created a SVG for you with Inkscape:

Play_white.svg2 KBDownload

Play_black.svg2 KBDownload

You may use them for testing or change them the way you like.

Those are nice, but I would rather use those in a subsequent diff not to bloat this long standing one.

src/panels/information/informationpanelcontent.cpp
171–172

The comment was moved to InformationPanelContent::refreshPreview but I have re-added it here as well.

meven marked 2 inline comments as done.Aug 26 2019, 8:33 AM
meven updated this revision to Diff 65058.Aug 31 2019, 5:31 PM

Avoid displaying the play arrow on audio files covers

elvisangelaccio added a comment.Sep 1 2019, 12:36 PM

Patch looks almost good now.

Is it normal that that play arrow gets painted only at the beginning of a video?

src/panels/information/phononwidget.cpp
105 ↗(On Diff #60911)

Still not fixed ;)

meven updated this revision to Diff 65128.Sep 1 2019, 2:34 PM

Remove a semicolon too much

meven marked an inline comment as done.Sep 1 2019, 2:34 PM

ping @elvisangelaccio

I might have a conflict appearing with D23538.

elvisangelaccio accepted this revision.Sep 1 2019, 2:41 PM

Please push it. D23538 will have to be rebased.

This revision is now accepted and ready to land.Sep 1 2019, 2:41 PM
meven added a comment.Sep 1 2019, 2:42 PM
In D22183#523507, @elvisangelaccio wrote:

Is it normal that that play arrow gets painted only at the beginning of a video?

I don't see what you mean.
Which play arrow ? On the video preview, or on the seekslider bar ?
Either way they are painted when the videos are being previewed.
Just like :

Very happy to reach acceptability, thank you for your review @elvisangelaccio

Closed by commit R318:eaec53a7af4e: Add click to play/pause feature on previews for audio/video (authored by meven). · Explain WhySep 1 2019, 2:48 PM
This revision was automatically updated to reflect the committed changes.
elvisangelaccio added a comment.Sep 1 2019, 3:04 PM

Yes, I meant the one on the video preview.

meven added a comment.Sep 1 2019, 3:39 PM
In D22183#523598, @elvisangelaccio wrote:

Yes, I meant the one on the video preview.

I don't get what you exactly mean gets painted only at the beginning of a video
The preview shown is the frame at 20% playback of the video currently, and the play arrow is painted on top.

Revision Contents
Changeset List

Diff 61406

View Options

src/panels/information/informationpanelcontent.cpp

Loading...
View Options

src/panels/information/phononwidget.h

Loading...
View Options

src/panels/information/phononwidget.cpp

Loading...