mediathekview / MLib

lib für das Projekt MediathekView
GNU General Public License v3.0
35 stars 27 forks source link

MSearch & MServer: Crawler #45

Closed xaverW closed 7 years ago

xaverW commented 7 years ago

Original:

Wie ist das genaue Vorgehen bei Änderungen die (nur) den Crawler betreffen und er neu gebaut werden müsste? Vielleicht wären da ein paar Punkte als Vorlage sinnvoll.

Betrifft Branch zdf-geo-tagging.

Die Zeilen in MediathekZDF.java:

} else {
titel = thema; 

habe ich mit bca056d auch wieder entfernt, die sind bei den Umstellungen wieder "zurückgekommen" (im laufendem Crawler sind sie nicht mehr drin, da habe ich die schon mal entfernt).

alex1702 commented 7 years ago

Also ich denke alles was nicht MediathekView und der Crawler gemeinsam in MSearch haben sollte vllt auch aus MSearch raus oder das was sie gemeinsam haben in eine weitere Bibliothek (hatte da schon heute mit derreisende77 drüber gesprochen). Theoretisch müsste die Bibliothek jetzt neu gebaut werden und zumindest der Crawler bräuchte dann ein Update (der Bibliothek), aber eigentlich brauch der Crawler halt an sich nicht neu gebaut werden sondern braucht nur eine neue Versionsnummer, die die neure Bibliothek dann enthält.

xaverW commented 7 years ago

Es wird aber nicht möglich sein, dass Crawler/lib1 und MediathekView/lib2 komplett getrennt sind. Vielleicht wäre ein Branch crawler sinnvoll aus dem dann gelegentlich gemerged wird und ein neuer Crawler gebaut wird. Änderungen wird es da immer wieder geben, da wird es nie eine "stabile" Version geben.

criztovyl commented 7 years ago

Ich würde nicht sagen das es nicht möglich ist, aber womöglich mit Aufwand verbunden ist, der ganz von der Verstrickung von MediathekView <-> MSearch* und MServer <-> MSearch\ abhängt.

* kann ich nicht einschätzen \ eher gering

Ich würde die Crawler-Sachen aus MSearch auch (vorerst) nicht in eine eigene Bibliothek packen, sonder einfach alles das was nur ihn betrifft zu MServer umziehen.

xaverW commented 7 years ago

Wenn es komplett getrennt wird (was natürlich geht), wären aber Klassen (Filmliste, DatenFilm, etc) in beiden Libs, also doppelt vorhanden. Wenn aber die Crawler-Sachen umziehen, wären ja die laufenden Anpassungen an die Mediathek abgedeckt.

alex1702 commented 7 years ago

ja das klingt auch gut, dass man einfach die crawler sachen auch in den crawler umzieht.

criztovyl commented 7 years ago

Also soweit trennen nicht ^^ Der Sinn von MSearch ist ja dann erfüllt, wenn nur der Code drin ist, den sowohl MServer alsauch MView brauchen. Ansonsten schleppt ja entweder MView oder MServer unnütz Code (in MSearch) rum, der gar nicht in MView resp. MServer gebraucht wird.

xaverW commented 7 years ago

da ist noch einiges drin, dass nur in MView gebraucht wird. Ich hatte da noch vor, die Version "MView -auto" als eigenes jar zu entwickeln und so sicherzustellen, dass nichts "vom GUI" mehr drin ist. Ist der Grund warum ich einiges nach MSearch ausgelagert habe. Das kann man aber ruhig zurückstellen.

criztovyl commented 7 years ago

Dann sollte MView wieder allen GUI-Code enthalten.

Ich würde folgendes Vorgehen vorschlagen:

  1. Neue Lib: MLib
  2. Allen nicht-Crawler-Code von MSearch in MLib kopieren. (Code is also noch in MSearch)
  3. MView von MSearch auf MLib umstellen
  4. Allen nicht-Crawler-Code aus MSearch entfernen
  5. Allen Crawler-Code aus MServer nach MSearch .
  6. MServer aufgeben & MSearch wird Crawler.

Um Verwirrung zu vermeiden sollte MSearch dann möglw. noch einen anderen Namen bekommen, bspw MCrawler.

Außerdem dann auch noch allen GUI-Code aus MLib nach MView umziehen, aber betrifft nicht diesen Issue.

Dann haben wir MLib = Bibliothek MCrawler = Crawler MView = nunja... MediathekView ... :D

xaverW commented 7 years ago

MView = nunja... MediathekView ... :D

ich hätte da kein Problem damit

criztovyl commented 7 years ago

Dann hau' ich mal in die Tasten. (Huh, vergessen Comment zudrücken :D gn8)

xaverW commented 7 years ago

https://github.com/mediathekview/MSearch/issues/53 ist wieder ein Beispiel, warum es leicht möglich sein muss, einen neuen Crawler zu bauen und in Betrieb zu nehmen: Das ORF hat einen variablen Teil in der URL der sich im laufe des Tages ändert, der muss beim Vergleich auf "neue Filme" ausgeblendet werden und der variable Teil hat sich geändert. Da werden jetzt Filme mehrfach im Laufe eines Tages hinzukommen.

alex1702 commented 7 years ago

Ja da haste recht. Was sagst du zu #52 ? Wenn ich das entfernen könnte würde ich eine neue Version erstellen für beides.

Nicklas2751 commented 7 years ago

@criztovyl Hast du bereits code umgezogen? Wenn ja kannst du den bitte Zeitnah einchecken? Dann kann ich hier mit angreifen. Bzw. wenn es dafür noch nicht zu spät ist, würde es nicht schneller gehen grundsätzlich erstmal MSearch zu kopieren und dann alles raus zu werfen was hier nicht reingehört?

xaverW commented 7 years ago

Bzw. wenn es dafür noch nicht zu spät ist, würde es nicht schneller gehen grundsätzlich erstmal MSearch zu kopieren und dann alles raus zu werfen was hier nicht reingehört?

genau so muss es gemacht werden, vorher auch noch MView darauf umbiegen, dass man auch sieht, was gebraucht wird

alex1702 commented 7 years ago

So war es geplant ja. Da oben nix weiter abgehakt ist, kannst du das denk ich einfach aufgreifen und umsetzen Nicklas.

criztovyl commented 7 years ago

Huh, der Code ist sehr miteinander verschränkt, konnte bisher nur MSearch.java und Main.java entfernen.

War als letztes dabei ListeFilme.java von MediathekKiKa zu lösen*, aber dann sind die Filme vom KiKa wieder doppelt drin. Ich habe das noch versucht zu beheben und bin dazu jetzt noch nicht gekomm'. Ich committe den Status Quo heut' abend mal vernünftig & push das hoch. Vorerst würde ich auf ner Branch in MSearch bleiben und die dann nach Abschluss mit filter-branch zu nem eigenen Repo zu machen.

* Habe eine Subklasse ListeFilmeSuchen angelegt, in die ich die Crawler-spezifischen Sachen pack'.

derreisende77 commented 7 years ago

Ich bitte darum bei Änderungen an ListeFilme extrem vorsichtig zu sein. Diese wird sehr häufig in MV verwendet und könnte dort funktionierende Logik zerstören.

Am 22. November 2016 12:15:03 schrieb Christoph Schulz notifications@github.com:

Huh, der Code ist sehr miteinander verschränkt, konnte bisher nur MSearch.java und Main.java entfernen.

War als letztes dabei ListeFilme.java von MediathekKiKa zu lösen*, aber dann sind die Filme vom KiKa wieder doppelt drin. Ich habe das noch versucht zu beheben und bin dazu jetzt noch nicht gekomm'. Ich committe den Status Quo heut' abend mal vernünftig & push das hoch. Vorerst würde ich auf ner Branch in MSearch bleiben und die dann nach Abschluss mit filter-branch zu nem eigenen Repo zu machen.

  • Habe eine Subklasse ListeFilmeSuchen angelegt, in die ich die Crawler-spezifischen Sachen pack'.

You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/mediathekview/MSearch/issues/45#issuecomment-262214182

xaverW commented 7 years ago

War als letztes dabei ListeFilme.java von MediathekKiKa zu lösen*

BITTE da keine Experimente!! So wie ich es verstanden habe, soll doch die lib auch im Crawler importiert werden?? Also muss die "ListeFilme" nicht aus MediathekKiKa entfernt werden (KikA muss aus der Lib).

Ich würde dringend empfehlen, erst mal eine 1:1 Kopie von MSearch zu nehmen, MView darauf umbiegen und dann erst anfangen die Sachen die dann dort (in der neuen Lib) nicht mehr gebraucht werden (Gui, MediathekReader, ..) zu entfernen.

alex1702 commented 7 years ago

Da bin ich auch für.

criztovyl commented 7 years ago

@xaverw Ohne dir jetzt widersprechen zu wollen, aber natürlich wird hier erstma' experimentiert werden, v.a. was geht & was nicht. Es ist ja nicht so, das ich jetzt vorhab' die Änderungen direkt, ungetestet, auszurollen :D Und das mit MediathekKiKa & ListeFilme hast du falschrum verstanden: Es geht nicht darum in MediathekKiKa keine ListeFilme mehr zu nutzen, sondern darum, keine MediathekKiKa mehr in ListeFilme zu nutzen. (ListeFilme#updateListe war's wenn ich mich richtig erinnere).

Und das mit der 1:1 Kopie geschieht ja mit der Branch so. Das entflechten der Klassen gehört für mich aber noch zu MSearch und bevor das nicht richtig abgeschlossen ist, würde ich auch noch nicht mit MLib anfangen. (Ich passe die ToDo Liste oben dementsprechend an).

criztovyl commented 7 years ago

Ich werde es heut' leider weder schaffen zu commiten noch zu pushen. Ich kümmer' mich da aber morgen drum.

criztovyl commented 7 years ago

@all Da ich mir jetzt nicht sicher bin ob's alle mitbekomm' haben, erwäh'n ich's sicherheitshalber nochmal: Ich habe ganz oben im Issue-Text die ToDo-Liste aufgeschrieben. Damit es nicht zu Verwirrung kommt.

xaverW commented 7 years ago

Wenn du in MSearch einen branch dafür angelegt hast, wäre es da nicht auch besser, in MServer auch einen anzulegen und dort das entfernte gleich einzufügen und so auch den neuen Crawler zu bauen. Dann könnte man, wenns läuft, mergen und die 2 Projekte einfach nur mit den Wunschnamen versehen. An MView müsste dann gar nichts geändert werden und ein neues Projekt muss auch nicht eingerichtet werden.

alex1702 commented 7 years ago

Wäre auch eine idee ja.

criztovyl commented 7 years ago

Noch habe ich keine Branch angelegt. Um der Nachvollziehbarkeit* Willen würde ich mit dem Löschen/Verschieben noch warten bis die Trennung vollständig abgeschlossen ist.

Das wirkliche Trennen später sollte auch nicht mit wirklichem löschen sondern via git filter-branch geschehen, wg. der o.g. Nachvollziehbarkeit*.

* Nachvollziehbarkeit in dem Sinne, weiterhin die Historie der Datei nachverfolgen zu können. Würden wir die Datei einfach nur mit nem 100% - und nem 100% + Commit verschieben würde ja die Historie verloren gehen.

criztovyl commented 7 years ago

Lasst uns wirklich nicht parallel über das verschieben reden, erstmal die Klassen entflechten. Ich würde bevorzugen beides getrennt voneinander zu betrachten.

criztovyl commented 7 years ago

[...] konnte bisher nur MSearch.java und Main.java entfernen.

Diese Aussage ist so nicht ganz richtig und hat für Verwirrung gesorgt. Ich meinte damit, das ich es geschafft habe MV mit einer MSearch JAR ohne diese beiden Klassen laufen zu lassen, wobei eine Anpassung in mSearch.tool.Functions nötig war (Funtions.java Z110).

criztovyl commented 7 years ago

Okay, meine bisherige Arbeit ist jetzt oben. Ich habe sie in feature/#45-crawler-code gepusht.

Wenn wir auf dieser Branch zusammenarbeiten wollen, schlage ich vor diese wie eine develop Branch zu behandeln, also davon Featurebranches zu erstellen. Wenn wir das so machen, sollten wir den feature/-Teil vor dem Branchnamen feature/#45-crawler-code wegzulassen, sie also nur crawler-code o.ä. nennen.

xaverW commented 7 years ago

ganz klar ist mir das nicht wie der (spätere) Ablauf ist, schaus mir aber mal an (versteh auch nicht ganz, was der Vorteil von dem Aufwand ist?)

criztovyl commented 7 years ago

Hm, wo liegt für dich hier der Mehraufwand?

xaverW commented 7 years ago

wenn ich früher das Programm umgebaut habe, wurden die Klassen die in einem entfernt wurden tatsächlich verschoben, also dorthin gesteckt, wo sie sollten, Netbeans hat das Refactoring perfekt beherrscht. Aber wie gesagt, ich schau mir das gerne mal an, wie du das vorhast, lerne gerne dazu. Durch das "live" hat man auch sofort gesehen, obs "klappt"

criztovyl commented 7 years ago

@xaverW Das Entflechten was ich gerade mache findet auf Methodenebene statt, nicht auf Klassenebene, wennauch da sicher trotzdem das NetBeans Refactoting greift ^^ (Wobei ich selbst ohne IDE arbeite.) Und nen bisschen "live" seh' ich das auch, allerspätestens Gradle zickt rum wenn da was nicht stimmt. (Ich arbeite da mit gradle compileJava -t, baut dann bei ner Änderung sofort neu.)

Ansonsten werde ich jetzt eine Branch anlegen, auf der wir dann gemeinsam arbeiten können. Ich würde auf dieser Branch dann auch git-flowig arbeiten, also nicht direkt auf der Branch sondern mit "Feature"-Branches. In dem Fall würde ich's dann aber als "Refactor"-Branch bezeichen und auch das Präfix dementsprechend von feature auf refactor ändern. (Über das Präfix können wir gerne noch diskutieren.) Und um die Verifikation/Kontrolle/Bestätigung etwas zu vereinfach würde ich weiterhin vorschlagen mit PRs zu arbeiten und/oder hier die Merge-Commits zu verlinken.

criztovyl commented 7 years ago

ich hab meine Änderungen an ListeFilme mal als PR gemacht ^^

criztovyl commented 7 years ago

Ich würde dann als nächstes DatenFilm angehen oder arbeitet da schon jmd dran? :)

Nicklas2751 commented 7 years ago

Von mir aus passt das.

xaverW commented 7 years ago

Das Entflechten was ich gerade mache findet auf Methodenebene statt, nicht auf Klassenebene, wennauch da sicher trotzdem das NetBeans Refactoting greift

und mit NetBeans ist die ganz große Arbeitserleichterung, dass es sich um die ganzen notwendigen "Umbenennungen" usw. selbständig kümmert

Nicklas2751 commented 7 years ago

Wie eclipse und IntelliJ auch ;)

criztovyl commented 7 years ago

Dem Workflow widersprech' ich ja gar nicht ^^ (Wennauch ich ihn so nicht nutze).

Mir ging's/geht's nur darum, das die Klasse in die die Methode kommt, noch in MSearch angelegt wird, nicht in MServer, MLib oder MV. :)

xaverW commented 7 years ago

ListeFilmeSuchen.updateListe

das wird 2x aufgerufen (das erste ist zu früh): listeEinsortieren.forEach(this::addInit);

und die Liste wird durch das Aufteilen in 2 Methoden auch 2x abgearbeitet. In die ganzen Methoden ist viel Zeit geflossen, um sie möglichst effizient zu machen und so wird das alles wieder verspielt. Kann man nicht einfach Konstanten für die Sendernamen anlegen damit die allgemein genutzt werden können? MediathekKika.SENDERNAME ist ja nur eine statische Konstante?

xaverW commented 7 years ago

ich würde mal wieder Vorschlag machen: am wichtigsten ist es doch, die Sender umzuziehen, das ist es doch, das am häufigsten geändert wird. Da wir da eh nicht alle gleichzeitig daran arbeiten können: ich würde im Server/Crawler je einen branch anlegen: 'move' und mal die Sender umziehen. So dass das wesentliche schon mal im Server ist. Dann kann man die anderen Sachen Stück für Stück (wo überhaupt notwendig) nachziehen.

Wir sollen hier doch keine Programmierparadigmen Lehrbuchreif umsetzen sondern es soll ein gutes und wartbares Produkt herauskommen?

(weiß dass meine Vorschläge immer abgelehnt werden, wäre aber doch zumindest einen Blick wert?)

criztovyl commented 7 years ago

Ich würde nix mehr zu MServer umziehen, ich wollte MServer als Crawler aufgeben und MSearch zum Crawler machen, nen Umzug wär' unnötige „Müh'”.

Sobald DatenFilm keine MediathekOrf mehr referenziert, kann die Klasse & ListeFilme schon mal nach MLib und womit zwei Kernelemente umgezogen wären.

Deine Vorschläge sind ihren Blick wert. Sie werden ja nicht abgeleht weil wir dich nicht mögen oder so. Bspw. ist mein Workflow & „Anspruch” an den Code einfach nen anderer, das ist der Grund warum ich meist dagegen bin, das geht nicht gegen dich persönlich :)

Und ich glaube die Geschwindigkeit hat in diesem Fall auch keinen großen Einfluss auf das Produkt MV. Das hier ist nix derart kritisches, das es zur Not nicht auch bis zum übernächsten Release (14) warten könnte, die letzten zwölf Versionen ging's ja auch :)

lookshe commented 7 years ago

Ich würde nix mehr zu MServer umziehen, ich wollte MServer als Crawler aufgeben und MSearch zum Crawler machen, nen Umzug wär' unnötige „Müh'”.

Wäre es nicht sinnvoller bestimmte Dinge getrennt zu halten? Meines Wissens ist MSearch für die Suche selbst zuständig und MServer steuert die Suche und die Such-Threads. Hier ist also eine schöne und sinnvolle Trennung gemacht worden. Warum sollte man das jetzt aufgeben und vermischen?

alex1702 commented 7 years ago

Ich stelle diese Entwicklung erstmal auf Pause, ich komme hier grad nicht mehr hinterher und es sollte erstmal ein Plan gemacht werden. Es scheint doch mehr in einander verworben zu sein.

alex1702 commented 7 years ago

Menschen haben unterschiedliche Ansichten, wie was gehört. Du hast dieses Produkt geschaffen, oder wenigstens lange weitergeführt, ich kenne die Geschichte hier nicht genau. Es ist dein Produkt, dein Baby. Aber du hast dich entschlossen es und, der Community, zu überlassen. Ich, wir, schätzen deine Leistungen. Danke, Xaver. Aber jetzt sind wir dran. Du hast uns die Verantwortung übergeben. Ich, wir, sind anders. Denken anders. Möchten Dinge anders machen. Und wir sind viele gegen einen, daher kommt dein Eindruck, wir lehnen deine Vorschläge immer ab, ob's nu' so ist oder nicht. Das geht nicht gegen dich, das neue Team hat einfach nur eine andere Dynamik.

Also deine Ausdrucksweise gefällt mir hier nicht.

  1. Xaver gehört zum Team und der Community.

Aber jetzt sind wir dran

  1. Sollche Aussagen gehören hier nicht hin.
criztovyl commented 7 years ago

Hm, vllt habe ich mich etwas... nunja. ausgedrückt. Ich wollte nur sagen, das hier Xavers Vorschläge nicht generell abgeschmettert werden weil wir ihn jetzt doof finden weil er mit der Haupt"beruflichen" Entwicklung aufhört. Sry.

criztovyl commented 7 years ago

@lookshe Ich widerspreche nicht, dass das getrennt gehört, für mich reicht es in dem Fall aber das innerhalb eines Projektes "Crawler" getrennt wird, das dort der Code an sich nicht vermischt wird. Das wäre für mich MSearch.

@alex1702 Sowohl in der Issue-Beschreibung als auch hier habe ich einen Plan beschrieben.

criztovyl commented 7 years ago

@xaverW AW: https://github.com/mediathekview/MSearch/issues/45#issuecomment-263008372 Herrje, diesen Kommentar habe ich vollkommen übersehen. Ich denk nochmal drüber nach.

xaverW commented 7 years ago

und ich habe auch schon mal geschrieben, dass man die Aufteilung erst mal in Server / MSearch machen kann und die Projekte dann in die gewünschten Namen umbenennen kann. Es käme dann das gleiche raus wie oben vorgegeben, alles nur einfacher und sicherer

criztovyl commented 7 years ago

Bei dem mit ListeFilme#updateListe meinst du hier und hier?

xaverW commented 7 years ago

ja