Opensource Lichtsteuerung PC_DIMMER2008

[christian.noeding]

Hi,

soeben lade ich den neuen Sourcecode auf Sourceforge.net. Ich habe etliche hundert Code-Zeilen eliminieren können und einige Funktionen laufen jetzt auch flotter ab. Zudem ist die größe der Exe von 1,53 auf 1,43 MB geschrumpft - schon mal sehr angenehm.

Ich habe die Vereinfachungen allerdings nicht über Arrays umgesetzt, sondern habe im Code nur "FindComponent" Deklarationen mit Schleifen genutzt, da ich zwar die Komponenten in Arrays packen und damit auch die ganzen Funktionen laufen lassen konnte, mir aber die Komponenten auf der MainForm nicht aktualisiert wurden. Wenn ich dann eine Zuweisung der MainForm-Komponenten zu den Array-Komponenten machen wollte, gabs immer Fehlermeldungen aufgrund Inkompatibilität!? Ich hätte das jetzt umgehen können, indem ich eine Zusatzfunktion zum Aktualisieren der Oberfläche,wie z.B.

Delphi-Quellcode:

for i:=1 to 8 do
TScrollBar(FindComponent('ch'+inttostr(i))).position:=scrollbars[i].position;
// ch1..ch8 sind die Fader und scrollbars[1..8] sind dann die Komponenten-Arrays ;)

aber schon wieder ne Aktualisierungsfunktion einbauen, wollte ich dann auch nicht.

Jetzt bleiben halt die etlichen Zeilen bestehen, die zum Initialisieren der Komponenten notwendig sind - schön ist aber, dass die riesigen Zeilenblöcke durch einzeilige For-Schleifen ersetzt werden konnten


Danke nochmals, für die Tipps...

bis denn,
Christian

[DGL-luke]

ohne irgendwie OT werden zu wollen, ein ausschnitt aus einem projekt von mir:

Delphi-Quellcode:

//globale vars:
var
  Form1: TForm1;
  labels:array[0..255] of TLabel;
  bars:  array[0..255] of TRBProgressbar;
//formcreate:
procedure TForm1.FormCreate(Sender: TObject);
var i:integer;
begin
dragacceptfiles(handle,true); //diese funktion gar nicht anschauen, ist unwichtig
for i:= 0 to 127 do
 begin
  labels[i]               := TLabel.Create(RBPanel1);
  labels[i].Parent        := Panel1;
  labels[i].Left          := 4 + (95*(i mod 4));
  labels[i].Top           := (i div 4)*17 + 8;
  labels[i].Caption       := '#'+format('%.3d',[i])+':'+chr(i);
  labels[i].Tag           := i;
  labels[i].OnDblClick    := bardblclick;
  labels[i].OnClick       := barclick;

  bars[i]                 := TRBProgressbar.Create(RBPanel1);
  bars[i].Parent          := labels[i].Parent;
  bars[i].Left            := labels[i].Left+48;
  bars[i].Top             := labels[i].Top;
  bars[i].Width           := 40;
  bars[i].ShowHint        := true;

  labels[i+128]           := TLabel.Create(RBPanel1);
  labels[i+128].Parent    := Panel2;
  {ganz viel mehr init-kram, genaugenommen der selbe wie oben}

  bars[i+128]             := TRBProgressbar.Create(RBPanel1);
  bars[i+128].Parent      := labels[i+128].Parent;
  {noch mehr init-kram, wieder so wie eins weiter oben}
 end;

panel2.Hide; //unwichtig
panel1.Show; //unwichtig
end;

hilft dir das evtl. bei deinem problem weiter?

wie du siehst, gibt es bei mir keinen unterschied zwischen array-komponenten und "auf der form"-komponenten.
der array wird direkt auf die form gelegt, mit den zuweisungen an left und top werden die kompos einzeln positioniert.

fincomponent kannst du dann endgültig knicken, wenn ich das richtig verstanden habe

(dass ich keine scrollbars sondern labels und progressbars in meinem projekt habe, sollte dich ja nicht weiter stören, hoffe ich!)

[christian.noeding]

Danke für das Code-Beispiel das werde ich mir in Ruhe mal anschauen und sehen, ob und wie ich es anwenden kann. Aber ich müsste dann ja komplett mein Design umändern, also von IDE-Aufbau weg zu Form-Aufbau über Code... das scheint mir etwas aufwendig, da ich ja keine dynamischen Erweiterungen meiner Oberfläche geplant habe... andererseits wäre es sicher sinnvoll, alles per Array auf deine Weise zumachen, um so vielleicht z.B. das Andocken zweier Panels zu vereinfachen. Ich müsste dann ja nur zwischen zwei Arraykomponenten die Nähe abfragen und nicht zwischen 20 verschiedenen einzelnen Panels (aber das ist ein anderes Problem)


also nochmals Danke
Gute Nacht,
Christian

[Robert Marquardt]

Kommen wir mal zum schwachen Verstaendnis von booleschen Ausdrucecken.

Delphi-Quellcode:

procedure TMainForm.CheckBox1MouseUp(Sender: TObject; Button: TMouseButton;
  Shift: TShiftState; X, Y: Integer);
begin
  if checkbox1.checked=true then data.manfade[(data.page*8)+1]:=true else data.manfade[(data.page*8)+1]:=false;
  if checkbox2.checked=true then data.manfade[(data.page*8)+2]:=true else data.manfade[(data.page*8)+2]:=false;
  if checkbox3.checked=true then data.manfade[(data.page*8)+3]:=true else data.manfade[(data.page*8)+3]:=false;
  if checkbox4.checked=true then data.manfade[(data.page*8)+4]:=true else data.manfade[(data.page*8)+4]:=false;
  if checkbox5.checked=true then data.manfade[(data.page*8)+5]:=true else data.manfade[(data.page*8)+5]:=false;
  if checkbox6.checked=true then data.manfade[(data.page*8)+6]:=true else data.manfade[(data.page*8)+6]:=false;
  if checkbox7.checked=true then data.manfade[(data.page*8)+7]:=true else data.manfade[(data.page*8)+7]:=false;
  if checkbox8.checked=true then data.manfade[(data.page*8)+8]:=true else data.manfade[(data.page*8)+8]:=false;
end;

Abgesehen das man nicht explizit auf "= true" abfragen muss, geht es natuerlich so viel einfacher:

Delphi-Quellcode:

procedure TMainForm.CheckBox1MouseUp(Sender: TObject; Button: TMouseButton;
  Shift: TShiftState; X, Y: Integer);
begin
  data.manfade[data.page*8+1] := CheckBox1.Checked;
  data.manfade[data.page*8+2] := CheckBox2.Checked;
  data.manfade[data.page*8+3] := CheckBox3.Checked;
  data.manfade[data.page*8+4] := CheckBox4.Checked;
  data.manfade[data.page*8+5] := CheckBox5.Checked;
  data.manfade[data.page*8+6] := CheckBox6.Checked;
  data.manfade[data.page*8+7] := CheckBox7.Checked;
  data.manfade[data.page*8+8] := CheckBox8.Checked;
end;

Die Checkboxen in einem Array wuerden das jetz nochmals deutlich vereinfachen.
Im Uebrigen ist dies ein klassischer Fall von Copy & Paste Code. An einer anderen Stelle habe ich genau die gleichen Zeilen gesehen.
Dies ist auch offensichtlich eine Basisaktion im Code. Es sollte eine eigene Methode geschrieben werden, um die Aktion zu kapseln.

Der naechste Fehler liegt in Zeilen wie dieser:
  device1.Caption:=inttostr(strtoint(device1.Caption)+4); Hier wird ein Wert in der Caption der Control aufbewahrt. Das ist schlechter Stil, da keine Trennung von Controls und Algorithmus vorliegt.
Besser den Wert getrennt in einer Variablen aufbewahren. Die Funktionsweise des Programms sollte die Eingaben des Benutzers in eine
Zustandsaenderung der Variablen = Eingabewerte umsetzen und dann die Algorithmen aufrufen. Diese aendern den Zustand, der dann auf die
Benutzeroberflaeche zurueckuebertragen wird.

Man koennte zumindest die Tag-Property ausnutzen, aber fuer diese gibt es eine bessere Verwendung. Indem man dort den Index des
Control-Arrays ablegt, kann man mit Ausdruecken der Art "Checkboxes[TCheckbox(Sender).Tag]" allen diesen Checkboxen einen einzigen Handler geben.

Delphi-Quellcode:

procedure TMainForm.CheckBox1MouseUp(Sender: TObject; Button: TMouseButton;
  Shift: TShiftState; X, Y: Integer);
begin
  data.manfade[data.page*8 + Checkboxes[TCheckBox(Sender).Tag] := TCheckBox(Sender).Checked;
end;

Damit wird auch klar das die Uebertragung aller acht Werte Overkill ist.

[Robert Marquardt]

Zitat von christian.noeding:

Ich habe die Vereinfachungen allerdings nicht über Arrays umgesetzt, sondern habe im Code nur "FindComponent" Deklarationen mit Schleifen genutzt, da ich zwar die Komponenten in Arrays packen und damit auch die ganzen Funktionen laufen lassen konnte, mir aber die Komponenten auf der MainForm nicht aktualisiert wurden. Wenn ich dann eine Zuweisung der MainForm-Komponenten zu den Array-Komponenten machen wollte, gabs immer Fehlermeldungen aufgrund Inkompatibilität!? Ich hätte das jetzt umgehen können, indem ich eine Zusatzfunktion zum Aktualisieren der Oberfläche,wie z.B.

Delphi-Quellcode:

for i:=1 to 8 do
TScrollBar(FindComponent('ch'+inttostr(i))).position:=scrollbars[i].position;
// ch1..ch8 sind die Fader und scrollbars[1..8] sind dann die Komponenten-Arrays ;)

Hier hast du den Trick nicht ganz verstanden.
Die Komponenten-Arrays sind Aliase zu den Komponenten auf der Form.
Nur im FormCreate werden ch1 bis ch8 ueberhaupt direkt angefasst. Im Rest des Programms wird dann nur noch Scrollbars verwendet.
Die Elemente von Scrollbars sind ch1 bis ch8 selbst, nicht Kopien. Die obige Source ist also Unsinn da sie ueberhaupt nichts macht.

[Robert Marquardt]

..und der naechste Schnipsel uneleganten Codes:

Delphi-Quellcode:

case data.preheatvalue of
0: begin vorwaermung:=127; vorwaerm0.Checked:=true; StatusBar1.Panels.Items[1].Text:='Vorwärmung: 0%'; end;
1: begin vorwaermung:=125; vorwaerm1.Checked:=true; StatusBar1.Panels.Items[1].Text:='Vorwärmung: 1%'; end;
2: begin vorwaermung:=124; vorwaerm2.Checked:=true; StatusBar1.Panels.Items[1].Text:='Vorwärmung: 2%'; end;
3: begin vorwaermung:=123; vorwaerm3.Checked:=true; StatusBar1.Panels.Items[1].Text:='Vorwärmung: 3%'; end;
4: begin vorwaermung:=121; vorwaerm4.Checked:=true; StatusBar1.Panels.Items[1].Text:='Vorwärmung: 4%'; end;
5: begin vorwaermung:=120; vorwaerm5.Checked:=true; StatusBar1.Panels.Items[1].Text:='Vorwärmung: 5%'; end;
6: begin vorwaermung:=119; vorwaerm6.Checked:=true; StatusBar1.Panels.Items[1].Text:='Vorwärmung: 6%'; end;
7: begin vorwaermung:=118; vorwaerm7.Checked:=true; StatusBar1.Panels.Items[1].Text:='Vorwärmung: 7%'; end;
8: begin vorwaermung:=116; vorwaerm8.Checked:=true; StatusBar1.Panels.Items[1].Text:='Vorwärmung: 8%'; end;
9: begin vorwaermung:=115; vorwaerm9.Checked:=true; StatusBar1.Panels.Items[1].Text:='Vorwärmung: 9%'; end;
end;

Offensichtlich gibt es hier eine Tabelle von konstanten Werten fuer die Vorwaermung.
Na dann deklarieren wir die doch einfach als Konstante.

Delphi-Quellcode:

const
  Vorwaermungstabelle: array [0..9] of Integer =
    (127, 125, 124, 123, 121, 120, 119, 118, 116, 115);

  Vorwaermung := Vorwaermungstabelle[data.preheatvalue];
  VorwaermCheckboxes[data.preheatvalue].Checked := True;
  StatusBar1.Panels.Items[1].Text := Format('Vorwärmung: %d%%', [data.preheatvalue]);

[Robert Marquardt]

Weiter gehts mit den Verbesserungsvorschlaegen.
Nochmals zur Erinnerung. Das Hauptfenster ist recht gelungen. Viele kommerzielle Programme sind schlechter designt.
Trotzdem gibt es noch genug zu meckern

Die initiale Beschriftung der Buttons ist falsch. Es muss nicht genannt werden das es ein Button ist, sondern die auszuloesende Aktion muss benannt werden.
Also "Szene 1" bis "Szene 8" und "Effekt 1" bis "Effekt 8". Durch die Trennung der Nummerierung entfaellt auch der falsche Zusammenhang.
es waere noch sehr praktisch wenn die Buttons ein Popupmenue zur individuellen Umbenennung bekommen koennten. Es sollte nicht zum grossen Umbenennungsdialog fuehren, sondern zu einem Dialog mit nur einem Eingabefeld fuehren.

Kommen wir wieder zur Source.
FormCreate:
Der SplashScreen gehoert ins .dpr File. Nur dort kann er vor der Erstellung der MainForm angezeigt werden. Dazu gibt es glaube ich einen Code-Library-Eintrag.
Alles ab "// Statusleiste bereit machen" bis (ausschliesslich) "startfademem := 1;" sind Einstellungen die in der IDE an den Komponenten gemacht werden sollten.
Das Laden des Konfigurationsfiles gehoert in eine eigene Methode, da es vielleicht auch an anderer Stelle zu gebrauchen ist.
Die folgenden Zuweisungen lassen sich natuerlich mit Komponenten-Arrays entscheidend vereinfachen.
"case data.comport of" ist wiederum ein typischer Fall fuer eine konstante Tabelle.
"// Kanäle 1-32 von Opto Control Deluxe 32 auf 0 setzen" schreit nach einer eigenen Methode "ResetOCDChannels".
"// Alle Toolboxes wie zuletzt gespeichert anordnen" ist ein Kandidat fuer ein Komponenten-Array. Spaetestens wenn es eine Aktion zum Aufraeumen gibt.
"case data.preheatvalue of" wurde ja schon in der vorigen Message behandelt.

"TMainForm.Senddataout" sollte anders organisiert werden. Das zu sendende Datenpaket erst in einem Puffer zusammenstellen und dann in einem comport.senddata senden.

Delphi-Quellcode:

var
  Puffer: array [0..15] of Byte;
begin
  // alles auf 0 setzen
  FillChar(Puffer, SizeOf(Puffer), 0);

  // ... die einzelnen Bytes eintragen

  // in einem Rutsch senden (auf die verschiedene Laenge pro Firmware achten)
  comport.senddata(Puffer, 15);

Der Grund ist, das bei einem USB zu RS232-Adapter das Senden eines einzelnen Bytes 3 ms dauert, ein laengerer Puffer aber nicht mehtr Zeit braucht.

Delphi-Quellcode:

procedure TMainForm.CheckBtns();
var
  B: Boolean;
begin
  B := listbox.items.count <= 0;
  AddBtn.Enabled := B;
  AddPMnu.Enabled := B;
  RunBtn.enabled := not B;
  ...

und die Methode schrumpft auf die Haelfte zusammen.

Exit1Click() sollte einfach nur Close audfrufen. Der Dialog wird in den OnCloseQuery-Event der Form verlegt, dann kommt die Abfrage immer egal wie man die Form schliesst.

Es folgen Codewuesteneien, die der Komponenten-Arrays beduerfen.

Bei COMx1Click() hoer ich jetzt mal auf.
Mit dem obigen und besonders mit Komponenten-Arrays duerften noch mehrere Tausend Codezeilen verschwinden, ohne das das Programm irgendetwas verliert.

[christian.noeding]

Puh, das sind etliche Korrekturen und Verbesserungsvorschläge, die mir durchaus einleuchten... ich werde versuchen, sie bestmöglich umzusetzen (aber nicht heute und morgen, da ich noch andere Dinge zu machen habe )

Sicher werde ich trotz Anleitung an manchen Dingen hängenbleiben (wie gesagt: ich habe mit Delphi erst im November/Dezember 2004 angefangen) - bisher war ich zufrieden, wenn der erstbeste Weg funktioniert hat (-> siehe irre lange Listen)




Zur Boolschen Algebra: die Ausdrücke habe ich z.B. bei Abfragen =true gesetzt, damit ich halbwegs die Übersicht behalte. Somit kann ich da gleich erkennen, dass ich boolsche Ausdrücke habe aber ich kann da sicher auch drauf verzichten *g*



Zu dem Missverständnis:

Delphi-Quellcode:

for i:=1 to 8 do
TScrollBar(FindComponent('ch'+inttostr(i))).position:=scrollbars[i].position;
// ch1..ch8 sind die Fader und scrollbars[1..8] sind dann die Komponenten-Arrays ;)

ich hatte den Trick schon halbwegs verstanden, nur nicht umsetzen können. Mit obigen Zeilen wollte ich die Oberfläche aktualisieren (hatte bei Tests auch geklappt), da ich ja die Verlinkung von Komponenten-Arrays und MainForm-Komponenten nicht hinbekommen hatte - aber mit der erneuten Beschreibung werde ich das nochmals versuchen



Ich werde also doch noch einiges am Programm zu tun habe ich habe auch immer noch nicht die Menüeinträge überarbeitet (-> Script=Skript,etc.) und die ganzen anderen (wichtigeren) Vorschläge ...



schönen Abend noch,
Christian

[christian.noeding]

es hat mir doch keine Ruhe gelassen und ich habe ein paar Tipps heute noch umgesetzt. Allerdings noch nicht den ganzen Code auf Arrays umgebaut, was ich ja für weitere Vereinfachungen dann brauche.

Mal sehen, da ich in den nächsten Wochen dann mit dem Programm die Theaterproben beleuchten und dann im September schon den Auftritt habe, werde ich wohl nichts großartiges mehr am Code ändern, sondern ihn jetzt mal ausgiebig auf Fehler testen und schauen, ob er stabil in jeder Situation läuft.


dennoch nehme ich gerne noch ein paar Tipps entgegen

Die aktuelle Version lautet nun 2.0.3.0 und ist wieder hier downloadbar



schönen Restmontag noch,
Christian

[christian.noeding]

Hallo,

bis gestern hatte ich gedacht, ich hätte jetzt alle schlimmen Fehler aus meinem Programm, aber jetzt gibt es ein Problem, welches ich nicht eingrenzen kann...

Wenn ich eine Windows-Fehlermeldung produziere oder ein Open-/Savedialog aufrufe, dann bleibt die Titelleiste meines Programmes deaktiviert nachdem ich die Dialogfelder per OK oder Abbrechen schließe und ich kann auf keine Funktionen (auch nicht mehr das "X" der Form) klicken.

Es sieht so aus, als würde das Programm nicht mehr als "Active" gelten...


ich habe den Sourcecode hier zum runterladen: Sourcecode 2.0.3.6 Beta


Es wäre toll wenn einer da mal drübergucken könnte, oder falls das Problem bekannt ist, mir vielleicht einen Tipp geben könnte, wo ich suchen soll - ich bin da jetzt recht ratlos.



Besten Dank,
Christian!