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

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@alani1
Contributor

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

This comment has been minimized.

Contributor

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

This comment has been minimized.

@buchen

buchen Jun 13, 2015

Owner

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.

addExportButton(toolBar);
}

private void addExportButton(ToolBar toolBar)

This comment has been minimized.

@buchen

buchen Jun 13, 2015

Owner

Name der Methode führt etwas in die irre. Ist ja nicht ein "export button" sondern eher ein "add button".

public class BookmarksListView extends AbstractListView implements ModificationListener
{

private static final String DEFAULT_LOCATION = "WebLocation"; //$NON-NLS-1$

This comment has been minimized.

@buchen

buchen Jun 13, 2015

Owner

Was hältst Du davon das Label eines neuen Bookmarks irgendwas wie "Neues Bookmark" zu nennen und auch zu übersetzen? Der Benutzer kann mit WebLocation vielleicht nicht so viel anfangen.

});
}

private void fillContextMenu(IMenuManager manager)

This comment has been minimized.

@buchen

buchen Jun 13, 2015

Owner

bildschirmfoto 2015-06-13 um 07 53 11

Die Kontextmenüs haben ich mich zuerst irritiert. Zweimal hinzufügen?!? 😕 Dann habe ich es verstanden. 😄 Solange man die Reihenfolgen nicht ändern kann, würde ich vorschlagen irgendwas wie "Vorher einfügen" und "Nachher einfügen". Ein neues Item am Ende einfügen kann man ja immer noch über den "+" Plus Button oben rechts.

private static Map<Client,ClientSettings> instances = new HashMap<Client,ClientSettings>();

private Client client;
private ArrayList<WebLocation> bookmarksData;

This comment has been minimized.

@buchen

buchen Jun 13, 2015

Owner

Besser 'List' als 'ArrayList' - also das Interface, nicht die Implementierung.

"https://meine.deutsche-bank.de/trxm/db/init.do" //$NON-NLS-1$
+ "?style=mb&style=mb&login=br24order&action=PurchaseSecurity2And3Steps&wknOrIsin={isin}")); //$NON-NLS-1$
}
public void loadBookmarks() {

This comment has been minimized.

@buchen

buchen Jun 13, 2015

Owner

Bei mir hat das Laden der Bookmarks nicht funktioniert. Im Debugger habe ich dann gesehen, dass ich folgende Exception bekomme:

java.io.StreamCorruptedException: invalid stream header: EFBFBDEF

Zunächst wäre es super einfach mit PortfolioPlugin.log(e) die Exceptions wegzuschreiben. Der Benutzer sieht das ja nicht, aber wenn nachfragen kommen, hat man eine Chance zu verstehen, was abgeht.

Dann würde ich vorschlagen keine serialisierten Objekte im XML zu speichern. Erstens binden wir uns dann an die Sprache Java (was ist mit der zukünftigen iOS Version? 😉). Zweitens läuft man leicht in solche Serialisierungsprobleme rein. Und dann finde ich es hilfreich zur Not im Texteditor Dinge korrigieren zu können.

Die loadBookmarks und saveBookmarks Methoden könnten aber einfach einen String generieren. Man muss etwas auf das Esapting achten.

}
catch (IOException e)
{
return;

This comment has been minimized.

@buchen

buchen Jun 13, 2015

Owner

Exception loggen.


}

public ArrayList<WebLocation> getBookmarks()

This comment has been minimized.

@buchen

buchen Jun 13, 2015

Owner

List interface.

}

@Override
public void propertyChange(PropertyChangeEvent arg0)

This comment has been minimized.

@buchen

buchen Jun 13, 2015

Owner

Als Namen lieber "event" als "arg0"?

@Override
public void propertyChange(PropertyChangeEvent arg0)
{
if (arg0.getPropertyName() == "bookmarks") { //$NON-NLS-1$

This comment has been minimized.

@buchen

buchen Jun 13, 2015

Owner

Stringvergleich mit ".equals" und nicht mit "==". Kann schiefgehen...

import java.io.UnsupportedEncodingException;
import java.net.URLEncoder;
import java.nio.charset.StandardCharsets;

import name.abuchen.portfolio.model.Security;

public class WebLocation
public class WebLocation implements Serializable

This comment has been minimized.

@buchen

buchen Jun 13, 2015

Owner

Die "WebLocation" sollten wir jetzt in das "model" Package verschieben. Dadurch das "ClientSettings" im "model" Package liegt (und da gehört es auch hin) und WebLocation aus dem "online" Package braucht das wiederum das "model" Package nutzt, haben wir einen package cycle.

Ich weiß das ich im "ui" Projekt davon ein paar habe. Aber wenn die mit überschaubaren Aufwand lösbar sind, dann mache ich das auch.

}

@Override
public void setFocus()

This comment has been minimized.

@buchen

buchen Jun 13, 2015

Owner

Inherit only.

This comment has been minimized.

@alani1

alani1 Jun 13, 2015

Contributor

Was meinst du hiermit ? Ich habe ConsumerPriceIndexListView als Vorlage genommen, da ist setFocus ebenfalls @override.

This comment has been minimized.

@buchen

buchen Jun 13, 2015

Owner

Ich meinte #setFocus ist hier leer. Muss also nicht überschrieben werden. Man erbt sie nur. Kleinigkeit.

@buchen

This comment has been minimized.

Owner

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

This comment has been minimized.

Contributor

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

This comment has been minimized.

Owner

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

This comment has been minimized.

Owner

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 from First version to fix #309 to Implement editable Bookmarks (WebLocation) #309 Jun 17, 2015

@alani1

This comment has been minimized.

Contributor

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.

* Second version to implement editable Bookmarks
* 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 alani1:feature_configurable_weblocations branch from 6c6a3dc to be6fc1f Jun 17, 2015

@buchen

This comment has been minimized.

Owner

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

This comment has been minimized.

Owner

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

This comment has been minimized.

Contributor

alani1 commented Jun 20, 2015

Danke Andreas ! Ich versuche mich zu verbessern :-)

@buchen

This comment has been minimized.

Owner

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