Einfacher MP3 Tagger

[nooby]

Da ich häufig Hörbücher höre, hab ich dieses Programm geschrieben. Wenn ich nämlich die CDs auf meinen Computer kopiere um sie dann auf meine MP3 Player zu hören, möchte ich nicht immer die MP3 Tags von Hand setzen müssen.
Mein MP3 Player sortiert aber nach dem MP3 Tag Titel...
Mein Programm übernimmt nun den Dateinamen, setzt auf Wunsch eine Nummer davor und setzt ihn als MP3 Tag.
Das ganze hab ich mit PyQt, eyed3 und Python 2.7 geschrieben.

Hier das Hauptprogramm ohne PyQt Gui. Wenn jemand das Programm testen möchte: http://www.mediafire.com/download/ixaqc ... Tagger.zip

Code: Alles auswählen

import sys
import os
import eyed3
from PyQt4 import QtGui, QtCore
from MP3_Tag_GUI import  Ui_MainWindow as Dlg#Hauptwindow
from about_dialog import Ui_Dialog as AboutDlg#Aboutwindow
from subfolder_dialog import Ui_Form as SubfolderDlg

class MainWindow(QtGui.QMainWindow, Dlg):
    def __init__(self):
        QtGui.QMainWindow.__init__(self)
        self.setupUi(self)
        #Setzt alle Textfelder auf read-only
        self.title_text.setReadOnly(True)
        self.interpreten_text.setReadOnly(True)
        self.albuminterpreten_text.setReadOnly(True)
        #Verbindet alle Checkboxen, Buttons, Menubar-Actions mit Funktionen
        self.connect(self.title_checkBox,
                     QtCore.SIGNAL("clicked()"), self.onTitle_checkBox)
        self.connect(self.interpreten_checkBox,
                     QtCore.SIGNAL("clicked()"), self.onInterpreten_checkBox)
        self.connect(self.albuminterpreten_checkBox,
                     QtCore.SIGNAL("clicked()"), self.onAlbuminterpreten_checkBox)
        self.connect(self.start_button,
                     QtCore.SIGNAL("clicked()"), self.onStart_button)
        self.connect(self.reset_button,
                     QtCore.SIGNAL("clicked()"), self.onReset_button)
        self.connect(self.actionBeenden,
                     QtCore.SIGNAL("triggered()"), self.onActionBeenden)
        self.connect(self.actionAbout,
                     QtCore.SIGNAL("triggered()"), self.onActionAbout)
        self.connect(self.actionSubfolder,
                     QtCore.SIGNAL("triggered()"), self.onActionSubfolder)

    def onTitle_checkBox(self):
        if self.title_checkBox.checkState():
            self.title_text.setReadOnly(False)
        else:
            self.title_text.setReadOnly(True)
            
    def onInterpreten_checkBox(self):
        if self.interpreten_checkBox.checkState():
            self.interpreten_text.setReadOnly(False)
        else:
            self.interpreten_text.setReadOnly(True)

    def onAlbuminterpreten_checkBox(self):
        if self.albuminterpreten_checkBox.checkState():
            self.albuminterpreten_text.setReadOnly(False)
        else:
            self.albuminterpreten_text.setReadOnly(True)

    def onStart_button(self):
        folder_path = self.path_text.toPlainText()
        folder_path = unicode(folder_path)
        folder_path = folder_path.split("\n")
        title = self.title_text.toPlainText()
        interpreten = self.interpreten_text.toPlainText()
        albuminterpreten = self.albuminterpreten_text.toPlainText()
        progressBar_value = 0
        album_count = 1
        failure = []
        for path in folder_path:
            percent_per_path = round(100 / len(folder_path))
            try:
                file_list = os.listdir(path)
            except EnvironmentError:
                messagebox = MessageBox("Fehler", "Es wurde ein ungueltiger Pfad angegeben" + "\n" + path, 2)
                messagebox.show()
            count = 1
            for file in file_list:
                percent_per_file = round(percent_per_path/len(file_list))
                progressBar_value += percent_per_file
                self.progressBar.setValue(progressBar_value)
                path_neu = os.path.join(path, file)
                try:
                    audiofile = eyed3.load(path_neu)
                    audiofile.tag#Testet, ob es eine MP3 ist, da Dateien ohne Dateiendung einfach so geladen werden.
                except (IOError, AttributeError):
                    failure.append(file)
                    continue
                if title == "filename":
                    audiofile.tag.title = u"" + str(count)+ " " + file
                elif title == "filename /e":
                        audiofile.tag.title = u"" + file
                else:
                        audiofile.tag.title = u"" + str(count) + " " + unicode(title)
                audiofile.tag.artist = u"" + unicode(interpreten)
                try:
                    if albuminterpreten[-2] +  albuminterpreten[-1] == "/e":
                        albuminterpreten2 = albuminterpreten[:-3]
                        audiofile.tag.album = u"" + unicode(albuminterpreten2)
                    else:
                        audiofile.tag.album = u"" + unicode(albuminterpreten) + " " + str(album_count)
                except IndexError:
                    continue
                audiofile.tag.save()
            album_count += 1
        self.progressBar.setValue(100)
        if len(failure) != 0:
            self.messageBox = MessageBox("Fehler", "Es konnten nicht alle Dateien umgetaggt werden. Vielleicht befinden sich auch nicht MP3s im Orndner. Diese wurden uebersprungen.", 2)
            self.messageBox.show()
            self.path_text.clear()
            for fail in failure:
                self.path_text.insertPlainText(fail+"\n")
        else:
            self.messageBox = MessageBox("Erflogreich", "Es wurden alle angegeben MP3s umgetagt.", 1)
            self.messageBox.show()

    def onReset_button(self):
        self.path_text.clear()
        self.title_text.clear()
        self.interpreten_text.clear()
        self.albuminterpreten_text.clear()
        self.title_checkBox.setCheckState(QtCore.Qt.Unchecked)
        self.interpreten_checkBox.setCheckState(QtCore.Qt.Unchecked)
        self.albuminterpreten_checkBox.setCheckState(QtCore.Qt.Unchecked)
        self.progressBar.setValue(0)

    def onActionBeenden(self):
        sys.exit(app.exec_())

    def onActionAbout(self):
        self.dialog = AboutDialog()
        self.dialog.exec_()

    def onActionSubfolder(self):
        self.dialog = SubfolderDialog()
        self.dialog.exec_()
    
class AboutDialog(QtGui.QDialog, AboutDlg):
    def __init__(self):
        QtGui.QDialog.__init__(self)
        self.setupUi(self)
        self.connect(self.ok_button,
                     QtCore.SIGNAL("clicked()"), self.onOk_button)
    def onOk_button(self):
        self.close()

class MessageBox(QtGui.QMessageBox):
    def __init__(self, title, text, flag, parent=None):
        QtGui.QMessageBox.__init__(self, parent)
        self.setGeometry(300, 300, 250, 150)
        self.setWindowTitle(title)
        self.setText(text)
        self.setIcon(flag)

class SubfolderDialog(QtGui.QDialog, SubfolderDlg):
    def __init__(self):
        QtGui.QDialog.__init__(self)
        self.setupUi(self)
        self.connect(self.start_button,
                     QtCore.SIGNAL("clicked()"), self.onStart_button)
        self.connect(self.reset_button,
                     QtCore.SIGNAL("clicked()"), self.onReset_button)
        self.subpath_text.setReadOnly(True)        
    def onStart_button(self):
        path = unicode(self.path_text.toPlainText())
        subpath = os.listdir(path)
        for folder in subpath:
            new_path = os.path.join(path, folder)
            if os.path.isfile(new_path):
                pass
            else:
                self.subpath_text.insertPlainText(new_path +  "\n")

    def onReset_button(self):
        self.path_text.clear()
        self.subpath_text.clear()
        
app = QtGui.QApplication(sys.argv)
app.setStyle(QtGui.QStyleFactory.create("plastique"))
window = MainWindow()
window.show()
sys.exit(app.exec_())

Kritik ist erwünscht!

Nooby

[nooby]

Habe grad selber noch etwas bemerkt.
Ich erstelle eine Liste failure, da ich ursprünglich die Fehler ausgeben wollte...
Doch so wie es jetzt ist, würde ein Bool reichen und erst noch Speicher sparen.

[BlackJack]

@nooby: Die Umbenennungen beim Importieren der generierten GUI-Module sind unschön. Zum einen könnte man die Klassen in diesen Modulen von vorn herein richtig benennen. Und dann sollte man nicht unnötig abkürzen. `Dialog` ist nun wirklich nicht so viel länger als `Dlg`.

Wenn man PyQt4 verwendet, könnte man sich den unnötigen Zwischenschritt Quelltext für Module zu generieren gänzlich sparen und die ``*.ui``-Dateien zur Laufzeit mit dem `PyQt4.uic`-Modul laden. Damit spart man sich die Importe und die Mehrfachvererbung.

Kommentare sollten einen Mehrwert zum Code bieten und nicht das offensichtliche nochmal sagen was im Code schon da steht.

Statt der `connect()`-Methode auf den Widgets sollte man die „neue” Art Signale mit Slots zu verbinden nutzen, also die `connect`-Methode auf den Signalobjekten. Das ist kürzer, weniger fehleranfällig, und IMHO auch einfacher zu lesen. Beispiel:

Code: Alles auswählen

        # statt:
        self.connect(self.title_checkBox,
                     QtCore.SIGNAL("clicked()"), self.onTitle_checkBox)
        # besser:
        self.title_checkBox.clicked().connect(self.onTitle_checkBox)

Die Methoden für die Checkboxen sind eigentlich Einzeiler:

Code: Alles auswählen

    def onTitleCheckBox(self):
        self.title_text.setReadOnly(not self.title_checkBox.checkState())

Da alle drei Methoden gleich aufgebaut sind, juckt es mich natürlich irgendwie da eine verallgemeinerte Funktion draus zu machen und die mit `functools.partial()` zu individualisieren.

Die `MainWindow.onStart_button()`-Methode ist für meinen Geschmack zu lang. An der Stelle könnte man auch die GUI von der Programmlogik trennen und eine entsprechende Funktion schreiben, die das Taggen übernimmt und unabhängig von der GUI verwendet und getestet werden kann.

Die drei Zeilen am Anfang hätte man zusammenfassen können, ausserdem müsste der Name in der Mehrzahl stehen, denn es wird ja nicht nur *ein* Pfad sondern eine Liste mit Pfaden an den Namen gebunden.

Die Zähler `album_count` und `count` kann man mit der `enumerate()`-Funktion erzeugen statt sie manuell zu initialisieren und hoch zu zählen. Wobei mir gerade auffällt, dass Du `count` überhaupt gar nirgends hoch zählst. Der Wert ist immer 1.

`percent_per_path` und `percent_per_file` werden jeweils in Schleifen immer und immer wieder berechnet, wobei da aber jedes mal das selbe Ergebnis heraus kommt. Man könnte die Berechnung also vor die jeweilige Schleife ziehen. Den Fortschrittsbalken so zu berechnen ist übrigens sehr ungenau.

Konkrete Datentype in Namen sollte man vermeiden. Wenn man den Typ später mal ändern will, macht das nur unnötig Arbeit oder man hat irreführende Namen im Quelltext.

Die Ausnahmebehandlung beim `listdir()` ist fehlerhaft. Danach wird einfach weitergemacht als wäre nichts gewesen, also entweder rennt man beim ersten Schleifendurchlauf in einen `NameError` oder es wird mit den Dateinamen vom letzten Schleifendurchlauf bei dem das `listdir()` geklappt hat, noch einmal gearbeitet.

Eingebaute Namen wie `file` sollte man nicht an andere Werte binden. Das ist verwirrend.

Von ``continue`` halte ich persönlich nicht besonders viel, weil es ein unbedingter Sprung aus einer verschachtelten Struktur ist, den man der Formatierung des Quelltextes nicht ansieht. Es macht es auch unmöglich am Ende der betreffenden Schleife etwas zu schreiben was in jedem Fall vor dem nächsten Schleifendurchlauf ausgeführt wird, weil das ``continue`` sozusagen eine Abkürzung an den Schleifenanfang nimmt.

Was denkst Du was ``u'' + text`` bewirkt wenn `text` an einen Unicodewert gebunden ist?

Die Verarbeitung von `albuminterpreten` ist umständlich und ich denke das ``continue`` beziehungsweise das der Code überhaupt so geschrieben ist, dass ein `IndexError` auftreten kann, ist ein Programmfehler.

Die Anweisung ``if len(sequence) != 0:`` lässt sich kürzer als ``if sequence:`` schreiben.

Warum am Ende der Methode die `MessageBox`-Exemplare an das `MainWindow`-Exemplar gebunden werden, ist mir nicht verständlich. Genauso warum die Dialoge in `onActionAbout()` und `onActionSubfolder()` an das Hauptfenster gebunden werden.

`MainWindow.onActionBeenden()` greift auf das „globale” `app()` zu, was nicht sein sollte, ausserdem wird dort die Hauptschleife ein zweites mal aufgerufen, was sicher nicht im Sinne des Erfinders ist.

Die „magischen” Zahlen für das Icon der `MessageBox` sind unschön weil der Leser da überhaupt nicht weiss wofür die stehen. Dafür gibt es doch Konstanten mit aussagekräftigen Namen.

`SubfolderDialog.onStartButton()` ist umständlich ausgedrückt. Wenn man in einem ``if``- oder einem ``else``-Zweig nur ein ``pass`` stehen hat, dann hat man was falsch gemacht. Das lässt sich immer ohne einen solchen Zweig ausdrücken.

Der `SubfolderDialog` interagiert mit dem Rest des Programms nicht wirklich, oder? Das ist irgendwie komisch.

Ich lande am Ende bei etwas das ungefähr so aussieht (ungetestet):

Code: Alles auswählen

#!/usr/bin/env python
# coding: utf-8
import os
import sys

import eyed3
from PyQt4 import QtGui, QtCore

from MP3_Tag_GUI import Ui_MainWindow
# 
# TODO: Rename the classes in the first place. Or even better
#   use `PyQt4.uic` to load the UI data files instead.
#       
from about_dialog import Ui_Dialog as Ui_AboutDialog
from subfolder_dialog import Ui_Form as Ui_SubfolderDialog



class MainWindow(QtGui.QMainWindow, Ui_MainWindow):
    def __init__(self):
        QtGui.QMainWindow.__init__(self)
        self.setupUi(self)
        self.title_text.setReadOnly(True)
        self.interpreten_text.setReadOnly(True)
        self.albuminterpreten_text.setReadOnly(True)

        self.title_checkBox.clicked().connect(self.onTitleCheckBox)
        self.interpreten_checkBox.clicked().connect(self.onInterpretenCheckBox)
        self.albuminterpreten_checkBox.clicked().connect(
            self.onAlbuminterpretenCheckBox
        )
        self.start_button.clicked().connect(self.onStartButton)
        self.reset_button.clicked().connect(self.onResetButton)
        self.actionBeenden.triggered().connect(QtGui.qApp.quit)
        self.actionAbout.triggered().connect(self.onActionAbout)
        self.actionSubfolder.triggered().connect(self.onActionSubfolder)

    @staticmethod
    def onActionAbout():
        AboutDialog().exec_()

    @staticmethod
    def onActionSubfolder():
        SubfolderDialog().exec_()

    def onTitleCheckBox(self):
        self.title_text.setReadOnly(not self.title_checkBox.checkState())
            
    def onInterpretenCheckBox(self):
        self.interpreten_text.setReadOnly(
            not self.interpreten_checkBox.checkState()
        )

    def onAlbuminterpretenCheckBox(self):
        self.albuminterpreten_text.setReadOnly(
            not self.albuminterpreten_checkBox.checkState()
        )

    def onStartButton(self):
        # 
        # TODO: Split this method or even better separate the program logic
        #       from the UI code.
        # 
        folder_paths = unicode(self.path_text.toPlainText()).split('\n')
        title = unicode(self.title_text.toPlainText())
        interpreten = unicode(self.interpreten_text.toPlainText())
        albuminterpreten = unicode(self.albuminterpreten_text.toPlainText())
        progress_bar_value = 0
        failures = []
        percent_per_path = round(100.0 / len(folder_paths))
        for album_count, path in enumerate(folder_paths, 1):
            try:
                filenames = os.listdir(path)
            except EnvironmentError:
                MessageBox(
                    'Fehler',
                    u'Es wurde ein ungültiger Pfad angegeben\n' + path,
                    MessageBox.Warning
                ).show()
            else:
                percent_per_file = round(percent_per_path / len(filenames))
                for count, filename in enumerate(filenames, 1):
                    progress_bar_value += percent_per_file
                    self.progressBar.setValue(progress_bar_value)
                    try:
                        audio_file = eyed3.load(os.path.join(path, filename))
                        # 
                        # Testen ob es eine MP3 ist, da Dateien ohne
                        # Dateiendung einfach so geladen werden.
                        # 
                        tag = audio_file.tag
                    except (IOError, AttributeError):
                        failures.append(filename)
                    else:
                        tag.title = {
                            'filename': u'{count} {filename}',
                            'filename /e': u'{filename}',
                        }.get(title, u'{title}').format(
                            count=count, filename=filename, title=title
                        )
                        tag.artist = interpreten
                        tag.album = (
                            albuminterpreten[:-3]
                            if albuminterpreten.endswith(' /e')
                            else u'{0} {1}'.format(
                                albuminterpreten, album_count
                            )
                        )
                        tag.save()
        self.progressBar.setValue(100)
        if failures:
            MessageBox(
                'Fehler',
                u'Es konnten nicht alle Dateien umgetaggt werden.'
                u' Vielleicht befinden sich auch nicht MP3s im Ordner.'
                u' Diese wurden übersprungen.',
                MessageBox.Warning
            ).show()
            self.path_text.clear()
            self.path_text.insertPlainText('\n'.join(failures))
        else:
            MessageBox(
                'Erfolgreich',
                'Es wurden alle angegeben MP3s umgetaggt.',
                MessageBox.Information
            ).show()

    def onResetButton(self):
        self.path_text.clear()
        self.title_text.clear()
        self.interpreten_text.clear()
        self.albuminterpreten_text.clear()
        self.title_checkBox.setCheckState(QtCore.Qt.Unchecked)
        self.interpreten_checkBox.setCheckState(QtCore.Qt.Unchecked)
        self.albuminterpreten_checkBox.setCheckState(QtCore.Qt.Unchecked)
        self.progressBar.setValue(0)

    

class AboutDialog(QtGui.QDialog, Ui_AboutDialog):
    def __init__(self):
        QtGui.QDialog.__init__(self)
        self.setupUi(self)
        self.ok_button.clicked().connect(self.close)


class MessageBox(QtGui.QMessageBox):
    def __init__(self, title, text, icon, parent=None):
        QtGui.QMessageBox.__init__(self, parent)
        self.setGeometry(300, 300, 250, 150)
        self.setWindowTitle(title)
        self.setText(text)
        self.setIcon(icon)


class SubfolderDialog(QtGui.QDialog, Ui_SubfolderDialog):
    def __init__(self):
        QtGui.QDialog.__init__(self)
        self.setupUi(self)
        self.start_button.clicked().connect(self.onStartButton)
        self.reset_button.clicked().connect(self.onResetButton)
        self.subpath_text.setReadOnly(True)        

    def onStartButton(self):
        root = unicode(self.path_text.toPlainText())
        for folder in os.listdir(root):
            full_path = os.path.join(root, folder)
            if not os.path.isfile(full_path):
                self.subpath_text.insertPlainText(full_path + '\n')

    def onResetButton(self):
        self.path_text.clear()
        self.subpath_text.clear()


def main():        
    application = QtGui.QApplication(sys.argv)
    application.setStyle(QtGui.QStyleFactory.create('plastique'))
    window = MainWindow()
    window.show()
    sys.exit(application.exec_())


if __name__ == '__main__':
    main()

[Sirius3]

Da alle drei Methoden gleich aufgebaut sind, juckt es mich natürlich irgendwie da eine verallgemeinerte Funktion draus zu machen und die mit `functools.partial()` zu individualisieren.

sowas wie:

Code: Alles auswählen

…
    for field in ('title', 'interpreten', 'albuminterpreten'):
        text_field = getattr(self, '%s_text'%field)
        check_box = getattr(self, '%s_checkBox'%field)
        text_field.setReadOnly(True)
        check_box.clicked().connect(lambda t=text_field, c=check_box:
            t.setReadOnly(not c.checkState()))