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

plugin: add a tap bpm plugin #2264

Merged
merged 2 commits into from Feb 25, 2017
Merged

plugin: add a tap bpm plugin #2264

merged 2 commits into from Feb 25, 2017

Conversation

@ptitjes
Copy link
Collaborator

@ptitjes ptitjes commented Feb 23, 2017

This is a very little plugin that allows the user to tap the BPM for the selected song and save it in its tags.

@ptitjes ptitjes force-pushed the ptitjes:tapbpm branch from 00765b4 to 610189f Feb 24, 2017
self.pack_start(box, False, True, 0)

self.tap_btn = Gtk.Button(label=_("Tap"))
self.tap_btn.connect('button-press-event', self.tap)

This comment has been minimized.

@lazka

lazka Feb 24, 2017
Member

'clicked' event, otherwise pressing space/enter wont do anything if the button is focused.

This comment has been minimized.

@ptitjes

ptitjes Feb 24, 2017
Author Collaborator

I used press event because when you tap with a mouse or a pad, whereas the clicked comes at the release of the mouse/pad button. The user (well me) expects to tap at press. It is really not nice when tap is triggered by release, because not only you have to tap in rhythm but also kinda press the button for the same duration every time (not simple, I tried :p).

Maybe I should document it so that not one wonders in the future.

I could add an additional trigger for key-presses when the button is focused...

This comment has been minimized.

@lazka

lazka Feb 24, 2017
Member

I see, let's leave it that way then for now..


box = Gtk.HBox()
box.set_spacing(6)
box.pack_start(Gtk.Label(_("BPM:")), False, True, 0)

This comment has been minimized.

@lazka

lazka Feb 24, 2017
Member

add a translators comment above:

# TRANSLATORS: BPM measn "beats per minute"

This comment has been minimized.

@ptitjes

ptitjes Feb 24, 2017
Author Collaborator

Sure !

self.show_all()

def update(self):
GObject.idle_add(self.do_update)

This comment has been minimized.

@lazka

lazka Feb 24, 2017
Member

Why the idle_add?

This comment has been minimized.

@ptitjes

ptitjes Feb 24, 2017
Author Collaborator

It won't work without it, I think because we are in modal dialog.

This comment has been minimized.

@lazka

lazka Feb 24, 2017
Member

What doesn't work? It seems to work the same here as far as I can see.

This comment has been minimized.

@ptitjes

ptitjes Feb 24, 2017
Author Collaborator

If call do_update directly in update, the label is never updated.

This comment has been minimized.

@ptitjes

ptitjes Feb 24, 2017
Author Collaborator

OK, forget it you're right ! :)
I'll change that.

return

# Save metadata
self._panel.save()

This comment has been minimized.

@lazka

lazka Feb 24, 2017
Member

Not a bug, but try to save before destroying the window. After destroy() widgets shouldn't be used anymore and while it currently doesn't use it in save(), it might in the future.

This comment has been minimized.

@ptitjes

ptitjes Feb 24, 2017
Author Collaborator

Indeed, I'll modify that !

* clean code
* add key-press-event support
* save metadata before destroying widgets
@ptitjes
Copy link
Collaborator Author

@ptitjes ptitjes commented Feb 24, 2017

@lazka Changes made as requested. I kept using "button-press-event" but added code to handle space/return key presses.

@lazka lazka merged commit 6c4fa6d into quodlibet:master Feb 25, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@lazka
Copy link
Member

@lazka lazka commented Feb 25, 2017

Thanks

@ptitjes
Copy link
Collaborator Author

@ptitjes ptitjes commented Feb 25, 2017

Thank you !

@ptitjes ptitjes deleted the ptitjes:tapbpm branch Feb 25, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants