add Grouped Accounts view (more visibility for account filters) #4466
Conversation
which is dangerous
previously, all center tooltip of HoldingsPieChartView were "Statement of assets-Holdings" irrelevant of the used filter
Fix proposed. It also changes the main HoldingsSWT chart view : the center tooltip name is updated with the selected filter. Before in HoldingsPieChartView : amount was updated but label was not : When no filter is selected, "Entire portfolio" label is used. It is possible to override it to keep the "Statement of Assets - Holdings" in this case. |
Proposition of fix : hide the holding chart when the pane is open while no account is selected 1666c5f |
| ((Portfolio) portfolioOrGroupedAccount).getName()); | ||
| chart.refresh(snapshot); | ||
| } | ||
| else if (input instanceof ClientFilterMenu.Item) |
There was a problem hiding this comment.
Just thinking aloud: maybe the selection is the "Clientfilter" (not some ClientFilterMenu.Item). And instead of sending a portfolio, we can also "just" send a ClientFilter that only includes this portfolio. That would reduce the many if/else in the panes.
| public String getNote() | ||
| { | ||
| return note; | ||
| } |
There was a problem hiding this comment.
I don't think we should introduce a note. Or if we do, we should also persist it (but that is more complicated due to the condensed serialization). The filter just does not have a note.
| groupedAccountColumns = new ShowHideColumnHelper(GroupedAccountListView.class.getSimpleName() + "@top2", //$NON-NLS-1$ | ||
| getPreferenceStore(), groupedAccounts, layout); | ||
|
|
||
| Column column = new NameColumn("0", Messages.ClientEditorLabelClientMasterData, SWT.None, 100, getClient()); //$NON-NLS-1$ |
There was a problem hiding this comment.
Historically, the columns did not have a name. It was just the order as configured in the menu. That was a problem, because I could only add columns add the end. Therefore, some years back I added a name. To be backwards compatible, some columns still have the index as name. But now we can give proper names to the column.
|
@mierin12 great first change for grouped accounts. I like it. I have created a separate branch feature_grouped_accounts and also a pull request #4476. For now, I have squashed your commits into one and added a couple of additional commits. I will squash them as well, but while reviewing, it might help to look at the changes individually. I think it should be possible for you to create a pull request for this branch as well - in case you notice more things. Most likely, I will not find time to work on this the next couple days. I am closing this PR to continue in the new one. |
Follow up from #4458
This is what I reached. To be carefully tested!
Some notes:
I put in draft because I feel some optimization of duplication could be made, but it is ready for review.
In addition to this, to be tested to see if the UI feels "right". Is it disturbing to have a Tree while the Cash and Security Accounts are Tables ? Is the goal of Grouped Account clear enough (not used for transactions as opposed to the Cash/Security Account).
Bug:
-I am just seeing there is a bug for the tooltip in the center of the Holdings chart in this new view.ok-Second bug I am just noticing, more complex I think : the Note field for Grouped Account nodes is not remembered when changing view.
-when no grouped account exists, the bottom pane for Holding is not good lookingok