[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: "David Faure" <faure () kde ! org>
Date: 2015-12-25 9:11:44
Message-ID: 20151225091144.29845.5612 () mimi ! kde ! org
[Download RAW message or body]
--===============9087415924308297071==
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:
> > src/gui/kwindowconfig.h, lines 38-39
> > <https://git.reviewboard.kde.org/r/126324/diff/4/?file=422749#file422749line38>
> >
> > That doesn't match the method name. It's saveWindowSize, not saveWindowGeometry. \
> > It's highly unexpected that saveWindowSize saves the position.
> > If you want that: please introduce a new saveWindowGeometry method.
>
> René J.V. Bertin wrote:
> I was afraid someone was going to say that, which is why I tried to argue that it's \
> highly unexpected from a user viewpoint that only window size is saved and not \
> position. How often would it happen that a developer is "highly surprised" in a \
> *negative* way that window size AND position are restored on a platform where this \
> is the default behaviour?
> I have nothing against introducing a pair of new methods, but how is that supposed \
> to be done in transparent fashion? I do have a lot against a need to change all \
> dependent software to call those methods (maintenance burden and all that).
> Counter proposal: replace save/restoreWindowSize with save/restoreWindowGeometry \
> everywhere, with a platform-specific interpretation of what exactly geometry \
> encompasses. Much less surprise there, just a bit more need to read the \
> documentation. Are these functions ever called intentionally outside of what I \
> suppose is a more or less automatic feature that takes care of restoring window, \
> erm, layout (saving is clearly automatic).
> René J.V. Bertin wrote:
> Just to be clear: if I am going to introduce restore/saveWindowGeometry methods \
> they'll replace the WindowSize variants on OS X or at least those will then use a \
> different KConfig key to avoid conflicts. I'd also be dropping the MS Windows part \
> of the patch (as this is not a decision I want to make for a platform I don't use). \
> But please consider this: that KConfig key has been called `geometry` for a long \
> time. Where exactly is the surprise, that restore/saveWindowSize never did what the \
> key they operate with suggests, or that they have always been using an inaptly \
> named key? For me the answer is straightforward and based on what users will \
> expect...
> Martin Gräßlin wrote:
> I leave it to the maintainers. On API I maintain I would say no to something \
> changing the semantics like that.
> René J.V. Bertin wrote:
> As I just wrote in reply to a message from Valorie, I have absolutely no issue with \
> maintaining methods for saving and restoring only window size, for code that \
> somehow requires that. I'd guess that such code would probably enforce the intended \
> window position itself *after* restoring window size (because that operation *can* \
> affect window position), but in the end that's (indeed) up to the code's developers \
> to decide.
> IOW, I'm perfectly willing to discuss a better solution in which the burden to \
> ensure that window save/restore works as "natively" as possible on each platform is \
> shared. The best way to do that is of course to have a single pair of methods that \
> have platform-specific implementations.
> As far as I'm concerned such a solution might even be prepared completely in \
> KConfig/gui before changes are made everywhere else to deploy that new solution. In \
> that case I would for instance run temporary local (MacPorts) patches that replace \
> saveWindowSize/restoreWindowSize with wrappers for \
> saveWindowGeometry/restoreWindowGeometry.
> Side-observation: OS X (Cocoa) provides a `[NSWindow setFrameAutosaveName:]` \
> method, i.e. it avoids reference to specifics like size or geometry completely.
> That method also provides another thought that could be taken into consideration if \
> it is decided to evolve this part of the frameworks, something I'd be interested in \
> collaborating on. Currently, there is no support for saving and restoring multiple \
> windows per application. That may be more or less sufficient when applications \
> always follow a MDI approach, but even if they do that still doesn't make them \
> applications that are active only in a single instance. Example: KDevelop. One \
> might expect that opening a given, pre-existing session (collection of open \
> projects) restores the main window geometry (size and/or position) that used \
> previously for that session, rather than the geometry used by whatever KDevelop \
> session was run last. On OS X that would be done with something like `[NSWindow \
> setFrameautosaveName:[window representedFile]]`, where `[NSWindow representedFile]` \
> corresponds to `QWindow::filePath` (but AFAICS those are not coupled in Qt5).
> I already had a quick look, but realised I don't know if the KConfig mechanism has \
> facilities to handle cleanup of stale/obsolete key/value entries.
> David Faure wrote:
> Note that most apps use this via the higher-level \
> KMainWindow::setAutoSaveSettings() anyway, which is supposed to 'do the right \
> thing'. So my suggestion is to fix things one level higher - let saveWindowSize \
> save only the window size, but update \
> KMainWindow::saveMainWindowSettings/restoreMainWindowSettings to also store \
> geometry on platforms (windowing systems, more precisely) where it makes sense to \
> also store the position (i.e. non-X11, as I understand it?)
> René: you are wrong about "no support for saving and restoring multiple windows \
> per application", that is definitely there, see the "groupName" argument to \
> setAutoSaveSettings or the KConfigGroup argument to \
> KWindowConfig::saveWindowSize(). Different (types of) mainwindows in the same \
> application can use different config groups.
> René J.V. Bertin wrote:
> I just had a look: KMainWindow:setAutoSaveSettings() indeed leads to \
> `KMainWindow::saveMainWindowSettings()`, which still uses \
> KWindowConfig::saveWindowSize(). So you're proposing what, to add a call to save \
> position too where appropriate, or to replace saveWindowSize in those cases? It's a \
> solution, but I don't really like the idea of fixing things above the level where \
> the actual work is being done. In my experience it's a great way to get deja-vu \
> kind of situations where you wonder why that fix you applied isn't working anymore, \
> only to find out that some bit of code you hadn't encountered before uses the lower \
> level directly.
>
> How many apps do *not* use KMainWindow, and how many libraries (frameworks) use \
> KWindowConfig directly to keep dependencies down.
> I have been wondering why in fact position isn't saved on X11 desktops too, as far \
> as that is not in fact the case? (position *is* restored when restoring the session \
> state at login, at least on my KDE4 desktop.)
> David Faure wrote:
> I propose to add a saveWindowPosition next to saveWindowSize, and to call both from \
> KMainWindow.
> To find out who uses KWindowConfig directly, use http://lxr.kde.org
>
> Position is restored on X11 because ksmserver+kwin takes care of it, which fits \
> with "the window manager takes care of position on X11". Both during session \
> management and when launching apps interactively.
> René J.V. Bertin wrote:
> X11 also allows providing hints to the WM, which is how position restore could have \
> been made optional IIRC.
> Is this really a question of X11 vs. the rest of the world, what about Wayland?
>
> Anyway, I don't like the idea of having to call several functions (and introduce a \
> set of new functions) if there is no reason those new functions will ever be used \
> outside of saveMainWindowSettings/restoreMainWindowSettings
> KXmlGui already links to QtWidgets, so there is no extra cost in allowing \
> saveMainWindowSettings/restoreMainWindowSettings to let \
> QWidget::saveGeometry/restoreGeometry handle all settings related to window size \
> and position. Those are the functions designed to work as properly as possible on \
> all supported platforms.
> It's a pity that QWidget::restoreGeometry doesn't have an optional filter to select \
> the aspects to restore: that would be the most elegant option. Use a single \
> function to save the relevant information, and another single function with a \
> platform-specific filter argument to restore it.
> I presume that absence of such an option is why save/restoreMainWindowSettings \
> don't call QMainWindow::save/restoreState?
> PS: should I read `restoreMainWindowSettings` as "restore the main window settings" \
> as opposed to "restore the mainwindow settings" (`restoreMainwindowSettings`)?
No clue about whether WMs on wayland handle window positioning. Well, in a way all \
windowing systems including OSX and Windows do handle positioning of new windows, \
don't they? It's not like all your windows and dialogs appear at (0,0) on OSX or \
Windows. I'm wondering if there's really a difference here....
If you had used LXR as I suggested you would have a much stronger argument against me \
;) http://lxr.kde.org/ident?_i=saveWindowSize&_remember=1 actually shows a huge list \
of code that uses KConfigGui::saveWindowSize directly: for dialogs. I assume you \
would want dialog positions to be stored also, on non-X11? In that case a \
KConfigGui::saveWindowGeometry would indeed be better API to avoid having to call two \
methods in all these places.
I didn't know about QByteArray QWidget::saveGeometry() (when I worked on this \
kmainwindow code Qt 4.2 didn't exist yet). It has three problems though: 1) it's an \
ugly blob of binary data, 2) it's most probably broken on OSX (look at the ifdef in \
the implementation), 3) it's in QWidget rather than QWindow, so it's not the right \
solution for QML based windows.
Please forget saveState/restoreState, that's an even bigger hammer (which includes \
the position of toolbars and dockwidgets etc.), and also widget-only, even worse, \
mainwindows-only.
PS: it's called KMainWindow, hence the name restoreMainWindowSettings. It's the \
settings for that instance of KMainWindow, there can be many instances. Don't read \
"main" as "the one and only primary", that's not what the main in [QK]MainWindow \
means, it just means it's a window with toolbars and statusbars.
IMHO a good solution would be to contribute to Qt a \
QWindow::saveGeometry/restoreGeometry, similar to the QWidget one but at the QWindow \
level (it makes more sense there, not only for QML... who wants to save/restore the \
geometry of a checkbox....)
A good fallback solution is a KConfigGui::saveWindowGeometry/restoreWindowGeometry.
Martin: is there actually a problem with saving/restoring the position on X11? The WM \
does clever auto-positioning for new windows, but if as a user I position some dialog \
on the right side of the screen, and I find it there again next time, it's fine, \
right? If not then yeah, the best solution is to not save position, and document \
that in saveWindowGeometry. I think your objection was about *changing* semantics of \
existing methods, but this is now about the semantics of a new method.
- David
-----------------------------------------------------------
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
>
>
--===============9087415924308297071==
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 />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <p style="margin-top: 0;">On December 17th, 2015, 4:16 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="https://git.reviewboard.kde.org/r/126324/diff/4/?file=422749#file422749line38" \
style="color: black; font-weight: bold; text-decoration: \
underline;">src/gui/kwindowconfig.h</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; ">class \
QWindow;</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2">37</font></th> <td bgcolor="#fdfebc" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="cm"> * global or \
application config file.</span></pre></td> <th bgcolor="#e9eaa8" style="border-left: \
1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font \
size="2">38</font></th> <td bgcolor="#fdfebc" width="50%"><pre style="font-size: \
8pt; line-height: 140%; margin: 0; "><span class="cm"> * global or application config \
file.<span class="hl"> On MS Windows and Mac OS X this also</span></span></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">39</font></th> <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span \
class="cm"> * saves the window position.</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;"><p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">That \
doesn't match the method name. It's saveWindowSize, not saveWindowGeometry. It's \
highly unexpected that saveWindowSize saves the position.</p> <p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">If \
you want that: please introduce a new saveWindowGeometry method.</p></pre> \
</blockquote>
<p>On December 17th, 2015, 5:21 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;">I was \
afraid someone was going to say that, which is why I tried to argue that it's highly \
unexpected from a user viewpoint that only window size is saved and not position. How \
often would it happen that a developer is "highly surprised" in a <em style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: \
normal;">negative</em> way that window size AND position are restored on a platform \
where this is the default behaviour?</p> <p style="padding: 0;text-rendering: \
inherit;margin: 0;line-height: inherit;white-space: inherit;">I have nothing against \
introducing a pair of new methods, but how is that supposed to be done in transparent \
fashion? I do have a lot against a need to change all dependent software to call \
those methods (maintenance burden and all that).</p> <p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: \
inherit;">Counter proposal: replace save/restoreWindowSize with \
save/restoreWindowGeometry everywhere, with a platform-specific interpretation of \
what exactly geometry encompasses. Much less surprise there, just a bit more need to \
read the documentation. Are these functions ever called intentionally outside of what \
I suppose is a more or less automatic feature that takes care of restoring window, \
erm, layout (saving is clearly automatic).</p></pre> </blockquote>
<p>On December 17th, 2015, 5:36 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;">Just \
to be clear: if I am going to introduce restore/saveWindowGeometry methods they'll \
replace the WindowSize variants on OS X or at least those will then use a different \
KConfig key to avoid conflicts. I'd also be dropping the MS Windows part of the \
patch (as this is not a decision I want to make for a platform I don't use).</p> <p \
style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: \
inherit;">But please consider this: that KConfig key has been called <code \
style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: \
0;line-height: inherit;">geometry</code> for a long time. Where exactly is the \
surprise, that restore/saveWindowSize never did what the key they operate with \
suggests, or that they have always been using an inaptly named key? For me the answer \
is straightforward and based on what users will expect...</p></pre> </blockquote>
<p>On December 18th, 2015, 7:08 a.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;"><p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I \
leave it to the maintainers. On API I maintain I would say no to something changing \
the semantics like that.</p></pre> </blockquote>
<p>On December 18th, 2015, 12:02 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;">As I \
just wrote in reply to a message from Valorie, I have absolutely no issue with \
maintaining methods for saving and restoring only window size, for code that somehow \
requires that. I'd guess that such code would probably enforce the intended window \
position itself <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: normal;">after</em> restoring window size (because that \
operation <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: normal;">can</em> affect window position), but in the end that's \
(indeed) up to the code's developers to decide.</p> <p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">IOW, \
I'm perfectly willing to discuss a better solution in which the burden to ensure that \
window save/restore works as "natively" as possible on each platform is shared. The \
best way to do that is of course to have a single pair of methods that have \
platform-specific implementations.</p> <p style="padding: 0;text-rendering: \
inherit;margin: 0;line-height: inherit;white-space: inherit;">As far as I'm concerned \
such a solution might even be prepared completely in KConfig/gui before changes are \
made everywhere else to deploy that new solution. In that case I would for instance \
run temporary local (MacPorts) patches that replace saveWindowSize/restoreWindowSize \
with wrappers for saveWindowGeometry/restoreWindowGeometry.</p> <p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: \
inherit;">Side-observation: OS X (Cocoa) provides a <code style="text-rendering: \
inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: \
inherit;">[NSWindow setFrameAutosaveName:]</code> method, i.e. it avoids reference to \
specifics like size or geometry completely.</p> <p style="padding: 0;text-rendering: \
inherit;margin: 0;line-height: inherit;white-space: inherit;">That method also \
provides another thought that could be taken into consideration if it is decided to \
evolve this part of the frameworks, something I'd be interested in collaborating on. \
Currently, there is no support for saving and restoring multiple windows per \
application. That may be more or less sufficient when applications always follow a \
MDI approach, but even if they do that still doesn't make them applications that are \
active only in a single instance. Example: KDevelop. One might expect that opening a \
given, pre-existing session (collection of open projects) restores the main window \
geometry (size and/or position) that used previously for that session, rather than \
the geometry used by whatever KDevelop session was run last. On OS X that would be \
done with something like <code style="text-rendering: inherit;color: #4444cc;padding: \
0;white-space: normal;margin: 0;line-height: inh erit;">[NSWindow \
setFrameautosaveName:[window representedFile]]</code>, where <code \
style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: \
0;line-height: inherit;">[NSWindow representedFile]</code> corresponds to <code \
style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: \
0;line-height: inherit;">QWindow::filePath</code> (but AFAICS those are not coupled \
in Qt5).</p> <p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">I already had a quick look, but realised I don't know \
if the KConfig mechanism has facilities to handle cleanup of stale/obsolete key/value \
entries.</p></pre> </blockquote>
<p>On December 19th, 2015, 10:13 a.m. UTC, <b>David Faure</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;">Note \
that most apps use this via the higher-level KMainWindow::setAutoSaveSettings() \
anyway, which is supposed to 'do the right thing'. So my suggestion is to fix things \
one level higher - let saveWindowSize save only the window size, but update \
KMainWindow::saveMainWindowSettings/restoreMainWindowSettings to also store geometry \
on platforms (windowing systems, more precisely) where it makes sense to also store \
the position (i.e. non-X11, as I understand it?)</p> <p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: \
inherit;">René: you are wrong about "no support for saving and restoring multiple \
windows per application", that is definitely there, see the "groupName" argument to \
setAutoSaveSettings or the KConfigGroup argument to KWindowConfig::saveWindowSize(). \
Different (types of) mainwindows in the same application can use different config \
groups.</p></pre> </blockquote>
<p>On December 19th, 2015, 9:16 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;">I \
just had a look: KMainWindow:setAutoSaveSettings() indeed leads to <code \
style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: \
0;line-height: inherit;">KMainWindow::saveMainWindowSettings()</code>, which still \
uses KWindowConfig::saveWindowSize(). So you're proposing what, to add a call to save \
position too where appropriate, or to replace saveWindowSize in those cases? It's a \
solution, but I don't really like the idea of fixing things above the level where the \
actual work is being done. In my experience it's a great way to get deja-vu kind of \
situations where you wonder why that fix you applied isn't working anymore, only to \
find out that some bit of code you hadn't encountered before uses the lower level \
directly.</p> <p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">How many apps do <em style="padding: 0;text-rendering: \
inherit;margin: 0;line-height: inherit;white-space: normal;">not</em> use \
KMainWindow, and how many libraries (frameworks) use KWindowConfig directly to keep \
dependencies down.</p> <p style="padding: 0;text-rendering: inherit;margin: \
0;line-height: inherit;white-space: inherit;">I have been wondering why in fact \
position isn't saved on X11 desktops too, as far as that is not in fact the case? \
(position <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: normal;">is</em> restored when restoring the session state at \
login, at least on my KDE4 desktop.)</p></pre> </blockquote>
<p>On December 19th, 2015, 11:08 p.m. UTC, <b>David Faure</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 \
propose to add a saveWindowPosition next to saveWindowSize, and to call both from \
KMainWindow.</p> <p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">To find out who uses KWindowConfig directly, use \
http://lxr.kde.org</p> <p style="padding: 0;text-rendering: inherit;margin: \
0;line-height: inherit;white-space: inherit;">Position is restored on X11 because \
ksmserver+kwin takes care of it, which fits with "the window manager takes care of \
position on X11". Both during session management and when launching apps \
interactively.</p></pre> </blockquote>
<p>On December 20th, 2015, 10:01 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;">X11 \
also allows providing hints to the WM, which is how position restore could have been \
made optional IIRC.</p> <p style="padding: 0;text-rendering: inherit;margin: \
0;line-height: inherit;white-space: inherit;">Is this really a question of X11 vs. \
the rest of the world, what about Wayland?</p> <p style="padding: 0;text-rendering: \
inherit;margin: 0;line-height: inherit;white-space: inherit;">Anyway, I don't like \
the idea of having to call several functions (and introduce a set of new functions) \
if there is no reason those new functions will ever be used outside of \
saveMainWindowSettings/restoreMainWindowSettings</p> <p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: \
inherit;">KXmlGui already links to QtWidgets, so there is no extra cost in allowing \
saveMainWindowSettings/restoreMainWindowSettings to let \
QWidget::saveGeometry/restoreGeometry handle all settings related to window size and \
position. Those are the functions designed to work as properly as possible on all \
supported platforms.</p> <p style="padding: 0;text-rendering: inherit;margin: \
0;line-height: inherit;white-space: inherit;">It's a pity that \
QWidget::restoreGeometry doesn't have an optional filter to select the aspects to \
restore: that would be the most elegant option. Use a single function to save the \
relevant information, and another single function with a platform-specific filter \
argument to restore it.</p> <p style="padding: 0;text-rendering: inherit;margin: \
0;line-height: inherit;white-space: inherit;">I presume that absence of such an \
option is why save/restoreMainWindowSettings don't call \
QMainWindow::save/restoreState?</p> <p style="padding: 0;text-rendering: \
inherit;margin: 0;line-height: inherit;white-space: inherit;">PS: should I read <code \
style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: \
0;line-height: inherit;">restoreMainWindowSettings</code> as "restore the main window \
settings" as opposed to "restore the mainwindow settings" (<code \
style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: \
0;line-height: inherit;">restoreMainwindowSettings</code>)?</p></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;"><p \
style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: \
inherit;">No clue about whether WMs on wayland handle window positioning. Well, in a \
way all windowing systems including OSX and Windows do handle positioning of new \
windows, don't they? It's not like all your windows and dialogs appear at (0,0) on \
OSX or Windows. I'm wondering if there's really a difference here....</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">If you had used LXR as I suggested you would have a \
much stronger argument against me ;) \
http://lxr.kde.org/ident?_i=saveWindowSize&_remember=1 actually shows a huge list \
of code that uses KConfigGui::saveWindowSize directly: for dialogs. I assume you \
would want dialog positions to be stored also, on non-X11? In that case a \
KConfigGui::saveWindowGeometry would indeed be better API to avoid having to call two \
methods in all these places.</p> <p style="padding: 0;text-rendering: inherit;margin: \
0;line-height: inherit;white-space: inherit;">I didn't know about QByteArray \
QWidget::saveGeometry() (when I worked on this kmainwindow code Qt 4.2 didn't exist \
yet). It has three problems though: 1) it's an ugly blob of binary data, 2) it's most \
probably broken on OSX (look at the ifdef in the implementation), 3) it's in QWidget \
rather than QWindow, so it's not the right solution for QML based windows.</p> <p \
style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: \
inherit;">Please forget saveState/restoreState, that's an even bigger hammer (which \
includes the position of toolbars and dockwidgets etc.), and also widget-only, even \
worse, mainwindows-only.</p> <p style="padding: 0;text-rendering: inherit;margin: \
0;line-height: inherit;white-space: inherit;">PS: it's called KMainWindow, hence the \
name restoreMainWindowSettings. It's the settings for that instance of KMainWindow, \
there can be many instances. Don't read "main" as "the one and only primary", that's \
not what the main in [QK]MainWindow means, it just means it's a window with toolbars \
and statusbars.</p> <p style="padding: 0;text-rendering: inherit;margin: \
0;line-height: inherit;white-space: inherit;">IMHO a good solution would be to \
contribute to Qt a QWindow::saveGeometry/restoreGeometry, similar to the QWidget one \
but at the QWindow level (it makes more sense there, not only for QML... who wants to \
save/restore the geometry of a checkbox....)</p> <p style="padding: 0;text-rendering: \
inherit;margin: 0;line-height: inherit;white-space: inherit;">A good fallback \
solution is a KConfigGui::saveWindowGeometry/restoreWindowGeometry.</p> <p \
style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: \
inherit;">Martin: is there actually a problem with saving/restoring the position on \
X11? The WM does clever auto-positioning for new windows, but if as a user I position \
some dialog on the right side of the screen, and I find it there again next time, \
it's fine, right? If not then yeah, the best solution is to not save position, and \
document that in saveWindowGeometry. I think your objection was about <em \
style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: \
normal;">changing</em> semantics of existing methods, but this is now about the \
semantics of a new method.</p></pre> <br />
<p>- David</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->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>
--===============9087415924308297071==--
[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]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic