[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-panel-devel
Subject: Re: Review Request 114149: Fix plasma on multiple monitors
From: "David Edmundson" <david () davidedmundson ! co ! uk>
Date: 2013-11-27 13:44:15
Message-ID: 20131127134415.21775.3629 () vidsolbach ! de
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
> On Nov. 27, 2013, 1:02 p.m., Martin Gräßlin wrote:
> > src/shell/desktopview.cpp, lines 144-147
> > <http://git.reviewboard.kde.org/r/114149/diff/1/?file=220663#file220663line144>
> >
> > why did you introduce this new slot? It looks to me like you could just connet to \
> > setGeometry directly?
It's not a slot in QWindow and I can't use a lambda because then I can't disconnect.
> On Nov. 27, 2013, 1:02 p.m., Martin Gräßlin wrote:
> > src/shell/shellcorona.cpp, line 200
> > <http://git.reviewboard.kde.org/r/114149/diff/1/?file=220665#file220665line200>
> >
> > any specific reason why you use Q_FOREACH and not the new for-syntax?
> > for (QScreen *screen : QGuiApplications::screen())
> >
> > I prefer the new for syntax as it's not a macro and also works with multiple \
> > templates and is not YELLING ;-)
ok, I shall change to the NEW TEMPLATE FOR YOU.
> On Nov. 27, 2013, 1:02 p.m., Martin Gräßlin wrote:
> > src/shell/shellcorona.cpp, lines 396-397
> > <http://git.reviewboard.kde.org/r/114149/diff/1/?file=220665#file220665line396>
> >
> > suggestion:
> > for (auto i = d->views.begin(); i != d->views.end(); i++) {
> > // if-condition as is
> > }
++
- David
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/114149/#review44567
-----------------------------------------------------------
On Nov. 27, 2013, 11:34 a.m., David Edmundson wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/114149/
> -----------------------------------------------------------
>
> (Updated Nov. 27, 2013, 11:34 a.m.)
>
>
> Review request for Plasma.
>
>
> Repository: plasma-framework
>
>
> Description
> -------
>
> Set geometry to fill each screen correctly
>
> DesktopView incorrectly filled the geometry of screen()
> screen() will be the screen of the parent shell, not the correct
> screen.
>
> As we were already part-using QScreen, shellcorona is ported to use that instead
> of QDesktopWidget, so we can keep track of which screen is actually removed instead \
> of just assuming it was the last one.
>
> Diffs
> -----
>
> src/shell/desktopview.h b8b9caa
> src/shell/desktopview.cpp 90a5730
> src/shell/shellcorona.h ee5e2bc
> src/shell/shellcorona.cpp ef6fbe2
>
> Diff: http://git.reviewboard.kde.org/r/114149/diff/
>
>
> Testing
> -------
>
> Used two monitors, and I now have a different wallpaper on each \o/
>
> There's a crash on unplugging a monitor, this happens before my patch and appears \
> to be unrelated.
>
> Thanks,
>
> David Edmundson
>
>
[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/114149/">http://git.reviewboard.kde.org/r/114149/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <p style="margin-top: 0;">On November 27th, 2013, 1:02 p.m. UTC, <b>Martin \
Gräßlin</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/114149/diff/1/?file=220663#file220663line144" \
style="color: black; font-weight: bold; text-decoration: \
underline;">src/shell/desktopview.cpp</a> <span style="font-weight: normal;">
(Diff revision 1)
</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">141</font></th> <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span \
class="kt">void</span> <span class="n">DesktopView</span><span \
class="o">::</span><span class="n">geometryChanged</span><span \
class="p">(</span><span class="k">const</span> <span class="n">QRect</span> <span \
class="o">&</span><span class="n">newGeometry</span><span \
class="p">)</span></pre></td> </tr>
<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">142</font></th> <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span \
class="p">{</span></pre></td> </tr>
<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">143</font></th> <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span \
class="n">setGeometry</span><span class="p">(</span><span \
class="n">newGeometry</span><span class="p">);</span></pre></td> </tr>
<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">144</font></th> <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><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;">why did you introduce \
this new slot? It looks to me like you could just connet to setGeometry \
directly?</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;">It's \
not a slot in QWindow and I can't use a lambda because then I can't \
disconnect.</pre> <br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <p style="margin-top: 0;">On November 27th, 2013, 1:02 p.m. UTC, <b>Martin \
Gräßlin</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/114149/diff/1/?file=220665#file220665line200" \
style="color: black; font-weight: bold; text-decoration: \
underline;">src/shell/shellcorona.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; \
">QString ShellCorona::shell() const</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">197</font></th> <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span \
class="n">Q_FOREACH</span> <span class="p">(</span><span class="n">QScreen</span> \
<span class="o">*</span><span class="n">screen</span><span class="p">,</span> <span \
class="n">QGuiApplication</span><span class="o">::</span><span \
class="n">screens</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;">any specific reason why \
you use Q_FOREACH and not the new for-syntax? for (QScreen *screen : \
QGuiApplications::screen())
I prefer the new for syntax as it's not a macro and also works with multiple \
templates and is not YELLING ;-)</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 shall \
change to the NEW TEMPLATE FOR YOU.</pre> <br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <p style="margin-top: 0;">On November 27th, 2013, 1:02 p.m. UTC, <b>Martin \
Gräßlin</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/114149/diff/1/?file=220665#file220665line396" \
style="color: black; font-weight: bold; text-decoration: \
underline;">src/shell/shellcorona.cpp</a> <span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2">388</font></th> <td bgcolor="#fdfebc" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="p">}</span> \
<span class="k">else</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">383</font></th> <td bgcolor="#fdfebc" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span \
class="n">QList</span><span class="o"><</span><span \
class="n">DesktopView</span><span class="o">*>::</span><span \
class="n">Iterator</span> <span class="n">i</span> <span class="o">=</span> <span \
class="n">d</span><span class="o">-></span><span class="n">views</span><span \
class="p">.</span><span class="n">begin</span><span class="p">();</span></pre></td> \
</tr>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2">389</font></th> <td bgcolor="#fdfebc" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "> <span \
class="k">for</span> <span class="p">(</span><span class="kt">int</span> <span \
class="n">i</span> <span class="o">=</span> <span class="n">d</span><span \
class="o">-></span><span class="n">desktopWidget</span><span \
class="o">-></span><span class="n">screenCount</span><span class="p">();</span> \
<span class="n">i</span> <span class="o"><</span> <span class="n">d</span><span \
class="o">-></span><span class="n">views</span><span class="p">.</span><span \
class="n">count</span><span class="p">();</span> <span class="o">++</span><span \
class="n">i</span><span class="p">)</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">384</font></th> <td bgcolor="#fdfebc" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span \
class="k">while</span> <span class="p">(</span><span class="n">i</span><span \
class="o">!=</span><span class="n">d</span><span class="o">-></span><span \
class="n">views</span><span class="p">.</span><span class="n">end</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;">suggestion: for (auto i \
= d->views.begin(); i != d->views.end(); i++) { // if-condition as is
}</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;">++</pre> \
<br />
<p>- David</p>
<br />
<p>On November 27th, 2013, 11:34 a.m. UTC, David Edmundson 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 Plasma.</div>
<div>By David Edmundson.</div>
<p style="color: grey;"><i>Updated Nov. 27, 2013, 11:34 a.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
plasma-framework
</div>
<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;">Set geometry to fill each screen correctly
DesktopView incorrectly filled the geometry of screen()
screen() will be the screen of the parent shell, not the correct
screen.
As we were already part-using QScreen, shellcorona is ported to use that instead
of QDesktopWidget, so we can keep track of which screen is actually removed instead \
of just assuming it was the last one. </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;">Used two monitors, and I now have a different wallpaper on each \o/
There's a crash on unplugging a monitor, this happens before my patch and appears \
to be unrelated.</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>src/shell/desktopview.h <span style="color: grey">(b8b9caa)</span></li>
<li>src/shell/desktopview.cpp <span style="color: grey">(90a5730)</span></li>
<li>src/shell/shellcorona.h <span style="color: grey">(ee5e2bc)</span></li>
<li>src/shell/shellcorona.cpp <span style="color: grey">(ef6fbe2)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/114149/diff/" style="margin-left: \
3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>
_______________________________________________
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic