Fix KFilePreviewGenerator::LayoutBlocker
ClosedPublic

Authored by fvogt on Jan 11 2018, 9:12 PM.

Details

Summary

QAbstractItemViews does layout in a timer event handler to avoid unnecessary
layout calculations. Changes which cause a relayout only start the timer.
LayoutBlocker has the restriction that it only works if the event loop is not
entered during its lifetime. Without an event loop there's no expensive
relayout anyway, making the LayoutBlocker pointless in such cases.
LayoutBlocker works by changing the uniformItemSizes property of the QListView
to true and in the destructor back to the original value again. Those changes
do not trigger a relayout in QListView, so if the QListView did a layout with
uniformItemSizes set to true, it stays that way.
Fix it by triggering a relayout in ~LayoutBlocker.

This got exposed by a change in Qt, which results in QListView doing a relayout
while the LayoutBlocker is active.

BUG: 352776

Test Plan

kfilewidgettest_gui has proper item sizes now.

Diff Detail

Repository
R241 KIO
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
fvogt created this revision.Jan 11 2018, 9:12 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptJan 11 2018, 9:12 PM
fvogt requested review of this revision.Jan 11 2018, 9:12 PM
fvogt edited the summary of this revision. (Show Details)Jan 11 2018, 9:20 PM
dfaure accepted this revision.Jan 12 2018, 8:39 AM

Nice investigation and fix! Do you happen to know the actual change in Qt which broke this? Wondering if it should 1) be mentioned in the commit log, 2) lead to an #if QT_VERSION.
But if it's just a supposition, then that's fine, don't spend a week tracking down the change in Qt :-)

This revision is now accepted and ready to land.Jan 12 2018, 8:39 AM
fvogt added a comment.Jan 12 2018, 8:46 AM

Nice investigation and fix! Do you happen to know the actual change in Qt which broke this? Wondering if it should 1) be mentioned in the commit log, 2) lead to an #if QT_VERSION.
But if it's just a supposition, then that's fine, don't spend a week tracking down the change in Qt :-)

I don't - there were some reports before Qt 5.10 (I never hit it myself though) already, so it's always been broken, just not this severe.
The change in Qt 5.10 is probably that some operation used by the preview generator calls processEvents or an eventLoop now instead of doing a fully-blocking operation - behaviour which kio definitely can't rely on.

I certainly hope QListView doesn't trigger a nested event loop, that would be horribly nasty and the cause for a million more problems.... (up to and including crashes when closing the view/window while this is happening). But I'm assuming this is just a supposition, and I'm strongly hoping it's unfounded ;-)

fvogt added a comment.Jan 12 2018, 8:54 AM

I certainly hope QListView doesn't trigger a nested event loop, that would be horribly nasty and the cause for a million more problems.... (up to and including crashes when closing the view/window while this is happening). But I'm assuming this is just a supposition, and I'm strongly hoping it's unfounded ;-)

Not QListView - the preview generator can do all kinds of stuff. There's also a slim change that something else does a full relayout - I didn't spot anything like that however.
Normally I'd expect QListView::setUniformItemSizes to trigger a relayout on change though, so this fix is in any case correct.

This revision was automatically updated to reflect the committed changes.