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

Waveform Seekbar time indication (#2419) #2550

Merged
merged 13 commits into from Sep 10, 2017

Conversation

@Muges
Copy link
Contributor

@Muges Muges commented Sep 1, 2017

This pull request fixes issue #2419:

  • It adds a soundcloud-like visual effect when the mouse hovers the waveform seekbar, and shows the time pointed by the mouse instead of the current position.
    peek 01-09-2017 20-17
  • It allows to seek by dragging (pressing the mouse button, moving it, then releasing it), in the same way as a Gtk.Scale.

Here is what it looks like with various popular gtk themes (from top to bottom: Adwaita-dark, Adwaita, Arc-Dark, Arc, Ambiance, Radiance):

waveformseekbar-hover

As you can see the default "hover color" (the one to the left of the mouse cursor in the screenshots above) is not always great. It is computed by blending the two other colors, and I could not find any other way that works well for all the themes I tested. I hope it's good enough, considering that the three colors used in the waveform are configurable in the plugin's options. If not, I could remove the visual effect, and only change the time labels when hovering.

@declension
Copy link
Member

@declension declension commented Sep 3, 2017

Hmm, I can't get it to work as intended here (with Ubuntu 17.04 + HiDPI). The hovering seems to un-highlight the played section when going back in time but not at all when going forwards, and can't ever go further than half way.

It can also result in highlighting that looks like dark-light-dark-none (screenshots force a redraw which fixes this issue, though, so can't show this)

@Muges
Copy link
Contributor Author

@Muges Muges commented Sep 3, 2017

I did not take into account the gtk scale factor for HiDPI displays. This should be fixed.

@ptitjes
Copy link
Collaborator

@ptitjes ptitjes commented Sep 5, 2017

I did not test it yet, but the code looks good to me.

@declension
Copy link
Member

@declension declension commented Sep 6, 2017

@Muges thanks that fixes the DPI bugs.

Generally looks great to me. The only thing is the colours: I find it confusing that if you:

  • hover to the left (before) the current position, a light colour means currently played, dark means (tentative) new position.
  • hover to the right (after), dark means currently played, light means (tentative) new position

Perhaps the whole bar should be lighter on dragging (yes - this means you can't see the current position I guess, but it feels less strange than colours swapping meaning IMO)

@Muges
Copy link
Contributor Author

@Muges Muges commented Sep 6, 2017

The last commits I pushed makes the whole bar lighter as suggested, and shows the current position with a vertical line. It looks like this:
peek 06-09-2017 20-55

If you prefer I can remove the vertical line.

@ptitjes

This comment has been minimized.

Copy link
Collaborator

@ptitjes ptitjes commented on quodlibet/quodlibet/ext/events/waveformseekbar.py in 36c2bc1 Sep 6, 2017

Are you sure you want to use scale_factor and not line_width (or line_width * a constant) ?

scale_factor is proportional to the display quality.

This comment has been minimized.

Copy link
Contributor Author

@Muges Muges replied Sep 6, 2017

Are you sure you want to use scale_factor and not line_width (or line_width * a constant) ?

I don't think so.

If I understood well, line_width is used on line 361 to make cairo draw lines whose width are one "physical" pixel (as opposed to a "visual" pixel, which on a hidpi display may be composed of 2x2 "physical" pixels).

So if I wanted to draw a line of width one "physical" pixel, I would not need to use line_width. I would just need to replace line 386 by

if position_width - 1 <= x < position_width:

Now, I think a line of width one "physical" pixel will probably look too thin on HiDPI displays (I don't have one, so I can't check), so I used scale_factor to make it one "visual" pixel wide.

I probably didn't make myself very clear, so here's a screenshot of quodlibet running with GDK_SCALE=2 to simulate a HiDPI display:
sans titre
As you can see, the width of the blue vertical line is consistent with the other lines of the UI, which is what I wanted to achieve.

@declension
Copy link
Member

@declension declension commented Sep 7, 2017

@Muges good work - I think it's much clearer now. Personally I think it looks nicer without the line at all, but if other people feel strongly that's fine

@Muges
Copy link
Contributor Author

@Muges Muges commented Sep 9, 2017

Personally I think it looks nicer without the line at all, but if other people feel strongly that's fine

Okay, I removed it. I think I liked it a bit better with a cursor to show the current position, but it's fine either way.

@declension
Copy link
Member

@declension declension commented Sep 10, 2017

Thanks @Muges. Perhaps this could be an prefs option then longer term.

Seems good to get this merged now (I'll then rebase the patch for #2470 if I get some time).

@declension declension merged commit 8389e65 into quodlibet:master Sep 10, 2017
8 checks passed
8 checks passed
ci/circleci: job.debian8.py2 Your tests passed on CircleCI!
Details
ci/circleci: job.debian8.py3 Your tests passed on CircleCI!
Details
ci/circleci: job.ubuntu16.04.py2 Your tests passed on CircleCI!
Details
ci/circleci: job.ubuntu16.04.py3 Your tests passed on CircleCI!
Details
ci/circleci: job.ubuntu17.10.py3 Your tests passed on CircleCI!
Details
ci/circleci: job.win32.py2 Your tests passed on CircleCI!
Details
ci/circleci: job.win32.py3 Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Meriipu
Copy link
Contributor

@Meriipu Meriipu commented Oct 30, 2017

So if I understand this it is intended that hovering the waveform seekbar while a track is playing will hide the current position in the track and only show where you would seek to?

I miss having the current position show even when hovering

(I really like how it looks in the images in the first post)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants