Music player written in Python using PyQt

Question

Some days ago, in the middle of a flight, in one hour or so, I've written this basic music player using Python3 and PyQt4.

I needed some easy to use music player, so I decided to write this piece of code that just works.

This little guy is called Frederic. It uses, besides PyQt for the GUI, a library called mutagen to work with tags in the music files.

This is my first time with Code Review, so I decided to publish this little piece of code to try it.

Any criticism, suggestion and opinion is welcome. Thanks!

frederic.py:

import sys
from PyQt4 import QtCore, QtGui
from PyQt4.phonon import Phonon
from mutagen.id3 import ID3

class Frederic(QtGui.QDialog):

    def __init__(self):
        QtGui.QWidget.__init__(self)

        vbox = QtGui.QVBoxLayout(self)

        self.setWindowTitle("Frederic Music Player")
        self.move(400, 200)

        self.label = QtGui.QLabel(self)
        self.status = QtGui.QLabel(self)
        self.play = QtGui.QPushButton("Play")
        self.pause = QtGui.QPushButton("Pause")
        self.stop = QtGui.QPushButton("Stop")
        self.siguiente = QtGui.QPushButton("Siguiente")
        self.anterior = QtGui.QPushButton("Anterior")
        self.listWidget = QtGui.QListWidget(self)

        self.agregar = QtGui.QPushButton("Agregar")
        self.eliminar = QtGui.QPushButton("Eliminar")
        self.artist = None
        self.song = None

        # Phonon settings

        self.player = Phonon.createPlayer(Phonon.MusicCategory)
        self.player.setTickInterval(100)
        self.player.tick.connect(self.ticking)
        self.spin = Phonon.SeekSlider(self.player, self)

        # Implementing the layout

        vbox.addWidget(self.label)
        vbox.addWidget(self.status)

        hbox0 = QtGui.QHBoxLayout(self)

        hbox0.addWidget(self.play)
        hbox0.addWidget(self.pause)
        hbox0.addWidget(self.stop)
        hbox0.addWidget(self.anterior)
        hbox0.addWidget(self.siguiente)

        vbox.addLayout(hbox0)

        vbox.addWidget(self.listWidget)

        hbox = QtGui.QHBoxLayout(self)

        hbox.addWidget(self.agregar)
        hbox.addWidget(self.eliminar)

        vbox.addLayout(hbox)
        vbox.addWidget(self.spin)

        self.setLayout(vbox)

        self.play.clicked.connect(self.funcPlay)
        self.agregar.clicked.connect(self.funcAgregar)
        self.eliminar.clicked.connect(self.funcEliminar)
        self.pause.clicked.connect(lambda: self.player.pause())
        self.stop.clicked.connect(lambda: self.player.stop())
        self.player.aboutToFinish.connect(self.nextSongQueued)
        self.siguiente.clicked.connect(self.nextSong)
        self.anterior.clicked.connect(self.previousSong)

        self.file_name = None
        self.data = None
        self.songs = {}
        self.SongPlaying = None
        self.SongQueued = None

    def funcPlay(self):
        self.label.setText(self.listWidget.currentItem().text())
        CurrentSong = (self.listWidget.currentItem().text())
        self.SongPlaying = CurrentSong
        SongToPlay = self.songs[CurrentSong]
        self.player.setCurrentSource(Phonon.MediaSource(SongToPlay))
        self.player.play()

    def funcPause(self):
        self.player.pause()

    def ticking(self, time):
        displayTime = QtCore.QTime(0, (time / 60000) % 60, (time / 1000) % 60)
        self.status.setText(displayTime.toString('mm:ss'))

    def funcAgregar(self):
        self.file_name = QtGui.QFileDialog.getOpenFileName(
            self, "Open Data File", "", "MP3 (*.mp3)")

        id3 = ID3(self.file_name)

        try:
            self.data = self.artist = id3['TPE1'].text[0] + " - " + id3["TIT2"].text[0]
            self.songs[self.data] = self.file_name
            self.listWidget.addItem(self.data)
        except:
            self.data
            self.songs[self.file_name] = self.file_name
            self.listWidget.addItem(self.data)
            d = QtGui.QMessageBox()
            d.setWindowTitle('Error!')
            d.setText("El archivo que ha elegido no funciona. Seleccione otro archivo por favor.")
            d.exec_()

    def funcEliminar(self):
        self.listWidget.takeItem(self.ui.listWidget.currentRow())

    def nextSongQueued(self):
        next = self.listWidget.currentRow() + 1
        nextSong = self.listWidget.item(next).text()
        self.SongQueued = nextSong
        SongToPlay = self.songs[nextSong]
        self.player.enqueue(Phonon.MediaSource(SongToPlay))
        self.label.setText(self.SongQueued)
        self.SongPlaying = self.SongQueued

    def nextSong(self):
        next = self.listWidget.currentRow() + 1
        nextSong = self.listWidget.item(next).text()
        self.SongQueued = nextSong
        SongToPlay = self.songs[nextSong]
        self.label.setText(self.SongQueued)
        self.SongPlaying = self.SongQueued
        self.player.setCurrentSource(Phonon.MediaSource(SongToPlay))
        self.player.play()
        self.listWidget.setCurrentRow(next)

    def previousSong(self):
        next = self.listWidget.currentRow() - 1
        nextSong = self.listWidget.item(next).text()
        self.SongQueued = nextSong
        SongToPlay = self.songs[nextSong]
        self.label.setText(self.SongQueued)
        self.SongPlaying = self.SongQueued
        self.player.setCurrentSource(Phonon.MediaSource(SongToPlay))
        self.player.play()
        self.listWidget.setCurrentRow(next)

app = QtGui.QApplication(sys.argv)
w = Frederic()
w.show()
sys.exit(app.exec_())

Answer 1

It can be a pleasure to whip something up in an hour on the plane. This code could be improved by using pylint.

The most obvious improvements appear to be

• minimize and structure the instance variables
• refactor code to remove repetition
• corner case testing
• make styling standard and consistent

There are so many unstructured instant variables that it obscures the structure, some errors and the fact that many are not needed.

As a small example, of the following variables:

self.file_name = None
self.data = None
self.songs = {}
self.SongPlaying = None
self.SongQueued = None

only self.songs is actually used as anything but a local variable. Similarly, none of the buttons need to be instance variables. In the UI only three widgets are actually referenced outside of setup.

Where possible prefer self-documenting code. The songs variable might better be called filename_by_track_label.

There is a good amount of code repetition. For example, funcPlay, nextSong and previousSong all contain duplicated code. It could be factored out into something such as

def play_song(self, song_index):
    """Play song at :song_index: in UI list"""
    next_track_label = self.ui.update_label_by_index(song_index)
    next_filename = self.filenames_by_track_label[next_track_label]
    self.player.setCurrentSource(Phonon.MediaSource(next_filename))
    self.player.play()
    self.ui.songs_list_widget.setCurrentRow(song_index)

You could further remove code duplication in the UI construction by use of a helper, for example

def help_add(container, widget_list):
    for widget in widget_list:
        container.addWidget(widget)

hbox0 = QtGui.QHBoxLayout(self)
help_add(hbox0, (play_btn, pause_btn, stop_btn, anterior_btn, siguiente_btn))

hbox = QtGui.QHBoxLayout(self)
help_add(hbox, (agregar_btn, eliminar_btn))

vbox = QtGui.QVBoxLayout(self)
help_add(vbox, (self.label, self.status, hbox0, self.songs_list_widget, hbox, spin))

There are a number of bugs lingering in the code

• all next and previous track computations will overflow. Computation should be modulus number of tracks

  song_index = (self.ui.current_song_number() + 1) % self.ui.song_count()
  

• track label formation catches exceptions but (a) uses a bare except, (b) largely repeats the try block in the exception handler, (c) inserts the malformed label and (d) includes a dangling variable name. Perhaps something like this:

  try:
      id3 = ID3(file_name)
      data = id3['TPE1'].text[0] + " - " + id3["TIT2"].text[0]
      self.filenames_by_track_label[data] = file_name
      self.ui.listWidget.addItem(data)
  except (IOError, LookupError):
      self.ui.show_error("El archivo que ha elegido no funciona. Seleccione otro archivo por favor.")
  

You might consider separating the UI from the controller to provide some structure. I have shown some elements of that in my examples with self.ui

Of course in an hour you might not apply completely consistent styling but pylint can help you be disciplined. Applying its recommendations would further improve the code.