[prev in list] [next in list] [prev in thread] [next in thread]
List: kwin
Subject: Re: Review Request: Compositor singleton and remove superfluous Compositor checks in events.cpp
From: Martin_Gräßlin <kde () martin-graesslin ! com>
Date: 2012-09-03 4:27:47
Message-ID: 20120903042747.12048.62812 () vidsolbach ! de
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
> On Sept. 2, 2012, 10:05 p.m., Thomas Lübking wrote:
> > kwin/composite.cpp, line 885
> > <http://git.reviewboard.kde.org/r/106255/diff/4/?file=82846#file82846line885>
> >
> > The next line implies !c is legal, but Compositor::self() has an assert on this.
> > Either the assert must go and ::self() == NULL is legal or this (and similar?) \
> > checks must be prefixed by a ::isCreated() check
the assert will go, I only added it to catch possible errors.
- Martin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/106255/#review18443
-----------------------------------------------------------
On Sept. 2, 2012, 9:20 a.m., Martin Gräßlin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106255/
> -----------------------------------------------------------
>
> (Updated Sept. 2, 2012, 9:20 a.m.)
>
>
> Review request for kwin.
>
>
> Description
> -------
>
> Remove superfluous Compositor checks in events.cpp
>
> compositing() ensures that m_compositor is not null.
>
> Make the Compositor a proper Singleton
>
> The Compositor class actually behaves like a Singleton so it should be
> one. Therefore four static methods are added:
> * self() to access the Singleton
> * createCompositor() to be used by Workspace to create the instance
> * isCreated() to have a simple check whether the Singleton is already
> created
> * compositing() as a shortcut to test whether the compositor has been
> created and is active
>
> The isCreated() check is actually required as especially Clients might
> be created and trying to access the Compositor before it is setup.
>
>
> Diffs
> -----
>
> kwin/bridge.cpp 670063e
> kwin/client.cpp d2a6086
> kwin/composite.h a4a3710
> kwin/composite.cpp 74aed13
> kwin/events.cpp 23e1921
> kwin/geometry.cpp 19a911d
> kwin/paintredirector.cpp 55b20c4
> kwin/workspace.h fdd2223
> kwin/workspace.cpp e03e5e7
>
> Diff: http://git.reviewboard.kde.org/r/106255/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/106255/">http://git.reviewboard.kde.org/r/106255/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <p style="margin-top: 0;">On September 2nd, 2012, 10:05 p.m., <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/106255/diff/4/?file=82846#file82846line885" \
style="color: black; font-weight: bold; text-decoration: \
underline;">kwin/composite.cpp</a> <span style="font-weight: normal;">
(Diff revision 4)
</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 \
Toplevel::damageNotifyEvent(XDamageNotifyEvent* e)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2">877</font></th> <td bgcolor="#fdfebc" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "> <span \
class="n">Compositor</span> <span class="o">*</span><span class="n">c</span> <span \
class="o">=</span> <span class="n"><span class="hl">workspace</span></span><span \
class="p"><span class="hl">()</span></span><span class="o"><span \
class="hl">-></span></span><span class="n"><span \
class="hl">c</span>ompositor</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">885</font></th> <td bgcolor="#fdfebc" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span \
class="n">Compositor</span> <span class="o">*</span><span class="n">c</span> <span \
class="o">=</span> <span class="n"><span class="hl">C</span>ompositor</span><span \
class="o"><span class="hl">::</span></span><span class="n"><span \
class="hl">self</span></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;">The next line implies !c \
is legal, but Compositor::self() has an assert on this. Either the assert must go and \
::self() == NULL is legal or this (and similar?) checks must be prefixed by a \
::isCreated() check</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;">the assert \
will go, I only added it to catch possible errors.</pre> <br />
<p>- Martin</p>
<br />
<p>On September 2nd, 2012, 9:20 a.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.</div>
<div>By Martin Gräßlin.</div>
<p style="color: grey;"><i>Updated Sept. 2, 2012, 9:20 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;">Remove superfluous Compositor checks in events.cpp
compositing() ensures that m_compositor is not null.
Make the Compositor a proper Singleton
The Compositor class actually behaves like a Singleton so it should be
one. Therefore four static methods are added:
* self() to access the Singleton
* createCompositor() to be used by Workspace to create the instance
* isCreated() to have a simple check whether the Singleton is already
created
* compositing() as a shortcut to test whether the compositor has been
created and is active
The isCreated() check is actually required as especially Clients might
be created and trying to access the Compositor before it is setup.</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/bridge.cpp <span style="color: grey">(670063e)</span></li>
<li>kwin/client.cpp <span style="color: grey">(d2a6086)</span></li>
<li>kwin/composite.h <span style="color: grey">(a4a3710)</span></li>
<li>kwin/composite.cpp <span style="color: grey">(74aed13)</span></li>
<li>kwin/events.cpp <span style="color: grey">(23e1921)</span></li>
<li>kwin/geometry.cpp <span style="color: grey">(19a911d)</span></li>
<li>kwin/paintredirector.cpp <span style="color: grey">(55b20c4)</span></li>
<li>kwin/workspace.h <span style="color: grey">(fdd2223)</span></li>
<li>kwin/workspace.cpp <span style="color: grey">(e03e5e7)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/106255/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