Menüs aufgeräumt

[GetAway]

Code: Alles auswählen

Index: apps/tuxbox/neutrino/src/gui/movieplayer.cpp
===================================================================
RCS file: /cvs/tuxbox/apps/tuxbox/neutrino/src/gui/movieplayer.cpp,v
retrieving revision 1.193
diff -u -r1.193 movieplayer.cpp
--- a/apps/tuxbox/neutrino/src/gui/movieplayer.cpp	15 Feb 2011 20:56:52 -0000	1.193
+++ b/apps/tuxbox/neutrino/src/gui/movieplayer.cpp	26 Mar 2011 22:27:36 -0000
@@ -334,10 +334,6 @@
 	delete filebrowser;
 	if(bookmarkmanager)
 		delete bookmarkmanager;
-	CLCD::getInstance()->setMode(CLCD::MODE_TVRADIO);
-	g_Zapit->setStandby (false);
-	g_Sectionsd->setPauseScanning (false);
-
 }

Fehlt dann nicht zumindest ein g_Zapit->setStandby (false) in playstream() oder
wird das am Ende von ::exec gemacht?

[Gaucho316]

Da sollte das notwendigste diesbezüglich gemacht sein, obwohl ich da noch ein ToDo drin stehen habe, wo ich das mal generell aufräumen wollte. Das bin ich noch schuldig

Super, dass du so schnell Zeit gefunden hast, das zu machen. Ausprobieren kann ich es leider nicht, da ich kein IDE-Interface habe.

Fehlt dann nicht zumindest ein g_Zapit->setStandby (false) in playstream() oder wird das am Ende von ::exec gemacht?

Keine Sorge, das wird sowieso alles in CMoviePlayerGui::exec gemacht. Im Moment wird der Destruktor bei Benutzung des Movieplayer 1 ja auch nie aufgerufen, da das Objekt nie gelöscht wird, und es funktioniert ohne Probleme. Deshalb denke ich, dass das weg kann. Bei Benutzung des Movieplayer 2 wird der Destruktor derzeit übrigens auch nur aufgerufen, wenn man über den Moviebrowser reingeht. Also ist wohl auch dort der angesprochene Code überflüssig.

[GetAway]

@dbt

Ist Dir das nur reingepflutscht?

Code: Alles auswählen

driver_boot_setup.cpp:131: error: `LOCALE_DRIVERSETTINGS_STARTVES1820_V13' was not declared in this scope
driver_boot_setup.cpp:135: error: too many initializers for `const driver_setting_files_struct_t[8]'
make[5]: *** [driver_boot_setup.o] Fehler 1

[dbt]

@dbt

Ist Dir das nur reingepflutscht?

Code: Alles auswählen

driver_boot_setup.cpp:131: error: `LOCALE_DRIVERSETTINGS_STARTVES1820_V13' was not declared in this scope
driver_boot_setup.cpp:135: error: too many initializers for `const driver_setting_files_struct_t[8]'
make[5]: *** [driver_boot_setup.o] Fehler 1

Jo, stammt aus einem anderen Branch, aber alle anderen Teile sind ja sowieso von Gauchos Patch zu nehmen.

[Gaucho316]

@dbt

Ich habe mal über deinen Patch drübergeschaut und gleich einen Fehler entdeckt. Der Code in Zeile 203 und 204 deines Patches wird dir um die Ohren fliegen, da du das Objekt hdd_sleep auf dem Stack anlegst. Nach dem Verlassen des Blockes, der mit if (current_device != MMCARD) (siehe Zeile 196 deines Patches) eingeleitet wird, wird hdd_sleep ja gelöscht.

Außerdem kommt mir die Anzahl deiner Änderungen in drive_setup.cpp ziemlich gering vor. Ich sage dir deshalb, was ich genau gemacht habe. Es müssen alle Objekte, die mit new angelegt und als MenuTarget an einen MenuForwarder übergeben werden, entweder am Ende mit delete gelöscht oder ggf. stattdessen auf dem Stack angelegt werden. Dasselbe gilt für ChangeObserver-Objekte (d.h. die vielen Notifier-Objekte), die bspw. an MenuOptionChooser übergeben werden.

[dbt]

Hatte ja gesagt, dass es nur das Notwendigste war. Mit hdsleep hatte ich nicht getestet, da ich da momentan nur mmc dran hatte und wie gesagt, den Rest bin ich noch schuldig.

Es müssen alle Objekte, die mit new angelegt und als MenuTarget an einen MenuForwarder übergeben werden, entweder am Ende mit delete gelöscht...

bei einigen würde es knallen. Um die Untermenüs zu schalten, habe ich da Felder für die Verzweigungen eingesetzt. Das macht das etwas komplizierter was ich aber so nicht ganz stehen lassen wollte und sicher noch in einer Musestunde anpassen werde.

[Gaucho316]

Hatte ja gesagt, dass es nur das Notwendigste war.

Ok, dann deuten wir den Begriff "das Notwendigste" unterschiedlich.

[dbt]

Na dann muss ich mal schauen, dass ich das demnächst in Angriff nehme.

[Gaucho316]

Ich habe den Patch aktualisieren müssen, da die Datei osdlang_setup.cpp rausgeflogen ist. Das Speicherleck wird durch einen anderen Patch gleich mitbeseitigt.

Link entfernt, da Patch im CVS

[Gaucho316]

In einem von dbts gestrigen Commits stand:

neutrino personalize: unused variables and functions removed

note: Same variables and member function 'hide()' are also in most
of other menu classes. We should remove those...

Wenn nicht schon jemand angefangen hat, werde ich mich die Tage mal der Sache annehmen.

[dbt]

In einem von dbts gestrigen Commits stand:

neutrino personalize: unused variables and functions removed

note: Same variables and member function 'hide()' are also in most
of other menu classes. We should remove those...

Wenn nicht schon jemand angefangen hat, werde ich mich die Tage mal der Sache annehmen.

Kannst du, dann brauch ich das nicht , jedenfalls wird das bin wieder einiges schmäler. Die Members und framebuffer inits braucht man so gut wie nie, habe ich blöderweise im nachhinein auch erst gemerkt. Im NeutrinoHD hab ich das auch raus. Sieht sauberer aus...

[Gaucho316]

@dbt

Du hast ja in der Klasse CPersonalizeGui die Methode hide entfernt. Kann es sein, dass du dir als Aufräum-Beispiel genau die falsche Menü-Klasse ausgesucht hast? In Zeile 358 von personalize.cpp wird nämlich hide aufgerufen, damit der Bildschirm vor dem Anzeigen der Hilfe gelöscht wird. Jetzt wird einfach die geerbte Methode hide der Klasse CMenuTarget aufgerufen und die macht nichts. Somit wird die Hilfe über das Menü gezeichnet, wenn ich den Code richtig verstehe. Ausprobiert habe ich es nämlich noch nicht. Oder ist das Absicht? Dann ist aber der Befehl hide an dieser Stelle überflüssig und kann auch weg.

[dbt]

Ist weg, braucht man nicht!

[Gaucho316]

So, Klassen sind aufgeräumt.

Link entfernt, da Patch im CVS


Außerdem gibt es hier noch einen Patch für einen Segfault, der auftritt, wenn man das Tasten-Menü verlässt. Den Bug hatte ich mit meinem Memleak-Patch eingebaut. Da hatte ich es mit dem Löschen etwas zu gut gemeint. Eigentlich ein ziemlich dämlicher Fehler.

Link entfernt, da Patch im CVS

[Gaucho316]

Wenn jetzt schon screen_max.h in menue.cpp inkludiert wurde, und wenn auch nur unabsichtlich, dann kann man die Funktionen auch nutzen. Ich habe das mal an allen Stellen gemacht, wo ich meine, dass es richtig ist. Gleichzeitig sind damit die unnützen Leerzeichen, die mit dem vorletzten menue.cpp-Commit reingekommen sind, wieder verschwunden. Das ist so nebenbei abgefallen.

Link zum Patch entfernt

Außerdem habe ich in den Funktionen getScreenStartX und getScreenStartY in screen_max.cpp die Division durch 2 durch eine Bitverschiebung ersetzt. Ist zwar nur 'ne minimale Optimierung, aber immerhin.

Link entfernt, da Patch überflüssig

Getestet habe ich beides noch nicht. Dazu komme ich erst später.

[seife]

Außerdem habe ich in den Funktionen getScreenStartX und getScreenStartY in screen_max.cpp die Division durch 2 durch eine Bitverschiebung ersetzt. Ist zwar nur 'ne minimale Optimierung, aber immerhin.

Vergleiche mal den Assemblercode. Ich bin mir sicher, dass der identisch ist.
Selbst sehr alte Compiler optimieren das schon richtig.

Es macht aber den Code zweifelsohne etwas schwerer zu durchschauen und das ist immer ein ordentliches Ziel ;-)

[Gaucho316]

Es macht aber den Code zweifelsohne etwas schwerer zu durchschauen und das ist immer ein ordentliches Ziel ;-)

Ich dachte, das wäre das Ziel von Neutrino. Dann habe ich das falsch in Erinnerung.

Aber mal im Ernst, wie man hieran gut erkennen kann, habe ich von Compilern und deren automatischer Optimierung keine Ahnung. Ich hatte den Code mit der Bitverschiebung bei meinen obigen Änderungen in menue.cpp gesehen und im Hinterkopf gehabt, dass das ja schneller als eine Division ist. Aber wenn das sowieso automatisch geht, umso besser. Dann sollte es natürlich so bleiben, wie es ist.


Edit: Ich musste noch etwas im Patch korrigieren. Ich hatte an zwei Stellen den zweiten Parameter der max-Funktionen vergessen. Jetzt habe ich es auch mal getestet und erst einmal keine Auffälligkeiten feststellen können.

Link entfernt, da Patch im CVS

[seife]

Insbesondere solche einfachen Sachen machen die Compiler schon lange richtig.

Auch lohnt es sich typischerweise nicht, aus Optimierungsgründen konstrukte wie

Code: Alles auswählen

if (x == 1)
    foo();
else if (x == 2)
    bar();
else
    baz();

in

Code: Alles auswählen

switch(x) {
    case 1: foo(); break;
    case 2: bar(); break;
    default: baz();
}

umzubauen. Ich habe das schon Stunden mit solchen optimierungen verbracht und das Ergebnis war praktisch immer: der generierte Code war identisch.

Inzwischen ist meine Meinung zum Thema: keine "Compileroptimierung" des Codes machen, weil das die Leute die die Compiler bauen meist besser können als ich.

Insbesondere mit modernen Architekturen ist es oft auch so, dass Code, der auf den ersten Blick "schlecht" aussieht u.U. besser performt, weil die Pipelines oder die "speculative execution" der Prozessoren besser genutzt werden etc.
Ich habe einige Jahre bei Compiler- und Toolchainspezialisten "um die Ecke" gesessen und die haben alle (im positiven Sinn ;) "mächtig einen an der Waffel". Ich bin mir ziemlich sicher, dass die das besser hinbekommen als wir alle zusammen :-)

Ich persönlich versuche meist, das so zu machen, dass der code möglichst verständlich bleibt. Sprich:

ich will die hälfte von was: y = x / 2;
ich will aus 2 char's einen short machen: y = (c[0] << 8) | c[1];

Beim 2. finde ich "(c[0] * 256) + c[1]" irgendwie schlechter lesbar, auch wenn am Ende dasselbe rauskommt. Ist aber auch Geschmackssache.

[Gaucho316]

Mich nervt schon seit einiger Zeit, dass sich Neutrino die Position in Einstellungen im Movieplayermenü nicht merkt. Ich schalte nämlich gerne bei Bedarf die WabberQueue ein oder aus. Hier ein Patch dafür.

Link entfernt, da Patch im CVS


Außerdem habe ich ein Memleak im Video-Setup entfernt. RGBCSyncControler wird nämlich beim wiederholten Aufruf immer wieder neu angelegt und nicht gelöscht. Und dann habe ich noch zwei mögliche Segfaults im Video-Setup beseitigt. Falls nämlich eine Instanz der Klasse CVideoSetup irgendwann mal gelöscht wird, fliegt uns der Code um die Ohren, da SyncControlerForwarder und VcrVideoOutSignalOptionChooser im Destruktor gelöscht werden, obwohl sie schon vorher durch delete videosetup entfernt werden. Das ist im Moment allerdings irrelevant, da sowieso nur eine Instanz von CVideoSetup angelegt und nie gelöscht wird (siehe neutrino_menu.cpp). Aber sicher ist sicher. Auch hierfür ein Patch.

Edit: Ich habe den Patch noch etwas überarbeitet. RGBCSyncControler braucht man nämlich gar nicht als Membervariable.

Link zum Patch entfernt


Und einen hab ich heute noch. In neutrino_menu.cpp sind ein paar überflüssige Includes übrig geblieben. Ich habe die mal entfernt.

Edit: Ich habe den Patch erst einmal wieder entfernt. Ich guck da lieber nochmal in Ruhe drüber. Da kann nämlich noch mehr weg.

Link zum Patch entfernt

[GetAway]

@Gaucho316

Hier habe ich auch noch etwas. Das hatte ich schon mal gespostet, finde es aber nicht mehr.
Der Bug tritt auf, wenn man im MB die Dbox-Taste und anschließend die Blaue Taste drückt.
Danach zerreißt es den Moviebrowser, wenn man mit dem Einstellungsmenü hantiert.

[Gaucho316]

Außerdem habe ich ein Memleak im Video-Setup entfernt. RGBCSyncControler wird nämlich beim wiederholten Aufruf immer wieder neu angelegt und nicht gelöscht. Und dann habe ich noch zwei mögliche Segfaults im Video-Setup beseitigt. Falls nämlich eine Instanz der Klasse CVideoSetup irgendwann mal gelöscht wird, fliegt uns der Code um die Ohren, da SyncControlerForwarder und VcrVideoOutSignalOptionChooser im Destruktor gelöscht werden, obwohl sie schon vorher durch delete videosetup entfernt werden. Das ist im Moment allerdings irrelevant, da sowieso nur eine Instanz von CVideoSetup angelegt und nie gelöscht wird (siehe neutrino_menu.cpp). Aber sicher ist sicher. Auch hierfür ein Patch.

Ich habe den Patch noch etwas ergänzt. In der Methode showVideoSetup() wird unnötigerweise VcrVideoOutSignalOptionChooser auf NULL gesetzt, wenn Neutrino nicht auf Nokia- oder Sagem-Boxen läuft. Das wird aber schon im Konstruktor erledigt und muss deshalb hier nicht nochmal gemacht werden.

Link entfernt, da Patch im CVS

Und einen hab ich heute noch. In neutrino_menu.cpp sind ein paar überflüssige Includes übrig geblieben. Ich habe die mal entfernt.

Hier nun die neue Variante, die zusätzlich dafür sorgt, dass movieplayer_setup.h nicht mehr doppelt eingebunden wird.

Link entfernt, da Patch im CVS

Der Bug tritt auf, wenn man im MB die Dbox-Taste und anschließend die Blaue Taste drückt.
Danach zerreißt es den Moviebrowser, wenn man mit dem Einstellungsmenü hantiert.

Ich guck mal, ob ich den Fehler finde.

Edit: Hier nun der Patch dafür.
Link zum Patch entfernt

In Neutrino HD müsste der Fehler auch auftreten.
Link zum Patch entfernt

[GetAway]

Der Bug tritt auf, wenn man im MB die Dbox-Taste und anschließend die Blaue Taste drückt.
Danach zerreißt es den Moviebrowser, wenn man mit dem Einstellungsmenü hantiert.

Ich guck mal, ob ich den Fehler finde.

Edit: Hier nun der Patch dafür.
Link zum Patch entfernt

In Neutrino HD müsste der Fehler auch auftreten.
Link zum Patch entfernt

Das hatte bestimmt einen Grund, dass der Browser im Hintergund offen blieb. Wenn man jetzt Suche Filme
ausführt und anschließend Filminformationen aufrufen will, passiert nichts, weil der Focus des markierten Films
verloren geht. Vielleicht fällt dir dazu noch etwas ein.

[Gaucho316]

Das hatte bestimmt einen Grund, dass der Browser im Hintergund offen blieb. Wenn man jetzt Suche Filme
ausführt und anschließend Filminformationen aufrufen will, passiert nichts, weil der Focus des markierten Films
verloren geht.

Das Problem habe ich nun auch behoben:
Link entfernt, da Patch im CVS

Und hier für Neutrino HD:
Link entfernt, da Patch im SVN


Außerdem ist mir dabei aufgefallen, dass am unteren Bildschirmrand eine Linie übrig bleibt, wenn man das Einstellungsmenü im Moviebrowser aufruft. Dafür gibt's auch einen Patch.

Link entfernt, da Patch im CVS

[GetAway]

Positiv getestet.

[rhabarber1848]

Das hatte bestimmt einen Grund, dass der Browser im Hintergund offen blieb. Wenn man jetzt Suche Filme
ausführt und anschließend Filminformationen aufrufen will, passiert nichts, weil der Focus des markierten Films
verloren geht.

Das Problem habe ich nun auch behoben:
moviebrowser_fix-movie-scan-in-settings-menu_2011-09-18_1736.diff

committed: http://article.gmane.org/gmane.comp.vid ... x.scm/3053