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

List:       kde-frameworks-devel
Subject:    Re: Review Request 126324: [MSWin/OS X] save and restore window geometry instead of only size (WIP/S
From:       "Jaime Torres Amate" <jtamate () gmail ! com>
Date:       2015-12-26 9:17:43
Message-ID: 20151226091743.29845.34235 () mimi ! kde ! org
[Download RAW message or body]

--===============6584605355956298157==
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 8bit



> On Dec. 17, 2015, 4:16 p.m., Martin Gräßlin wrote:
> > 
> 
> Jaime Torres Amate wrote:
> Hello,
> 
> This is just a warning to know if this patch has been tested in a two monitor environment in \
> a laptop. In a pyqt application I save and restore the size and position of the window \
> (without additional checks), using: settings.setValue("size", self.ui.size())
> settings.setValue("pos", self.ui.pos())   # save
> ----
> self.ui.resize(settings.value("size", QSize(400, 400)))
> self.ui.move(settings.value("pos", QPoint(200, 200)))   # restore
> 
> It works perfectly fine except in this case:
> The window, when saved, was in a monitor that disappears in the next run. In the next run \
> (without the monitor) the window is not shown, because it is still in the other monitor, but \
> it is shown in the taskbar, I have to go to the taskbar or with an advanced task manager, \
> tell the window to become maximized, then it is shown. If this patch shows the window when \
> the monitor disappears, then, please, ignore this comment. 
> René J.V. Bertin wrote:
> What OS/windowing environment is that?
> 
> To the best of my knowledge, OS X will rearrange windows when a monitor is disconnected \
> (regardless whether on a laptop or desktop host) and should do the same when reopening a \
> window in an offscreen position if that window isn't meant to be offscreen. I haven't yet \
> tested this because of the rearranging thing, but good point. I'll see if it can be simulated \
> by storing an offscreen position (this is where the binary nature of the saved data is \
> annoying indeed!) 
> Jaime Torres Amate wrote:
> Oh!, I'm sorry, I forgot to say. It is a windows 7. It rearranges other windows, but as it's \
> position is restored, it goes to the non connected monitor. I hope it does not happen with \
> this patch (In linux it does not happen to that pyqt program). 
> René J.V. Bertin wrote:
> Even if it doesn't on OS X, someone will have to test on MS Windows.
> 
> I presume one can obtain the limits within which the position has to lie and constrain the \
> position so that at least part of the window is visible (even if those limits reflect only \
> the rectangle within which the available screens lie). 
> René J.V. Bertin wrote:
> I just checked this, doing `frameGeo.adjust()` with a selection of huge values in \
> `GeometryData::applyToWindow()`. As I thought, OS X doesn't allow you to open a regular \
> window completely offscreen. The requested position is altered such that there's always just \
> enough of it that remains visible and available for grabbing and moving it to a better \
> position. 
> Jaime: do you know what `self.ui.pos()` resolves to, `QWindow::position()` or \
> `QWindow::framePosition()`? I would not really surprise me if trying to set the former \
> (through `setPosition()`) gives different results than using the latter (through \
> `setFramePosition()`) in case of offscreen co-ordinates.

Hi, the documentation here is lacking the description, but I guess it is position(), as there \
are other bindings for frameGeometry(). Thanks for checking it. I have no more objections.
Best regards.


- Jaime Torres


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126324/#review89665
-----------------------------------------------------------


On Dec. 14, 2015, 4:04 p.m., René J.V. Bertin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126324/
> -----------------------------------------------------------
> 
> (Updated Dec. 14, 2015, 4:04 p.m.)
> 
> 
> Review request for KDE Software on Mac OS X and KDE Frameworks.
> 
> 
> Repository: kconfig
> 
> 
> Description
> -------
> 
> In KDElibs4, the KMainWindow::saveWindowSize() and KMainWindow::restoreWindowSize() function \
> saved and restored not only the size but also the position (i.e. the geometry) of windows, \
> using QWidget::saveGeometry and QWidget::restoreGeometry. 
> 2 main reasons for this (according to the comments):
> - Under X11 restoring the position is tricky
> - X11 has a window manager which might be considered responsible for that functionality (and \
> I suppose most modern WMs have the feature enabled by default?) 
> Both arguments are moot on MS Windows and OS X, and on both platforms users expect to see \
> window positions restored as well as window size. On OS X there is also little choice in the \
> matter: most applications offer the geometry restore without asking (IIRC it is the same on \
> MS Windows). 
> I would thus like to propose to port the platform-specific code that existed for MS Windows \
> (and for OS X as a MacPorts patch that apparently was never submitted upstreams). I realise \
> that this violates the message conveyed by the function names but I would like to think that \
> this is a case where function is more important. 
> You may also notice that the Mac version does not store resolution-specific settings. This \
> happens to work best on OS X, where multi-screen support has been present since the early \
> nineties, and where window geometry is restored regardless of the screen resolution (i.e. \
> connect a different external screen with a different resolution, and windows will reopen as \
> they were on that screen, not with some default geometry). I required I can update the \
> comments in the header to reflect this subtlety. 
> Note that for optimal functionality a companion patch to `KMainWindow::event` is required:
> ```
> --- a/src/kmainwindow.cpp
> +++ b/src/kmainwindow.cpp
> @@ -772,7 +772,7 @@ bool KMainWindow::event(QEvent *ev)
> {
> K_D(KMainWindow);
> switch (ev->type()) {
> -#ifdef Q_OS_WIN
> +#if defined(Q_OS_WIN) || defined(Q_OS_OSX)
> case QEvent::Move:
> #endif
> case QEvent::Resize:
> ```
> 
> This ensures that the window geometry save is performed also after a move (to update the \
> position) without requiring a dummy resizing operation. Do I need to create a separate RR for \
> this change or is it small enough that I can push it if and when this RR is accepted? 
> 
> Diffs
> -----
> 
> src/gui/kwindowconfig.h 48a8f3c 
> src/gui/kwindowconfig.cpp d2f355c 
> 
> Diff: https://git.reviewboard.kde.org/r/126324/diff/
> 
> 
> Testing
> -------
> 
> On OS X 10.6 through 10.9 with various KDElibs4 versions and now with Qt 5.5.1 and frameworks \
> 5.16.0 (and Kate as a test application). I presume that the MS Windows code has been tested \
> sufficiently in KDELibs4; I have only adapted it to Qt5 and tested if it builds. 
> 
> Thanks,
> 
> René J.V. Bertin
> 
> 


--===============6584605355956298157==
MIME-Version: 1.0
Content-Type: text/html; charset="utf-8"
Content-Transfer-Encoding: 8bit




<html>
 <body>
  <div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
   <table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 solid; \
border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">  <tr>
     <td>
      This is an automatically generated e-mail. To reply, visit:
      <a href="https://git.reviewboard.kde.org/r/126324/">https://git.reviewboard.kde.org/r/126324/</a>
  </td>
    </tr>
   </table>
   <br />




























 <p>On December 25th, 2015, 12:36 p.m. UTC, <b>Jaime Torres Amate</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;"><p style="padding: 0;text-rendering: \
inherit;margin: 0;line-height: inherit;white-space: inherit;">Hello,</p> <p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This is just a \
warning to know if this patch has been tested in a two monitor environment in a laptop.  In a \
pyqt application I save and restore the size and position of the window (without additional \
checks), using:  settings.setValue("size", self.ui.size())
        settings.setValue("pos", self.ui.pos())   # save
----
        self.ui.resize(settings.value("size", QSize(400, 400)))
        self.ui.move(settings.value("pos", QPoint(200, 200)))   # restore</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: \
inherit;">It works perfectly fine except in this case:  The window, when saved, was in a \
monitor that disappears in the next run. In the next run (without the monitor) the window is \
not shown, because it is still in the other monitor, but it is shown in the taskbar, I have to \
go to the taskbar or with an advanced task manager, tell the window to become maximized, then \
it is shown.  If this patch shows the window when the monitor disappears, then, please, ignore \
this comment.</p></pre>  </blockquote>





 <p>On December 25th, 2015, 1:33 p.m. UTC, <b>René J.V. Bertin</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;"><p style="padding: 0;text-rendering: \
inherit;margin: 0;line-height: inherit;white-space: inherit;">What OS/windowing environment is \
that?</p> <p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">To the best of my knowledge, OS X will rearrange windows when a \
monitor is disconnected (regardless whether on a laptop or desktop host) and should do the same \
when reopening a window in an offscreen position if that window isn't meant to be offscreen. I \
haven't yet tested this because of the rearranging thing, but good point. I'll see if it can be \
simulated by storing an offscreen position (this is where the binary nature of the saved data \
is annoying indeed!)</p></pre>  </blockquote>





 <p>On December 25th, 2015, 4:13 p.m. UTC, <b>Jaime Torres Amate</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;"><p style="padding: 0;text-rendering: \
inherit;margin: 0;line-height: inherit;white-space: inherit;">Oh!, I'm sorry, I forgot to say. \
It is a windows 7. It rearranges other windows, but as it's position is restored, it goes to \
the non connected monitor. I hope it does not happen with this patch (In linux it does not \
happen to that pyqt program).</p></pre>  </blockquote>





 <p>On December 25th, 2015, 4:45 p.m. UTC, <b>René J.V. Bertin</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;"><p style="padding: 0;text-rendering: \
inherit;margin: 0;line-height: inherit;white-space: inherit;">Even if it doesn't on OS X, \
someone will have to test on MS Windows.</p> <p style="padding: 0;text-rendering: \
inherit;margin: 0;line-height: inherit;white-space: inherit;">I presume one can obtain the \
limits within which the position has to lie and constrain the position so that at least part of \
the window is visible (even if those limits reflect only the rectangle within which the \
available screens lie).</p></pre>  </blockquote>





 <p>On December 26th, 2015, 12:06 a.m. UTC, <b>René J.V. Bertin</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;"><p style="padding: 0;text-rendering: \
inherit;margin: 0;line-height: inherit;white-space: inherit;">I just checked this, doing <code \
style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: \
0;line-height: inherit;">frameGeo.adjust()</code> with a selection of huge values in <code \
style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: \
0;line-height: inherit;">GeometryData::applyToWindow()</code>. As I thought, OS X doesn't allow \
you to open a regular window completely offscreen. The requested position is altered such that \
there's always just enough of it that remains visible and available for grabbing and moving it \
to a better position.</p> <p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">Jaime: do you know what <code style="text-rendering: \
inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: \
inherit;">self.ui.pos()</code> resolves to, <code style="text-rendering: inherit;color: \
#4444cc;padding: 0;white-space: normal;margin: 0;line-height: \
inherit;">QWindow::position()</code> or <code style="text-rendering: inherit;color: \
#4444cc;padding: 0;white-space: normal;margin: 0;line-height: \
inherit;">QWindow::framePosition()</code>? I would not really surprise me if trying to set the \
former (through <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: \
normal;margin: 0;line-height: inherit;">setPosition()</code>) gives different results than \
using the latter (through <code style="text-rendering: inherit;color: #4444cc;padding: \
0;white-space: normal;margin: 0;line-height: inherit;">setFramePosition()</code>) in case of \
offscreen co-or  dinates.</p></pre>
 </blockquote>








<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; \
white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: \
inherit;margin: 0;line-height: inherit;white-space: inherit;">Hi, the documentation here is \
lacking the description, but I guess it is position(), as there are other bindings for \
frameGeometry(). Thanks for checking it. I have no more objections.
Best regards.</p></pre>
<br />










<p>- Jaime Torres</p>


<br />
<p>On December 14th, 2015, 4:04 p.m. UTC, René J.V. Bertin wrote:</p>








<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: 1px \
#888a85 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">  <tr>
  <td>

<div>Review request for KDE Software on Mac OS X and KDE Frameworks.</div>
<div>By René J.V. Bertin.</div>


<p style="color: grey;"><i>Updated Dec. 14, 2015, 4:04 p.m.</i></p>









<div style="margin-top: 1.5em;">
 <b style="color: #575012; font-size: 10pt;">Repository: </b>
kconfig
</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;"><p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">In KDElibs4, \
the KMainWindow::saveWindowSize() and KMainWindow::restoreWindowSize() function saved and \
restored not only the size but also the position (i.e. the geometry) of windows, using \
QWidget::saveGeometry and QWidget::restoreGeometry.</p> <p style="padding: 0;text-rendering: \
inherit;margin: 0;line-height: inherit;white-space: inherit;">2 main reasons for this \
                (according to the comments):
- Under X11 restoring the position is tricky
- X11 has a window manager which might be considered responsible for that functionality (and I \
suppose most modern WMs have the feature enabled by default?)</p> <p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Both arguments \
are moot on MS Windows and OS X, and on both platforms users expect to see window positions \
restored as well as window size. On OS X there is also little choice in the matter: most \
applications offer the geometry restore without asking (IIRC it is the same on MS Windows).</p> \
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: \
inherit;">I would thus like to propose to port the platform-specific code that existed for MS \
Windows (and for OS X as a MacPorts patch that apparently was never submitted upstreams). I \
realise that this violates the message conveyed by the function names but I would like to think \
that this is a case where function is more important.</p> <p style="padding: 0;text-rendering: \
inherit;margin: 0;line-height: inherit;white-space: inherit;">You may also notice that the Mac \
version does not store resolution-specific settings. This happens to work best on OS X, where \
multi-screen support has been present since the early nineties, and where window geometry is \
restored regardless of the screen resolution (i.e. connect a different external screen with a \
different resolution, and windows will reopen as they were on that screen, not with some \
default geometry). I required I can update the comments in the header to reflect this \
subtlety.</p> <p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">Note that for optimal functionality a companion patch to <code \
style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: \
0;line-height: inherit;">KMainWindow::event</code> is required:</p> <p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;"><div \
class="codehilite" style="background: #f8f8f8"><pre style="line-height: 125%"><span \
style="color: #A00000">--- a/src/kmainwindow.cpp</span> <span style="color: #00A000">+++ \
b/src/kmainwindow.cpp</span> <span style="color: #800080; font-weight: bold">@@ -772,7 +772,7 \
@@ bool KMainWindow::event(QEvent *ev)</span>  {
     K_D(KMainWindow);
     switch (ev-&gt;type()) {
<span style="color: #A00000">-#ifdef Q_OS_WIN</span>
<span style="color: #00A000">+#if defined(Q_OS_WIN) || defined(Q_OS_OSX)</span>
     case QEvent::Move:
 #endif
     case QEvent::Resize:
</pre></div>
</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: \
inherit;">This ensures that the window geometry save is performed also after a move (to update \
the position) without requiring a dummy resizing operation. Do I need to create a separate RR \
for this change or is it small enough that I can push it if and when this RR is \
accepted?</p></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;"><p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">On OS X 10.6 \
through 10.9 with various KDElibs4 versions and now with Qt 5.5.1 and frameworks 5.16.0 (and \
Kate as a test application). I presume that the MS Windows code has been tested sufficiently in \
KDELibs4; I have only adapted it to Qt5 and tested if it builds.</p></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/gui/kwindowconfig.h <span style="color: grey">(48a8f3c)</span></li>

 <li>src/gui/kwindowconfig.cpp <span style="color: grey">(d2f355c)</span></li>

</ul>

<p><a href="https://git.reviewboard.kde.org/r/126324/diff/" style="margin-left: 3em;">View \
Diff</a></p>






  </td>
 </tr>
</table>







  </div>
 </body>
</html>


--===============6584605355956298157==--


[Attachment #3 (text/plain)]

_______________________________________________
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel


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