Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement editable Bookmarks (WebLocation) #309 #311

Conversation

alani1
Copy link
Contributor

@alani1 alani1 commented Jun 9, 2015

Hi Andreas,

Hier ein erster Versuch für #309. Bitte reviewen, ich hoffe die allgemeine Code-Quality ist etwas besser als in meinen anderen Contributions. Beachte ClientSettings kann etwas generischer geschrieben werden sobald wir das Bedürfnis haben weitere Settings im Client zu speichern. Wenn du denkst dass dies jetzt schon gemacht werden soll bitte melden.

Gruss,
Alain

@alani1
Copy link
Contributor Author

alani1 commented Jun 10, 2015

Bin mir nicht sicher ob 5142787 eine gute Lösung für den in #309 vorgeschlagene Menupunkt ist. Aber ist in etwa die beste Idee welche ich hatte.

import org.eclipse.swt.widgets.Composite;
import org.eclipse.swt.widgets.ToolBar;

public class BookmarksListView extends AbstractListView implements ModificationListener
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AbstractListView ist eine Basisklasse für "Master-Detail Views", also z.B. oben die Konten und unten die Buchungen. Da wir hier nur eine Liste brauchen, kannst Du direkt von AbstractFinanceView ableiten. Dann fällt auch die leere "createBottomTable" weiter unten weg.

@buchen
Copy link
Member

buchen commented Jun 13, 2015

Hi Alain,

die Bookmarks zu editieren wird ein nettes neues Feature werden. Ein paar Punkte sind mir aufgefallen. Eher Kleinigkeiten. Der einzige wirklich grössere Punkt ist für mich die Serialisierung der Bookmarks als Java Objekt. Ich würde lieber reinen Text haben, den ich zur Not auch mit anderen Tools bearbeiten kann. Nicht unbedingt XML (bei den anderen UI related properties mache ich da auch so). Man könnte aber auch XML generieren (sprich: Objekte in der properties Map speichern und von XStream serialisieren lassen). Wie siehst Du das?

Andreas

@alani1
Copy link
Contributor Author

alani1 commented Jun 13, 2015

XStream kannte ich bisher nicht, und sieht wirklich nett aus ! Ich habe mir zuerst überlegt JSON zu generieren. XStream ist sicher naheliegender. Ich frage mich jedoch macht es wirklich Sinn dies in den properties zu speichern oder sollen wir nicht gleich ein attribut clientsettings fuer die Klasse ClientSettings im client hinzufuegen welche dann normal von der ClientFactory gespeichert wird ? Ja du wolltest das eigentlich verhindern. Dann wuerde aber das XML File etwas lesbarer aussehen da das escapen nicht mehr notwendig ist. Your call, fuer mich ist es auch so ok.

Das mit dem Exception loggen scheint so nicht zu gehen da das PortfolioPlugin teil des .ui packages ist. Das Model sollte ja nicht vom ui abhaengig sein oder verstehe ich da was Falsch ?

@buchen
Copy link
Member

buchen commented Jun 14, 2015

Das mit dem Exception loggen scheint so nicht zu gehen da das PortfolioPlugin teil des .ui packages ist. Das Model sollte ja nicht vom ui abhängig sein oder verstehe ich da was Falsch ?

Du hast Recht. Aus dem "n.a.portfolio" Package hast Du keine Zugriff. Bisher konnte ich das auch vermeiden. Ich wollte in diesem OSGi Bundle keine Abhängigkeiten zum GUI/Eclipse/SWT. Also auch nicht zu deren Logging Framework.

Ich frage mich jedoch macht es wirklich Sinn dies in den properties zu speichern oder sollen wir nicht gleich ein attribut clientsettings für die Klasse ClientSettings im client hinzufuegen welche dann normal von der ClientFactory gespeichert wird ?

Klar. Mach ruhig mal ein ClientSettings Objekt in den Client. Und dann explizite Methode für die Bookmarks. Die Klasse könnte man auch mal von "WebLocation" in "Bookmark" umbenennen.

Grundsätzlich bin ich immer erst mal vorsichtig. Die Klasse kann man dann nie wieder löschen denn ansonsten kann man nicht mehr deserialisieren. Und da habe ich schon ein paar, wie z.B. n.a.p.model.Category die jetzt deprecated sind. Aber hier ist das angebracht.

Und das würde es auch einfacher machen. Man braucht keinen Property Listener mehr und das Kontextmenü muss nix laden oder so, sondern einfach über die Liste iterieren. Manchmal braucht es eine solche Iteration bis der Code stimmt - ich kann mir das nicht immer schon im Vorhinein vorstellen...

@buchen
Copy link
Member

buchen commented Jun 14, 2015

Und: auf jeden Fall in der ClientFactory unten einen "alias" eintragen. Ansonsten hat man fully qualified class names im XML. Und das macht das verschieben schwieriger...

@alani1 alani1 changed the title First version to fix #309 Implement editable Bookmarks (WebLocation) #309 Jun 17, 2015
@alani1
Copy link
Contributor Author

alani1 commented Jun 17, 2015

Hi Andreas,
Wie besprochen heisst WebLocation nun Bookmark, der TestCase ist angepasst und die ganze Serialisierung ist weg resp. wird von ClientFactory gehandelt. Ich habe aber die Klasse ClientSettings belassen, da diese so in Zukunft verwendet werden kann wenn weitere Einstellungen hinzukommen sollten. Die TestsCases laufen alle durch. Die vielen commits müssten man nun eigentlich Squashen und einen neuen PR machen, mal schauen ob ich das irgendwie hinkriege.

* renamed WebLocation to Bookmark and moved to name.abuchen.portfolio.model
* ClienSettings is now saved as part of the Client
@alani1 alani1 force-pushed the feature_configurable_weblocations branch from 6c6a3dc to be6fc1f Compare June 17, 2015 07:56
@buchen
Copy link
Member

buchen commented Jun 17, 2015

Hi Alain,

vielen Dank! Und auch für die Geduld noch mal eine Runde zu drehen und es etwas anders zu machen. Ich kann nicht versprechen dass ich unter der Woche dazu kommen werde. Aber am Wochenende sollte ich das mergen können. Und dann werde ich auch eine neue Version veröffentlichen - das wollen sicherlich auch andere nutzen.

Andreas.

@buchen
Copy link
Member

buchen commented Jun 20, 2015

Hi Alain,

ich habe die Change jetzt gemergt. An Deinem Commit habe ich nur die Commit Message leicht geändert. Ich versuche mich immer an den Git Standard zu halten, also in der ersten Zeile eine Überschrift, dann eine Leerzeile, dann Detailbeschreibung wenn nötig, wieder eine Leerzeile, dann die Properties wie die Issue Number, oder Signed-off, etc. Und "second version to implement" ist ja nicht mehr relevant wenn man in ein paar Jahren auf die Historie schaut. 😉

Ich habe noch einen Commit hinterher geschoben. In der Message habe ich notiert was ich geändert habe. Nix dramatisches. Ich habe vor allem noch ein "Add separater" hinzugefügt. So kann der Benutzer dieses Feature kennenlernen. Und die Client Version habe ich hochgezählt. Das mache ich bei jeder Änderungen die sich auch auf die Persistenz auswirkt. Ansonsten haben ältere Versionen des Programms Probleme die Datei zu lesen.

Andreas.

@buchen buchen closed this Jun 20, 2015
@alani1
Copy link
Contributor Author

alani1 commented Jun 20, 2015

Danke Andreas ! Ich versuche mich zu verbessern :-)

@buchen
Copy link
Member

buchen commented Jun 20, 2015

Ich habe für Contributions zu danken!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants