Feature grouped accounts

[buchen]

Continuation of #4466

(possible) open items:

• double-click editing the names does not mark the client as dirty (potentially no saving). And: does it work with editing the name of the cash or securities account?
• what about the ordering of the accounts within the group?

[mierin12]

Hello @buchen many thanks for the review. I will look into it.
Quickly some notes for later:
I think there is an issue with the TaxonomyView, there is the typo line 119 where SecuritiesPerformanceView.class is used, but even when using TaxonomyView.class.getSimpleName() + "-" + taxonomy.getId() I still have issues. Not super sure, but is creating a TaxonomyModel with a filteredclient ok ? The previous implementation keeps all accounts in the model but put a 0 value on them, is that it ? While now it is completely filtering them ? But then I wonder if by creating a TaxonomyModel with a filteredClient some mismatch happens later on such as in addUnassigned

To trig : filter, quit the filtered taxo view, then go back to it.

Regarding the Named interface : the goal was to make the label/name appear in the Information Pane : a Named adaption is used at some point to print the label. without it :


I think have a Name here is important as not all change of selection is updating the information panes (only a newly selected grouped account is updating the information pane, a sub-account selection does nothing).
In InformationPane.java :

/* package */ void setInput(Object input)
    {
        currentInput = input;

        Named named = Adaptor.adapt(Named.class, input);
        label.setText(named != null ? TextUtil.tooltip(named.getName()) : ""); //$NON-NLS-1$

If it is better to update setInput, it can be modified there instead.

Good catch for the two open points. I will look into it.
Well I think it was even worse than not making the client dirty : even after a save, close and reopen, the name is not remembered 😅 It is saved if the view is changed before saving & closing (as dispose is called).

For the ordering of the accounts : I think the Taxonomy drag & drop way could be reused. There it is possible to drag elements and not just main nodes. I went the easy way and used the drag&drop methods from EditClientFilterDialog which was straightforward instead of taxonomy. Otherwise it is possible to sort by the Value column.

[buchen]

Not super sure, but is creating a TaxonomyModel with a filteredclient ok ?

Yes, you are right. Now I remember. The problem is, that a filtered client may not have all instruments and then it does not work to resolve the assignments. I'll fix that. (We are "on the road" this weekend so it will take a couple days).

Regarding the Named interface : the goal was to make the label/name appear in the Information Pane

Ok, okay, understood. It is unfortunate that the Named (the name) and the Annotated (the note) depend on each other.
What I found immediately confusing is that I can enter a note to the group and then it is gone again. The "revert" is a separate commit so we can remove that one again. I also want to look at the code: maybe it is easy to separate Named and Annotated.

as not all change of selection is updating the information panes (only a newly selected grouped account is updating the information pane, a sub-account selection does nothing).

To be honest, I found that also confusing. Particularly, if I select an account, it does not mean that the group of this account is shown. It could be an arbitrary other one.

Why not show infos for the group, if the group is selected? and the infos for the account, if the account is selected? Then one can easily check for example in the holdings chart where does the position come from.

[mierin12]

What I found immediately confusing is that I can enter a note to the group and then it is gone again. The "revert" is a separate commit so we can remove that one again. I also want to look at the code: maybe it is easy to separate Named and Annotated.

I have started some propositions there : master...mierin12:portfolio:pr-4476-modified
The information pane name can be managed by a workaround without Named.class.
For the undirty client after a name changed, I think the listener associated with StringEditingSupport was not ok. I also updated onModified to include the save of modified filter.
Now, as you said, the named changes only works on the grouped account node and not sub element account because of new StringEditingSupport(ClientFilterMenu.Item.class, "label") I am not sure how to add two different StringEditingSupport ? Or we are back at using the Named.class and then column.getEditingSupport().addListener(this); would work for all elements. But this is bringing back the Note topic indeed.

To be honest, I found that also confusing. Particularly, if I select an account, it does not mean that the group of this account is shown. It could be an arbitrary other one.

Why not show infos for the group, if the group is selected? and the infos for the account, if the account is selected? Then one can easily check for example in the holdings chart where does the position come from.

Yes I was not super satisfied by this either. I will look into it. The challenge is that the different panes are not the same depending on if a Cash Account or a Security Account or a Grouped Account is selected.

[mierin12]

Why not show infos for the group, if the group is selected? and the infos for the account, if the account is selected? Then one can easily check for example in the holdings chart where does the position come from.

Proposition done in https://github.com/mierin12/portfolio/tree/pr-4476-modified
I think it works, with the exception that for Balance Chart, buttons are also created for Account Balance chart, but shouldn't.
A Holding chart for Account is not very useful but it was easier to adapt to it than to hide the pane (I do not see how to do that).

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.

I think it is possible for Holdings and Statement of Assets, but for Balance Chart this seems less trivial. I reused c4347d2 and its needs more info than clientFilter.

[mierin12]

what about the ordering of the accounts within the group?

I have proposed an extension of the drag and drop. To be noted : it has no effect if a column is sorted (such as the Balance column or Note column). Should a drag & drop action reset the sorted state of a column ?
There is still a little bug, I am not able to distinguish if the drop target is in the same Grouped Account as the dragged account. I assumed we want to prevent drag&drop between different Grouped Accounts. It is prevented except for the case where : drop target is a account which is also present in the origin Grouped Account.

In addition, maybe the Name column could be sortable but only for the sub elements, while top level Grouped Account are only movable through the d&d.

[buchen]

I have proposed an extension of the drag and drop. To be noted : it has no effect if a column is sorted (such as the Balance column or Note column). Should a drag & drop action reset the sorted state of a column ?

Good point. I agree. Drag & Drop does not make sense if it then creates no change because the values remain as sorted by the column. Maybe drag & drop for the accounts (cash and portfolio) does not make sense...

What about this behavior:

• The "groups" (= client filter) remain always exactly in the order that the users gives them - regardless of how the "Name" column is sorted. The very same order is used in the drop down menus when selecting a "group/filter".
• The accounts (cash and portfolio) are sorted by the column - either name or balance or note or whatever. And they cannot be dragged and dropped.

[mierin12]

Or there is also the behaviour of the Taxonomy which I think is quite good, it allows both drag and drop and sorting. There, the sort is not a "continuous" sort but a one time action, which therefore does not prevent a future drag & drop.
One difference maybe is that it is ok for Taxonomy to drag & drop between "parent node", while I was thinking that for Grouped Accounts the drag & drop of sub account should only happen within the same Grouped Account, but maybe this too restrictive for no real reason.

[buchen]

while I was thinking that for Grouped Accounts the drag & drop of sub account should only happen within the same Grouped Account, but maybe this too restrictive for no real reason.

Well, in the groups you can have the same account in multiple cash groups. If we allow drag and drop there, you also have to take care of this. For example, if you drop the account in a group where the account is already present, does it just remove the account from the previous group?

If we do drag and drop, then I would say we should split the view. On the left side are the groups, on the right side is a list of all accounts. And one can drag and drop an account from the left side into a group.

Within the group, I do not think we need an order. Or, more precisely, we should just use the order that is given from ordering a particular column. I am not sure I want to apply the taxonomy behavior here. It is custom ordering which is then persisted in the XML. In the taxonomy, the thinking was that uses want a specific order of classifications and want to order securities (I do that often when thinking about rebalancing - my "favorite" of a category is on the top). But a group does not need an order.

I'll have a look at your additional commits later this week.

[mierin12]

Hello @buchen, I apologize for the delay ! Understood, I have now revert the "taxonomy drag& drop" :

• sub account can be ordered through the different columns' sorting, but not drad&drop
• main node grouped account can be drag & drop

I think I have also fixed the TaxonomyView.
All the commits are here:
master...mierin12:portfolio:pr-4476-modified

I think it is looking good from UI point of view ! Maybe the code can be optimized though.

Maybe a detail still, in Taxonomy View : the "non null filter" stays on even outside of the list and rebalancing view (only views where it is used). I am not sure if it should grey out when switching view or if this would be a strange behaviour.

There is an hardcode limit of 30 grouped account. I found this : #738 (comment)
It went to 5 to 10 to 30. Is this max number still required ?

[buchen]

Hello @buchen, I apologize for the delay ! Understood, I have now revert the "taxonomy drag& drop" :

Hey, no need to apologize. If somebody has to apologize than it is me. I just did not find a big enough chunk of time to look into this one. It might not work out during the week. Hopefully next weekend.

[buchen]

There is an hardcode limit of 30 grouped account. I found this : #738 (comment)
It went to 5 to 10 to 30. Is this max number still required ?

Good catch. I think we can get rid of this now.

It was back when there was no way to delete the filter. Plus the menu would become arbitrarily long.

[mierin12]

Hello Andreas, I have mode some comments (I finally learned how to use the code review feature).
My notes and summary:

Main new comments :

• There was one bug with the Taxonomy view, where the active ClientFilter was not taken into account when initialiazing the view. I have made a proposition of fix in the review.
• Proposition to grey-out the inactive accounts of the Grouped Filters

Then, some notes to sum up the current UI behavior (I tend to forget what is the current behavior, as there was some back & forth):

SORT and DRAG & DROP :

• Main nodes can be ordered through drag & drop and therefore not by column sort.
• On the opposite, sub-elements are not dragable, but are sortable by column sort.
  -> Ok with this behavior ?
  -> Should a hint be provided that drag & drop is available in this view ?

RENAME THROUGH DOUBLE CLICK :

• Double click on the main nodes allows to rename them and makes the client dirty.
• Double click on sub accounts does not allow to rename them.
  -> I do not know how to deal with it. We can't have two different StringEditingSupport, right ?
  new StringEditingSupport(ClientFilterMenu.Item.class, "label").setMandatory(true).addListener(this).attachTo(column);
  gives the current behavior, while :
  new StringEditingSupport(Named.class, "name").setMandatory(true).addListener(this).attachTo(column);
  allows to rename the subaccounts but not the main ones.
  If ClientFilterMenu.Item is made into a Named class, this can work, but the Named class brings Annotated.

NOTE COLUMN :

• Notes are active for sub-element, but not for the main nodes.
  -> Ok with it ?
  -> Maybe the column could be simply removed and not available at all

EXPANDED/COLLAPSE STATE :

• Is remembered when switching view or when reopening a closed file.
  -> Would a "Collapse all" action be interesting ?

LABELS :

• There are some MessageLabels can we can further modify to complete the change from "filter" to "grouped accounts" labels.
  -> If the semantic change is confirmed, I can propose the changes.

BOTTOM PANES :

• Statement of Asset : I tried to shoehorn Account in this, I think this work, but I am assuming some code improvement could be existing, lots of if/else as you said.
• Holding Chart : I tried to shoehorn Account in this, I think this work, but I am assuming some code improvement could be existing, several of if/else as you said. It is adding a SnapshotName attribute to ClientSnapshot. To be noted that a Holding chart for an Account is simply 100% holdings.
• Balance Chart : this one is more complex, first a new Pane class has been created to see if the input is an Account or not. If yes, we use the AccountBalancePane, otherwise the PortfolioBalancePane. PortfolioBalancePane and the PortfolioBalanceChart are modified to take into account ClientFilterMenu.Item as input.

CODE DUPLICATION :
I used in GroupedAccountListView several methods from EditClientFilterDialog (drag and drop, insert/delete elements actions). The methods are quite close. I am not sure how to optimize/factorize this (and if we should).

[mierin12]

Suggested change

	this.clientFilterDropDown = new ClientFilterDropDown(getClient(), getPreferenceStore(),
	TaxonomyView.class.getSimpleName() + "-" + taxonomy.getId(), filter -> {
	setInformationPaneInput(null);
	Client filteredClient = filter.filter(getClient());
	setToContext(UIConstants.Context.FILTERED_CLIENT, filteredClient);
	model.updateClientSnapshot(filteredClient);
	updateTitle(getDefaultTitle());
	});
	this.model = new TaxonomyModel(factory, getClient(), taxonomy);
	Consumer<ClientFilter> listener = filter -> {
	setInformationPaneInput(null);
	Client filteredClient = filter.filter(getClient());
	setToContext(UIConstants.Context.FILTERED_CLIENT, filteredClient);
	model.updateClientSnapshot(filteredClient);
	};
	this.clientFilterDropDown = new ClientFilterDropDown(getClient(), getPreferenceStore(),
	TaxonomyView.class.getSimpleName() + "-" + taxonomy.getId(), listener); //$NON-NLS-1$
	// As the taxonomy model is initially calculated in the #init
	// method, we must recalculate the values if an active filter
	// exists.
	if (this.clientFilterDropDown.hasActiveFilter())
	listener.accept(this.clientFilterDropDown.getSelectedFilter());
	this.clientFilterDropDown.getClientFilterMenu().addListener(filter -> updateTitle(getDefaultTitle()));

[mierin12]

I think the Taxonomy view was not initialized correctly while a ClientFilter was active (despite an active client filter, the whole portfolio was still used by taxonomy view. It worked while changing the selection but not at initialization) :
1-

if (this.clientFilterDropDown.hasActiveFilter())
       listener.accept(this.clientFilterDropDown.getSelectedFilter());

fixes this (and was already there before), we just need this.model to be non null, so I moved it on top
2- however after this change, the updateTitle in the listener fails at initialization (Label title in AbstractFinanceView is null) so I am adding it outside of the initial listener

I think it works now, Consumer and ClientFilter needs to be imported again, and the second this.model has to be removed (but i did not manage to review unmodified lines), so like this: 7a21b16

[mierin12]

Suggested change

	});
	@Override
	public Color getForeground(Object e)
	{
	boolean isRetired;
	switch (e)
	{
	case Portfolio portfolio -> isRetired = portfolio.isRetired();
	case Account account -> isRetired = account.isRetired();
	default -> isRetired = false;
	}
	return isRetired ? Display.getDefault().getSystemColor(SWT.COLOR_DARK_GRAY) : null;
	}
	});

[mierin12]

Grouped Accounts can possess both active and retired accounts : proposition to show the retired accounts as gray, same as in Portfolio List/Account List. it needs import org.eclipse.swt.graphics.Color

[mierin12]

Suggested change

	LocalDate start = now; // init to be changed ?
	LocalDate end = now; // init to be changed ?
	LocalDate start = now;
	LocalDate end = now;

[mierin12]

This is from me, maybe there is a better approach for this method getInterval. Concatenating all transactions of all portfolios in a single list and then take the min/max of this list ?
Could account and portfolio transactions be combined by
groupedAccount.getFilter().filter(client).getAllTransactions(); ?
But then Collections.sort(tx, Transaction.BY_DATE); fails.