Opened 10 years ago

Closed 9 years ago

Last modified 9 years ago

#3897 closed bug (fixed)

[DriveSetup] Rescan menu increase vertical scrollbar

Reported by: diver Owned by: stippi
Priority: normal Milestone: R1
Component: Applications/DriveSetup Version: R1/Development
Keywords: Cc: mdisreali@…, elikcoreighn@…
Blocked By: Blocking:
Has a Patch: yes Platform: All

Description

Disk->Rescan menu increase vertical scrollbar

Attachments (3)

growingvertscrollbarfix.patch (338 bytes) - added by dru_ed 9 years ago.
Sets "fItemsHeight = 0.0" to prevent old values carrying into next call which was making the scrollbar grow.
BColumnListView_vscrollbarfix.patch (628 bytes) - added by Duggan 9 years ago.
Children were not being removed recursively, this patch fixes that and with it, this ticket.
foobar.patch (853 bytes) - added by Duggan 9 years ago.
I hope this is better :/

Download all attachments as: .zip

Change History (31)

comment:1 Changed 10 years ago by stippi

Resolution: fixed
Status: newclosed

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

comment:2 Changed 10 years ago by diver

Resolution: fixed
Status: closedreopened

Still reproducible in hrev32875, reopening.

comment:3 Changed 10 years ago by stippi

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

comment:4 Changed 10 years ago by Disreali

Cc: mdisreali@… added

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

comment:5 Changed 10 years ago by diver

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

comment:6 Changed 9 years ago by diver

Version: R1/pre-alpha1R1/Development

Still here in hrev35573.

comment:7 Changed 9 years ago by 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.

comment:8 Changed 9 years ago by Duggan

Cc: elikcoreighn@… added

Changed 9 years ago by dru_ed

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

comment:9 Changed 9 years ago by dru_ed

Has a Patch: set

comment:10 Changed 9 years ago by 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!

comment:11 Changed 9 years ago by 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.

comment:12 Changed 9 years ago by stippi

Has a Patch: unset

comment:13 Changed 9 years ago by 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.

comment:14 in reply to:  13 ; Changed 9 years ago by 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.

comment:15 in reply to:  14 Changed 9 years ago by 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.

Last edited 9 years ago by Duggan (previous) (diff)

comment:16 Changed 9 years ago by dru_ed

I took a closer look at Interface Kit and ColumnListView.cpp 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 deleted? 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.

Last edited 9 years ago by dru_ed (previous) (diff)

Changed 9 years ago by Duggan

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

comment:17 Changed 9 years ago by Duggan

Has a Patch: set

comment:18 Changed 9 years ago by 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.

comment:19 Changed 9 years ago by 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.

comment:20 Changed 9 years ago by Duggan

Has a Patch: unset

Changed 9 years ago by Duggan

Attachment: foobar.patch added

I hope this is better :/

comment:21 Changed 9 years ago by Duggan

Has a Patch: set

comment:22 Changed 9 years ago by 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.

comment:23 Changed 9 years ago by 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. :/

comment:24 Changed 9 years ago by 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.

comment:25 Changed 9 years ago by Duggan

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

comment:26 Changed 9 years ago by stippi

Resolution: fixed
Status: reopenedclosed

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.

comment:27 Changed 9 years ago by 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...

comment:28 Changed 9 years ago by Duggan

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

Note: See TracTickets for help on using tickets.