4
\$\begingroup\$

I wrote a Python MP3 player for Linux using PyGame, with a curses GUI and a mouse-only interface.

It's purely for personal use.

It hasn't been polished yet, but for now I'd like to know what I've done inefficiently so I can fill in some ignorance-holes. I know a few things could be handled more directly.

import curses
import pygame

pygame.mixer.init(frequency=22050, size=-16, channels=2, buffer=4096)
player = pygame.mixer.music
song = "testshort.mp3"
player.load(song)
player.play()
screen = curses.initscr()
curses.noecho()
curses.mousemask(curses.ALL_MOUSE_EVENTS)
screen.keypad(1)
curses.curs_set(0)
screen.resize(6, 30)
screen.border(0)
screen.addstr(4, 19, "*"*5+"-"*5)
player.set_volume(0.5)
screen.addstr(4, 2, "|")
paused = 0
curses.halfdelay(1)

while True:
            busycheck = player.get_busy()
            if busycheck == 0:
                screen.addstr(4, 2, ">")
            char = screen.getch()            
            if char == curses.KEY_MOUSE:
                cords = list(curses.getmouse())
                mouse_x = cords[1]
                mouse_y = cords[2]
                vol_on = mouse_x - 18
                vol_off = 10 - vol_on
                screen.border(0)
                if mouse_y == 4:
                    if 29 > mouse_x > 18:
                        vol_bar = screen.addstr(4, 19, "*"*vol_on+"-"*vol_off)
                        volchange = (mouse_x-19)/9.0
                        player.set_volume(volchange)
                    elif mouse_x == 2:
                        if paused == 0:
                            if busycheck == 1:
                                screen.addstr(4, 2, ">")
                                player.pause()
                                paused = 1
                            else:
                                player.play()
                                paused = 0
                                screen.addstr(4, 2, "|")
                        else:
                            player.unpause()
                            screen.addstr(4, 2, "|")
                            paused = 0
\$\endgroup\$

migrated from stackoverflow.com Dec 10 '11 at 1:42

This question came from our site for professional and enthusiast programmers.

4
\$\begingroup\$
import curses
import pygame

pygame.mixer.init(frequency=22050, size=-16, channels=2, buffer=4096)
player = pygame.mixer.music

When giving modules a shorter name, the usual method is from pygame.mixer import music as player It makes it a little clearer what you are doing.

song = "testshort.mp3"
player.load(song)
player.play()

For anything but a quick script, its recommended that you put your code inside a main function.

screen = curses.initscr()

I recommend that you take a look at the curses.wrapper function. It'll take care of closing curses correctly in the case of an error.

curses.noecho()
curses.mousemask(curses.ALL_MOUSE_EVENTS)
screen.keypad(1)
curses.curs_set(0)
screen.resize(6, 30)
screen.border(0)
screen.addstr(4, 19, "*"*5+"-"*5)
player.set_volume(0.5)
screen.addstr(4, 2, "|")

Your code jumps between curses settings, outputing to the screen, and setting up the player. I recommend dividing it cleanly between these three different tasks.

paused = 0
curses.halfdelay(1)

while True:
            busycheck = player.get_busy()
            if busycheck == 0:

use if not player.get_busy(). Using == to check in the case of bools is considered bad practice. And there is no reason to split across two lines here

                screen.addstr(4, 2, ">")
            char = screen.getch()            
            if char == curses.KEY_MOUSE:
                cords = list(curses.getmouse())

There is no reason to call list here.

                mouse_x = cords[1]
                mouse_y = cords[2]

I suggest mouse_x, mouse_y = coords[:2]

                vol_on = mouse_x - 18
                vol_off = 10 - vol_on
                screen.border(0)

Why are you calling screen.border here?

                if mouse_y == 4:
                    if 29 > mouse_x > 18:
                        vol_bar = screen.addstr(4, 19, "*"*vol_on+"-"*vol_off)

You are basically repeating what you had previously. I suggest refactoring so as to share code.

                        volchange = (mouse_x-19)/9.0
                        player.set_volume(volchange)

See how you are effectively changing the same piece of information three times? This suggests you should put all of that in a function.

                    elif mouse_x == 2:
                        if paused == 0:
                            if busycheck == 1:
                                screen.addstr(4, 2, ">")
                                player.pause()
                                paused = 1
                            else:
                                player.play()
                                paused = 0
                                screen.addstr(4, 2, "|")
                        else:
                            player.unpause()
                            screen.addstr(4, 2, "|")
                            paused = 0

Again, your code tangles together in a seeming haphazard manner the different things it does.

This is how I'd do it:

import curses
import pygame
from pygame.mixer.music import music

SONG = "testshort.mp3"

class MusicPlayer(object):
    def __init__(self):
        pygame.mixer.init(frequency=22050, size=-16, channels=2, buffer=4096)
        music.load(SONG)
        music.play()
        self.paused = False
        self.change_volume(5)

    def change_volume(self, volume):
        self.volume = volume
        music.set_volume( volume / 9.0 )

    def toggle_pause(self):
        self.paused = not self.paused:
        if self.paused:
            music.pause()
        else:
            music.unpause()


def update_screen(screen, player):
    screen.addstr(4, 19, "*" * player.volume + "-"* (10 -player.volume))

    if music.get_busy() or player.paused:
        screen.addstr(4, 2, "|")
    else:
        screen.addstr(4, 2, ">")


def main(screen):
    curses.mousemask(curses.ALL_MOUSE_EVENTS)
    screen.keypad(1)
    curses.curs_set(0)
    screen.resize(6, 30)
    screen.border(0)
    curses.halfdelay(1)

    player = MusicPlayer()

    while True:
        update_screen(screen, player)

        char = screen.getch()            
        if char == curses.KEY_MOUSE:
            mouse_x, mouse_y = curses.getmouse()[:2]
            if mouse_y == 4:
                if 29 > mouse_x > 18:
                    player.change_volume(mouse_x - 18)
            elif mouse_x == 2:
                player.toggle_pause()

if __name__ == '__main__':
    curses.wrapper(main)

See how each piece of the code worries about one small thing? We don't mix controlling the music with updating the display.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.