PYGTK Programm zum ändern von Config-Dateien [Lernübung]

[maces]

Hi

Ich hoffe, das ich das richtige Forum erwischt habe, ansonsten verschieben

Um meine Kenntnisse in PyGTK zu erweitern, habe ich ein Programm zum Bearbeiten des Autostarts der Linux Desktopumgebung LXDE geschrieben. Das Programm ließt die Konfigurationsdatei ein, dann kann man die Einträge editieren, löschen, oder neue hinzufügen. Am Ende wird gespeichert.

Das Programm sollte möglichst schnell, und ressourcensparend sein. Ich habe das Programm nur durch lesen von Howtos, Mailinglisten Beiträgen, der Dokumentation und sonstigen Codeschnipseln "zusammengeschustert". Das Ergebnis, es funktioniert, aber unter der Haube möchte ich gerne aufräumen. Daher würde ich mich sehr über Tipps zur Optimierung freuen.

Besonders zur Struktur des Programms. Zu Anfang habe ich versucht, mit 3 Klassen zu arbeiten, die erste Klasse für die Dateioperationen, die zweite für die gesamte Programmlogik und die dritte für die Grafische Oberfläche (Fenster und evtl. Dialoge sollten eine Extra Klasse bekommen). Allerdings ist die Programmlogik nun in der Klasse der Grafischen Oberfläche und die Dialoge haben auch keine eigene Klasse.

Hier habe ich durch entsprechende möglichst eindeutige Funktionsnamen versucht die Übersichtlichkeit zu erhalten. Wenn ich die Dialoge in eigene Klassen ausgelagert habe, dann bekam ich immer Fehlermeldungen, das Variablen Fehlen(z.B.:self.window) fehlen. Ist es sinnvoll diese Variablen als Parameter zu übergeben? Wenn ja, steigt dadurch der Speicherverbrauch(praktisch/Theoretisch)?

Zum testen vorweg, man kann den Pfad 'configfile = homedir+".config/lxsession/LXDE/autostart"' beliebig anpassen, zum testen empfohlen . Es handelt sich dabei um eine simple Textdatei, befühlen kann man sie mit dem Programm. Funktioniert nur auf Linux, nicht auf Windows/Mac .

Hier die lxautostart.py: http://pastebin.com/f35cb20a4
Und Hier die lxautostart.glade: http://pastebin.com/f5524faa2

maces

[Leonidas]

Also erstmal fällt auf, dass du ``excepts:`` hast, ohne eine konkrete Ausnahme zu abzufangen. ``ImportError`` wäre da ein guter Kandidat, den zu behandeln es gilt.

Weiterhin: Multi-Line-Strings existieren. Nutze sie doch auch, speziell für Multiline-Ausgaben.

Du solltest auch PEP8 lesen, den nach dem hast du die Klasse falsch benannt und auch die Funktionen heißen falsch (außerdem nutzt PyGTK glücklicherweise Underscores, kollidiert also selbst nicht mit PEP8 wie das andere GUI-Toolkits tun). Auch finde ich namen wie die mit ``l_s_n`` oder ``l_s_o`` anfangen einfafch nur schlecht.

Weiterhin: wozu setzt du Default-Argumente (noch dazu Strings was ziemlich unüblich ist, ``None`` wäre üblicher) wenn du sie nirgendwo verwendest? Gesehen in ``exiting()``, dort hätest du besser ``*args`` verwenden können.

Den Rest der 400-Zeilen-Klasse habe ich überspringen, ohne sie genauer anzusehen. Aber die Indexerei sah beim überfliegen nicht sonderlich toll aus.

Schließlich: Pfade setzt man nicht mit ``+`` zusammen sondern mit ``os.path.join``. Und dass es ``global`` gibt, vergisst du lieber mal.

[cofi]

Ein paar generelle Bemerkungen nach dem Ueberfliegen (evtl schreib ich noch mehr, wenn ich die Zeit finde)
Deine Funktionen sind zu lang.
Versuche den Code in echte Sinnabschnitte aufzuteilen. Schau dir alles was laenger als 10 Zeilen hat noch mal ganz genau an.

Dein Stil ist nicht konsistent.
Schau dir mal PEP8 an.

Deine Kommentare sind groesstenteils unnuetz.
Kommentiere nicht das, was der Code eh schon sagt, sondern das, was er nicht sagt (aber trotzdem wichtig ist).

Kommentiere keine Zeilen aus.
Loesche alten/kaputten Code und erspare es dem Leser herauszufinden, warum der Code auskommentiert ist. Wenn du das als Alternative zu einer VCS nutzt, vergiss es und setze eine ein. DVCS sind problemlos und ohne grossen Aufwand einsetzbar.

Importe sollten auf einer eigenen Zeile stehen.
Dann kannst du auch einzeln auf einen ``ImportError`` pruefen und dem User ein besseres Feedback geben. Blanke ``except`` solltest du dir auch abgewoehnen, wenn du alle Ausnahmen willst, dann ``except Exception:``. Bei Modulen der Stdlib brauchst du darauf nicht unbedingt pruefen, da bietet sich ein Vergleich der ``sys.version_info`` an, um festzustellen, ob das Modul schon in der Stdlib ist.

Code: Alles auswählen

if active == "yes":
    active = "yes"
elif active == "no":
    active = "no" # #in config file
if restart == "yes":
    restart = "yes" # @ in config file
elif restart == "no":
    restart = "no"

Unnuetz. Das Snippet tut genau gar nichts.
Da du das auch noch oefters hast, solltest du vllt ueber eine Funktion nachdenken, die die Ausgabe fuer dein Configfile formattiert.

Und last but noch least: Du vermischst GUI und Programmlogik. Ganz Poese.

[jbs]

Code: Alles auswählen

if active == True: pass
if active: pass

Wieso ist denn unter dem Paste noch ein anderer?
edit: oh, ich habs verstanden


macht es Sinn fuer diese yes/no eine Klasse zu schreiben, die so aussehen koennte?

Code: Alles auswählen

class YesNo(object):
    true = ('yes',)
    false = ('no',)
    
    def __init__(self, status):
        self.status = status
    
    @property
    def status(self):
        return self._status
    
    @status.setter
    def status(self, value):
        value = value.lower()
        if value not in self.true + self.false:
            raise ValueError('value  `%s` not in %s' %(value, self.true+self.false))
        self._status = value
    
    def __nonzero__(self):
        return self.status in self.true
    
    def __str__(self):
        return self.status

[maces]

Hi ihr drei,

Erstmal danke für die vielen Tipps, ich werde erstmal versuchen, sie so gut wie es geht umzusetzen. Wie schon gesagt, der Code ist nicht aus einem Guss, sondern zusammengeschrieben.

Ich denke es ist das beste wenn ich das Programm komplett neu schreibe und dieses mal die Trennung von GUI und Programmlogik besser umsetze und vor allem PEP8 lese

@jbs: werde es probieren, danke für die Hilfe

Ich würde nun die Dateioperationen in eine eigene Klasse stecken, die GUI in eine eigene Klasse, die Treeoperationen in die GUI-Klasse, die Dialoge jeweils in eine eigene Klasse und schließlich die Programmlogik in eine Klasse. So ungefähr:

Ist da bis jetzt ein konzeptioneller Fehler drin? Konzept

maces

[BlackJack]

@maces: IMHO mindestens zwei "Fehler".

1. Du scheinst alles unbedingt in Klassen stecken zu wollen, was bei den Dateioperationen nicht viel Sinn zu machen scheint. Das sind einfach Funktionen ohne gemeinsamen Zustand, also gehören die auch nicht in eine gemeinsame Klasse. Klassen sollen Daten und dazugehörige Operationen zusammenfassen. Du erstellst in Deinem Code Objekte vom Typ `file_operations` nur um eine "Methode" darauf aufzurufen und das Objekt dann letztendlich unbenutzt gleich wieder wegzuwerfen.

2. In der Beschreibung im Diagramm klingt mir bei der Programmlogik der Umgang mit der GUI zu aktiv. Die Programmlogik sollte nichts von der GUI holen oder zu ihr senden, sie sollte von der GUI letztendlich gar nichts wissen. Die GUI stösst Aktionen in der Programmlogik an und holt sich von dort das was sie anzeigen will. Auf diese Weise kann man die Trennung so gestalten, dass die GUI austauschbar ist, und die Programmlogik wiederverwendbar.

[maces]

HI BlackJack

1. Okay dann gehören also die Dateioperationen mit in die Klasse "Programmlogik", macht natürlich Sinn

2. Das Ziel wollte ich ja erreichen, die Umsetzung sollte folgendermaßen aussehen (wenn eine Zeile editiert werden soll): Button geklickt > Funktion "Editrow" der "Programmlogik" aufrufen > diese fordert die gerade aktive Zeile von der GUI > ruft die Klasse "EditDialog" auf und bekommt das Ergebnis zurück > die "Programmlogik ruft die Funktion "EditLine" der "GUI" auf, Welche die Änderungen im Treeview vornimmt.

Kann ich beim Funktionsaufruf, die gerade aktive Zeile mitgeben, dann wurde ein Schritt entfallen?

Deinen Vorschlag habe ich so verstanden: Button geklickt > Funktion "EditLine" der "GUI" wird aufgerufen > diese ruft "EditLine" der "Programmlogik" auf > die wiederum die Klasse "EditDialog" > "Editline" der Programmlogik gibt die Daten an "EditLine" der "GUI" weiter > die zeigt das Ergebnis an.

Der schritt über die "Programmlogik" finde ich unnütz (evtl. habe ich das flasch verstanden), daher würde ich nach deinem Vorschlag dann kurz gesagt so vorgehen: "GUI" > "EditDialog" > "GUI"

Jetzt würden die Daten aber in der Klasse "GUI" verarbeitet, das wäre für mich die Programmlogik (sehe ich das falsch?). Deswegen würde ich es wie oben beschrieben machen. So wie ich gerade denke soll die "GUI" nur get/set/return Aktionen ausführen, genau wie die "*Dialog" Klassen. Die eigentliche "Intelligenz" steckt also in "Programmlogik".

maces

[Leonidas]

1. Okay dann gehören also die Dateioperationen mit in die Klasse "Programmlogik", macht natürlich Sinn

Nein, ich denke BlackJack meinte dass die in gar keine Klasse gehören. Wenn Methoden auf keinem gemeinsamen Zustand arbeiten sind es keine Methoden sondern eigentlich Funktionen und sollten nicht in einer Klasse sein.

[BlackJack]

@maces:

1. Ich meinte in der Tat, dass das so wie's jetzt ist, Funktionen sein sollten. Andererseits kann man sicher auch eine Klasse modellieren, die so eine "config"-Datei repräsentiert und eine Methode zum Speichern und eine statische Methode zum Laden kennt.

2. Hm, es gibt Programmlogik und Programmlogik. Das was ich damit meine ist völlig unabhängig von der GUI und das ruft nichts in der GUI auf, also insbesondere werden von da keine Dialoge aufgerufen. Du vermischst das IMHO mit der Ablauflogik der GUI, die eben GUI-spezifisch ist.

Mein Vorschlag wäre Button -> ruft von Programmlogik die Daten für die entsprechende Zeile ab und öffnet den Dialog damit, der ruft am Ende die Programmlogik mit den geänderten Daten auf. Wenn dass dann in der GUI Auswirkungen auf mehrere Elemente hat, zum Beispiel eine Baumdarstellung und eine Listendarstellung, dann solltest Du Dir das "Model-View-Controller"-Entwurfsmuster (MVC) mal anschauen. Auf jeden Fall sind an der Stelle auch Rückrufmechanismen praktisch um die Programmlogik von der GUI zu trennen, also das sich die GUI bei der Model-Klasse für die Daten registrieren kann um über Änderungen in den Daten informiert zu werden.

Ich denke man darf sich hier von dem Wort "Programmlogik" nicht in die Irre führen lassen. Das muss nichts "grossartiges" sein, insbesondere bei Anwendugen, mit denen man im Grunde nur ein paar Daten manipuliert ist das wenig und langweiliger Code.

[maces]

Okay jetzt habe ich eine ganze Menge über Entwurfsmuster für die Softwarearchitektur gelesen, ich denke einfach mal laut:

Bei deinem Vorschlag stört mich, das die "GUI" von der "Programmlogik" die Werte der aktuellen Zeile haben möchte, aber die weiß nur die "GUI", welche dann extra aufgerufen werden müsste. So wie ich das im Moment verstehe, wären wir dann wieder bei meinem Konzept.

Im Moment bekomme ich das MVC Modell nicht ganz auf mein Programm "übersetzt", speziell die "indirekte Assoziation" macht mir Probleme, wie sieht so etwas in Pyhton aus und was welche Vorteile hab ich dadurch?

Nach ein paar Überlegungen, ist mir aufgefallen, das man es auch so machen könnte: Button geglickt > "EditLine" von "GUI" wird aufgerufen und ließt die aktuelle zeile aus > mit den Informationen wird "EditLine" der "Programmlogik" aufgerufen > welche die Klasse "EditDialog" aufruft > Der Dialog gibt die Eingaben des Benutzers zurück > "EditLine" der "Programmlogik" validiert diese und handelt entsprechend > sprich neue Zeile einfügen oder Fehlermeldung ausgeben, beides via "GUI". Nach Wikipedia wäre ich dann beim MVP und dem Passive View. Ich habe mal versucht das umzusetzen. Der Präsenter fungiert dann eigentlich nur als eine Art Wrapper. Für große Programme die auf Modularität ausgelegt sind, ist das denke ich eine gute Idee, aber für mein Programm wahrscheinlich etwas überdimensioniert.

Wenn ich jetzt die Schritte über die "Programmlogik" herausnehme, die die nur als eine Art Weiterleitung dienen, dann wäre ich hier. Allerdings wäre die "GUI" und die anderen Klassen nicht mehr sauber getrennt.

Hier das ganze nochmal als UML.

sorry für die denglischen Bezeichnungen.

Kann ich Funktionen mit Parameter von dem Glade-Dic aus aufrufen?

Ich bin jetzt ein wenig verwirrt, könnt ihr mir einen Denkansatz geben?

maces

[BlackJack]

@maces: Du hast ja immer noch die überflüssige Klasse für die Dateioperationen.

Und die Beispielskripte sind jetzt überhaupt nicht mehr OOP weil jetzt *alle* Deine Klassen sinnlose Container für Funktionen sind. Objekte erzeugen, nur um darauf "Methoden" aufzurufen, die keine gemeinsamen Datenattribute besitzen, und diese Objekte dann sofort wieder wegzuwerfen ist ein Antipattern.

[maces]

Mhm, ja stimmt auch wieder, muss wohl OOP nochmal genauer studieren, mein Code hat da ja nicht viel mit gemeinsam

Hat das was ich da versucht habe eigentlich einen bestimmten Namen?

Wie fasse ich meine Funktionen den in Gruppen zusammen, bis jetzt habe ich das ja mit Klassen gemacht, wie macht man das bei OOP? Müssen die Funktionen da ganz anders geschrieben werden?

maces

[cofi]

Hat das was ich da versucht habe eigentlich einen bestimmten Namen?

Java? SCNR

Wie fasse ich meine Funktionen den in Gruppen zusammen, bis jetzt habe ich das ja mit Klassen gemacht, wie macht man das bei OOP? Müssen die Funktionen da ganz anders geschrieben werden?

Warum willst du es denn auf biegen und brechen objektorientiert machen? Dein "Problem" ist, dass du generische Funktionen hast, die einfach keine Klassen brauchen. Wenn du die des Gruppieren willens Gruppieren willst, dann pack sie in ein eigenes Modul, ansonsten lass sie einfach bei dem Rest.

[maces]

Mhm okay leuchtet ein, dann werde ich mal versuchen das ganze ohne Klassen zu realisieren und die Gruppen einfach durch Kommentare markieren, ich denke Module sind hier zu viel.

Vielen Dank für eure Hilfe, melde mich sobald ich was neues geschrieben habe. Danke

maces

[BlackJack]

@maces: Funktionen gibt es bei rein objektorientierten Programmen gar nicht, da gibt es nur Objekte und Methoden. Allerdings ist Python in dem Sinn keine reine OOP-Sprache (auch wenn alles was man an einen Namen binden kann, ein Objekt ist), man muss also nicht auf biegen und brechen alles in eine Klasse zwängen.

Die Frage nach dem Gruppieren von Funktionen ist nur die halbe Miete beim OOP-Entwurf. Die Frage ist welche *Daten* mit welchen Funktionen gruppiert werden und somit aus den Funktionen auch wirklich Methoden werden. Du musst Dir überlegen welche Daten(typen) Du in Deinem Programm modellieren möchtest, und welche Eigenschaften und Operationen die haben sollen.

Wenn ich mir das erste Skript anschaue, dann würde ich mal bei dem nicht-GUI-relevanten Teil sagen, dass Du Kommandos als Objekte hast. Und ein einzelnes Kommando besteht aus der Kommandozeichenkette und den Eigenschaften "active" und "restart". Die werten in einer Textdatei jeweils als eine Zeile als Zeichenkette repräsentiert, also brauchst Du eine Operation um aus einem `Command` eine Zeichenkette zu machen und eine um aus einer Zeichenkette ein `Command` zu erstellen. Die Kommandos können dann in einer Containerklasse zusammengefasst werden. Ungetestet:

Code: Alles auswählen

#!/usr/bin/env python
# -*- coding: utf-8 -*- 
from __future__ import with_statement


class Command(object):
    def __init__(self, command, restart=False, active=True):
        self.command = command
        self.restart = restart
        self.active = active
    
    def __str__(self):
        return ('' if self.active else '#'
                + '@' if self.restart else ''
                + self.command
                + '\n')
    
    @classmethod
    def parse(cls, line):
        line = line.rstrip('\n')
        offset = 0
        active = True
        restart = False
        if line[offset] == '#':
            offset += 1
            active = False
        if line[offset] == '@':
            offset += 1
            restart = True
        return cls(line[offset:], restart, active)


class Commands(object):
    def __init__(self, commands=None):
        if commands is None:
            commands = list()
        self.commands = commands
    
    def __str__(self):
        return ''.join(map(str, self.commands))
    
    def __len__(self):
        return len(self.commands)
    
    def __getitem__(self, index):
        return self.commands[index]
    
    def __setitem__(self, index, command):
        self.commands[index] = command
    
    def __delitem__(self, index):
        del self.commands[index]
    
    def add(self, command):
        self.commands.append(command)
    
    @classmethod
    def read(cls, lines):
        return cls(map(Command.parse, lines)

    def write(self, out_file):
        out_file.write(str(self))
    
    @classmethod
    def load(cls, filename):
        with open(filename) as lines:
            return cls.read(lines)
    
    def save(self, filename, append=False):
        with open(filename, 'a' if append else 'w') as out_file:
            self.write(out_file)

[maces]

Okay, also ist das was ich unter Funktion verstanden habe eine Methode. eine Methode um Daten zu manipulieren.

Also würde man die Methoden die Objekte des Typs aktiv,neustart,Kommando manipulieren in eine Klasse stecken?

Okay ich muss gestehen, das ich den Code nicht ganz verstehe,

Command:
__init__ ist klar
__str__ definiert den Aufbau des Objekts "aktiv,neustart,Kommando"
@classmethod die Folgende Methode ist Klasse und Methode?
parse parst eine Zeile und gibt ein Objekt des Typs "aktiv,neustart,Kommando" zurück

Commands:
__init__ list()?
__str__ macht das Objekt "aktiv,neustart,Kommando" zu einem String
__len__ gibt die Länge eines Objekts zurück
__getitem__ gibt den Wert eines Items zurück
__setitm__ setzt den Wert eine Items
__delitm__ löscht ein Item
add fügt eine Objekt dem Treeview hinzu
read ließt mittels der parse Funktion von Command die Aktuelle Zeile aus und gibt sie zurück
load ließt die Configdatei aus
save speichert die übergebene Zeile in die Configdatei

also wäre das jetzt nach dem MVC oder MVP Modell geschrieben? Die einzelnen Methoden werden dann von den Buttons der GUI aufgerufen?

[BlackJack]

@maces: Das was Du Typ "aktiv,neustart,Kommando" nennst, hat durch die Klasse den etwas unkomplizierteren Namen `Command`. Mit Klassen modelliert man eigene Datentypen.

Mit `classmethod()` erzeugt man Methoden, die für die Klasse gedacht sind, und die Klasse auf der sie aufgerufen werden, als erstes Argument übergeben bekommen. Im Gegensatz zu den normalen Methoden, die man auf dem Exemplar einer Klasse aufruft und die eben dieses Exemplar als erstes Argument bekommen.

`Commands` ist kein "aktiv,neustart,Kommando"-Objekt, sondern ein Container der viele dieser Objekte aufnimmt. Und die entsprechenden Methoden lesen bzw. schreiben all diese Objekte.

Wo siehst Du bei `add()` einen Treeview? Das hier ist der Teil, der von der GUI komplett unabhängig ist.

`read()` liest die komplette Datei ein bzw. verarbeitet ein itierierbares Objekt, das Zeilen liefert zu einem `Commands`-Objekt.

`save()` bekommt keine Zeile übergeben, sondern speichert das `Commands`-Exemplar, auf dem die Methode aufgerufen wird, unter dem Dateinamen, der übergeben wird.

Ich habe so ein bisschen den Verdacht, dass Du Klassen/Objekte noch nicht so richtig verstanden hast. Den Schritt solltest Du IMHO auf jeden Fall hinter Dir haben, bevor Du an die GUI herangehst.

Der Quelltext war wie gesagt ungetestet. Hier ist eine leicht fehlerbereinigte Version: http://paste.pocoo.org/show/138365/ und hier eine kleine Sitzung in einer Python-Shell mit folgenden Daten:

Code: Alles auswählen

bj@s8n:~$ cat test.txt
#foo
@bar
#@baz

Code: Alles auswählen

In [37]: commands = Commands.load('test.txt')

In [38]: len(commands)
Out[38]: 3

In [39]: c = commands[1]

In [40]: c.active
Out[40]: True

In [41]: c.restart
Out[41]: True

In [42]: c.command
Out[42]: 'bar'

In [43]: str(c)
Out[43]: '@bar\n'

In [44]: c = Command('hallo', active=False)

In [45]: print c
#hallo


In [46]: commands.add(c)

In [47]: print commands
#foo
@bar
#@baz
#hallo

[maces]

Ich habe so ein bisschen den Verdacht, dass Du Klassen/Objekte noch nicht so richtig verstanden hast. Den Schritt solltest Du IMHO auf jeden Fall hinter Dir haben, bevor Du an die GUI herangehst.

Ich denke das ist das Hauptproblem im Moment, ich denke ich werde erstmal wieder die Grundlagen aufarbeiten.

Sobald ich glaube es verstanden zu haben, werde ich mal den Code neu schreiben und hier posten.

Nochmal Danke für eure Hilfe und die Geduld

maces