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

Show original currency in earnings view #1565

Open
KopolJunam opened this issue May 24, 2020 · 7 comments
Open

Show original currency in earnings view #1565

KopolJunam opened this issue May 24, 2020 · 7 comments

Comments

@KopolJunam
Copy link
Contributor

KopolJunam commented May 24, 2020

I wrote in the forum about a new feature which allows to display earnings in their original currency (and adding a currency column). In the forum you see screenshots of this. I already implemented this "superficially" (= some issues left, see below). Here is the commit you can cherry pick (I hope it's accessible) to see the feature in action:
KopolJunam@81d7d0c

IMPORTANT: The values are changed for all tabs but only the "year matrix" shows the new column.

Open issues that come to mind now (there are certainly more):

  • Localize hard-coded texts
  • Add column to other table based tabs of the view
  • suppress the feature in tabs where it doesn't make sense (non-table based views)
@ghost
Copy link

ghost commented May 24, 2020

@KopolJunam
Die Idee hört sich per se gut an, jedoch würde ich anstelle einer weiteren Konverter Klasse bereits im Datenmodelle die Logik implementieren. So etwas wie folgt.

value -= t.getUnitSum(Unit.Type.TAX);
if (useOriginalCurrency) 
   value.with(converter.at(t.getDateTime())).getAmount();

Evtl. müsste das Datenmodelle hier neben InvestmentVehicle um Currency erweitert werden. Hintergrund, Buchungen je Wertpapier können in verschiedenen Währungen gebucht werden bspw. (EUR, CHF und USD).

Gruß
Marco

@KopolJunam
Copy link
Contributor Author

KopolJunam commented May 24, 2020

@KopolJunam
Die Idee hört sich per se gut an, jedoch würde ich anstelle einer weiteren Konverter Klasse bereits im Datenmodelle die Logik implementieren. So etwas wie folgt.

@Ragas13
Ich muss gestehen, ich habe jetzt Deinen Lösungsansatz noch nicht im Detail studiert, aber mein "Neutral Converter" ist minimal invasiv, d.h. man muss am Model nicht gross rumflicken. Ich denke, früher oder später wird sich @buchen noch melden, und auch eine Meinung haben. Dann können wir ja nochmals diskutieren. Ich habe jetzt die Steuererklärung für 2019 erledigt und brauche das Feature erst 2021 wieder :-). Im Ernst nun: Ich würde natürlich den Schwung jetzt nutzen, sofern das Feature überhaupt erwünscht ist.

@KopolJunam
Copy link
Contributor Author

@Ragas13 @buchen Leider ist fast ein Jahr vergangen, und die Zeit der Steuererklärung wieder da. Ich habe die Änderung jetzt lokal bei mir wieder eingespielt, aber ich denke, auch andere wären vielleicht um dieses Feature froh. Was meint Ihr?

@ghost
Copy link

ghost commented Feb 13, 2021

Gegenfrage, aber wenn du es bereits bei dir implementiert hast, kannst du es nicht als PR hier einstellen? Du hast es doch bei dir fertig 🤔

@KopolJunam
Copy link
Contributor Author

Gegenfrage, aber wenn du es bereits bei dir implementiert hast, kannst du es nicht als PR hier einstellen? Du hast es doch bei dir fertig 🤔

Grundsätzlich richtig, aber Du selbst hattest ja noch Anmerkungen, und man müsste ja noch ein paar Arbeiten machen, z.B. Lokalisierung. Englisch und Deutsch könnte schon ich machen, aber wie ist das organisiert, dass das in den anderen Sprachen nachgeführt wird?

Sagen wir es so: Wenn das dann jemand auch wirklich merged, leiste ich den Aufwand gerne, das noch fertig zu machen.

@ghost
Copy link

ghost commented Feb 13, 2021

Was die Lokalisierung betrifft, ist mE zunächst jeder der mitentwickeln möchte, die einzelnen Sprachen selbst anzupassen. Google Translate ist hier hilfreich. Dies kann über den Sourcecode oder POEditor geschehen. Technisch verwendet Eclipse immer Englisch per Default, erst wenn eine Übersetzung vorhanden ist wird diese verwendet.

Was meine Anmerkungen betrifft, so war es lediglich eine Hinweis. Ob Du es es so dann berücksichtigen möchtest, ist etwas anderes.

Im Endeffekt, auch wenn Andreas versucht das Projekt im Auge zu behalten, erst wenn hier ein Pull Request vorliegt kann wird er sich äußern.

By the way, deine und meinen Erweiterung
#2084 würden mE gut harmonisieren.

@KopolJunam
Copy link
Contributor Author

@Ragas13. Gut, dann werde ich mal einen PR vorbereiten. Das mit der Lokalisierung ist sicher nicht das Gelbe vom Ei, aber bei meinem kurzen Text wird es wohl nicht allzu schief gehen.

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

No branches or pull requests

1 participant