-
Notifications
You must be signed in to change notification settings - Fork 613
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
base: master
Are you sure you want to change the base?
Conversation
upcoming dividends and their amounts
before the read of the language files takes place
using the actual localized values themselves
Hello @kimmerin Perhaps you could format the source code before we continue working on it. That would help us a lot. We should also keep the translation correct and complete it with deepl.com and correct the capitalization. We should also take care to keep the translations consistent. Example: We should also take care to keep the variable names consistent. But it looks good... formatting the source. 👍🏻 |
There was a problem hiding this 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?
....abuchen.portfolio.ui/src/name/abuchen/portfolio/ui/views/columns/DividendPaymentColumn.java
Outdated
Show resolved
Hide resolved
....abuchen.portfolio.ui/src/name/abuchen/portfolio/ui/views/columns/DividendPaymentColumn.java
Outdated
Show resolved
Hide resolved
....abuchen.portfolio.ui/src/name/abuchen/portfolio/ui/views/columns/DividendPaymentColumn.java
Outdated
Show resolved
Hide resolved
....abuchen.portfolio.ui/src/name/abuchen/portfolio/ui/views/columns/DividendPaymentColumn.java
Outdated
Show resolved
Hide resolved
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 ;-)
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
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.
To be honest I don't understand your example. Can you rephrase?
I cah change
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. |
…columns/DividendPaymentColumn.java Co-authored-by: Alexander Ott <[email protected]>
…columns/DividendPaymentColumn.java Co-authored-by: Alexander Ott <[email protected]>
No problem... good job 👍🏻 |
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).
@Nirus2000 checked in some small improvement today. 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) |
There was a problem hiding this 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?
...portfolio.ui.tests/src/name/abuchen/portfolio/ui/views/dashboard/DividendListWidgetTest.java
Show resolved
Hide resolved
name.abuchen.portfolio.ui/src/name/abuchen/portfolio/ui/views/dashboard/DividendListWidget.java
Outdated
Show resolved
Hide resolved
name.abuchen.portfolio.ui/src/name/abuchen/portfolio/ui/views/dashboard/DividendListWidget.java
Outdated
Show resolved
Hide resolved
name.abuchen.portfolio.ui/src/name/abuchen/portfolio/ui/views/dashboard/DividendListWidget.java
Show resolved
Hide resolved
name.abuchen.portfolio.ui/src/name/abuchen/portfolio/ui/views/dashboard/WidgetFactory.java
Outdated
Show resolved
Hide resolved
I think yes. When creating the widget I've imagined questions that users might want to get answered by this:
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.
That wasn't intended, so if it currently does I need to change this (after writing a test case for it of course ;-)
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.
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?
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.
If we add this, the DividendEvent should be extended to keep the record date as well (which is often different from the ex-date)
I think you know now ;-) |
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. |
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? |
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
@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
There was a problem hiding this 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.
....abuchen.portfolio.ui/src/name/abuchen/portfolio/ui/views/columns/DividendPaymentColumn.java
Outdated
Show resolved
Hide resolved
....abuchen.portfolio.ui/src/name/abuchen/portfolio/ui/views/columns/DividendPaymentColumn.java
Outdated
Show resolved
Hide resolved
....abuchen.portfolio.ui/src/name/abuchen/portfolio/ui/views/columns/DividendPaymentColumn.java
Outdated
Show resolved
Hide resolved
name.abuchen.portfolio.ui/src/name/abuchen/portfolio/ui/views/PortfolioListView.java
Outdated
Show resolved
Hide resolved
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)
@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? |
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. |
@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 ;-) |
sorry for asking but what happens next? will the merg come? |
@stoeggich I'm curious myself but it's summer vacation time here in Germany, so things can slow down because of this. |
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? |
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