[DriveSetup] Rescan menu increase vertical scrollbar

[diver]

Disk->Rescan menu increase vertical scrollbar

[stippi]

This should be fixed since hrev32850. Please re-open if not.

[diver]

Still reproducible in hrev32875, reopening.

[stippi]

Sorry about that, I thought you meant the horizontal scrollbar.

[Disreali]

I'm not experiencing this on hrev32871, installed from iso to virtual disk using VirtualBox 3.0.4. How are you testing?

[diver]

IIRC you need to have several partitions/drives visible in DriveSetup to see this bug in action.

[diver]

Still here in hrev35573.

[Ziusudra]

Just noticed this on 37298.

If I collapse all the drives so that only the raw devices are shown it does not happen. If I leave only my USB stick expanded, which has two lines in addition to the raw device, the size added to the view seems to be equal to those two lines.

[dru_ed]

Sets "fItemsHeight = 0.0" to prevent old values carrying into next call which was making the scrollbar grow.

[Duggan]

Hey Dru, thanks for looking into this. Although the patch may fix the symptoms of the problem, I suspect something is still broken underneath. I've received word from AnEvilYak (by way of the grapevine) that RemoveRow most likely should be recursive, so I may poke around with that a little bit and see what kind of results I can get. I assume your patch is a good temporary fix though, kudos!

[stippi]

The patch looks wrong. It doesn't seem like it was compiled (missing semicolon), and fItemsHeight is already maintained at the beginning of the patched method. The method specifically tries to find the height of the sub tree being removed by iterating over the rows/items.

[Duggan]

Somewhere the height of the subtree isn't being reset as thats exactly the height that the scrollbar increases each time at least as far as I've seen looking into it, which admittedly hasn't happened in a few days. I still think its a recursion issue but I haven't had time to persue that.

[stippi]

Replying to Duggan:

Somewhere the height of the subtree isn't being reset as thats exactly the height that the scrollbar increases each time at least as far as I've seen looking into it, which admittedly hasn't happened in a few days. I still think its a recursion issue but I haven't had time to persue that.

This is definitely a good working theory, but your patch was the wrong approach, since it removes the maintainance of fItemHeight in its own way.

[Duggan]

Replying to stippi:

This is definitely a good working theory, but your patch was the wrong approach, since it removes the maintainance of fItemHeight in its own way.

Dru's patch ;) And I agree (see comment:10). I haven't installed the patch, I just took his word that it worked and again, it may solve the symptom but that doesn't mean it fixes the issue.

[dru_ed]

I took a closer look and the patch was treating the symptom. The problem is still caused by fItemsHeight not reaching 0 when all rows have been deleted. Yes it looks like the code will take care of it with two instances of "fItemsHeight -= subTreeHeight + 1;" but, unfortunately, the code doesn't operate like assumed.

In DriveSetup, let's say I have 3 volumes (parents), 2 with partition tables (children), for a total of 5 rows in the UI. That 5 UI rows (with two hideable/expandable) is only seen as 3 rows by the code.

That's fine except fItemsHeight includes the height of all 5 UI rows so when the 3 parent rows are deleted, the space taken up by the 2 child rows is still reflected by fItemsHeight. This error is compounded with each successive refresh. You can see now, I think, why simply setting fItemsHeight to 0 at the end of RemoveRow seemed to solve the problem.

Why doesn't the RemoveRow code fix this?

First, I found FindParent always returns False. I've not discovered why but it is connected to "*outParent" (in "OutlineView::FindParent") being NULL (it's "parentRow" in OutlineView::RemoveRow). I suspect this is related to the 3 vs 5 row problem mentioned earlier. It makes sense for the 3 parent rows themselves to return NULL since they have no parent. What does this lead to? The line "if (parentRow)" does not seem to ever be TRUE, skipping one case of "fItemsHeight -= subTreeHeight + 1;"

The 3 rows, which are conceptually parents, are removed by what is the "else" code that seemed to have been meant for children, not parents. This is where the 3 rows are removed and with "fItemsHeight -= subTreeHeight + 1;"

Is it enough to only compensate for the height of children a parent had when delete? Perhaps, but that may be no better than the patch which set fItemsHeight to 0, treating symptoms not illness. I'm not facile enough with the OutlineView code to know if there's a deeper conceptual problem but I was surprised the UI "parent" rows are removed by "child" code.

I would suggest looking closer at the FindParent call. In particular does "parentRow" (called "outParent" in FindParent code) act as expected since the parents are handled as children and if (parentRow) code is not executed.

[Duggan]

Children were not being removed recursively, this patch fixes that and with it, this ticket.

[Duggan]

For anyone curious, the first part is related to my patch for #3036 but added a little back to give the mouse a bigger handle to grab on to. It can be excluded if necessary.

[stippi]

This looks still wrong to me. If you invoke BOutlineListView::RemoveRow(someRow), the operation should be reversible by invoking BOutlineListView::AddRow(someRow, itsPeviousIndex). With your change however, all child rows of the row have been removed -- and leaked, too. When you add the row back, it will have none of it's children anymore. When the OutlineView::RemoveRow() code tries to figure out the height of the entire subtree being removed, it uses an RecursiveOutlineIterator. Why is that not working? I would search the problem in that direction.

[Duggan]

I hope this is better :/

[Duggan]

FindParent() would always return false when the head node was passed to it because it has no parent so the iterator would never have a chance to function.

[Duggan]

And the +1 is due to a discrepancy after the fact. I believe it might be due to some sort of room left for a spacer between rows but I'm probably completely wrong about that which equally leads me to believe my whole solution is wrong. :/

[stippi]

This would be the proper fix. :-) Hope you didn't get too frustrated with my reviews... ;-) I've done some cleanup in the RemoveRow() method along with your fix and will commit it soon.

[Duggan]

Criticism is always welcome as long as it's constructive :) The frustration came from the bug...

[stippi]

The FindParent() semantics are simply not obvious, I think that was the worst part. I didn't change them either, since I was afraid of side-effects in other parts of the code. Thanks for your work! I've committed your fix along with much more cleanup in hrev38372.

[Duggan]

PulkoMandy suggested integrating the +1 in the computation into the Height() function, but I told him I didn't do so for the exact same reason: theres no telling what it would affect elsewhere. I'm not sure where the necessity for it originates though...

[Duggan]

... and by the looks of your changes, you would've fixed this ticket anyway :P