moneymanagerex / moneymanagerex

Money Manager Ex is an easy to use, money management application built with wxWidgets
http://moneymanagerex.org
GNU General Public License v2.0
1.8k stars 279 forks source link

Organize Category dialog does not scroll newly added items into view #5056

Closed tactilis closed 2 years ago

tactilis commented 2 years ago

MMEX version:

Operating System:

Description of the bug

The work done for #4989 has improved the Organize Categories dialog so that the category tree collapsed/expanded state is now remembered when it refreshes following the addition of a category/subcategory.

However the tree still scrolls to the top and highlights the root node (Categories) when the new item is added. The user then has to scroll to view the new item to check that it was entered OK, and perhaps to enter more subcategories within the category.

It would be more helpful if:

  1. The list is refreshed and scrolled to always show the new item.
  2. The new item is highlighted so that it can be easily identified.

Reproduction

Is the bug reproducible?

Reproduction steps:

Add a new subcategory to a category near the bottom of the tree:

image

Expected result:

The new item is visible and is highlighted:

image

Actual result:

The list is displayed with the root node highlighted.

image

Additional information

Note that when an item is deleted, the tree does not scroll and the item that was immediately above the deleted item is highlighted. This makes it easy for the user to see the change they have made, and perhaps to delete more subcategories within the category. This behaviour should be retained.

whalley commented 2 years ago

Hmmm, thought I had tested all this and just tried this on MacOS. It seems to work.

https://user-images.githubusercontent.com/487345/189175687-9714e53c-f967-46d4-9d31-3d9dfd221970.mp4

tactilis commented 2 years ago

Just tried this on MacOS. It seems to work.

I'm testing 1.5.21-Beta.1 commit: 5e8eff3 (2022-09-08), which includes your Show All fixes, so is the correct version.

Some odd tree widget behaviour on Windows not actioning the highlighting you are doing?

whalley commented 2 years ago

I can't see why it does not work in Windows. https://github.com/moneymanagerex/moneymanagerex/blob/ee98b3fd393180df9e465f4349d16edf93397c62/src/categdialog.cpp#L255

@vomikan Are you able to investigate.

n-stein commented 2 years ago

https://github.com/moneymanagerex/moneymanagerex/blob/ee98b3fd393180df9e465f4349d16edf93397c62/src/categdialog.cpp#L404

https://github.com/moneymanagerex/moneymanagerex/blob/ee98b3fd393180df9e465f4349d16edf93397c62/src/categdialog.cpp#L434

https://github.com/moneymanagerex/moneymanagerex/blob/ee98b3fd393180df9e465f4349d16edf93397c62/src/categdialog.cpp#L177-L184

The call to fillControls at the end of OnAdd resets the selected item ID to the root node. Just removing the call to fillControls() on 404 and 434 leaves the parent expanded but still selected. If you want the newly added one to be focused you need to add

m_treeCtrl->SelectItem(tid);
m_treeCtrl->SetFocus();
whalley commented 2 years ago

The call to fillControls at the end of OnAdd resets the selected item ID to the root node. Just removing the call to fillControls() on 404 and 434 leaves the parent expanded but still selected. If you want the newly added one to be focused you need to add

m_treeCtrl->SelectItem(tid);
m_treeCtrl->SetFocus();

Yes, that works on MacOS fine.

FYI, in FillControls the selected item ID was recalculated here: https://github.com/moneymanagerex/moneymanagerex/blob/882bdda4f11c8e694c937a600f1ec15d1e3e5b3d/src/categdialog.cpp#L205 and here: https://github.com/moneymanagerex/moneymanagerex/blob/882bdda4f11c8e694c937a600f1ec15d1e3e5b3d/src/categdialog.cpp#L224

and then selected and made visible here https://github.com/moneymanagerex/moneymanagerex/blob/882bdda4f11c8e694c937a600f1ec15d1e3e5b3d/src/categdialog.cpp#L254-L255

This, oddly, worked on MacOs but not Windows...

Anyway, fixed now. Thanks for the PR.

tactilis commented 2 years ago

Confirmed working on Windows now. Thanks

whalley commented 2 years ago

Slight issue. By not calling fillControls() the newly added item is not sorted in the list.

n-stein commented 2 years ago

Good catch. Fixed by sorting children of m_selectedItemId.