Bug 438675 - Last horizontal disk size indicator is rendered on first entry of Places
Summary: Last horizontal disk size indicator is rendered on first entry of Places
Status: RESOLVED FIXED
Alias: None
Product: dolphin
Classification: Applications
Component: general (show other bugs)
Version: 21.04.1
Platform: Other Linux
: NOR normal
Target Milestone: ---
Assignee: Dolphin Bug Assignee
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-06-15 12:15 UTC by medin
Modified: 2022-05-17 19:11 UTC (History)
5 users (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments
Last horizontal disk size indicator is rendered on first entry of Places (1.30 MB, video/mp4)
2021-06-15 12:15 UTC, medin
Details

Note You need to log in before you can comment on or make changes to this bug.
Description medin 2021-06-15 12:15:27 UTC
Created attachment 139338 [details]
Last horizontal disk size indicator is rendered on first entry of Places

This happens only when you scroll up/down using mouse wheel and not when you grab scroll bar with right click mouse pointer.
See attached video for more info.
Comment 1 Nate Graham 2021-06-15 15:12:05 UTC
Wat
Comment 2 Harald Sitter 2021-06-16 10:57:05 UTC
I bet this either has to do with recycling of delegates (which I'm not sure we do) OR, possibly more likely, the first draw is wrong. Since the disk delegate and the folder delegate are the same functional class the folder entry functionally is really just a disk entry, pointing to a specific folder. So assuming the initial state is "wrong", in that the space indicator is visible by default, that'd explain why it briefly appears with a space indicator. For me this seems somewhat reproducible with some additional things I'd like to note:

a) it's not necessarily the first entry for me
b) it's always the same entry when I scroll up and down multiple times
c) the indicator actually doesn't disappear until I move the mouse after scrolling - does also kind of point at a redraw problem that isn't necessarily related
Comment 3 nttkde 2021-06-24 20:12:29 UTC
I took a quick look into this couple of months ago, and as far as I understand it is indeed related to recycling of the placesitemlistwidget objects (which happens only when part of the places panel is hidden - the scrollbar is there).
Take this with a grain of salt but hopefully it's at least somewhat readable/helpful. (Zren has mostly seen this already.)



The capacity bar state (m_freeSpaceInfo & m_drawCapacityBar) is stored inside placesitemlistwidgets and when the widget gets "recycled" into an other index it still carries the old data with it. ResetCapacityBar() is not called during recycling.

Other widget parameters are normally reset from kitemlistview::updateWidgetProperties()
(or initializeItemListWidget) when recycle/reuse happens in kitemlistview::doLayout().
(doLayout->createWidget->create->popRecycleableWidget and doLayout() reusableitems [line 1761])

The setData() call in updateCapacityBar() resets widgets' other data but of course doesn't affect the m_freeSpaceInfo/m_drawCapacityBar since those are stored in the widget object.


((IIRC for testing purposes, if you create a method in *itemlistwidget classes that gets called from updateWidgetProperties(), and overload it in placesitemlistwidget so that it calls resetCapacityBar(), it should fix the bug.))


First paint will use the wrong data but the next repaint after updateCapacityBar makes the bar go away

except...



The more annoying aspect is that I can reproduce it on the disk devices too, and when it appears on an unmounted device the bar won't go away at all in a following repaint since then url.isEmpty() in placesitemlistwidget.cpp:60 just returns without recalculating the bar.
('url' is stored in the "model" data externally, and becomes empty for an unmounted device. 'udi' above exists for unmounted devices too).
Scrolling may make it go away if a different widget gets recycled on its place.

Thus moving the updateCapacityBar() call to the beginning of paint() won't fix the bug for the disk device items.

----------------------------


If MVC model is to be followed, the placesitemlistwidget (kind of part of the "view" here, isn't it?) has some tasks that should maybe be rather divided into the model or controller.

If m_freeSpaceInfo and m_drawCapacityBar could be read and updated from data() or something similar (from the model that doesn't get recycled) as eg. the url is read, that would solve the problems, 
but currently there doesn't seem to be a way to access the model directly from the placesitemlistwidget.

PlacesItemListWidget can only read data() and it's copied from the model to the widget every time it's recycled/reused in the updateWidgetProperties()->setData().
Comment 4 Chris Holland 2021-06-24 20:29:18 UTC
The solution is to follow kio's filepicker code properly. I wrote the dolphin code simpler since I thought I'd squirreled out all the edge cases of it being recycled. Seems like I can't do that though.

https://invent.kde.org/frameworks/kio/-/blob/master/src/filewidgets/kfileplacesview.cpp#L121
https://invent.kde.org/frameworks/kio/-/blob/master/src/filewidgets/kfileplacesview.cpp#L235
mutable QMap<QPersistentModelIndex, PlaceFreeSpaceInfo> m_freeSpaceInfo;
Comment 5 nttkde 2022-05-17 19:11:36 UTC
K.Broulik reworked places panel to use KFilePlacesView a few months ago, and this bug seems to be fixed now. (https://invent.kde.org/system/dolphin/-/merge_requests/309)