[prev in list] [next in list] [prev in thread] [next in thread]
List: kwin
Subject: Re: Review Request 110199: Move RootInfo and WinInfo into an own header and impl file
From: Thomas_Lübking <thomas.luebking () gmail ! com>
Date: 2013-04-29 18:42:52
Message-ID: 20130429184252.21718.63874 () vidsolbach ! de
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
> On April 29, 2013, 2:05 p.m., Thomas Lübking wrote:
> > kwin/netinfo.cpp, line 50
> > <http://git.reviewboard.kde.org/r/110199/diff/2/?file=141334#file141334line50>
> >
> > it's out of scope here, but:
> >
> > #define REQUIRES_CLIENT_FOR(_W_) \
> > Client* c = workspace->findClient(WindowMatchPredicate(w)); \
> > if (!c) return;
> >
> > ?
>
> Martin Gräßlin wrote:
> not sure whether it's generally useful as it would require that the Client* is \
> called "c". With C++11 and closure one could probably construct something:
> withClient(WindowMatchPredicate(w), []() {
> // actual code
> });
>
> (no idea whether my lambda syntax is correct)
>
> Thomas Lübking wrote:
> I just thought about the local scope, because really every function somehow invokes \
> this as precondition here.
> Martin Gräßlin wrote:
> well we have similar code at other places too. E.g. the activeClient check in \
> useractions. It's something I already have on the radar for once we are on C++11 as \
> I find that an interesting topic ;-)
Stupid question and totally OT:
i just checked and wheezy apparently has gcc 4.7 for only some architectures, but not \
for
armel armhf ia64 mips mipsel powerpc s390 s390x sparc
If that's a showstopper, there's likely no C++11 before <whenever current \
experimental becomes stable, since sid doesn't have full 4.7 support either> ...
- Thomas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/110199/#review31759
-----------------------------------------------------------
On April 29, 2013, 7:14 a.m., Martin Gräßlin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/110199/
> -----------------------------------------------------------
>
> (Updated April 29, 2013, 7:14 a.m.)
>
>
> Review request for kwin.
>
>
> Description
> -------
>
> Move RootInfo and WinInfo into an own header and impl file
>
> Main motivation for this change is that it's unhandy to have the class
> definition in workspace.h and client.h while the implementation is in
> events.cpp although nothing in events.cpp uses it directly.
>
> By getting it out of workspace.h we get the header a little bit smaller
> which should improve compile time given that it's included almost
> everywhere.
>
> In events.cpp the enum usage is changed to NETWinInfo as that's the class
> where they are defined.
>
> RootInfo does no longer hold a workspace pointer. Where it's needed it
> uses the singleton accessor of Workspace.
>
>
> Diffs
> -----
>
> kwin/client.h 36f57dd12acd1ccc74142ad0ddcdaa940e128651
> kwin/activation.cpp 19b962c20e656197ae4ec0b764036a7dcd689027
> kwin/CMakeLists.txt e386e88aa82f1dbced291d98f6620fd6fdf6e960
> kwin/deleted.cpp a2840ba60c71fd2f22c583516b39bc1310d96194
> kwin/events.cpp 253fb01ba9f26401b3aadc3ccedbc3580a11a3d7
> kwin/geometry.cpp 673edbf39be9506ebb844a88754e0ac6b7337249
> kwin/layers.cpp 63529f8c540be6f34cbd2db5e59f2406b9705573
> kwin/manage.cpp 36d3578d0358251fa803e723569c967b6101dc36
> kwin/netinfo.h PRE-CREATION
> kwin/netinfo.cpp PRE-CREATION
> kwin/toplevel.h c4412a0515956ff810e4d27d8260a37ba0649071
> kwin/workspace.h 3ea9c2e27add01c4c6130442c95dc2486efb84d4
> kwin/workspace.cpp 5aee3f8e7f7e90cc102126155a169be1406eef07
>
> Diff: http://git.reviewboard.kde.org/r/110199/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/110199/">http://git.reviewboard.kde.org/r/110199/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <p style="margin-top: 0;">On April 29th, 2013, 2:05 p.m. UTC, <b>Thomas \
Lübking</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/110199/diff/2/?file=141334#file141334line50" \
style="color: black; font-weight: bold; text-decoration: \
underline;">kwin/netinfo.cpp</a> <span style="font-weight: normal;">
(Diff revision 2)
</span>
</th>
</tr>
</thead>
<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">50</font></th> <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span \
class="k">if</span> <span class="p">(</span><span class="n">Client</span><span \
class="o">*</span> <span class="n">c</span> <span class="o">=</span> <span \
class="n">workspace</span><span class="o">-></span><span \
class="n">findClient</span><span class="p">(</span><span \
class="n">WindowMatchPredicate</span><span class="p">(</span><span \
class="n">w</span><span class="p">)))</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;">it's out of scope \
here, but:
#define REQUIRES_CLIENT_FOR(_W_) \
Client* c = workspace->findClient(WindowMatchPredicate(w)); \
if (!c) return;
?</pre>
</blockquote>
<p>On April 29th, 2013, 2:23 p.m. UTC, <b>Martin Gräßlin</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">not sure whether \
it's generally useful as it would require that the Client* is called \
"c". With C++11 and closure one could probably construct something:
withClient(WindowMatchPredicate(w), []() {
// actual code
});
(no idea whether my lambda syntax is correct)</pre>
</blockquote>
<p>On April 29th, 2013, 2:33 p.m. UTC, <b>Thomas Lübking</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <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 just thought about the \
local scope, because really every function somehow invokes this as precondition \
here.</pre> </blockquote>
<p>On April 29th, 2013, 3:12 p.m. UTC, <b>Martin Gräßlin</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">well we have similar \
code at other places too. E.g. the activeClient check in useractions. It's \
something I already have on the radar for once we are on C++11 as I find that an \
interesting topic ;-)</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;">Stupid \
question and totally OT: i just checked and wheezy apparently has gcc 4.7 for only \
some architectures, but not for
armel armhf ia64 mips mipsel powerpc s390 s390x sparc
If that's a showstopper, there's likely no C++11 before <whenever current \
experimental becomes stable, since sid doesn't have full 4.7 support either> \
...</pre> <br />
<p>- Thomas</p>
<br />
<p>On April 29th, 2013, 7:14 a.m. UTC, Martin Gräßlin wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" \
style="background-image: \
url('http://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); \
background-position: left top; background-repeat: repeat-x; border: 1px black \
solid;"> <tr>
<td>
<div>Review request for kwin.</div>
<div>By Martin Gräßlin.</div>
<p style="color: grey;"><i>Updated April 29, 2013, 7:14 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;">Move RootInfo and WinInfo into an own header and impl file
Main motivation for this change is that it's unhandy to have the class
definition in workspace.h and client.h while the implementation is in
events.cpp although nothing in events.cpp uses it directly.
By getting it out of workspace.h we get the header a little bit smaller
which should improve compile time given that it's included almost
everywhere.
In events.cpp the enum usage is changed to NETWinInfo as that's the class
where they are defined.
RootInfo does no longer hold a workspace pointer. Where it's needed it
uses the singleton accessor of Workspace.</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/client.h <span style="color: \
grey">(36f57dd12acd1ccc74142ad0ddcdaa940e128651)</span></li>
<li>kwin/activation.cpp <span style="color: \
grey">(19b962c20e656197ae4ec0b764036a7dcd689027)</span></li>
<li>kwin/CMakeLists.txt <span style="color: \
grey">(e386e88aa82f1dbced291d98f6620fd6fdf6e960)</span></li>
<li>kwin/deleted.cpp <span style="color: \
grey">(a2840ba60c71fd2f22c583516b39bc1310d96194)</span></li>
<li>kwin/events.cpp <span style="color: \
grey">(253fb01ba9f26401b3aadc3ccedbc3580a11a3d7)</span></li>
<li>kwin/geometry.cpp <span style="color: \
grey">(673edbf39be9506ebb844a88754e0ac6b7337249)</span></li>
<li>kwin/layers.cpp <span style="color: \
grey">(63529f8c540be6f34cbd2db5e59f2406b9705573)</span></li>
<li>kwin/manage.cpp <span style="color: \
grey">(36d3578d0358251fa803e723569c967b6101dc36)</span></li>
<li>kwin/netinfo.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>kwin/netinfo.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>kwin/toplevel.h <span style="color: \
grey">(c4412a0515956ff810e4d27d8260a37ba0649071)</span></li>
<li>kwin/workspace.h <span style="color: \
grey">(3ea9c2e27add01c4c6130442c95dc2486efb84d4)</span></li>
<li>kwin/workspace.cpp <span style="color: \
grey">(5aee3f8e7f7e90cc102126155a169be1406eef07)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/110199/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