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

Added Waveform seek bar #2046

Merged
merged 10 commits into from Oct 1, 2016

Conversation

Projects
None yet
3 participants
@0x1777
Contributor

0x1777 commented Sep 30, 2016

No description provided.

def do_draw(self, cr):
# Paint the background
context = self.get_style_context()
bg_color = context.get_background_color(Gtk.StateFlags.NORMAL)

This comment has been minimized.

@lazka

lazka Sep 30, 2016

Member

This no longer works on newer gtk ( and is probably the source of 100% CPU) You need to do:

context.save()
context.set_state(Gtk.StateFlags.NORMAL)
bg_color = context.get_background_color(context.get_state())
context.restore()
cr.stroke()
def do_realize(self):

This comment has been minimized.

@lazka

lazka Sep 30, 2016

Member

Why no subclass a Gtk.EventBox?

This comment has been minimized.

@0x1777

0x1777 Sep 30, 2016

Contributor

Yeah that's a better solution. Fixed the other things too.

elapsed_color = Gdk.RGBA()
elapsed_color.parse(get_fg_color())
remaining_color = context.get_color(Gtk.StateFlags.SELECTED)

This comment has been minimized.

@lazka

lazka Sep 30, 2016

Member

same as above

def _on_bus_message(self, bus, message):
if message.type == Gst.MessageType.ERROR:
error, debug = message.parse_error()
print("Error received from element {name}: {error}".format(

This comment has been minimized.

@lazka

lazka Sep 30, 2016

Member

Please use quodlibet.print_d instead of print() everywhere. It adds logging and works better under Windows.

@lazka

This comment has been minimized.

Member

lazka commented Sep 30, 2016

Thanks for the update. A few more things I noticed while using it:

  • It crashes when clicking on it after enabling the plugin before switching songs
  • When enabling it it needs a song change for it to generate the waveform
  • When no song is active (after the playlist is finished) the last waveform stays visible
  • It would be nice if there was some kind of "computing state" when switching songs before the waveform is computed instead of showing the old one. Maybe just a thick line in the same style or something. The current behavior makes it hard to tell if the shown waveform is the current one or it just wasn't computed yet.

Except for the first point (the crash), this can be deferred to future PR/commits if you want.

@nodiscc

This comment has been minimized.

nodiscc commented Sep 30, 2016

Thank you! Definitely looks like a killer feature to navigate in long tracks, etc. I have not tested it yet, looking forward to it.

It would be nice if there was some kind of "computing state" when switching songs

What usually works is to show a striped pattern (eg light gray diagonal stripes ///////) in place of the waveform. In any case it should not be confusing whether waveform is computing/track is broken/track is silence/track is 0db noise, so the pattern must be remarkable.

@0x1777

This comment has been minimized.

Contributor

0x1777 commented Sep 30, 2016

Ok, I think I fixed all 4 points. It simply displays a horizontal line now when no waveform is available.

@lazka

This comment has been minimized.

Member

lazka commented Oct 1, 2016

What usually works is to show a striped pattern (eg light gray diagonal stripes ///////) in place of the waveform. In any case it should not be confusing whether waveform is computing/track is broken/track is silence/track is 0db noise, so the pattern must be remarkable.

Valid point.

Ok, I think I fixed all 4 points. It simply displays a horizontal line now when no waveform is available.

Looks good!

@lazka

lazka approved these changes Oct 1, 2016

@lazka lazka merged commit 5f5d448 into quodlibet:master Oct 1, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@lazka lazka referenced this pull request Oct 1, 2016

Closed

Waveform seekbar plugin #1530

@lazka

This comment has been minimized.

Member

lazka commented Oct 1, 2016

One more thing: Any thoughts on using a theme color for the "elapsed time" part of the waveform instead of the configurable value? That would make it work with all themes.

@0x1777

This comment has been minimized.

Contributor

0x1777 commented Oct 1, 2016

Sounds good, that would make it look more consistent.
I'd say the colors of the scale widget would be good. But I don't know how to get them from the theme.

I tried context.get_color() and context.get_background_color() with different Gtk.StateFlags.* values but they are all the same.

context.lookup_color("selected_bg_color")[1] seems to work with some themes (like Arc) but not with others (like Adwaita).

@lazka

This comment has been minimized.

Member

lazka commented Oct 1, 2016

The fg color for the LINK state should work. That's for inline links in
labels.

On Sat, Oct 1, 2016, 15:05 0x1777 notifications@github.com wrote:

Sounds good, that would make it look more consistent.
I'd say the colors of the scale widget would be good. But I don't know how
to get them from the theme.

I tried context.get_color() and context.get_background_color() with
different Gtk.StateFlags.* values but they are all the same.

elapsed_color = context.lookup_color("selected_bg_color")[1] seems to
work with some themes (like Arc) but not with others (like Adwaita).


You are receiving this because you modified the open/close state.

Reply to this email directly, view it on GitHub
#2046 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AA8i8nv8QxRxZ3aXPoATSNpEbjMSjPEsks5qvlqMgaJpZM4KK99N
.

@0x1777

This comment has been minimized.

Contributor

0x1777 commented Oct 1, 2016

Yes that works. It'll use the theme's color by default now unless the user overrides it in the config.
I'll open a new pull request then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment