Qt und Phonon wäre naheliegende Alternativen zu wxWidgets und gstreamer. Den Quelltext selbst habe ich nicht näher angesehen, da ich weder wx noch gstreamer sonderlich gut kenne, und zumindest im Falle von wx auch nicht kennen will. Daher nur ein paar allgemeine Anmerkungen.
Die Abhängigkeiten mitzuliefern, ist zwar bequem, aber unschön. Besser ist es, mit distutils/setuptools ein vernünftiges Installationsskript zu schreiben, und die Abhängigkeiten dort aufzuführen. Dann kann man das Programm einfach mit "python setup.py install" bzw. besser mit "pip install ." installieren, wobei die Abhängigkeiten automatisch mitinstalliert werden.
In allen Dateien ist die Kodierungsangabe falsch, statt "utf8" sollte es "utf-8" heißen.
"pympapp.py" habe ich mir aus genannten Gründen nur oberflächlich angesehen, aber trotzdem ist mir einiges aufgefallen. Die Einrückung ist schlicht kaputt, sie mischt Tabulatoren und Leerzeichen, es ist aller Wahrscheinlichkeit nur Zufall, dass das Programm tatsächlich wie gewünscht funktioniert. Verwende außerdem keine keine "*"-Importe, sie müllen den Namensraum zu und führen folglich früher oder später zu Konflikte und in der Folge zu kryptischen und schwer zu findenden Fehlern, und keine magischen Zahlen wie in "PympApp._onKeyUp()":
Code: Alles auswählen
# Pfeil oben
if keycode == 315:
self._ctrls['lctPlaylist'].item_up()
return
Zum einen ist das schwer lesbar, zum anderen umständlich, und wahrscheinlich nicht einmal portabel. So wäre es besser, der Kommentar ist dann auch überflüssig:
"KEY_UP" sollte idealerweise irgendwo aus wx kommen, da es eigentlich Aufgabe der Bibliothek ist, Konstante für solche "magischen" Werte zur Verfügung zu stellen. Ansonsten musst Du sie eben selbst definieren.
Auch würde dieser Datei dringend mehr Struktur und Ordnung gut tun. Viele Stellen sind komplex, dabei aber vollkommen undokumentiert, unkommentiert und voller kryptischer Namen, e.g. "PympApp._build_tree()" oder "PympApp.__valid_entry()". "__valid_entry()" ist ein gutes Beispiel dafür, wie man nicht programmieren sollte:
Code: Alles auswählen
mand = True if 'and' in pattern else False
# […]
br = []
for p in pattern:
p = p.strip()
b0 = b1 = b2 = b3 = False
if item[-1]: # path
if p in item[-1].lower():
b0 = True
if item[1]: # title
if p in item[1].lower():
b1 = True
if item[2]: # artist
if p in item[2].lower():
b2 = True
if item[3]: # album
if p in item[3].lower():
b3 = True
br.append(any([b0, b1, b2, b3]))
Jede Wette, dass Du in zwei Wochen selber nicht mehr weißt, was "b1" bis "b3" bedeuten, was in "item[3]" steht, und was die gesamte Methode überhaupt tut. Die erste Zeile ist übrigens einfach "mand = 'and' in pattern", der bedingte Ausdruck ist mithin überflüssig. Du wirst verstehen, dass ich besseres zu tun hatte, als versuchen, diesen Quelltext zu nachzuvollziehen. Ähnliche strukturelle Probleme hat der Großteil des Quelltext, auch in den anderen Modulen, ich spare mir jetzt die einzelne Auflistung aller Punkte, die mir aufgefallen sind.
Es ist reichlich überflüssig, "__version__", "__author__" und andere Metadaten in
jeder Datei zu definieren, üblicherweise schreibt man das nur ein einziges Mal in die "__init__.py". Hast Du Dir dabei irgendetwas besonderes gedacht?!
In "collection.py" finden sich oft Konstrukte wie "except TypeError: pass". Was soll den das?! Typfehler sind normalerweise ein Zeichen für gravierende Fehler in einem Programm, die etwas mehr Aufmerksamkeit verdienen als sie einfach zu ignorieren! Die Ausnahmebehandlung ist hier insgesamt verbesserungsbedürftig, viel zu oft werden
alle Ausnahmen mit "except Exception:" abgefangen und dann einfach mehr oder weniger ignoriert. Wenn schon, dann gibt in einem solchen Fall doch wenigstens den Traceback aus, sonst wird die Fehlersuche zur Lotterie.
Was Du Dir bei all den Konstanten in "config.py" gedacht hast, kann ich ebenfalls nicht nachvollziehen. Außerdem sollte der bloße Import eines Moduls keine Seiteneffekte wie beispielsweise das Erzeugen von Verzeichnissen haben, denn so kann man config.py weder testen, noch vernünftig im Interpreter ausprobieren.
In "lyric.py" solltest Du die Behandlung von Dateien dringend verbessern, denn auch diese Datei gilt als Beispiel, wie man es nicht machen sollte:
Code: Alles auswählen
fp = os.path.join(cfg[LYRICS_DIR], "%s-%s.txt" % (self.artist, self.title))
if os.path.isfile(fp):
try:
f = open(fp, 'r')
lyric = f.read()
f.close()
return lyric
except Exception, e:
print("Lyrics Datei nicht gefunden: %s" % (fp))
return ''
Die Prüfung in der zweiten Zeile ist vollkommen überflüssig, da "open" eine Ausnahme auslöst, wenn die Datei nicht existiert. Dateien öffnet man mithilfe der "with"-Anweisung, um sicherzustellen, dass sie auch im Falle einer Ausnahme korrekt geschlossen werden. Die Ausnahmebehandlung ist an dieser Stelle mit Verlaub mies. Die Fehlermeldung ist falsch, denn eine Ausnahme kann auch aus anderen Gründen auftreten. Außerdem ist sie deplaziert, weil die Ausnahme hier noch gar nicht sinnvoll behandelt werden kann. Die Ausnahme muss nach oben durchgereicht werden, bis zu einer Stelle, wo man sie sinnvoll behandeln kann.
"mp3.py" halte ich für vollkommen überflüssig. Der einzige Zweck dieser Datei scheint mir die Abbildung der ID3-Frames auf generische Tag-Namen, was "mutagen" auch selbst kann:
Code: Alles auswählen
>>> import mutagen
>>> mutagen.version
(1, 20)
>>> metadata = mutagen.File('01 - Colossal.mp3', easy=True)
>>> metadata['artist']
[u'Wolfmother']
>>> metadata['album']
[u'Wolfmother']
>>> metadata['title']
[u'Colossal']
Insofern scheint mir diese Datei überflüssig. Wie dem auch sei, ändern sollte man die Fehlerbehandlung in dieser Datei. Statt im Falle fehlender Tags Fehlerwerte wie -1 zurückzugeben, löst man besser eine Ausnahme aus, die man dann aber bitte auch richtig behandelt.
Den Rest des Quelltexts habe ich mir nach allem, nicht mehr angetan, denn ehrlich gesagt war das, was ich gesehen habe, schlecht genug, um mich Abstand halten zu lassen. Verzeihe mir bitte die offenen Worte, es tut mir auch leid, dass dies sicherlich keine Kritik ist, die man gerne hört, aber Du hattest darum gebeten, und das ist alles, was ich dazu sagen kann.
