pywv - Eine Landkartenanwendung

Stellt hier eure Projekte vor.
Internetseiten, Skripte, und alles andere bzgl. Python.
Antworten
myoggradio
User
Beiträge: 14
Registriert: Freitag 20. November 2020, 12:24

Einen schönen guten Tag Euch allen,
möchte mein Projekt pywv vorstellen. Es handelt sich um eine Landkarte auf der man mit einfachen Mitteln navigieren kann.
Man kann dort GPX Tracks anzeigen und rudimentär bearbeiten.
Die zweite Anwendungsmöglichkeit ist in OpenStreetMap eingetragene Webseiten anzuzeigen.
Das Projekt befindet sich auf Github: https://github.com/homer65/pywv
Es beinhaltet eine help.html Datei die eine ausführlichere Beschreibung des Projekts gibt.

Würde mich über konstruktive Kritik und Anregungen freuen.
Ebenfalls willkommen sind Mitstreiter, die Interesse haben mit mir das Projekt weiter zu entwickeln.
Es ist ein reines Hobby Projekt, mit dem ich versuche mir die Programmiersprache Python näher zu bringen.
Wer ebenfalls Spaß dran hat Python anhand eines halbwegs sinnvollen Projekts zu lernen ist herzlich eingeladen mitzumachen.
Benutzeravatar
sparrow
User
Beiträge: 4164
Registriert: Freitag 17. April 2009, 10:28

Ich habe nur schnell ins Repository geschaut. Folgende Anmerkungen:

Das __pycache__-Verzeichnis gehört nicht ins Repository. Genausowenig .project und .pydevproject.
Bei dem Rest der Dateien bin ich mir nicht sicher. Das solltest du ordentlich strukturieren.

Es fehlt eine requierements.txt, die die Informationen beinhaltet, welche Module benötigt werden.

Du verwendest keine Leerzeilen. Wer soll denn solchen Code lesen? Wie man Code strukturiert, steht in der PEP8. Funktionen werden mit einer Leerzeile getrennt, Klassen mit 2.

Modulnamen werden klein geschrieben. "Parameter.py" ist also falsch - "parameter.py" wäre richtig. Aber eigentlich gehört das, was darin steht sowieso in das Hauptprogramm. Ich sehe keinen Grund die einzige darin enthaltene Klasse in ein eigenes Modul zu stecken.
Speziell bei der Klasse zweifele ich sogar deren Sinnhaftigkeit überhaupt an - denn das was du das nachprogrammierst, ist ja eigentlich nur eine Wörterbuch - nur versteckst du die Zugriffe hinter anderen Methoden.
und du verwendest dort "parms" (schrecklicher Name) als Klassenvariable. Das halte ich für falsch.

Du verwendest ständig leere returns. Die sind unsinnig, denn eine Funktion gibt automatisch None zurück, wenn es kein return gibt.

run.py: Auf Modulebene (also uneingerückt) gehört nur: Importe; Konstanten; die Definition von Klassen und Funktionen; der Einstiegspunkt:

Code: Alles auswählen

if __name__ == "__main__":
    main()
Das Programm beginnt dann in main().
Damit verhinderst du auch gleich die Verwendung von globalen Variablen, die du hier benutzt. "parameter" wird in myinit (wo ist denn yourinit oder herinit?) verändert. Funktionen bekommen alles, was sie zum Arbeiten brauchen, als Parameter und geben das Ergebnis mit return zurück. Magie auf globale Variablen kann niemand nachvollziehen.
Dateien öffnet man mit dem with-statement.

Tile.py sollte ebenfalls tile.py heißen.
Hier wieder keine einzige Leerzeile.
Globale Variablen.
Leere returns.
Das Zusammenbasteln der Zeichenketten sieht nicht schön aus. Benutz die entsprechenden Möglichkeiten zur Zeichenkettenformatierung.
Dateien öffnet man mit dem with-Statement um das saubere Schließen zu gewährleisten.
Wenn man anfängt Variablen durchzunummerieren möchte man eignetlich eine Datenstruktur. In der Regel eine Liste und anstatt fast den selben Code 16 Mal hintereinander zu kopieren, würde man das über eine Schleife lösen.


Soviel beim ersten Drüberschauen.
myoggradio
User
Beiträge: 14
Registriert: Freitag 20. November 2020, 12:24

Vielen Dank für Deine Anmerkungen. Ist aber für mich ein bischen viel auf einmal. Deshalb der Reihe nach. Den Anfang zu erst.
sparrow hat geschrieben: Samstag 12. Dezember 2020, 23:51 Ich habe nur schnell ins Repository geschaut. Folgende Anmerkungen:

Das __pycache__-Verzeichnis gehört nicht ins Repository. Genausowenig .project und .pydevproject.
Bei dem Rest der Dateien bin ich mir nicht sicher. Das solltest du ordentlich strukturieren.

Es fehlt eine requierements.txt, die die Informationen beinhaltet, welche Module benötigt werden.
Habe die drei Dateien/Directory ins .gitignore gepackt und aus dem Repository gelöscht.

Dann käme die requierements.txt. Es wird PyQt5 benötigt. Wie sehe da so eine Datei aus?

Das wäre erst mal der Anfang.
myoggradio
User
Beiträge: 14
Registriert: Freitag 20. November 2020, 12:24

Habe eine Nachtschicht eingelegt und den größten Teil Deiner Anregungen umgesetzt.
Bleibt noch die Frage nach der requirements.txt.
Das mit tile1 ... tile16 habe ich so gelassen, sehe da keinen Vorteil es in eine Liste zu überführen.
Habe auch nicht verstanden was Du mit:
"Das Zusammenbasteln der Zeichenketten sieht nicht schön aus. Benutz die entsprechenden Möglichkeiten zur Zeichenkettenformatierung."
meinst.
Ansonsten nochmal Danke für die zahlreichen Anregungen.
nezzcarth
User
Beiträge: 1632
Registriert: Samstag 16. April 2011, 12:47

Meistens legt man für jedes Projekt ein eigenes virtual environment an (mit python -m venv oder virtualenv z. B.) und installiert dort die Pakete, die man für das konkrete Projekt benötigt. Geht man so vor, kann man die requirements.txt dann einfach mit 'pip freeze > requirements.txt erzeugen'. Ansonsten gibt es hier ein Paar Beispiele: https://pip.pypa.io/en/stable/reference ... ments-file
__deets__
User
Beiträge: 14493
Registriert: Mittwoch 14. Oktober 2015, 14:29

Du siehst keinen Vorteil bei solchem Code?

Code: Alles auswählen

w1.tuwas()
w2.tuwas()
w3.tuwas()
w4.tuwas()
w5.tuwas()
w6.tuwas()
w7.tuwas()
w8.tuwas()
w9.tuwas() 
w10.tuwas()
w11.tuwas()
w12.tuwas()
w13.tuwas()
w14.tuwas()

zu

Code: Alles auswählen

for w in the_ws:
    w.tuwas()
? Und wenn jetzt nach tuwas() noch ein weiterer Aufruf kommt, dann kopierst du x-mal Code. Und sollte ein User sagen “ich hätte aber gerne 5 mal 5 oder 6 mal 6 Kartenteile, und das einstellbar” - wie genau machst du das mit deinem Ansatz?
Sirius3
User
Beiträge: 17711
Registriert: Sonntag 21. Oktober 2012, 17:20

Die run.ini und runLinux.ini sehen komisch aus, was sollen denn die Gleichheitszeichen am Ende jeder Zeile?
Eine ini-Datei würde man auch an einem fixen Ort erwarten, und nicht immer über ein Commandozeilenargument übergeben, so dass run.sh überflüssig wird.
Ein Blick in run.py macht das aber noch unübersichtlicher. `run.ini` wird anscheinend im aktuellen Arbeitsverzeichnis erwartet. Warum sind dann aber die Parameter nochmal fix im Code?
Eine Funktion, die ein Wörterbuch als Input erhält, sollte dieses nicht als Rückgabewert haben.
Dass Du in init immer das letzte Zeichen jeder Zeile wegwirfst ist falsch. Auch ist diese Funktion der falsche Ort um 0.5 abzuziehen. Warum überhaupt?
Zum Lesen von ini-Dateien gibt es etwas in der Standardbibliothek und dann auch fehlerfrei.
Nun zu tile.py:
Benutze keine Abkürzungen. gpx scheint ja noch was geläufiges zu sein, aber was bitte soll gpxtrkpt heißen? Vokale sind nicht teuer. minidom wird schon seit seiner Einführung nicht mehr benutzt. ElementTree ist der Standard zum Verarbeiten von XML.
for-Schleifen, wo bis auf das letzte Element alles weggeschmissen wird, sind unsinnig. Sowas programmiert man nicht.

Code: Alles auswählen

import xml.etree.ElementTree as et

GPX_NAMESPACE = "{http://www.topografix.com/GPX/1/1}"
def read_gpx():
    track_points = []
    dlg = QFileDialog()
    dlg.setFileMode(QFileDialog.ExistingFile)
    if dlg.exec_():
        filenames = dlg.selectedFiles()
        for filename in filenames:
            tree = et.parse(filename)
            points = tree.findall(f'.//{GPX_NAMESPACE}trkpt')
            for point in points:
                elevation = point.findtext(f'{GPX_NAMESPACE}ele')
                time = point.findtext(f'{GPX_NAMESPACE}time')
                lat = float(item.attrib['lat'])
                lon = float(item.attrib['lon'])
                track_points.append((lat, lon, elevation, time))
    return track_points
Und erst recht bastelt man keine XML-Dateien mit Stringstückelei zusammen.
In parseOSMXml dann das selbe nochmal: minidom und Stringstückelei. Dateien öffnet man innerhalb des with-Statements.
os.system verwendet man nicht, und auch die meisten Linux-Oberflächen kennen eine Methode, um ein Programm das mit html-Dateien assoziiert ist, zu starten. Das sollte man verwenden, statt fix firefox zu benutzen. Auf anderen Platformen funktioniert Dein Code ohne Kommentar sowieso nicht.
In downloadOSMData stückelst Du Query-Parameter zusammen, statt die passende Methode zu verwenden, und Du öffnest eine Datei zum Schreiben, ohne sie wieder zu schließen.
Der Codeblock in `calculateXY` ist falsch eingerückt und da hat es unsinnige Strichpunkte.
getTile erwartet Strings, aber überall, wo es aufgerufen wird, erzeugst Du aus int Strings, die Du vorher noch mit math.trunc umständlich in solche umgewandelt hast.
Pfade setzt man nicht mit + zusammen, und so wie Du das geschrieben hast, funktioniert das auch nur unter Windows, Deine ganzen Bemühungen, das irgendwie unter Linux zum Laufen zu bringen waren also für die Katz.
Zu der sechzehnfachen Codewiederholung wurde hier schon genug gesagt.
myoggradio
User
Beiträge: 14
Registriert: Freitag 20. November 2020, 12:24

Habe die tile1 bis tile16 in eine 4*4 Matrix überführt. Ihr habt recht gehabt, das hat den Code verschlankt.
Edit:
Es gibt jetzt auch eine requirements.txt
Benutzeravatar
sparrow
User
Beiträge: 4164
Registriert: Freitag 17. April 2009, 10:28

@myoggradio: Ich habe mir die Stelle im Code eben mal angeschaut. Was hat denn jetzt die Temperatur ("temp") damit zu tun?

Die "tiles" kannst du dir jetzt sogar komplett sparen.
Du tust die in verschachtelte Listen, nur um gleich danach wieder über die verschachtelten Listen zu iterieren und mit dem Inhalt etwas zu tun - und hinterher tiles nicht wieder zu verwenden. Dann kannst du auch gleich an der Stelle, wo du sie in die Liste tun würdest, das tun, was du 3 Zeilen weiter machst, wenn du sie wieder aus der Liste raus kramst.

getTile sollte ungetestet mindestens so aussehen (man beachte die Namensänderungn und die Leerzeichen in der Parameterliste):

Code: Alles auswählen

from pathlib import Path

def get_tile(x, y, z, parameter):
    path = Path(parameter["tileCache"]) / f"tile.{x}.{y}.{z}.png")
    if not path.is_file():
        downloadTile(x, y, z, parameter)
    return QImage(pfad)
Dann wird das hier auch weniger hässlich:

Code: Alles auswählen

#vorher:
tile = getTile(str(x-1+j),str(y-1+i),str(z),parameter)
#etwas leserlicher:
tile = get_tile(x-1+j ,y-1+i, z, parameter)
Dieses benutzen von "parameter" als Cache-Ersatz finde ich sehr schwierig. Woher soll der Leser das wissen? Sind das nun Parameter? Ist das ein Cache? Ist das einfach eine Mülltonne für alles, damit es so aussieht, als würde man keine globalen Variablen benutzen? Hint: Es ist Letzteres. Und das ist furchtbar unsauber.

Malst du ab Zeile 213 in tile.py ein Gitternetz? Die Werte sind da doch so schön gleichmäßig, die ~ 20 Zeilen Code lassen sich wieder durch Schleifen vereinfachen.

Code: Alles auswählen

# Was ist lesbarer?
# 1)
                if deltax < 0.0: ok = False
                if deltay < 0.0: ok = False
                if deltax > 765.0: ok = False
                if deltay > 765.0: ok = False

# 2)
                if 0 < deltax < 765 and 0 < deltay < 765:
                    ok = True
                
Das war alles, was ich auf dem erten Blick gesehen habe.

Und lies bitte PEP8 und die darin enthaltenen Namenskonvetionen.
Das Programm hat ja ein gewisses Potential - zum Beispiel den ganzen Kram selbst anzuzeigen, statt das einem Browser zu geben, aber dafür muss das Fundament stimmen.

Edit: Es ist spät und ich bin müde ;) Die if-Abfrage im letzten Code-Blog ist ja schon ein boolscher Wert. Den könnte man direkt an ok binden. Aber ich bin schon im Bett und mag nicht gucken, ob mit "ok" noch andere Dinge gemacht werden.
myoggradio
User
Beiträge: 14
Registriert: Freitag 20. November 2020, 12:24

sparrow hat geschrieben: Sonntag 13. Dezember 2020, 22:52 @myoggradio: Ich habe mir die Stelle im Code eben mal angeschaut. Was hat denn jetzt die Temperatur ("temp") damit zu tun?

Die "tiles" kannst du dir jetzt sogar komplett sparen.
Du tust die in verschachtelte Listen, nur um gleich danach wieder über die verschachtelten Listen zu iterieren und mit dem Inhalt etwas zu tun - und hinterher tiles nicht wieder zu verwenden. Dann kannst du auch gleich an der Stelle, wo du sie in die Liste tun würdest, das tun, was du 3 Zeilen weiter machst, wenn du sie wieder aus der Liste raus kramst.

getTile sollte ungetestet mindestens so aussehen (man beachte die Namensänderungn und die Leerzeichen in der Parameterliste):

Code: Alles auswählen

from pathlib import Path

def get_tile(x, y, z, parameter):
    path = Path(parameter["tileCache"]) / f"tile.{x}.{y}.{z}.png")
    if not path.is_file():
        downloadTile(x, y, z, parameter)
    return QImage(pfad)
Dann wird das hier auch weniger hässlich:

Code: Alles auswählen

#vorher:
tile = getTile(str(x-1+j),str(y-1+i),str(z),parameter)
#etwas leserlicher:
tile = get_tile(x-1+j ,y-1+i, z, parameter)
Dieses benutzen von "parameter" als Cache-Ersatz finde ich sehr schwierig. Woher soll der Leser das wissen? Sind das nun Parameter? Ist das ein Cache? Ist das einfach eine Mülltonne für alles, damit es so aussieht, als würde man keine globalen Variablen benutzen? Hint: Es ist Letzteres. Und das ist furchtbar unsauber.

Malst du ab Zeile 213 in tile.py ein Gitternetz? Die Werte sind da doch so schön gleichmäßig, die ~ 20 Zeilen Code lassen sich wieder durch Schleifen vereinfachen.

Code: Alles auswählen

# Was ist lesbarer?
# 1)
                if deltax < 0.0: ok = False
                if deltay < 0.0: ok = False
                if deltax > 765.0: ok = False
                if deltay > 765.0: ok = False

# 2)
                if 0 < deltax < 765 and 0 < deltay < 765:
                    ok = True
                
Das war alles, was ich auf dem erten Blick gesehen habe.

Und lies bitte PEP8 und die darin enthaltenen Namenskonvetionen.
Das Programm hat ja ein gewisses Potential - zum Beispiel den ganzen Kram selbst anzuzeigen, statt das einem Browser zu geben, aber dafür muss das Fundament stimmen.
Also temp steht für temporär; habe den Variablennamen geändert in temporaer.
Aus meiner Sicht ist es einfacher ein Zwischenbild 4x4 zu erzeugen und dann den richtigen 3x3 Teil auszuschneiden. Anderfalls müßte man häßliche Fallunterscheidungen machen, da eine Kachel des 3x3 Bildes bis zu vier Thunderforest Kachel zum Teil enthält.
Der Code mit dem Path funktioniert bei mir nicht. Es wird eine Klammer mehr zu als aufgemacht. Ich selber bin aber wohl zu dusselig um das richtig zu stellen.
Das mit den Parametern finde ich auch schwierig. Es sind halt die Werte aus der ini-Datei und die werden oft gebraucht. Aber globale Variablen soll man ja nicht gebrauchen. :-(
Ja ab 213 wird ein Gitternetz gemalt. Habe das jetzt auch in eine Schleife gepackt.
Dein Punkt # 2) ist einfacher, habe es eingebaut. Danke für den Hinweis.
PEP8, Ohje, Namenskonventionen werden wohl nie meine Stärke sein. Sieht man ja wohl.
Danke das Du in dem Program ein gewisses Potential siehst. Aufmunternde Worte tun der Seele gut.
Übrigens habe ich den Code jetzt mit Kommentaren versehen.
Und vielen Dank für Deine Mühe.
Benutzeravatar
__blackjack__
User
Beiträge: 13004
Registriert: Samstag 2. Juni 2018, 10:21
Wohnort: 127.0.0.1
Kontaktdaten:

@myoggradio: Bei der Path-Zeile scheint einfach nur eine schliessende Klammer zu viel zu sein. Die am Ende einfach wegnehmen.

Wenn Du ein Bündel Namen und Werte für alles mögliche herum reichst, dann sind das effektiv ja auch globale Variablen, oder haben zumindest genau das gleiche Problem mit der Übersichtlichkeit und der Testbarkeit.

Im ersten Schritt könntest Du es wenigstens mal in `config` umbenennen, damit der Leser weiss was das ist.

Kommentare die eine Funktion, Klasse, oder Methode beschreiben, sollten keine Kommentare sondern Docstrings sein.

Wenn bei den drei Namen `x`, `y`, und `z`, die da ja *gemeinsam* auftauchen, dann sollte `z` bitte der Dritte Wert einer XYZ-Koordinate sein. Wenn das die `zoomstufe` ist, dann sollte das auf keinen Fall `z` heissen. Bei Kommentaren die einen Namen erklären immer Prüfen ob man die nicht einfach so benennen kann, dass man den Namen nicht erklären muss.
“Most people find the concept of programming obvious, but the doing impossible.” — Alan J. Perlis
myoggradio
User
Beiträge: 14
Registriert: Freitag 20. November 2020, 12:24

__blackjack__ hat geschrieben: Montag 14. Dezember 2020, 16:27 @myoggradio: Bei der Path-Zeile scheint einfach nur eine schliessende Klammer zu viel zu sein. Die am Ende einfach wegnehmen.
Hat leider nicht funktioniert. Es kommt

Traceback (most recent call last):
File "C:\Users\chris\git\pywv\run.py", line 49, in <module>
main()
File "C:\Users\chris\git\pywv\run.py", line 44, in main
panel = BildController(x,y,z,parameter)
File "C:\Users\chris\git\pywv\tile.py", line 308, in __init__
self.bild = BildPanel(x,y,z,parameter,self.gpxtrackpoint) # Baue das Bild auf
File "C:\Users\chris\git\pywv\tile.py", line 229, in __init__
tile = get_tile(x-1+j, y-1+i, z, parameter)
File "C:\Users\chris\git\pywv\tile.py", line 151, in get_tile
return QImage(path)
TypeError: QImage(): argument 1 has unexpected type 'WindowsPath'

Bei folgendem Code:

Code: Alles auswählen

def get_tile(x, y, z, parameter):
    # Lese eine Thunderforest/Openstreetmap Kachel
    path = Path(parameter["tileCache"]) / f"tile.{x}.{y}.{z}.png"
    if not path.is_file():
        downloadTile(str(x), str(y), str(z), parameter)
    return QImage(path)
Benutzeravatar
__blackjack__
User
Beiträge: 13004
Registriert: Samstag 2. Juni 2018, 10:21
Wohnort: 127.0.0.1
Kontaktdaten:

@myoggradio: Da muss man den Pfad dann in eine Zeichenkette umwandeln weil Qt mit `Path`-Objekten (noch?) nichts anfangen kann.
“Most people find the concept of programming obvious, but the doing impossible.” — Alan J. Perlis
myoggradio
User
Beiträge: 14
Registriert: Freitag 20. November 2020, 12:24

Ok, so funktioniert es jetzt:

Code: Alles auswählen

def get_tile(x, y, z, parameter):
    # Lese eine Thunderforest/Openstreetmap Kachel
    path = Path(parameter["tileCache"]) / f"tile.{x}.{y}.{z}.png"
    if not path.is_file():
        downloadTile(str(x), str(y), str(z), parameter)
    return QImage(str(path))
myoggradio
User
Beiträge: 14
Registriert: Freitag 20. November 2020, 12:24

Habe ein erstes (v0.01) Release herausgegeben: https://github.com/homer65/pywv/releases
Nochmals vielen Dank an Alle die sich beteiligt haben.
Antworten