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

add Grouped Accounts view (more visibility for account filters) #4466

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

mierin12
Copy link
Contributor

@mierin12 mierin12 commented Jan 11, 2025

Follow up from #4458

This is what I reached. To be carefully tested!

Some notes:

  • Drag & Drop in the tree (copy paste from EditClientFilterDialog)
  • I have kept the EditFilter dialog (still a useful shortcut). What is important then is that both the new view and the Edit dialog are synchronized. I think this is the case.
  • New and Edit can be called from the + icon for the new Grouped Account view
  • Add element and delete element on the Tree items (~copy paste from EditClientFilterDialog)
  • Expand state of the tree is remembered (copy paste from Taxonomy's AbstractNodeTreeViewer)
  • Three bottom Panes, I tried to adapt the existing ones to avoid duplication. Maybe there are cleaner way to adapt them.
  • I tried the 1px border 16 * 16 icon and the 2px border for 32 * 32 icon, but I think I may not use the correct one, it feels smaller
  • If confirmed that we go this way, there are still "filter" vs "grouped account" inconsistencies to be standardize. I did not do the change in case other names option are found.
  • proposition of a column with the cash balance for all of them. If cash account currency is not the global one, the specific currency is used. But for sorting this column, the converted value is used. Is this the correct behavior ?
  • what should be the behavior if a cash/security icon is selected in the tree ? Should the bottom panes be updated ? I think no, but maybe it could be updated to their parent Grouped Account.
  • optionally in the second commit : removal of the menu option allowing to clear all the filter. Seems quite dangerous !

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).

Capture d’écran 2025-01-11 025545
Capture d’écran 2025-01-11 030431

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 looking ok

previously, all center tooltip of HoldingsPieChartView were "Statement
of assets-Holdings" irrelevant of the used filter
@mierin12
Copy link
Contributor Author

mierin12 commented Jan 11, 2025

I am just seeing there is a bug for the tooltip in the center of the Holdings chart in this new view.

Fix proposed. It also changes the main HoldingsSWT chart view : the center tooltip name is updated with the selected filter.
This is adding a non mandatory snapshotName property in ClientSnapshot, maybe more null check could be added, or a default name.

Before in HoldingsPieChartView : amount was updated but label was not :
2025-01-11 14_13_46-Portfolio Performance
After :
2025-01-11 14_13_17-Portfolio Performance

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.

@mierin12
Copy link
Contributor Author

-when no grouped account exists, the bottom pane for Holding is not good looking ok

Proposition of fix : hide the holding chart when the pane is open while no account is selected 1666c5f

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant