Bug 213068 - KPluginSelector causes applications to hang (when using some widget styles) if plugins are added later (seen on Amarok Script Manager and KWin Desktop Effects selection)
Summary: KPluginSelector causes applications to hang (when using some widget styles) i...
Status: RESOLVED FIXED
Alias: None
Product: kdelibs
Classification: Unclassified
Component: kdeui (show other bugs)
Version: unspecified
Platform: unspecified Linux
: NOR normal (vote)
Target Milestone: ---
Assignee: kdelibs bugs
URL:
Keywords:
: 217326 222295 225557 230239 231027 231398 239925 240853 241275 241369 241946 242977 250593 258316 (view as bug list)
Depends on:
Blocks:
 
Reported: 2009-11-04 12:19 UTC by simon
Modified: 2011-12-09 20:18 UTC (History)
21 users (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments
screenshot taken (64.98 KB, image/jpeg)
2009-11-04 12:20 UTC, simon
Details
Fix layout recursion (1.10 KB, patch)
2010-02-03 01:01 UTC, Christoph Feck
Details

Note You need to log in before you can comment on or make changes to this bug.
Description simon 2009-11-04 12:19:06 UTC
Version:           Unbekannt (using 4.3.73 (KDE 4.3.73 (KDE 4.4 >= 20091026)), Gentoo)
Compiler:          x86_64-pc-linux-gnu-gcc
OS:                Linux (x86_64) release 2.6.31-zen4

the get hot new stuff dialog makes amarok use 100% and hang

check the screenshot attached for the wrong layout in which it freezes; ksnapshot fails with corrupt images when trying to capture just the window so there must be happening something bad here
Comment 1 simon 2009-11-04 12:20:20 UTC
Created attachment 38079 [details]
screenshot taken
Comment 2 Frederik Gladhorn 2010-01-12 01:15:44 UTC
Judging from the screenshot this is not the get hot new stuff dialog but the script management dialog.
Comment 3 Christoph Feck 2010-01-12 01:20:13 UTC
*** Bug 222295 has been marked as a duplicate of this bug. ***
Comment 4 Christoph Feck 2010-01-12 01:21:12 UTC
Bug 222295 indicates that this may depend on the used style. Can you confirm that changing style to Plastique helps?
Comment 5 Hugo Pereira Da Costa 2010-01-12 01:39:12 UTC
Since there is additional information provided in Bug 222295 I copy it here:
this does not happen when using plastique style
this does happen when using oxygen, or QtCurve.
Comment 6 Christoph Feck 2010-01-12 02:11:20 UTC
I just compiled Amarok from git, and the bug also happens with Skulpture, which is not KStyle based. It looks like a problem in KPluginSelector not updating its size somehow when the plugins are added after it has been shown. I looked quickly at Amarok sources, and the plugins are added later.

The bug may be invisible on KDE 4.3, because KDE 4.4 has a new implementation of the KCategorizedView, so that may be the reason Amarok developers never hit that.

I will look into it later, but if my assumption makes sense, please reassign to kdelibs/kdeui.
Comment 7 Hugo Pereira Da Costa 2010-01-12 04:16:29 UTC
eheh. Glad to see I'm off the hook.
Comment 8 Myriam Schweingruber 2010-01-12 13:34:27 UTC
Since this only happens with KDE 4.4, we suspect a KDE bug, reassigning
Comment 9 Christoph Feck 2010-01-12 23:45:12 UTC
Myriam, would it be possible to test with KDE 4.3.4, but running on Qt 4.6.0? Not every distribution supports this combination, but would be nice if I got feedback on this.

What I did: I removed the proxy model from KPluginSelector, i.e. the list directly shows the model via d->listView->setModel(d->pluginModel); Now everything works fine (except the filter and sorting does not work, of course).

Looking at the proxy model, it has not changed for KDE 4.4, neither in kutils, nor in kdelibs/kdeui. What has changed is QSortFilterProxyModel. So this might be a Qt 4.6 problem, but my experience with Model-View classes is limited.

Stephen, any idea? KPluginSelector is in kdelibs/kutils, the other model classes are in kdelibs/kdeui/itemviews.
Comment 10 Christoph Feck 2010-01-13 02:29:15 UTC
*** Bug 217326 has been marked as a duplicate of this bug. ***
Comment 11 Christoph Feck 2010-01-13 02:30:27 UTC
Bug 217326 has a video. Issue is with KTorrent KPluginSelector.
Comment 12 Stephen Kelly 2010-01-13 15:22:37 UTC
I tried but couldn't build either KTorrent or Amarok straight away. I don't have time to spend on it, but I'll be at CampKDE if anyone there can show me the issue.

It would be odd for the proxy model to cause that. Has anyone tried using a different view to see if the issue remains?
Comment 13 Hugo Pereira Da Costa 2010-01-13 23:39:49 UTC
On 01/12/2010 03:45 PM, Christoph Feck wrote:
> https://bugs.kde.org/show_bug.cgi?id=213068
>
>
> Christoph Feck<christoph@maxiom.de>  changed:
>
>             What    |Removed                     |Added
> ----------------------------------------------------------------------------
>                   CC|                            |steveire@gmail.com
>
>
>
>
> --- Comment #9 from Christoph Feck<christoph maxiom de>   2010-01-12 23:45:12 ---
> Myriam, would it be possible to test with KDE 4.3.4, but running on Qt 4.6.0?
> Not every distribution supports this combination, but would be nice if I got
> feedback on this.
>
> What I did: I removed the proxy model from KPluginSelector, i.e. the list
> directly shows the model via d->listView->setModel(d->pluginModel); Now
> everything works fine (except the filter and sorting does not work, of course).
>
>    
Interestingly, just commenting out

d->proxyModel->setCategorizedModel(true);
in kdelibs/kutils/kpluginselector.cpp
(line 267) fixes it for me ...



> Looking at the proxy model, it has not changed for KDE 4.4, neither in kutils,
> nor in kdelibs/kdeui. What has changed is QSortFilterProxyModel. So this might
> be a Qt 4.6 problem, but my experience with Model-View classes is limited.
>
> Stephen, any idea? KPluginSelector is in kdelibs/kutils, the other model
> classes are in kdelibs/kdeui/itemviews.
>
>
Comment 14 Hugo Pereira Da Costa 2010-01-14 22:51:21 UTC
More info after digging the code.
Some how there is an semi-infinite loop generated (possibly due to the style) between 
KCategorizedView::rowsAboutToBeRemoved
and KCategorizedView::visualRect

(which gets disabled when you don't set the list as cathegorized)

I'm still trying to investigate what causes this.
Comment 15 Hugo Pereira Da Costa 2010-01-14 23:01:35 UTC
sorry read
KCategorizedView::updateGeometries 
KCategorizedView::visualRect
Comment 16 Hugo Pereira Da Costa 2010-01-14 23:37:53 UTC
ok. I think what happens is that the range and the position of the vertical is trying to be updated in updateGeometries(), while this scrollbar is hidden (and not necessary). This makes the scrollbar visible (maybe this is new to Qt 4.6, which triggers updateGeometries again. And from there the recursive loop).

Anyway if you force the scrollbar to be visible, or if you disable the update if the scrollbar is hidden, things go well.
See: 

--- kdeui/itemviews/kcategorizedview.cpp        (revision 1074886)
+++ kdeui/itemviews/kcategorizedview.cpp        (working copy)
@@ -1214,8 +1214,11 @@
         bottomRange = lastItemRect.bottomRight().y() + verticalOffset() - viewport()->height();
     }

-    verticalScrollBar()->setRange(0, bottomRange);
-    verticalScrollBar()->setValue(verticalOff);
+    if( verticalScrollBar()->isVisible() )
+    {
+        verticalScrollBar()->setRange(0, bottomRange);
+        verticalScrollBar()->setValue(verticalOff);
+    }

     //TODO: also consider working with the horizontal scroll bar. since at this level I am not 

Can someone test this ? Confirm whether this is the right thing to do or not ?
Comment 17 Stephen Kelly 2010-01-17 08:43:22 UTC
I used the wrong commit keyword (CCMAIL instead of CCBUG). Repasting manually:


    Fix infinite recursion issues when scrolling KCategorizedView

    I don't know this code, but it seems the view is not ready to handle
    QAbstractItemView::ScrollPerItem. See approx line 1222:

        if (viewMode() == ListMode && verticalScrollMode() == ScrollPerItem) {
            //TODO: think more about this case
        } else {

    The default is not specified in the QAbstractItemView docs, so it may have been
    changed in Qt 4.6. I have no idea.

    Todays hack is to use QAbstractItemView::ScrollPerPixel by default instead.

    Please re-test the bug and I will backport if it fixes Amarok and KTorrent.
    I've so far only reproduced in a test app.

    CCMAIL: 213068

    git-svn-id: svn+ssh://svn.kde.org/home/kde/trunk/KDE/kdelibs@1075987 283d02a7-25f6-0310-bc7c-ecb5cbfe19da

diff --git a/kdeui/itemviews/kcategorizedview.cpp b/kdeui/itemviews/kcategorizedview.cpp
index 63d5dfa..4d5beb1 100644
--- a/kdeui/itemviews/kcategorizedview.cpp
+++ b/kdeui/itemviews/kcategorizedview.cpp
@@ -474,6 +474,7 @@ KCategorizedView::KCategorizedView(QWidget *parent)
     : QListView(parent)
     , d(new Private(this))
 {
+  setVerticalScrollMode(ScrollPerPixel);
 }

 KCategorizedView::~KCategorizedView()
Comment 18 Hugo Pereira Da Costa 2010-01-17 09:18:18 UTC
mmm. Unless I did something wrong, Comment #17 does not work for me ...
Comment 19 Stephen Kelly 2010-01-17 12:02:27 UTC
Yeah, I just realized the thing which made my example not infinite loop was commenting QListView::resizeEvent(event); in the reimplementation. Probably not an acceptable workaround to commit.
Comment 20 Christoph Feck 2010-02-03 01:01:38 UTC
Created attachment 40486 [details]
Fix layout recursion

Hugo, if you do not change the values for invisible scrollbars, then they are never shown?

Anyway, I prepared a patch that only changes the scrollbars when the layout of the model changes, not on every updateGeometries().

It fixes the Amarok issue, but I have no clue if it causes any regressions.

@Raphael, tagging of 4.4.0 is in some 20+ hours, maybe we have a chance to fix that... :)
Comment 21 Christoph Feck 2010-02-03 01:30:09 UTC
Comment on attachment 40486 [details]
Fix layout recursion

Scratch that... it does not respect the changed size of the items after updateGeometries() creates the "Goya" widgets...
Comment 22 Christoph Feck 2010-03-11 16:20:14 UTC
*** Bug 230239 has been marked as a duplicate of this bug. ***
Comment 23 Christoph Feck 2010-03-16 22:41:57 UTC
*** Bug 231027 has been marked as a duplicate of this bug. ***
Comment 24 Rafael Fernández López 2010-03-17 04:23:14 UTC
I can't reproduce this issue. It is possible that this got fixed with some changes I have done to this class (KCategorizedView).

Please if you can still reproduce this on trunk, tell me how.
Comment 25 Christoph Feck 2010-03-17 04:38:13 UTC
Still reproducable, on kdelibs trunk from today with Qt 4.7 branch from last week.

Start Amarok, select Settings/Configure, resize the config window as small as possible, then click on "Internet Services". The effect can be seen best when you now try to maximize the config window.

From what I understand, the problem is in Qt, getting into a loop where showing a scroll bar causes a relayout, causing the second scroll bar to be shown, causing a relayout, causing the second scroll bar to be hidden, causing a relayout, ...
Comment 26 Rafael Fernández López 2010-03-17 11:00:25 UTC
Interesting. I could now reproduce. Thanks. Will have a look.
Comment 27 Rafael Fernández López 2010-03-17 12:45:20 UTC
SVN commit 1104321 by ereslibre:

Fix infinite loop. However scrollbars still need to be polished to improve their look (the step 
size probably). This is more important than the look, since it is blocking some applications 
(such as Amarok) under certain circumstances. I could reproduce the issue and I believe now is
fixed. Please if you could recheck.

BUG: 213068


 M  +1 -2      kcategorizedview.cpp  


WebSVN link: http://websvn.kde.org/?view=rev&revision=1104321
Comment 28 Rafael Fernández López 2010-03-17 12:54:31 UTC
SVN commit 1104322 by ereslibre:

Backport revision 1104321 to 4.4. Fix infinite recursion under certain circumstances.

CCBUG: 213068


 M  +1 -2      kcategorizedview.cpp  


WebSVN link: http://websvn.kde.org/?view=rev&revision=1104322
Comment 29 Christoph Feck 2010-04-25 02:09:06 UTC
SVN commit 1118520 by cfeck:

Never show horizontal scroll bar

Revert commit r1104321, and try a different workaround.

Something inside Qt causes an infinite layout recursion (see bug 213068),
but not updating scroll bar geometry causes a major usability regression
(see bug 233163).

The only workaround I can see for now is to force hiding the horizontal
scroll bar; KCategorizedView does not yet support it anyway.

BUG: 233163
CCBUG: 213068


 M  +4 -1      kcategorizedview.cpp  


WebSVN link: http://websvn.kde.org/?view=rev&revision=1118520
Comment 30 Christoph Feck 2010-04-25 02:10:23 UTC
SVN commit 1118521 by cfeck:

Never show horizontal scroll bar (backport r1118520)

Revert commit r1104322, and try a different workaround.

CCBUG: 233163
CCBUG: 213068


 M  +4 -1      kcategorizedview.cpp  


WebSVN link: http://websvn.kde.org/?view=rev&revision=1118521
Comment 31 Christoph Feck 2010-06-03 00:46:25 UTC
It looks like all the fixes we tried are mutually exclusive ... Rafael, any other idea?
Comment 32 Christoph Feck 2010-06-03 00:46:49 UTC
*** Bug 239925 has been marked as a duplicate of this bug. ***
Comment 33 Thomas Lübking 2010-06-06 14:43:58 UTC
*** Bug 240853 has been marked as a duplicate of this bug. ***
Comment 34 Myriam Schweingruber 2010-06-10 11:16:06 UTC
*** Bug 225557 has been marked as a duplicate of this bug. ***
Comment 35 Beat Wolf 2010-06-10 13:40:36 UTC
*** Bug 241275 has been marked as a duplicate of this bug. ***
Comment 36 Myriam Schweingruber 2010-06-11 12:15:04 UTC
*** Bug 241369 has been marked as a duplicate of this bug. ***
Comment 37 Christoph Feck 2010-06-11 13:59:21 UTC
SVN commit 1137013 by cfeck:

Next attempt at KPluginSelector lockup

Force a scrollbar on the plugin selector. For users with few
plugins, it might look odd why it still shows the scroll bar,
but still better than an application freeze.

If I only could prove it is a Qt bug ...

BUG: 213068


 M  +0 -6      kdeui/itemviews/kcategorizedview.cpp  
 M  +1 -0      kutils/kpluginselector.cpp  


WebSVN link: http://websvn.kde.org/?view=rev&revision=1137013
Comment 38 Christoph Feck 2010-06-15 23:54:04 UTC
SVN commit 1138391 by cfeck:

Next attempt at KPluginSelector lockup (backport r1137013)

CCBUG: 213068


 M  +0 -6      kdeui/itemviews/kcategorizedview.cpp  
 M  +1 -0      kutils/kpluginselector.cpp  


WebSVN link: http://websvn.kde.org/?view=rev&revision=1138391
Comment 39 Myriam Schweingruber 2010-06-16 22:37:39 UTC
*** Bug 231398 has been marked as a duplicate of this bug. ***
Comment 40 Myriam Schweingruber 2010-06-18 00:13:16 UTC
*** Bug 241946 has been marked as a duplicate of this bug. ***
Comment 41 Myriam Schweingruber 2010-06-30 10:56:20 UTC
*** Bug 242977 has been marked as a duplicate of this bug. ***
Comment 42 Thomas Lübking 2010-09-12 15:52:24 UTC
*** Bug 250593 has been marked as a duplicate of this bug. ***
Comment 43 Christoph Feck 2010-09-12 16:05:02 UTC
Bug 250593 is from KDE SC 4.5.1, reopening :/
Comment 44 Christoph Feck 2010-11-29 23:45:23 UTC
*** Bug 258316 has been marked as a duplicate of this bug. ***
Comment 45 Thomas Lübking 2011-12-01 19:00:12 UTC
Git commit 70c6350fac3ab2b74d61811606eb250da7c5a061 by Thomas Lübking.
Committed on 01/12/2011 at 19:46.
Pushed by luebking into branch 'KDE/4.7'.

Fix regression induced (but likely not caused) by commit e91e5fed6b1aad365e12e919f430c3e8147552d3

Commiting on behalf of Jaime Torres.

Ratio:
On k-c-d I suggested this would be another ouotcome of bug #213068 and Jaime could confirm this and the
scrollbar policy workaround to work.
He suggested to fix it by "setHorizontalScrollBarPolicy(Qt::ScrollBarAlwaysOff);" instead (since presumingly the
pluginselector is always top-down oriented)
I tested that variant as well and could not reproduce the issue.

Since Jaime has no acces to his development environment for today he asked me to either revert his commit or
apply the hotfix working around bug #213068 as mentioned above.

Because i could not spot any bug in his commit i'll go with the hotfix to prevent a regression.
Resolving the actual recursion bug is an outstanding issue.

CCBUG: 213068
CCBUG: 287847

M  +1    -0    kutils/kpluginselector.cpp

http://commits.kde.org/kdelibs/70c6350fac3ab2b74d61811606eb250da7c5a061
Comment 46 Aaron J. Seigo 2011-12-02 20:53:21 UTC
Git commit ee00a9f572a383b98c4fd01f0b0477ff9148c5d7 by Aaron Seigo, on behalf of Thomas Lübking.
Committed on 01/12/2011 at 19:46.
Pushed by aseigo into branch 'frameworks'.

Fix regression induced (but likely not caused) by commit e91e5fed6b1aad365e12e919f430c3e8147552d3

Commiting on behalf of Jaime Torres.

Ratio:
On k-c-d I suggested this would be another ouotcome of bug #213068 and Jaime could confirm this and the
scrollbar policy workaround to work.
He suggested to fix it by "setHorizontalScrollBarPolicy(Qt::ScrollBarAlwaysOff);" instead (since presumingly the
pluginselector is always top-down oriented)
I tested that variant as well and could not reproduce the issue.

Since Jaime has no acces to his development environment for today he asked me to either revert his commit or
apply the hotfix working around bug #213068 as mentioned above.

Because i could not spot any bug in his commit i'll go with the hotfix to prevent a regression.
Resolving the actual recursion bug is an outstanding issue.

CCBUG: 213068
CCBUG: 287847

M  +1    -0    kutils/kpluginselector.cpp

http://commits.kde.org/kdelibs/ee00a9f572a383b98c4fd01f0b0477ff9148c5d7
Comment 47 Thomas Lübking 2011-12-06 18:38:55 UTC
Git commit 08325ba32b72326030004cc28430431193d82fc2 by Thomas Lübking.
Committed on 05/12/2011 at 14:56.
Pushed by luebking into branch 'KDE/4.7'.

fix KCategorizedView race

BUG: 213068
BUG: 287847
REVIEW: 103335

QListView::updateGeometries() has it's own opinion on whether the scrollbars should be visible (valid range) or not
and triggers a (sometimes additionally timered) resize through ::layoutChildren()
http://qt.gitorious.org/qt/qt/blobs/4.7/src/gui/itemviews/qlistview.cpp#line1499
(the comment above the main block isn't all accurate, layoutChldren is called regardless of the policy)

As a result QListView and KCategorizedView occasionally started a race on the scrollbar visibility, effectively blocking the UI
So we prevent QListView from having an own opinion on the scrollbar visibility by
fixing it before calling the baseclass QListView::updateGeometries() and restoring the policy afterwards

M  +23   -1    kdeui/itemviews/kcategorizedview.cpp
M  +0    -2    kutils/kpluginselector.cpp

http://commits.kde.org/kdelibs/08325ba32b72326030004cc28430431193d82fc2
Comment 48 Thomas Lübking 2011-12-09 20:18:07 UTC
Git commit 8095c11d181e967cb161de3452f45c1df81bbc66 by Thomas Lübking.
Committed on 08/12/2011 at 18:58.
Pushed by luebking into branch 'KDE/4.7'.

Fix KCategorizedView race - better :p

BUG: 213068
BUG: 287847
REVIEW: 103335

M  +24   -9    kdeui/itemviews/kcategorizedview.cpp

http://commits.kde.org/kdelibs/8095c11d181e967cb161de3452f45c1df81bbc66