From kde-core-devel Sun Dec 04 18:47:14 2011 From: =?utf-8?q?Thomas_L=C3=BCbking?= Date: Sun, 04 Dec 2011 18:47:14 +0000 To: kde-core-devel Subject: Re: Review Request: fix infinite recurssion in kcategorizedview Message-Id: <20111204184714.7465.10288 () vidsolbach ! de> X-MARC-Message: https://marc.info/?l=kde-core-devel&m=132302456313650 MIME-Version: 1 Content-Type: multipart/mixed; boundary="--===============7943018268296793705==" --===============7943018268296793705== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103313/#review8704 ----------------------------------------------------------- Ok, i think i know what's wrong. KCategorizedView::updateGeometries() calls QListView::updateGeometries() wh= ich has it's own opinion on whether the scrollbars should be visible (valid= range) or not and triggers a (sometimes additionally timered) resize throu= gh ::layoutChildren() http://qt.gitorious.org/qt/qt/blobs/4.7/src/gui/itemv= iews/qlistview.cpp#line1499 - the comment above the main block isn't all ac= curate, layoutChldren is called regardless of the policy. My approach was then to prevent QListView from having an own opinion on the= scrollbar visibility by fixing it before calling the baseclass QListView::= updateGeometries() // bug 213068 ------------------------------------------------------------ const Qt::ScrollBarPolicy verticalP =3D verticalScrollBarPolicy(), hori= zontalP =3D horizontalScrollBarPolicy(); setVerticalScrollBarPolicy(verticalScrollBar()->isVisibleTo(this) ? Qt:= :ScrollBarAlwaysOn : Qt::ScrollBarAlwaysOff); setHorizontalScrollBarPolicy(horizontalScrollBar()->isVisibleTo(this) ?= Qt::ScrollBarAlwaysOn : Qt::ScrollBarAlwaysOff); // /bug 213068 --------------------------------------------------------= ---- QListView::updateGeometries(); // bug 213068 ---------------------------------------------------------= --- setVerticalScrollBarPolicy(verticalP); setHorizontalScrollBarPolicy(horizontalP); // /bug 213068 --------------------------------------------------------= ---- in addition i ensure the horizontal range was also set on the early skip const int rowCount =3D d->proxyModel->rowCount(); if (!rowCount) { horizontalScrollBar()->setRange(0, 0); verticalScrollBar()->setRange(0, 0); return; } but i actually doubt this is relevant. However, i cannot reproduce the issue (commenting the fixed policies in kpl= uginselector.cpp) and as a bonus kpluginselector resizes faster (resize/geo= metryUpdate came in pairs here before, not tested with the patch but it fee= ls faster ;-) Can please anyone attempt to confirm or deny the approach? - Thomas L=C3=BCbking On Dec. 4, 2011, 8:42 a.m., Jaime Torres Amate wrote: > = > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/103313/ > ----------------------------------------------------------- > = > (Updated Dec. 4, 2011, 8:42 a.m.) > = > = > Review request for kdelibs and Rafael Fern=C3=A1ndez L=C3=B3pez. > = > = > Description > ------- > = > Basically, what I do is: > If there are one or zero columns, hide the horizontalScrollBar until it i= s needed. (it has worked in the past, but in another file). > Apply the same strategy with files. > = > Additional stuff: > Moved the common calculus of itemSize outside of the if then else. > = > = > This addresses bugs 213068 and 287847. > http://bugs.kde.org/show_bug.cgi?id=3D213068 > http://bugs.kde.org/show_bug.cgi?id=3D287847 > = > = > Diffs > ----- > = > kdeui/itemviews/kcategorizedview.cpp 46a1cde = > kutils/kpluginselector.cpp ca0691d = > = > Diff: http://git.reviewboard.kde.org/r/103313/diff/diff > = > = > Testing > ------- > = > Krunner config does not loop (neither kgetnewstuff from kstars). I can no= t test with amarok (I've hit by an amarok start bug). > Please, test with other programs. > = > = > Thanks, > = > Jaime Torres Amate > = > --===============7943018268296793705== Content-Type: text/html; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable
This is an automatically generated e-mail. To reply, visit: http://git.revie= wboard.kde.org/r/103313/ 

Ok, i thin=
k i know what's wrong.
KCategorizedView::updateGeometries() calls QListView::updateGeometries() wh=
ich has it's own opinion on whether the scrollbars should be visible (v=
alid range) or not and triggers a (sometimes additionally timered) resize t=
hrough ::layoutChildren() http://qt.gitorious.org/qt/qt/blobs/4.7/src/gui/i=
temviews/qlistview.cpp#line1499 - the comment above the main block isn'=
t all accurate, layoutChldren is called regardless of the policy.

My approach was then to prevent QListView from having an own opinion on the=
 scrollbar visibility by fixing it before calling the baseclass QListView::=
updateGeometries()


// bug 213068 ------------------------------------------------------------
    const Qt::ScrollBarPolicy verticalP =3D verticalScrollBarPolicy(), hori=
zontalP =3D horizontalScrollBarPolicy();
    setVerticalScrollBarPolicy(verticalScrollBar()->isVisibleTo(this) ? =
Qt::ScrollBarAlwaysOn : Qt::ScrollBarAlwaysOff);
    setHorizontalScrollBarPolicy(horizontalScrollBar()->isVisibleTo(this=
) ? Qt::ScrollBarAlwaysOn : Qt::ScrollBarAlwaysOff);
    // /bug 213068 --------------------------------------------------------=
----

    QListView::updateGeometries();

    // bug 213068 ---------------------------------------------------------=
---
    setVerticalScrollBarPolicy(verticalP);
    setHorizontalScrollBarPolicy(horizontalP);
    // /bug 213068 --------------------------------------------------------=
----

in addition i ensure the horizontal range was also set on the early skip

const int rowCount =3D d->proxyModel->rowCount();
    if (!rowCount) {
        horizontalScrollBar()->setRange(0, 0);
        verticalScrollBar()->setRange(0, 0);
        return;
    }

but i actually doubt this is relevant.

However, i cannot reproduce the issue (commenting the fixed policies in kpl=
uginselector.cpp) and as a bonus kpluginselector resizes faster (resize/geo=
metryUpdate came in pairs here before, not tested with the patch but it fee=
ls faster ;-)

Can please anyone attempt to confirm or deny the approach?

- Thomas


On December 4th, 2011, 8:42 a.m., Jaime Torres Amate wrote:

Review request for kdelibs and Rafael Fern=C3=A1ndez L=C3=B3pez.
By Jaime Torres Amate.

Updated Dec. 4, 2011, 8:42 a.m.

Descripti= on 

Basically, what I do is:
If there are one or zero columns, hide the horizontalScrollBar until it is =
needed. (it has worked in the past, but in another file).
Apply the same strategy with files.

Additional stuff:
Moved the common calculus of itemSize outside of the if then else.

Testing <= /h1> 
Krunner config does not loop (neither kgetnewstuff from ksta=
rs). I can not test with amarok (I've hit by an amarok start bug).
Please, test with other programs.
Bugs: 213068, = 287847

Diffs=

  • kdeui/itemviews/kcategorizedview.cpp (46a1= cde)
  • kutils/kpluginselector.cpp (ca0691d)

View Diff
--===============7943018268296793705==--