-
Notifications
You must be signed in to change notification settings - Fork 642
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 @abuchen, after nearly a year, I assume this PR is never going to be merged. May I ask what's reason behind it? Because it seemed to me that there's demand out there to have an overview in the dashboard rather than just the columns. |
Sorry, yes. So here again ;-) Hi @buchen, after nearly a year, I assume this PR is never going to be merged. May I ask what's reason behind it? Because it seemed to me that there's demand out there to have an overview in the dashboard rather than just the columns. |
@kimmerin: Since this patch, there were other developments for PP, e.g. a generic event list widget for dashboard: #4431 . That widget can show any type of event, not just dividend events (a culprit is that somebody now needs to add more types of events). Likewise, there's already a type of column which can be used to track date of any event - that's custom attribute of "Date" time, which can be shown as "Date - days between" column, which can be sorted upon and allow users to pretty quickly see what're upcoming (or recently) passed events are. Of course, it requires some legwork from users to set up and tie it all together, but then it allows to track any kind of event, be it dividend payments or ex-dates, earnings date, CEO birthday, or whatever. And it of course would be nice to have automagic support for dividends, but is it really easy to tie it all together and will it really work the way different people expect? |
OK, I've tried that in the dashboard. Since the original issue is closed, here is some feedback: The widget itself is way to greedy in terms of real estate. The type of event is selected and fixed, i.e. it doesn't need to be repeated for each entry next to the date. If multiple entries share the same date, they should be grouped together. Concerning the ex-dividend date: Currently they are missing completely as selectable event-type simply because the event-type "dividend" contains two dates and this generic widget only supports to be configured to show one of them. You aren't able to configure the widget to show dates of two types of events (e.g. ex-date and payment-date) in one widget, so you have to - again - use up precious real estate on your dashboard to see both dates. So while this issue allows some solution to see upcoming dividend-dates I still see some use in the widget I've created because it's optimized to show these informations in a more optimized way concerning required space. But it's not up to me and if your widget is considered sufficient for the people who voiced a demand for this kind of thing, @buchen should just close this issue for good so people don't wonder why it's in limbo for so long. |
That's absolutely true. It's just an initial version of a generic event list widget, based on another similar list widget ("Securities: Date reached"). That's in contrast with #2623 , which was a big code-drop whose author claimed that he solved all problem events, past, present, or future, but which was never merged. You can see if there's any similarity with your patch or no.
There actually can be multiple event types shown, so while it shouldn't be shown redundantly, it still should be shown as needed.
Right. Except that, based on the above, they should be grouped by date and event type. That's exactly the change I've made in my fork for the "Securities: Date reached" widget. As you remember, that was a prototype for the event list widget. My idea was to keep them as much as possible in sync, so the improvements to one can be easily ported to another. Here actually @buchen, the maintainer, surprised me, and instead of going ahead with the simple initial version proposed, he went ahead to improved it beyond the functionality offered by the "Securities: Date reached" widget. I don't know if that was a case of premature featurisation and actually complicates the further evolution of both types of widgets. All in all, I hope the idea I advocate is clear: instead of making big codedrops which are unrealistic to be merged smoothly by any reasonable maintainer, it's better to do small changes/additions incrementally, step by step. Next step (of many yet to do!) on this way is submitting the "groupping" patch for "Securities: Date reached" (not even event list widget). And you won't believe why I didn't do that yet - change of that nature would require "before" and "after" screenshot. I don't have permission to show any real data, and just too lazy and can't make me to prepare sample data to make a screenshot otherwise. It's actually interesting to make some comparison matrix:
There's currently no "dividend" event type, currently there's only adhoc "divvydiary.com" proprietary event type. Which is useless for anyone who doesn't use divvydiary.com. If you think, it's the same problem as discussed above - people prefer to dig narrow and deep, instead of wide. Wide necessarily mean shallow, but at least other people can continue and extend based on it, whereas narrow adhoc solutions oftentimes just complicate further work.
It's absolutely not sufficient, doesn't claim to be such, and intended for long and progressive elaboration by different people. I also wish that @buchen gave specific instructions what to do to get patches merged as fast as possible, but so far I didn't see that. Whereas I had to figure out answers to such questions, both for my own patches and others', and just share my thoughts with a fellow contributor. |
There is no real similarity in my eyes but YMMV.
You're aware under what PR you've responded? ;-) If you want to see a sceenshot: https://forum.portfolio-performance.info/t/widget-gesucht-bekannte-kunftige-dividendenzahlungen/28020/4
It's an event type that is currently only been populated with data by divvydiary. If tomorrow there were another source the created event type should be the same, so that widgets/columns/... don't need to be changed in order to work as they did before. Separating ex-date and payment date to two different event-types I don't see as practical because as a user I'd like to see both dates at the same time and you will create confusion with securities with regular dividends, where the payment of the last dividend happens after the ex-date of the next one (Prospect Capital as an example).
Let me rephrase then: If the new feature covers the features of my PR here, then - by all means - close this one because it's no longer requred and that way it's clearly documented for everybody. |
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