Recent

Author Topic: [Solved] ShellTreeView Node Expand: why second expand is slower than first one?  (Read 834 times)

d7_2_laz

  • Sr. Member
  • ****
  • Posts: 336
Coincidentally seen (Win x64): open (eg. via the ShellView demo) a highly populated folder (eg. Windows\WinSxS).
Collapse it. Expand it again ... appears to be much slower than the first expand. Why?
Would have expected that the second expand runs instantly, as the node is already populated.
Why the node is populated each time? (I assume there is a reason)

Changed simply for testing CanExpand  with instant second expand:

Code: Pascal  [Select][+][-]
  1. function TCustomShellTreeView.CanExpand(Node: TTreeNode): Boolean;
  2. var
  3.   OldAutoExpand: Boolean;
  4. begin
  5.   Result:=inherited CanExpand(Node);
  6.   if not Result then exit;
  7.   OldAutoExpand:=AutoExpand;
  8.   AutoExpand:=False;
  9.   BeginUpdate;
  10.   try
  11.     Result := True;
  12.     if Node.Count = 0 then begin     // Added simply for test
  13.         //Node.DeleteChildren;       //  Not becessary regarding the condition
  14.         Result := PopulateTreeNodeWithFiles(Node, GetPathFromNode(Node));
  15.     end
  16.     else  Result := True;           // Added
  17.     AutoExpand:=OldAutoExpand;
  18.   finally
  19.     EndUpdate;
  20.   end;
  21. end;

« Last Edit: November 30, 2022, 09:32:54 pm by d7_2_laz »

wp

  • Hero Member
  • *****
  • Posts: 10664
I had thought that child nodes would be destroyed when their parent is collapsed, in order to save memory load. But I cannot find anything related to "Collapse" in the ShellTreeView code - so, probably I am wrong, and your modification would be a solution.

On the other hand, why not implement that "Free-on-collapse" thing? Would there be a disadvantage in having it?

d7_2_laz

  • Sr. Member
  • ****
  • Posts: 336
Hi wp, i'm simply not yet deep enough inside the Shell tools for to understand
- why at all a population might be triggered by a CanExpand query
- and why at all nodes might be cleared or childs destroyed when a node is collapsed.
To my simple understanding maybe intuition a node collapsed is a potential candidate to be re-opened, so i never would clear it when being collapsed.
I assume there is a good reason to do so, but i don't know it. Simply missing of info on my side. What's the benefit to clear? Save memory? Never did run into a bottleneck here.

Beyond intuition, most other file tools i know about re-open (re-expand) nodes instantly and that wuld be my own expectation too.

A nice side-effect i could imagine is that a re-populated node will on-the-fly catch all modifications of the folder subtree needed from actions outside.
But imo that would be rather a question of how to deal with file change monitors.

I was simply really surprised that a subsequent expand takes (felt:) the doubled time of the first one (may be internally the population is done twice?).
The disadvantage of a "Free-on-collapse" in my eyes: why not keep information that is already there but destroy it? Would lead to much redundant work.

I don't think the nodes are cleared intentionally (eg. at collapse). It's simply that they are ckeared and re-built each time a node is re-opened again. (no memory save advantage could be seen here).

For to avoid misunderstanding: i feel the shell tools are excellent - that's why I want to use them more and more  for new things.

« Last Edit: November 27, 2022, 11:43:42 pm by d7_2_laz »

d7_2_laz

  • Sr. Member
  • ****
  • Posts: 336
I experimented a while using the change mentioned above (within CanExpand) and did not find any negative indication why it should not be applied.

So i did open a issue note on that:
https://gitlab.com/freepascal.org/lazarus/lazarus/-/issues/40022

Supplementary remark: i'd expect that when selecting a subnode "Sub1" beyond parent node "Parent1", that when i collapse Parent1 by click on the expand sign, and re-expand it again, the previous selection marker on Sub1 is preserved.
Using the change, it stays preserved. By the current behaviour (= sub nodes cleanup) it goes lost. Not ok. Please keep my selection!

GetMem

  • Hero Member
  • *****
  • Posts: 4030
@d7_2_laz
From the bugreport:
Quote
when re-expanding the node, this is done instantly without clearing/repopulating the node
No more node population is done again redundantly. And certainly not it should take twice the time.

I'd expect that an already populated node can be re-expanded instantly.
What if the collapsed node's children are physically deleted from the hard drive while the node is collapsed? Isn't the (re)read on expand intentional? I wouldn't call it redundant. Maybe you can introduce a new flag, when set to true,  the disk changes are ignored for an already loaded node. 

d7_2_laz

  • Sr. Member
  • ****
  • Posts: 336
I'dthought about that as above
Quote
A nice side-effect i could imagine is that a re-populated node will on-the-fly catch all modifications of the folder subtree needed from actions outside.
But imo that would be rather a question of how to deal with file change monitors.

Of course it makes sense to take care about external done file changes (implying fle change monitors). But then - consequently,  amd not only at one single place (a re-opened node). What about siblings of the parent node, if a folder is deleted: why should they stay within the tree, whereas a re-expanded node should respect duch changes?
At this very basic level external file and folder changes necessarily are not yet in scope.

d7_2_laz

  • Sr. Member
  • ****
  • Posts: 336
Additionally.if you have a dirtree - filelist twin pair, you re-expand a node and expect to see folder changes hrtr,  then one necesarily needs to re-read the listview's content completely too, otherwise dirtree and listview would go out of sync.
Expand = reread whole subtree. plus reread whole lfile - listview?

wp

  • Hero Member
  • *****
  • Posts: 10664
I think I'll add an ExpandCollapseMode property:
Code: Pascal  [Select][+][-]
  1. type
  2.   TExpandCollapseMode = (
  3.     ecmDefault,          // Current behaviour (--> default)
  4.     ecmKeepExpanded,     // Do not clear children of already-expanded, but collaped nodes (d7-2-laz's code)
  5.     ecmCollapseAndClear  // Clear children when node is collapsed
  6.   );

d7_2_laz

  • Sr. Member
  • ****
  • Posts: 336
That'*s surely the best ... seems different preferences might exist  :)

Else i had not other findings. Wow! And it's so fast! (i'm using ownerdata for the shell listview)
 :)

Bart

  • Hero Member
  • *****
  • Posts: 4938
    • Bart en Mariska's Webstek
I think I'll add an ExpandCollapseMode property:
Code: Pascal  [Select][+][-]
  1. type
  2.   TExpandCollapseMode = (
  3.     ecmDefault,          // Current behaviour (--> default)
  4.     ecmKeepExpanded,     // Do not clear children of already-expanded, but collaped nodes (d7-2-laz's code)
  5.     ecmCollapseAndClear  // Clear children when node is collapsed
  6.   );

ecmDefault is not very descriptive.
The fact that you make it default should already be reflected with the default modifier for the property.

Bart

wp

  • Hero Member
  • *****
  • Posts: 10664
ecmLegacy? ecmClearExpandingAndKeepCollapsing?

d7_2_laz

  • Sr. Member
  • ****
  • Posts: 336
ecmRefreshedExpanding?

wp

  • Hero Member
  • *****
  • Posts: 10664
In the attachment there is a modified shellctrls.pas in which I attempt to implement the ExpandCollapseMode. Make a backup of your original shellctrls.pas in the lcl folder of your lazarus installation and copy the attached file over the original one.

The attachment contains also a small project for testing. I tried to measure the time it takes to expand or collapse a folder by storing the clock times at which the OnExpanding/OnExpanded or OnCollapsing/OnCollapsed event fire. My impression however is that this is not very correct since it feels much longer when the WinSxS folder collapses. However the qualitative difference between the ExpandCollapse modes is as expected

I also called GetHeapStatus.TotalAllocated to measure the change in occupied heap memory with respect to the memory used in the form's OnCreate event. Maybe there is some doubt on the exactness of these numbers, but again, the memory usage is as expected.

d7_2_laz

  • Sr. Member
  • ****
  • Posts: 336
Thank you wp!
Seems it needs to be applied on a recent laz-main extract (due to other external dependencies), not the 2.2.4.

Tried the test project.
Of course i was focused onto the option 2 "Do not clear expanding nodes, keep collapsing nodes".
For the re-expand: yes, perfect! Just as expected.
For the collapse: when using option 2,  it appears to run same instantly too (if there should be any minimal delay, i think it's neglecticable).
Also, with option 2, a selected child node keeps to be selected when collapsing/re-expanding its parent node (no mystery, why); ok.

Exactly just how it should be ...   :)

wp

  • Hero Member
  • *****
  • Posts: 10664
Committed the new shellctrls to main.

What's strange is that deletion of the child nodes in the C:\Windows\WinSxS subtree takes much longer than adding them...

 

TinyPortal © 2005-2018