Differential D14578

Fix video controls volume slider not vertically centered sometimes
ClosedPublic
Actions

Authored by muhlenpfordt on Aug 3 2018, 1:33 PM.

Details

Reviewers
rkflx
Group Reviewers
Gwenview
Commits
R260:fec2cd8b655e: Fix video controls volume slider not vertically centered sometimes
Summary

The volume slider of the video controls overlay is sometimes not
centered vertically. This happens mostly on KDE Neon or if Gwenview
is started to show a video by commandline argument.
This is caused by calculating the slider and handle positions before
initializing the widget and therefore using wrong values. After the
first slider change this is correctly updated.
This patch delays the initialization of the slider to give the
overlay widget time to setup.

BUG: 390929
FIXED-IN: 18.08.0

Before:

After:

Test Plan
  • Start Gwenview with video file as commandline argument
  • Check that all sliders are vertically centered in controls overlay
  • Check in current KDE Neon system

Diff Detail

Repository
R260 Gwenview
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
muhlenpfordt requested review of this revision.Aug 3 2018, 1:33 PM
muhlenpfordt created this revision.
rkflx accepted this revision.Aug 3 2018, 9:55 PM
rkflx added a subscriber: rkflx.

Nice! This fixes even the time slider which showed the same issue when trying to open a video with an unsupported codec.

FIXED-IN: 18.08.1

Any reason you are not going for 18.08.0?

BTW, unrelated but it would be interesting to know if you can reproduce:

  • When switching many times between a video and an image, sometimes the volume slider is not at the left-most position, but somewhere in the middle. In particular I got this for a webm file without audio, e.g. F5477455. (For this file the slider position is remembered correctly, and the right-most position means loudest.)
  • Starting Gwenview like in your test plan, sometimes the status bar overlaps the HUD.

Last question :): Do you happen to have access to a mouse with forward/backward buttons on the side, to help testing https://phabricator.kde.org/D14583, or a touchscreen for https://phabricator.kde.org/D13901?

lib/hud/hudslider.cpp
125–129

A small thing I noticed: showEvent is also called when navigating away and calling updateHandleRect would not really needed. In other places we use something like this (in the constructor):

QTimer::singleShot(0, this, [this]() {
    d->updateHandleRect();
});

What do you think? (I would also be fine with your version.)

This revision is now accepted and ready to land.Aug 3 2018, 9:55 PM
rkflx added a comment.Aug 3 2018, 10:34 PM

Initialize HudSlider for video controls after widget is setup

Ah, one more suggestion in the quest for perfection, which I became aware of based on recent experiences: When gathering information for the release notes, it makes the process a lot faster if the title included a more user-oriented description. Ideally, we could just copy the output of git log --pretty=oneline --abbrev-commit to the etherpad.

Your current title is technically accurate, but does not really describe what the change results in from a user's point of view (or at least can be understood and rewritten by the promo team). What I sometimes see is something along the lines of "Change $something to fix a $problem", or "Fix $problem by changing $something", or simply "Fix $problem". Of course, not always is there enough space.

You don't have to change it here, but maybe something to keep in mind for the future ;)

muhlenpfordt updated this revision to Diff 39047.Aug 4 2018, 8:32 AM
muhlenpfordt marked an inline comment as done.
muhlenpfordt retitled this revision from Initialize HudSlider for video controls after widget is setup to Fix video controls volume slider not vertically centered sometimes.
muhlenpfordt edited the summary of this revision. (Show Details)

Use singleShot timer instead of showEvent

muhlenpfordt added a comment.Aug 4 2018, 8:32 AM
In D14578#303023, @rkflx wrote:

BTW, unrelated but it would be interesting to know if you can reproduce:

  • When switching many times between a video and an image, sometimes the volume slider is not at the left-most position, but somewhere in the middle. In particular I got this for a webm file without audio, e.g. F5477455. (For this file the slider position is remembered correctly, and the right-most position means loudest.)

I noticed this on Kubuntu but not in Neon VM (maybe because of the disabled audio). It seems to come from a Phonon::AudioOutput::volumeChanged signal but no idea why this happens only sometimes.

  • Starting Gwenview like in your test plan, sometimes the status bar overlaps the HUD.

I never saw this happen, on none of the test systems.

Last question :): Do you happen to have access to a mouse with forward/backward buttons on the side, to help testing https://phabricator.kde.org/D14583, or a touchscreen for https://phabricator.kde.org/D13901?

I digged in my hardware collection but have none of these (Touchscreen only in Android tablet).
It's not a real life test but the mouse clicks could be simulated with xdotool:

  • xdotool click 8 -> Qt::BackButton
  • xdotool click 9 -> Qt::ForwardButton
In D14578#303038, @rkflx wrote:

You don't have to change it here, but maybe something to keep in mind for the future ;)

Good point. I thought about that but lost sight of it.

lib/hud/hudslider.cpp
125–129

It doesn't make a difference in number of calls since the HudSlider object is deleted and recreated on navigation. But it's a more compact change now. :)

muhlenpfordt added a comment.Aug 4 2018, 11:16 AM
In D14578#303087, @muhlenpfordt wrote:
In D14578#303023, @rkflx wrote:

BTW, unrelated but it would be interesting to know if you can reproduce:

  • When switching many times between a video and an image, sometimes the volume slider is not at the left-most position, but somewhere in the middle. In particular I got this for a webm file without audio, e.g. F5477455. (For this file the slider position is remembered correctly, and the right-most position means loudest.)

I noticed this on Kubuntu but not in Neon VM (maybe because of the disabled audio). It seems to come from a Phonon::AudioOutput::volumeChanged signal but no idea why this happens only sometimes.

After playing the Bunny-video from your link the behaviour changed on my Kubuntu system - seems to be the first video with audio I ever played with Gwenview.

  • Playing a video with audio from commandline the volume slider jumps from 0 to the last manually set position (visible short delay)
  • Playing a video without audio from commandline the volume slider stays at 0 position
  • Navigating to a video (no matter with or without audio) the volume slider is at the last manually set position (no visible jump)
rkflx accepted this revision.Aug 4 2018, 10:51 PM

Thanks for the updates, LGTM ;)

[off-topic]

Thanks for testing. Looks like the volume slider still has some problems.

In D14578#303087, @muhlenpfordt wrote:
In D14578#303023, @rkflx wrote:
  • Starting Gwenview like in your test plan, sometimes the status bar overlaps the HUD.

I never saw this happen, on none of the test systems.

Odd, now I cannot reproduce either. I'm pretty sure I saw this (the video was positioned correctly, only the HUD was too close to the bottom), perhaps it depends on the window size or the aspect ratio of the video.

  • xdotool click 8 -> Qt::BackButton

Good idea, I'll try this.

lib/hud/hudslider.cpp
125–129

For a moment you scared me ;)

I double-checked, there will be a difference in the number of calls per slider: showEvent is apparently called when going to a video and when navigating away, but the constructor is thankfully called only a single time, i.e. when going to a video.

muhlenpfordt added inline comments.Aug 5 2018, 7:16 AM
lib/hud/hudslider.cpp
125–129

Sorry! Sure, when leaving a video it's one call less. I missed the "navigating away" in your first comment.

Closed by commit R260:fec2cd8b655e: Fix video controls volume slider not vertically centered sometimes (authored by muhlenpfordt). · Explain WhyAug 5 2018, 7:16 AM
This revision was automatically updated to reflect the committed changes.
rkflx added a comment.Aug 7 2018, 10:17 PM
In D14578#303087, @muhlenpfordt wrote:

It seems to come from a Phonon::AudioOutput::volumeChanged signal but no idea why this happens only sometimes.

Looked more into this: This seems to be caused by calling mMediaObject->play() at an unfortunate time so PulseAudio (not sure, I was missing some debug symbols) and eventually Phonon changes the volume a bit randomly. Maybe it has problems when we request playing too often in a given timespan or too early after initialization.

I don't think there's anything Gwenview is doing wrong, so I'll probably leave it at that.

Revision Contents
Changeset List

PathPackages
Mlib/hud/hudslider.cpp (5 lines)

Diff 39109

View Options

lib/hud/hudslider.cpp

Loading...