'Re: Review Request: Correctly display popups when using multiple'

[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-panel-devel
Subject:    Re: Review Request: Correctly display popups when using multiple
From:       "Matthias Fuchs" <mat69 () gmx ! net>
Date:       2011-07-28 16:46:15
Message-ID: 20110728164615.10330.37996 () vidsolbach ! de
[Download RAW message or body]

[Attachment #2 (multipart/alternative)]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/102118/
-----------------------------------------------------------

(Updated July 28, 2011, 4:46 p.m.)

Review request for Plasma and Aaron J. Seigo.

Summary (updated)
-------

If there are multiple screens with different resolutions or which
are not alligned the same way then it could happen that popups at
the edge were drawn (partially) offscreen.
This patch fixes that issue.

I have the feeling that the code in this method is really ugly, I wonder if there are \
plans to refactor it or to clean it up?

NOTE: I don't know if this patch might cause problems if there are animations that \
are supposed to start offscreen etc. So please you with more insight look at the \
patch to see if there could be some negative side effects.

====
I just realised that the real problem is most likely that the existing code assumes \
that the screen begins at 0,0 e.g: default:
        if (pos.y() - s.height() > 0) {//if 0 was replaced with screenRect.top() it \
would also work in the specific case described in the report  pos.ry() = pos.y() - \
s.height();  } else {
             pos.ry() = pos.y() + (int)actualItem->boundingRect().size().height() + \
1;  }

This addresses bug 276336.
    http://bugs.kde.org/show_bug.cgi?id=276336

Diffs
-----

  plasma/corona.cpp 4afef7b 

Diff: http://git.reviewboard.kde.org/r/102118/diff

Testing
-------

Thanks,

Matthias

[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/102118/">http://git.reviewboard.kde.org/r/102118/</a>
  </td>
    </tr>
   </table>
   <br />

<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 Plasma and Aaron J. Seigo.</div>
<div>By Matthias Fuchs.</div>

<p style="color: grey;"><i>Updated July 28, 2011, 4:46 p.m.</i></p>

<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description  \
(updated)</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;">If there are multiple screens with different resolutions or which are \
not alligned the same way then it could happen that popups at the edge were drawn \
(partially) offscreen. This patch fixes that issue.

I have the feeling that the code in this method is really ugly, I wonder if there are \
plans to refactor it or to clean it up?

NOTE: I don&#39;t know if this patch might cause problems if there are animations \
that are supposed to start offscreen etc. So please you with more insight look at the \
patch to see if there could be some negative side effects.

====
I just realised that the real problem is most likely that the existing code assumes \
that the screen begins at 0,0 e.g: default:
        if (pos.y() - s.height() &gt; 0) {//if 0 was replaced with screenRect.top() \
it would also work in the specific case described in the report  pos.ry() = pos.y() - \
s.height();  } else {
             pos.ry() = pos.y() + (int)actualItem-&gt;boundingRect().size().height() \
+ 1;  }</pre>
  </td>
 </tr>
</table>

<div style="margin-top: 1.5em;">
 <b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>

 <a href="http://bugs.kde.org/show_bug.cgi?id=276336">276336</a>

</div>

<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">

 <li>plasma/corona.cpp <span style="color: grey">(4afef7b)</span></li>

</ul>

<p><a href="http://git.reviewboard.kde.org/r/102118/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] 