[prev in list] [next in list] [prev in thread] [next in thread]
List: kwin
Subject: Re: Review Request: Extend ScopedCPointer from QScopedPointer with QScopedPointerPodDeleter and make
From: Martin_Gräßlin <kde () martin-graesslin ! com>
Date: 2013-01-10 7:05:16
Message-ID: 20130110070516.14829.74251 () vidsolbach ! de
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
> On Jan. 9, 2013, 5:25 p.m., Fredrik Höglund wrote:
> > kwin/geometry.cpp, line 770
> > <http://git.reviewboard.kde.org/r/108242/diff/1/?file=105527#file105527line770>
> >
> > This change is problematic, because this function will have to update the \
> > xcb_get_geometry_reply_t to reflect the new position when createClient() is \
> > ported to xcb.
> > Right now it doesn't matter because createClient() and createUnmanaged() request \
> > the attributes and geometry again, but that should be fixed.
ok, I can change it back
> On Jan. 9, 2013, 5:25 p.m., Fredrik Höglund wrote:
> > kwin/workspace.cpp, line 415
> > <http://git.reviewboard.kde.org/r/108242/diff/1/?file=105531#file105531line415>
> >
> > I'm a bit skeptical of this change. The wrappers are many times larger than a \
> > cookie, and are relatively expensive to create and destroy since QSharedPointer \
> > uses a d-pointer.
> > This probably isn't a big deal when sending one or two requests, but when dealing \
> > with arrays of unknown sizes it can be.
what would you say to have the Wrapper not using a QSharedPointer? The class just \
frees the reply in the dtor unless a method take() is invoked with the same semantics \
as QScopedPointer::take()?
That way we would (in general) get rid of the overhead of the QSharedPointer.
> On Jan. 9, 2013, 5:25 p.m., Fredrik Höglund wrote:
> > kwin/xcbutils.h, line 101
> > <http://git.reviewboard.kde.org/r/108242/diff/1/?file=105532#file105532line101>
> >
> > The name of this class is inconsistent with the other wrappers.
hey, you know that I'm bad with naming, so you have to do a better suggestion ;-) \
What about "WindowTree"?
- Martin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/108242/#review25083
-----------------------------------------------------------
On Jan. 7, 2013, 1:58 p.m., Martin Gräßlin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/108242/
> -----------------------------------------------------------
>
> (Updated Jan. 7, 2013, 1:58 p.m.)
>
>
> Review request for kwin and Fredrik Höglund.
>
>
> Description
> -------
>
> Extend ScopedCPointer from QScopedPointer with QScopedPointerPodDeleter
>
>
> Make use of new Xcb Wrapper classes
>
> Use WindowAttributes and WindowGeometry everywhere where the xcb commands
> had already been used.
>
> Introduces another wrapper for overlay window and a subclass for query
> tree which also wrapps the children command.
>
>
> Diffs
> -----
>
> kwin/composite.cpp a58a7e0918807f55785c17e74beb8baf83053b71
> kwin/geometry.cpp 7eb7e94d2804aa512b3da23b81c40ffe4b899811
> kwin/overlaywindow.cpp d6359c7829dee6f70babc4585b61dcae521be3ea
> kwin/utils.h e0e565248c85359db6c924a3ee2da6845d75b5eb
> kwin/workspace.h 252a959e89167d2f938c9e62d460087b50f75412
> kwin/workspace.cpp 07e65ae49589230a9813a1f4ec7a230f77dc9d39
> kwin/xcbutils.h ebe7c283c1704328337ce56402999f98e57dffed
>
> Diff: http://git.reviewboard.kde.org/r/108242/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Martin Gräßlin
>
>
[Attachment #5 (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/108242/">http://git.reviewboard.kde.org/r/108242/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <p style="margin-top: 0;">On January 9th, 2013, 5:25 p.m., <b>Fredrik \
Höglund</b> wrote:</p> <blockquote style="margin-left: 1em; border-left: 2px solid \
#d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; \
border-collapse: collapse; margin: 2px padding: 2px;"> <thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; \
font-size: 9pt; padding: 4px 8px; text-align: left;"> <a \
href="http://git.reviewboard.kde.org/r/108242/diff/1/?file=105527#file105527line770" \
style="color: black; font-weight: bold; text-decoration: \
underline;">kwin/geometry.cpp</a> <span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">void \
Workspace::setClientIsMoving(Client *c)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2">770</font></th> <td bgcolor="#fdfebc" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="kt">void</span> \
<span class="n">Workspace</span><span class="o">::</span><span \
class="n">fixPositionAfterCrash</span><span class="p">(</span><span \
class="n">xcb_window_t</span> <span class="n">w</span><span class="p">,</span> <span \
class="k">const</span> <span class="n"><span \
class="hl">xcb_get_geometry_reply_t</span></span><span class="hl"> </span><span \
class="o"><span class="hl">*</span></span><span class="n">geometry</span><span \
class="p">)</span></pre></td> <th bgcolor="#e9eaa8" style="border-left: 1px solid \
#C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font \
size="2">770</font></th> <td bgcolor="#fdfebc" width="50%"><pre style="font-size: \
8pt; line-height: 140%; margin: 0; "><span class="kt">void</span> <span \
class="n">Workspace</span><span class="o">::</span><span \
class="n">fixPositionAfterCrash</span><span class="p">(</span><span \
class="n">xcb_window_t</span> <span class="n">w</span><span class="p">,</span> <span \
class="k">const</span> <span class="n"><span class="hl">QRect</span></span><span \
class="hl"> </span><span class="o"><span class="hl">&</span></span><span \
class="n">geometry</span><span class="p">)</span></pre></td> </tr>
</tbody>
</table>
<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 change is \
problematic, because this function will have to update the xcb_get_geometry_reply_t \
to reflect the new position when createClient() is ported to xcb.
Right now it doesn't matter because createClient() and createUnmanaged() request \
the attributes and geometry again, but that should be fixed. </pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; \
white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">ok, I can \
change it back</pre> <br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <p style="margin-top: 0;">On January 9th, 2013, 5:25 p.m., <b>Fredrik \
Höglund</b> wrote:</p> <blockquote style="margin-left: 1em; border-left: 2px solid \
#d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; \
border-collapse: collapse; margin: 2px padding: 2px;"> <thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; \
font-size: 9pt; padding: 4px 8px; text-align: left;"> <a \
href="http://git.reviewboard.kde.org/r/108242/diff/1/?file=105531#file105531line415" \
style="color: black; font-weight: bold; text-decoration: \
underline;">kwin/workspace.cpp</a> <span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">void \
Workspace::init()</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2">414</font></th> <td bgcolor="#fdfebc" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">Q<span \
class="hl">Vector</span></span><span class="o"><span \
class="hl"><</span></span><span class="n"><span \
class="hl">xcb_get_w</span>indow<span class="hl">_a</span>ttributes<span \
class="hl">_cookie_t</span></span><span class="o"><span \
class="hl">></span></span><span class="hl"> </span><span class="n"><span \
class="hl">a</span>ttr<span class="hl">_cooki</span>es</span><span \
class="p">;</span></pre></td> <th bgcolor="#e9eaa8" style="border-left: 1px solid \
#C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font \
size="2">413</font></th> <td bgcolor="#fdfebc" width="50%"><pre style="font-size: \
8pt; line-height: 140%; margin: 0; "> <span class="n">Q<span \
class="hl">List</span></span><span class="o"><span class="hl"><</span></span><span \
class="n"><span class="hl">Xcb</span></span><span class="o"><span \
class="hl">::</span></span><span class="n"><span class="hl">W</span>indow<span \
class="hl">A</span>ttributes</span><span class="o"><span \
class="hl">></span></span><span class="hl"> </span><span class="n"><span \
class="hl">windowA</span>ttr<span class="hl">ibut</span>es</span><span \
class="p">;</span></pre></td> </tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I'm a bit skeptical \
of this change. The wrappers are many times larger than a cookie, and are relatively \
expensive to create and destroy since QSharedPointer uses a d-pointer.
This probably isn't a big deal when sending one or two requests, but when dealing \
with arrays of unknown sizes it can be. </pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; \
white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">what would \
you say to have the Wrapper not using a QSharedPointer? The class just frees the \
reply in the dtor unless a method take() is invoked with the same semantics as \
QScopedPointer::take()?
That way we would (in general) get rid of the overhead of the QSharedPointer.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <p style="margin-top: 0;">On January 9th, 2013, 5:25 p.m., <b>Fredrik \
Höglund</b> wrote:</p> <blockquote style="margin-left: 1em; border-left: 2px solid \
#d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; \
border-collapse: collapse; margin: 2px padding: 2px;"> <thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; \
font-size: 9pt; padding: 4px 8px; text-align: left;"> <a \
href="http://git.reviewboard.kde.org/r/108242/diff/1/?file=105532#file105532line101" \
style="color: black; font-weight: bold; text-decoration: \
underline;">kwin/xcbutils.h</a> <span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">class \
WindowGeometry : public Wrapper<xcb_get_geometry_reply_t, \
xcb_get_geometry_cookie_t, &xcb_get_geometry_reply, \
&xcb_get_geometry_unchecked></pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2"></font></th> <td bgcolor="#c5ffc4" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td> <th \
bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid \
#C0C0C0;" align="right"><font size="2">101</font></th> <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span \
class="n">class</span> <span class="n">QueryTree</span> <span class="o">:</span> \
<span class="n">public</span> <span class="n">Wrapper</span><span \
class="o"><</span><span class="n">xcb_query_tree_reply_t</span><span \
class="p">,</span> <span class="n">xcb_query_tree_cookie_t</span><span \
class="p">,</span> <span class="o">&</span><span \
class="n">xcb_query_tree_reply</span><span class="p">,</span> <span \
class="o">&</span><span class="n">xcb_query_tree_unchecked</span><span \
class="o">></span></pre></td> </tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">The name of this class \
is inconsistent with the other wrappers.</pre> </blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; \
white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">hey, you \
know that I'm bad with naming, so you have to do a better suggestion ;-) What \
about "WindowTree"?</pre> <br />
<p>- Martin</p>
<br />
<p>On January 7th, 2013, 1:58 p.m., Martin Gräßlin 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 kwin and Fredrik Höglund.</div>
<div>By Martin Gräßlin.</div>
<p style="color: grey;"><i>Updated Jan. 7, 2013, 1:58 p.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;">Extend ScopedCPointer from QScopedPointer with QScopedPointerPodDeleter
Make use of new Xcb Wrapper classes
Use WindowAttributes and WindowGeometry everywhere where the xcb commands
had already been used.
Introduces another wrapper for overlay window and a subclass for query
tree which also wrapps the children command.</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>kwin/composite.cpp <span style="color: \
grey">(a58a7e0918807f55785c17e74beb8baf83053b71)</span></li>
<li>kwin/geometry.cpp <span style="color: \
grey">(7eb7e94d2804aa512b3da23b81c40ffe4b899811)</span></li>
<li>kwin/overlaywindow.cpp <span style="color: \
grey">(d6359c7829dee6f70babc4585b61dcae521be3ea)</span></li>
<li>kwin/utils.h <span style="color: \
grey">(e0e565248c85359db6c924a3ee2da6845d75b5eb)</span></li>
<li>kwin/workspace.h <span style="color: \
grey">(252a959e89167d2f938c9e62d460087b50f75412)</span></li>
<li>kwin/workspace.cpp <span style="color: \
grey">(07e65ae49589230a9813a1f4ec7a230f77dc9d39)</span></li>
<li>kwin/xcbutils.h <span style="color: \
grey">(ebe7c283c1704328337ce56402999f98e57dffed)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/108242/diff/" style="margin-left: \
3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>
_______________________________________________
kwin mailing list
kwin@kde.org
https://mail.kde.org/mailman/listinfo/kwin
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic