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

added widget for the dashboard and columns for the information of dividends #3927

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

kimmerin
Copy link
Contributor

Hi,

in #1669 and in the forum there seems to be demand for information widgets and columns showing informations about upcoming dividends. So here is my shot at it. This adds a widget for the dashboard, showing a list of upcoming dates (ex dividend, payment or both) of securities and the expected amounts. As well I've added columns for various tables (security view, etc.) that shows the date of the next ex/payment date for each security.

I've added new entries to be localized at the end of Messages and the properties-files for german and english to make it easier to spot them for the other languages. They can be resorted at the end of the process, of course.

Hope that helps and cheers, Lothar

@Nirus2000
Copy link
Member

Nirus2000 commented Apr 11, 2024

Hello @kimmerin
Thank you for this pull request. That is definitely a great idea.

Perhaps you could format the source code before we continue working on it. That would help us a lot.
Example:
"nexpdd" change to "nextDividendsPaymentDate"
"nexdd" ...
""nexdpa" ...
... and so on.

We should also keep the translation correct and complete it with deepl.com and correct the capitalization.
Example:
"ColumnDividendsNextExDate_MenuLabel" = "Next Dividend Ex-Date" --> "Next dividend ex-date"
... and so on.

We should also take care to keep the translations consistent.
Example:
"ColumnDividendsNextExDate" = "Div Ex. Tag"
"ColumnDividendsNextExDate_MenuLabel" = "Next ex-dividend date"
"Div Ex. Tag" <--> "Next ex-dividend date"
... and so on.

Example:
"LabelEarningsDividendList" = "Übersicht anstehender Dividenden" --> "Übersicht anstehender Dividendenzahlungen"
... and so on.

We should also take care to keep the variable names consistent.
Example:
"Security sec" --> "Security security"
"SecurityEvent se" --> ...
... and so on.

But it looks good... formatting the source. 👍🏻

Copy link
Member

@Nirus2000 Nirus2000 left a comment

Choose a reason for hiding this comment

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

Perhaps these points can also help you?

@kimmerin
Copy link
Contributor Author

Perhaps you could format the source code before we continue working on it.

I've followed the setup of Ecslipe for contribution according to the provided documenation and my understanding is that there is an automated source formatter. At least my formatting gets changed all the time as soon as I save ;-)

That would help us a lot. Example: "nexpdd" change to "nextDividendsPaymentDate" "nexdd" ... ""nexdpa" ... ... and so on.

These are unique identifiers used by the SWT table to distinguish the columns from each other and are used - at least I'm expecting that - to persist the state of the table-configuration. I assumed that they aren't required to be human readable after looking at other occurreces like column = new Column("6", Messages.ColumnShareInPercent, SWT.RIGHT, 80). I can change these identifiers but camelcase would be a first for all of these, though.

We should also keep the translation correct and complete it with deepl.com and correct the capitalization. Example: "ColumnDividendsNextExDate_MenuLabel" = "Next Dividend Ex-Date" --> "Next dividend ex-date" ... and so on.

The capitalization is correct in this case, because it's an entry for the menu where other entries are in uppercase as well, e.g.

ColumnCapitalGainsMovingAveragePercent_MenuLabel = Capital Gains % (moving average, current holdings)

We should also take care to keep the translations consistent. Example: "ColumnDividendsNextExDate" = "Div Ex. Tag" "ColumnDividendsNextExDate_MenuLabel" = "Next ex-dividend date" "Div Ex. Tag" <--> "Next ex-dividend date" ... and so on.

To be honest I don't understand your example. Can you rephrase?

Example: "LabelEarningsDividendList" = "Übersicht anstehender Dividenden" --> "Übersicht anstehender Dividendenzahlungen" ... and so on.

I cah change Dividenden to Dividendenzahlungen but both phrases are used in the properties-file already.

We should also take care to keep the variable names consistent. Example: "Security sec" --> "Security security" "SecurityEvent se" --> ... ... and so on.

TBH: As long as they are consistently named within a class and are memorizable within the code block they are used in, I'd rather keep the names as they currently are.

@kimmerin kimmerin requested a review from Nirus2000 April 11, 2024 11:08
@Nirus2000
Copy link
Member

No problem... good job 👍🏻

@kimmerin
Copy link
Contributor Author

I've added texts for the other languages but this is definitely something to be looked over by people who actually speak these languages.

date are the same. Fixed wrong coloring for ex-date (should be red) and
payment date (should be green).
@kimmerin
Copy link
Contributor Author

@Nirus2000 checked in some small improvement today. The PR is still in status "Changes requested". What requests are still open?

@buchen
Copy link
Member

buchen commented Apr 18, 2024

The PR is still in status "Changes requested". What requests are still open?

I think it is on me :-) Will look at it later this week

(Not sure excactly how to remove the changes requested by @Nirus2000 - I understand there is nothing open at the moment)

Copy link
Member

@buchen buchen left a comment

Choose a reason for hiding this comment

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

First, I think that is a great new feature which is making use of existing data. Thanks.

Then about the code review: I have added a couple of smaller comments in the code. Overall it looks good to me. Thanks.

The about the feature. My general proposal is to simplify the options.

I am not sure about the "period start" and "period end" selection options in the dashboard widget. Is this selection really relevant? Do we want the user to fiddle with these selection options? My proposal is:

  • show all future dividend payments (assumption: this list is limited to at most one future payment per held instrument)
  • show all dividend payments of the last 3 months for which we do not find a dividend transaction in the file

I think we should not show in the widget dividend payments for instruments that the user does not hold. For this, the user can use the newly added columns in the security list.

Then about the layout of the widget: I propose to make it one line per instrument (security) similar to the list of earnings. The line should include payment and ex-date and the dividend payment and the (expected) total payment account. That requires understanding the number of shares held at the ex-date.

And this layout removes the "type of date" selection. Having just one line also makes the "number of payments" easier to understand. Because if one selects "both", the number does not match to the number of lines.

Then about the sizing of the widget: It took me some time to understand that it has a fixed size and that (some) entries are just missing. I would propose to do it similarly to the EarningsListWidget which just uses as much space as needed. The fixed size makes sense for charts. Thinking about it, it also does not make sense for the "follow up" and "limit exceeded" widgets (didn't think about it when developing those widgets).

Beyond this code change (in a separate pull request!), we could add new feature:

  • Right click on an upcoming payment and create the actual transaction
  • Allow editing of the dividend events (right now one must use DivvyDiary, but I think it is a valid use case to allow adding such events)

What do you think?

@buchen buchen self-assigned this Apr 27, 2024
@kimmerin
Copy link
Contributor Author

The about the feature. My general proposal is to simplify the options.

I am not sure about the "period start" and "period end" selection options in the dashboard widget. Is this selection really relevant?

I think yes. When creating the widget I've imagined questions that users might want to get answered by this:

  • When are the next payments to be expected?
  • What payments have happened so far
  • When do I have to buy additional stocks at the latest to still get the dividend

The options I've provided allow the configuration of individual widgets to answer these questions. There should be a useful default (at the moment all types of future dates till the end of the year), so the majority of users don't need to "fiddle" but for these more specific questions, they are necessary.

I think we should not show in the widget dividend payments for instruments that the user does not hold.

That wasn't intended, so if it currently does I need to change this (after writing a test case for it of course ;-)

Then about the layout of the widget: I propose to make it one line per instrument (security) similar to the list of earnings. The line should include payment and ex-date and the dividend payment and the (expected) total payment account. That requires understanding the number of shares held at the ex-date.

To be honest, I see usage for both types of widgets, so your proposed widget should be an additional one rather than "my" widget being changed to this new layout. I can do that as part of this PR but I think "out there" people are craving for this already, especially with the already started dividend season, so I think a new PR for the new widget should be in order as well.

Then about the sizing of the widget: It took me some time to understand that it has a fixed size and that (some) entries are just missing. I would propose to do it similarly to the EarningsListWidget which just uses as much space as needed.

I've derived my widget from your abstract class so it's all your fault!!1! ;-) I can change it to behave like EarningsListWidget. Should I change "follow up" and "limit exceeded" as well while on it?

* Right click on an upcoming payment and create the actual transaction

That might look nice in the first place but you'd still end up using the PDF-import, because that event doesn't contain any information about taxes, fees, conversion rates, etc. that you'd need to provide manually in that case. And that's just the simple stuff, it gets more interesting with stock being held in multiple depots.

* Allow editing of the dividend events (right now one must use DivvyDiary, but I think it is a valid use case to allow adding such events)

If we add this, the DividendEvent should be extended to keep the record date as well (which is often different from the ex-date)

What do you think?

I think you know now ;-)

@buchen
Copy link
Member

buchen commented Apr 28, 2024

I am not sure about the "period start" and "period end" selection options in the dashboard widget. Is this selection really relevant?
I think yes. When creating the widget I've imagined questions that users might want to get answered by this:

When are the next payments to be expected?
What payments have happened so far
When do I have to buy additional stocks at the latest to still get the dividend

The first and the third question can be answered with the widget having the heuristic that I proposed (show all in the future + last 3 months that are not found).

The second question is a different "thing". I believe the user is not interested in what dividend payments happened in principle, but which payments actually happened. Thinking about it, I wonder if this really is a separate new widget or whether we should extend the earnings list widget with (optionally) future payments.

@kimmerin
Copy link
Contributor Author

I believe the user is not interested in what dividend payments happened in principle, but which payments actually happened.

Well... as a user I can tell you that I'm interested into both ;-) I'd like to know when to expect sudden drops of prices (ex date) and when to expect money on my account (payment date). For the former the dividend amount for a single share is relevant because that's the expected drop in price. For the latter, both amounts (per share and all) might be of interest dependent on the context of the widget's usage.

I agree that for the information about the expected dividend amounts it makes sense to extend the existing widget that shows the earnings of a particular year to show an additional sum per month and entries for expected dividends, i.e. dividends that you owned - in lack of a record date - one day before the ex-dividend date. This should be done in another PR, together with a couple of UX-improvements (I'm not going into detail here, because that would be off topic).

TL;DR: I still see use for this widget, so I'm going to add the account filter like in the transaction-widget. Concerning the time range: What about removing the range for the future, i.e. all future dividend-events are shown and keeping the "from-range" but with the possible values "None", "1 Week", "1 Month", "1 Quarter", to allow people to see dates in the past that they otherwise missed because they don't fire up the app on a daily basis?

kimmerin added 2 commits May 1, 2024 14:44
if the number of entries exceed the maximum number of "allowed"
securities. Use color coding to show the different event types and make
the date more prominent
@kimmerin
Copy link
Contributor Author

kimmerin commented May 1, 2024

@buchen I've redesigned the widget to always show all upcoming events and changed the presentation so that only one line per security is needed so more entries fit into the same space.

don't leak any personal info) if discreet mode is active. Rmove currency
from value if it the same as the client's base currency
Copy link
Member

@buchen buchen left a comment

Choose a reason for hiding this comment

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

I started to "divide and conquer" this pull request. I now cherry-picked the new dividend columns as they are fairly straightforward and separate from the widget. The code comments are apply only to the new columns. I have rebased and merged this functionality.

One more comment: it helps me reviewing/handling the pull requests if you rebase the pull request instead of merging changes from the master into the pull request. Rebasing means you review your changes against the master - that is easier than the other way around.

For my next block of uninterrupted time, I'll look at the widget.

buchen pushed a commit that referenced this pull request May 10, 2024
Issue: #3927
Signed-off-by: Lothar Kimmeringer <[email protected]>
[cherry-picked new columns; rebased to master]
Signed-off-by: Andreas Buchen <[email protected]>
doesn't contain dividend columns anymore (made no sense there)
@kimmerin
Copy link
Contributor Author

@buchen This PR now shows up as one with to-be-resolved conflicts. You've said in a previous post that you'll have a look into the widget. Should I resolve these conflicts or is this going to be done by you anyways when deciding on the non-merged part of this PR?

@buchen
Copy link
Member

buchen commented Jun 5, 2024

@kimmerin

Should I resolve these conflicts or is this going to be done by you anyways when deciding on the non-merged part of this PR?

I already have a local change that works on top of the master branch, so there is no need for you do push changes here. Unfortunately, I am quite busy these days. And if I only have a couple minutes here and an hour there, it is sometimes hard to tackle the big rocks. And then I review and merge smaller changes because those I can intellectually process in that time. After this weekend, I hope it will become better.

Sorry for the inconvenience and thanks for your patience.

@kimmerin
Copy link
Contributor Author

kimmerin commented Jun 8, 2024

@buchen No problem. I just wanted to make sure that this PR isn't in limbo because everybody is waiting for the others to do something ;-)

@stoeggich
Copy link

sorry for asking but what happens next? will the merg come?

@kimmerin
Copy link
Contributor Author

@stoeggich I'm curious myself but it's summer vacation time here in Germany, so things can slow down because of this.

@kimmerin
Copy link
Contributor Author

kimmerin commented Nov 2, 2024

Hi @buchen, seven months into this ;-), is there a chance this PR is going to be integrated fully or is there something you need from me in order to do so, that I'm not aware off, leading to this stand-still?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

4 participants