[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-core-devel
Subject: Re: Review Request: Allow KWidgetItemDelegate::createItemWidgets to
From: "Commit Hook" <null () kde ! org>
Date: 2011-05-12 4:34:53
Message-ID: 20110512043453.11200.77667 () vidsolbach ! de
[Download RAW message or body]
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/101219/#review3274
-----------------------------------------------------------
This review has been submitted with commit a1a697436e16f86a545d0aced1f35dbd=
16772856 by eli mackenzie.
- Commit
On April 24, 2011, 7:49 a.m., Eli MacKenzie wrote:
> =
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/101219/
> -----------------------------------------------------------
> =
> (Updated April 24, 2011, 7:49 a.m.)
> =
> =
> Review request for kdelibs and Rafael Fern=C3=A1ndez L=C3=B3pez.
> =
> =
> Summary
> -------
> =
> Ideally KWidgetItemDelegate::createItemWidgets would take a const QModelI=
ndex& argument. Unfortunately its public API, so the signature cannot be ch=
anged until KDE5. Additionally, its pure virtual so the option of adding a =
createItemWidgets(const QModelIndex&) method won't work either.
> =
> Instead, I've added a dynamic property called "creatingWidgetForIndex" to=
the delegate, that is only present while the widget pool is creating widge=
ts. This means the only change kwidgetitemdelegate.h is documentation. The =
QModelIndex could be stashed on one of the d-objects and made available thr=
ough an accessor, but that would mean that d-object would have to carry a Q=
ModelIndex that would be invalid almost all of the time.
> =
> Another benefit is that an application where this feature is desired (for=
KDE < 4.7) only has to include an the new version of kwidgetitemdelegatepo=
ol.cpp, which is easy to keep in sync with kdelibs.
> =
> This change allows you to set a delegate for a particular column in the m=
odel, and put a type of widget in each row that works best for the data.
> =
> =
> Diffs
> -----
> =
> kdeui/itemviews/kwidgetitemdelegate.h 58dd60868f476d925f3abd53e67b22c1e=
d7149ac =
> kdeui/itemviews/kwidgetitemdelegatepool.cpp b287584a594e97a091f96447386=
a588acb08c59e =
> =
> Diff: http://git.reviewboard.kde.org/r/101219/diff
> =
> =
> Testing
> -------
> =
> In my test app I started with a model that only included 5 rows, and woul=
d only create a widget for row 2. As long as KWidgetItemDelegate::updateWid=
gets doesn't assume that the QList<QWidget*> argument has something in it, =
all is well. After implementing this change I caused the model to change th=
e type of widget for each row per the index pulled out of the property, aga=
in keeping updateWidgets in sync, and have encountered no problems.
> =
> =
> Screenshots
> -----------
> =
> with no filtering
> http://git.reviewboard.kde.org/r/101219/s/136/
> With filtering
> http://git.reviewboard.kde.org/r/101219/s/137/
> =
> =
> Thanks,
> =
> Eli
> =
>
[Attachment #3 (text/html)]
<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 \
solid;"> <tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/101219/">http://git.reviewboard.kde.org/r/101219/</a>
</td>
</tr>
</table>
<br />
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">This review has been \
submitted with commit a1a697436e16f86a545d0aced1f35dbd16772856 by eli \
mackenzie.</pre> <br />
<p>- Commit</p>
<br />
<p>On April 24th, 2011, 7:49 a.m., Eli MacKenzie wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" \
style="background-image: \
url('http://git.reviewboard.kde.org/media/rb/images/review_request_box_top_bg.png'); \
background-position: left top; background-repeat: repeat-x; border: 1px black \
solid;"> <tr>
<td>
<div>Review request for kdelibs and Rafael Fernández López.</div>
<div>By Eli MacKenzie.</div>
<p style="color: grey;"><i>Updated April 24, 2011, 7:49 a.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: \
1px solid #b8b5a0"> <tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;">Ideally KWidgetItemDelegate::createItemWidgets would take a const \
QModelIndex& argument. Unfortunately its public API, so the signature cannot be \
changed until KDE5. Additionally, its pure virtual so the option of adding a \
createItemWidgets(const QModelIndex&) method won't work either.
Instead, I've added a dynamic property called "creatingWidgetForIndex" \
to the delegate, that is only present while the widget pool is creating widgets. This \
means the only change kwidgetitemdelegate.h is documentation. The QModelIndex could \
be stashed on one of the d-objects and made available through an accessor, but that \
would mean that d-object would have to carry a QModelIndex that would be invalid \
almost all of the time.
Another benefit is that an application where this feature is desired (for KDE < \
4.7) only has to include an the new version of kwidgetitemdelegatepool.cpp, which is \
easy to keep in sync with kdelibs.
This change allows you to set a delegate for a particular column in the model, and \
put a type of widget in each row that works best for the data.</pre> </td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: \
1px solid #b8b5a0"> <tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;">In my test app I started with a model that only included 5 rows, and \
would only create a widget for row 2. As long as KWidgetItemDelegate::updateWidgets \
doesn't assume that the QList<QWidget*> argument has something in it, all \
is well. After implementing this change I caused the model to change the type of \
widget for each row per the index pulled out of the property, again keeping \
updateWidgets in sync, and have encountered no problems.</pre> </td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>kdeui/itemviews/kwidgetitemdelegate.h <span style="color: \
grey">(58dd60868f476d925f3abd53e67b22c1ed7149ac)</span></li>
<li>kdeui/itemviews/kwidgetitemdelegatepool.cpp <span style="color: \
grey">(b287584a594e97a091f96447386a588acb08c59e)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/101219/diff/" style="margin-left: \
3em;">View Diff</a></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Screenshots </h1>
<div>
<a href="http://git.reviewboard.kde.org/r/101219/s/136/"><img \
src="http://git.reviewboard.kde.org/media/uploaded/images/2011/04/26/tb2_400x100.png" \
style="border: 1px black solid;" alt="with no filtering" /></a>
<a href="http://git.reviewboard.kde.org/r/101219/s/137/"><img \
src="http://git.reviewboard.kde.org/media/uploaded/images/2011/04/26/tb3_400x100.png" \
style="border: 1px black solid;" alt="With filtering" /></a>
</div>
</td>
</tr>
</table>
</div>
</body>
</html>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic