So, ich habe mal meinen Code ein wenig anonymisiert (weil gewisse Daten nicht frei im Netz verfügbar sein sollen).
Es ist ein Player, bzw. es wird damit der mplayer benutzt, um einen Stream zu hören Zugangsdaten werden eingebunden (ist ein geschützter Stream), Lied-Tags ausgelesen und angezeigt, und aus einer SLT-Datei im Netz die Infos über den Stream selbst ausgelesen.
Wäre nett, wenn mal der ein oder andere drüber sehen könnte, und mir sagen könnte, was daran absoluter Müll ist, oder was man vielleicht wesentlich einfacher und effizienter gestalten könnte.
Ich bin mir nahezu sicher, dass das einiges sein wird, aber ich möchte mich gerne bessern. ) Und ich bin mir sicher, dass das an einigen Stellen möglich ist.
http://paste.pocoo.org/show/129309/
Das dazugehörige XRC:
http://paste.pocoo.org/show/129311/
Umständlich? Völlig falsch? Code zum zerreissen ;o)
-
- User
- Beiträge: 996
- Registriert: Mittwoch 9. Januar 2008, 13:48
- PEP8 folgen
- global rausnehmen
- Niemals *alle* Fehler mit `except`abfangen
- `if mountpoint == 1` geht auch als `if mountpoint` (ebenso `== True`)
- entweder alles Deutsch oder alles Englisch, Mix draus ist doof
- global rausnehmen
- Niemals *alle* Fehler mit `except`abfangen
- `if mountpoint == 1` geht auch als `if mountpoint` (ebenso `== True`)
- entweder alles Deutsch oder alles Englisch, Mix draus ist doof
Werd's dann nochmal in der Richtung überarbeiten
Wegen den globals allerdings... ich weiß nicht, wie ich die umgehen soll. Hab ich schon hin und her überlegt, aber beispielsweise "mplayer_beendet" kann aus unterschiedlichen Gründen geändert werden, ebenso "proc"... müsste ich dann schon im Bindung für "stop" definieiert haben, aber ich kann es ja erst an einer anderen stelle zusammen setzen.
Vielleicht denke ich in der Richtung auch falsch, aber da wäre ich für Hilfe dankbar.
Wegen den globals allerdings... ich weiß nicht, wie ich die umgehen soll. Hab ich schon hin und her überlegt, aber beispielsweise "mplayer_beendet" kann aus unterschiedlichen Gründen geändert werden, ebenso "proc"... müsste ich dann schon im Bindung für "stop" definieiert haben, aber ich kann es ja erst an einer anderen stelle zusammen setzen.
Vielleicht denke ich in der Richtung auch falsch, aber da wäre ich für Hilfe dankbar.
-
- User
- Beiträge: 996
- Registriert: Mittwoch 9. Januar 2008, 13:48
Du hast doch die Anwendung eh schon in einer Klasse, warum machst du `mplayer_beendet` dann nicht einfach als Klassen- oder Instanzvariable?
Die XRC ist in Ordnung, XRCEd stört sich nur an den Umlauten (ist wohl ein Bug in XRCed).
Aber: Ich würde Dir auf jeden Fall raten, die Elemente sauber mit Sizern und nicht absolut zu positionieren.
Einen Font würde ich auch nicht unbedingt vorgeben, damit die jeweiligen OS- bzw. Theme-Defaults benutzt werden.
Aber: Ich würde Dir auf jeden Fall raten, die Elemente sauber mit Sizern und nicht absolut zu positionieren.
Einen Font würde ich auch nicht unbedingt vorgeben, damit die jeweiligen OS- bzw. Theme-Defaults benutzt werden.
Danke.
Ich gebe zu, mit den Sizern komme ich absolut nicht klar. Bei mir gehen die immer eigene Wege, und selbst als ich es das erste Mal mit gemacht habe (mit wxGlade) habe ich keinen Vorteil darin gesehen, da es bei Veränderung der Fenstergröße irgendwie dermaßen doof aussah...
Ich gebe zu, mit den Sizern komme ich absolut nicht klar. Bei mir gehen die immer eigene Wege, und selbst als ich es das erste Mal mit gemacht habe (mit wxGlade) habe ich keinen Vorteil darin gesehen, da es bei Veränderung der Fenstergröße irgendwie dermaßen doof aussah...
Mir ist noch aufgefallen, dass Du in Deinem Code keine Unicode-Strings verwendest (wobei, das stimmt nur für Python 2.x, nicht für 3.x - ich nehme jetzt aber mal an, dass Du mit ersterem unterwegs bist). Zumindest bei den Strings, die Umlaute o.ä. enthalten könn(t)en, solltest Du Unicode nehmen, sonst gibts Darstellungsprobleme, wenn ein Nutzer nicht UTF-8 als System-Encoding eingestellt hat.
Absolut positioniertes Layout bei 144dpi
Mit Sizern positioniertes Layout bei 144dpi
Hier noch das von mir mit Sizern umgebaute XRC, bei denen für kein einziges Element irgendeine Größe vorgegeben wurde. Alles wird anhand des Platzes, den die Elemente beanspruchen, dynamisch angepasst. Außerdem gibt es keine Encoding-Vorgaben mehr für die Fonts, da das zu Warnmeldungen führen kann, wenn kein Font mit entsprechendem Encoding gefunden wird.
http://paste.pocoo.org/show/130138/
Das XRC enthält jetzt auch die Statusleiste, die Du vorher im Code hattest. Darum musst Du Deinen Code in Zeile 44 wie folgt ändern:
(das Fit() sorgt dafür, dass die Statusleiste nicht den Text überlappt)
(Edit: XRC überarbeitet, das Layout passt jetzt bei Änderung der Fenstergröße sinnvoll (imho) in der Breite an.)
Ja, das Layout mit Sizern ist erst mal komplexer als ohne. Und um die Vorteile zu sehen, muss man sich erst einmal die Nachteile eines Layouts ohne Sizer verdeutlichen, z.B. wenn ein Nutzer den dpi-Wert seines OS verändert. Bilder sprechen mehr als 1000 Worte, darum hier mal ein Beispiel, bei dem die dpi auf 144 gesetzt wurden (ja, ist bei normalen Monitoren ein übertriebener Wert, aber dadurch wirds deutlicher):Ich gebe zu, mit den Sizern komme ich absolut nicht klar. Bei mir gehen die immer eigene Wege, und selbst als ich es das erste Mal mit gemacht habe (mit wxGlade) habe ich keinen Vorteil darin gesehen, da es bei Veränderung der Fenstergröße irgendwie dermaßen doof aussah...
Absolut positioniertes Layout bei 144dpi
Mit Sizern positioniertes Layout bei 144dpi
Hier noch das von mir mit Sizern umgebaute XRC, bei denen für kein einziges Element irgendeine Größe vorgegeben wurde. Alles wird anhand des Platzes, den die Elemente beanspruchen, dynamisch angepasst. Außerdem gibt es keine Encoding-Vorgaben mehr für die Fonts, da das zu Warnmeldungen führen kann, wenn kein Font mit entsprechendem Encoding gefunden wird.
http://paste.pocoo.org/show/130138/
Das XRC enthält jetzt auch die Statusleiste, die Du vorher im Code hattest. Darum musst Du Deinen Code in Zeile 44 wie folgt ändern:
Code: Alles auswählen
self.frmStatusbar = xrc.XRCCTRL(self.frame, "frmStatusbar")
self.frame.Fit()
(Edit: XRC überarbeitet, das Layout passt jetzt bei Änderung der Fenstergröße sinnvoll (imho) in der Breite an.)
Ups, das mit den unicode-Strings hab ich echt vergessen. *grmpf*
Unter Linux fällt es nicht auf, daher vermutlich. Danke für den Hinweis.
Habe ich jetzt soweit eingebaut, allerdings ist dabei beim Test unter Win auch ein merkwürdiges Phänomen aufgetreten, welches ich nicht festmachen kann, wieso das ist.
Und zwar ist die Anzeige beim ersten Titel noch richtig, beim Wechsel auf den nächsten wird aber ein Teil der Anzeige nicht dargestellt, genauer im Titel.
Aber nicht dass Buchstaben fehlen, man kann noch Reste vom vorherigen sehen. Mehr als wenn da etwas drüber geschoben wird.
Ich habe es unter anderem so versucht:
und
Beides liefert das gleiche Ergebnis, bzw. die gleiche Fehleranzeige.
Zufällig eine Idee was das verursacht?
Sizer: Verstehen tu ich den Nutzen schon, auch wenn beim ir der Fall noch nicht aufgetreten ist. ;o) Aber danke erstmal für das geänderte XRC, ich werde mir das mal zu Gemüte führen, vielleicht macht es das so für mich verständlicher wegen dem wie und wo.
Das komische Aussehen war übrigens das, dass die Breite zwar angepasst wurde, aber die Höhe nicht. Daher sah es dann in meinen Augen merkwürdig aus. Also so ähnlich, wie es jetzt auch ist.
Das hatte ich nie hin bekommen, dass er auch die Höhe anpsst, dann hatte ich es gelassen.
Wegen der Statusbar... ist übrigens seltsam... genauso hatte ich das - könnte ich wetten - vorher auch drin, aber ich konnte es nicht ansprechen, egal was ich gemacht habe. Daher hatte ich es per Code eingebunden. Nun funktioniert es... Wer weiß, was ich da falsch hatte...
Unter Linux fällt es nicht auf, daher vermutlich. Danke für den Hinweis.
Habe ich jetzt soweit eingebaut, allerdings ist dabei beim Test unter Win auch ein merkwürdiges Phänomen aufgetreten, welches ich nicht festmachen kann, wieso das ist.
Und zwar ist die Anzeige beim ersten Titel noch richtig, beim Wechsel auf den nächsten wird aber ein Teil der Anzeige nicht dargestellt, genauer im Titel.
Aber nicht dass Buchstaben fehlen, man kann noch Reste vom vorherigen sehen. Mehr als wenn da etwas drüber geschoben wird.
Ich habe es unter anderem so versucht:
Code: Alles auswählen
def artistanzeige(self, artist):
# Anzeige des Interpreten
if artist != self.lblInterpret.Label:
self.lblInterpret.Label = unicode(artist, 'utf-8')
Code: Alles auswählen
def artistanzeige(self, artist):
# Anzeige des Interpreten
if artist != self.lblInterpret.Label:
self.lblInterpret.Label = artist.decode("utf-8")
Zufällig eine Idee was das verursacht?
Sizer: Verstehen tu ich den Nutzen schon, auch wenn beim ir der Fall noch nicht aufgetreten ist. ;o) Aber danke erstmal für das geänderte XRC, ich werde mir das mal zu Gemüte führen, vielleicht macht es das so für mich verständlicher wegen dem wie und wo.
Das komische Aussehen war übrigens das, dass die Breite zwar angepasst wurde, aber die Höhe nicht. Daher sah es dann in meinen Augen merkwürdig aus. Also so ähnlich, wie es jetzt auch ist.
Das hatte ich nie hin bekommen, dass er auch die Höhe anpsst, dann hatte ich es gelassen.
Wegen der Statusbar... ist übrigens seltsam... genauso hatte ich das - könnte ich wetten - vorher auch drin, aber ich konnte es nicht ansprechen, egal was ich gemacht habe. Daher hatte ich es per Code eingebunden. Nun funktioniert es... Wer weiß, was ich da falsch hatte...
Ich hab noch einen kurzen Blick drüber geworfen und ein paar weitere Anmerkungen:
- MyApp ist ein reichlich nichtssagender Name
- Zeilen 75-79:
- Such in der Doku mal nach "String Formatting", ist z.B. ist Zeile 92 sinnvoll
- ein "except" sollte niemals angewendet werden, ohne dass man explizit angibt, was abgefangen soll. Andernfalls gehen wirklich alle Meldungen, auch Fehlermeldungen, verloren.
- Zeile 130: such mal nach der "enumerate"-Funktion
- die sender-Methode würde ich anders schreiben (beachte auch die neue Bedingung des if):
Man könnte hier auch noch zusätzlich meinen zweiten Tipp anwenden.
- Methoden und Funktionen werden als Verben oder nach Tätigkeiten benannt, du verwendest aber öfters Nomen (sender, artistanzeige, ...).
- "read-Methode": hier solltest du wirklich mal über Konstanten nachdenken. Dann wird es viel flexibler:
- es bietet sich auch die "startswith"-Methode auf den Strings an, da du dies eigentlich ausdrücken willst. "in" matcht bei beliebigen Positionen.
- "elif" kann bei der Gelegenheit auch verwendet werden, da sich einige Fälle offensichtlich ausschließen.
- "updatestatus" macht den Eindruck, als wolle es noch zusammengefasst werden.
- Dateien sollten mit "open" geöffnet werden, nicht mit "file". Da fällt mir auf, dass auch der ein oder andere mögliche Fehler abgefangen werden sollte.
- die Schleife in "laden" kann noch vereinfacht werden:
- MyApp ist ein reichlich nichtssagender Name
- Zeilen 75-79:
Code: Alles auswählen
mountpoint = self.radMountpoint.GetSelection()
url = ("@xxx.xxx.xxx/xxx.ogg", "@sss.sss.sss/xxxXXX.ogg")[mountpoint]
- ein "except" sollte niemals angewendet werden, ohne dass man explizit angibt, was abgefangen soll. Andernfalls gehen wirklich alle Meldungen, auch Fehlermeldungen, verloren.
- Zeile 130: such mal nach der "enumerate"-Funktion
- die sender-Methode würde ich anders schreiben (beachte auch die neue Bedingung des if):
Code: Alles auswählen
def sender(self, info):
if info[1]:
streamtitel = info[1] + " mit " + '"' + info[5] + '"'
else:
streamtitel = "Keine Sendung oder Moderator nicht richtig eingerichtet"
self.lblStreamtitel.Label = streamtitel
- Methoden und Funktionen werden als Verben oder nach Tätigkeiten benannt, du verwendest aber öfters Nomen (sender, artistanzeige, ...).
- "read-Methode": hier solltest du wirklich mal über Konstanten nachdenken. Dann wird es viel flexibler:
Code: Alles auswählen
STATUS_ARTIST = "Artist:"
if STATUS_ARTIST in line:
artist = line[len(STATUS_ARTIST)+2:]
wx.CallAfter(self.artistanzeige, artist)
- "elif" kann bei der Gelegenheit auch verwendet werden, da sich einige Fälle offensichtlich ausschließen.
- "updatestatus" macht den Eindruck, als wolle es noch zusammengefasst werden.
- Dateien sollten mit "open" geöffnet werden, nicht mit "file". Da fällt mir auf, dass auch der ein oder andere mögliche Fehler abgefangen werden sollte.
- die Schleife in "laden" kann noch vereinfacht werden:
Code: Alles auswählen
while True:
line = f.readline().strip("\n")
if not line:
break
elif i == 1:
benutzername = line[::-1]
elif i == 2:
passwort = line[::-1]
elif i == 3:
mountpoint = int(line)
i += 1
Das Leben ist wie ein Tennisball.
Seltsam. Ich kanns leider nicht testen, zuerst dachte ich, da fehlt evtl. ein wx.CallAfter beim Setzen des Labels aus dem andren Thread heraus, aber das benutzt Du ja schon.Und zwar ist die Anzeige beim ersten Titel noch richtig, beim Wechsel auf den nächsten wird aber ein Teil der Anzeige nicht dargestellt, genauer im Titel.
Aber nicht dass Buchstaben fehlen, man kann noch Reste vom vorherigen sehen. Mehr als wenn da etwas drüber geschoben wird.
Versuch mal die 'Fit()'-, 'Refresh()'- oder 'Update()'-Methoden am Label (oder vielleicht dem Frame), nachdem er gesetzt wurde, oder mit 'Freeze()' einzufrieren, bevor Du den Label setzt, und dann 'Thaw()'. Sorry, ich rate hier gerade nur, keine Ahnung, obs was bringt.
Wenn Fragen sind, nur zu. Ich bin zwar kein wx-Crack, aber mit Sizern kenn ich mich zwischenzeitlich recht gut aus (solangs nicht um BagSizer geht)Aber danke erstmal für das geänderte XRC, ich werde mir das mal zu Gemüte führen, vielleicht macht es das so für mich verständlicher wegen dem wie und wo.
Ah so. Eine Höhenanpassung wäre ansich kein Problem, ich hab sie aber aus gutem (imho) Grund weggelassen: Sie bringt für den Nutzer keinen Gewinn (ausser mehr Freiraum um die Controls, oder grössere Controls - wobei für meinen Geschmack sieht beides auch merkwürdig aus, da die Schriftgröße ja gleich bleibt). Bei der Breitenanpassung kann man wenigstens dann auch mehr Text sehen (wenn er lang genug ist), darum machts da imho auch Sinn.Das komische Aussehen war übrigens das, dass die Breite zwar angepasst wurde, aber die Höhe nicht. Daher sah es dann in meinen Augen merkwürdig aus. Also so ähnlich, wie es jetzt auch ist.
Oh, 'ne Menge input. :o)
Dann habe ich ja die Tage was zu tun. :o)
Nochmal bzgl. des Anzeigefehlers,.... das seltsame ist halt, dass dieser Fehler nur auftritt, wenn ich versuche ihm unicode beizubringen. Ansonsten nicht. Das ist das was ich extrem seltsam finde, vor allem da ja nicht einfach Buchstaben fehlen, sondern wie mit Radiergummi behandelt aussehen.
Ansonsten hab ich jetzt ja erstmal was zu tun. Danke. :o)
Dann habe ich ja die Tage was zu tun. :o)
Nochmal bzgl. des Anzeigefehlers,.... das seltsame ist halt, dass dieser Fehler nur auftritt, wenn ich versuche ihm unicode beizubringen. Ansonsten nicht. Das ist das was ich extrem seltsam finde, vor allem da ja nicht einfach Buchstaben fehlen, sondern wie mit Radiergummi behandelt aussehen.
Ansonsten hab ich jetzt ja erstmal was zu tun. Danke. :o)