MP3 player for Linux

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()

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.

StackExchange Code Review Q#6669, answer score: 4