Merge lp:~mzanetti/unity8/saveRestoreWindowSizePosition into lp:unity8
| Status: | Merged |
|---|---|
| Approved by: | Daniel d'Andrada on 2015-02-11 |
| Approved revision: | 1597 |
| Merged at revision: | 1605 |
| Proposed branch: | lp:~mzanetti/unity8/saveRestoreWindowSizePosition |
| Merge into: | lp:unity8 |
| Diff against target: |
413 lines (+256/-22) 7 files modified
plugins/Utils/CMakeLists.txt (+2/-1) plugins/Utils/plugin.cpp (+9/-0) plugins/Utils/windowstatestorage.cpp (+96/-0) plugins/Utils/windowstatestorage.h (+38/-0) qml/Stages/DesktopStage.qml (+2/-2) qml/Stages/WindowMoveResizeArea.qml (+17/-1) tests/qmltests/Stages/tst_WindowMoveResizeArea.qml (+92/-18) |
| To merge this branch: | bzr merge lp:~mzanetti/unity8/saveRestoreWindowSizePosition |
| Related bugs: |
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| Gerry Boland | Approve on 2015-02-12 | ||
| PS Jenkins bot | continuous-integration | Approve on 2015-02-11 | |
| Daniel d'Andrada (community) | 2015-01-28 | Approve on 2015-02-11 | |
|
Review via email:
|
|||
Commit Message
save and restore window size and position
Description of the Change
* Are there any related MPs required for this MP to build/function as expected? Please list.
no
* Did you perform an exploratory manual test run of your code change and any related functionality?
yes
* Did you make sure that your branch does not contain spurious tags?
yes
* If you changed the packaging (debian), did you subscribe the ubuntu-unity team to this MP?
no
* If you changed the UI, has there been a design review?
n/a
| Daniel d'Andrada (dandrader) wrote : | # |
| Daniel d'Andrada (dandrader) wrote : | # |
247 + // This will destroy the window and recreate it
248 + windowLoader.active = false;
249 + windowLoader.active = true;
I was doing that in tst_Shell.qml in the past and found out that the loaded item wasn't really getting recreated (ie, current one destroyed and a brand new created). Check the shellLoader.
| Daniel d'Andrada (dandrader) wrote : | # |
I find it confusing that you have two "fakeWindow" entities in the test:
"""
property var fakeWindow: windowLoader.item
"""
and
"""
Component {
Rectangle {
id: fakeWindow
}
}
"""
Makes you wonder which one is being referred to in the code
| Daniel d'Andrada (dandrader) wrote : | # |
+ onReleased: {
+ WindowPositionS
+ }
I think it would be more efficient and symmetric if the position got saved in "Component.
| Gerry Boland (gerboland) wrote : | # |
+ if(db !== null) return;
space after "if"
+ // db = LocalStorage.
comment for informative purposes?
+ tx.executeSql(
"settings" not a great table name for this
+ res = unefined;
typo
| Daniel d'Andrada (dandrader) wrote : | # |
On 28/01/15 14:05, Gerry Boland wrote:
> + // db = LocalStorage.
> comment for informative purposes?
I think so. I would rather keep it
| PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:1578
http://
Executed test runs:
UNSTABLE: http://
UNSTABLE: http://
SUCCESS: http://
SUCCESS: http://
UNSTABLE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
| PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:1579
http://
Executed test runs:
UNSTABLE: http://
FAILURE: http://
SUCCESS: http://
SUCCESS: http://
UNSTABLE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
| Michael Zanetti (mzanetti) wrote : | # |
> 247 + // This will destroy the window and recreate it
> 248 + windowLoader.active = false;
> 249 + windowLoader.active = true;
>
> I was doing that in tst_Shell.qml in the past and found out that the loaded
> item wasn't really getting recreated (ie, current one destroyed and a brand
> new created). Check the shellLoader.
> in tst_Shell.qml. You might need to do something similar to ensure you get a
> fresh new item.
This seems to work fine for me... If I remove the saving code, the test fails as expected because the item is created at the default position. Also I use this in some apps and I'm quite confident that it does the destruction and recreation of the object.
| Michael Zanetti (mzanetti) wrote : | # |
> I find it confusing that you have two "fakeWindow" entities in the test:
>
> """
> property var fakeWindow: windowLoader.item
> """
>
> and
>
> """
> Component {
> Rectangle {
> id: fakeWindow
> WindowMoveResiz
> }
> }
> """
>
> Makes you wonder which one is being referred to in the code
Given that you can't reference things inside a Component from outside of it, I think it's quite obvious, isn't it?
| Michael Zanetti (mzanetti) wrote : | # |
> + onReleased: {
> + WindowPositionS
> target.width, target.height)
> + }
>
> I think it would be more efficient and symmetric if the position got saved in
> "Component.
Seems a good point. I've tried it, but unfortunately it doesn't seem to work... First of all, the onDestruction signal behaves quite weird, for instance it doesn't get executed when the whole DesktopStage is destroyed, hence it is not triggered when we switch between Windowed and Staged mode. Also it would not save the state in case of a crash.
| Michael Zanetti (mzanetti) wrote : | # |
> + if(db !== null) return;
> space after "if"
fixed
> + // db = LocalStorage.
> estimated_size, callback(db))
> comment for informative purposes?
yeah, I copy/pasted most of this code and it had this comment. I looked at it and thought this might be useful again. Looking at it again today it doesn't seem like that any more :D I've dropped it now.
>
> + tx.executeSql(
> "settings" not a great table name for this
fixed.
>
> + res = unefined;
> typo
fixed
| PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:1581
http://
Executed test runs:
UNSTABLE: http://
UNSTABLE: http://
SUCCESS: http://
SUCCESS: http://
UNSTABLE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
| PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:1582
http://
Executed test runs:
FAILURE: http://
UNSTABLE: http://
SUCCESS: http://
SUCCESS: http://
FAILURE: http://
Click here to trigger a rebuild:
http://
| Daniel d'Andrada (dandrader) wrote : | # |
I just realized that the naming is slightly wrong.
We're saving the window geometry (size + position, expressed as a rectangle), not only its position.
thus:
s/WindowPositio
s/savePosition/
s/getPosition/
| Michał Sawicz (saviq) wrote : | # |
Make it "state" rather, we'll probably be saving more in there soon (like maximized / fullscreen etc.).
| PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:1584
http://
Executed test runs:
FAILURE: http://
UNSTABLE: http://
SUCCESS: http://
SUCCESS: http://
FAILURE: http://
Click here to trigger a rebuild:
http://
| PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:1585
http://
Executed test runs:
UNSTABLE: http://
UNSTABLE: http://
SUCCESS: http://
SUCCESS: http://
UNSTABLE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
| PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:1586
http://
Executed test runs:
UNSTABLE: http://
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
UNSTABLE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
| Daniel d'Andrada (dandrader) wrote : | # |
Looks good.
Cherry on top would some UI control so that I could unload & load the window, therefore being able to manually test the state saving. But I won't block on that.
| Daniel d'Andrada (dandrader) wrote : | # |
* Did you perform an exploratory manual test run of the code change and any related functionality?
Yes
* Did CI run pass? If not, please explain why.
Unrelated autopilot failure
* Did you make sure that the branch does not contain spurious tags?
Yes, it's clean
| Daniel d'Andrada (dandrader) wrote : | # |
Saving the window geometry to the database on MouseArea.release causes jerkiness as you can see in this video:
https:/
The issue goes away if I comment-out the "onReleased: WindowStateStor
- 1591. By Michael Zanetti on 2015-02-10
-
make the moveResizeArea visible
| PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:1591
http://
Executed test runs:
UNSTABLE: http://
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
UNSTABLE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
- 1592. By Michael Zanetti on 2015-02-10
-
replace WindowStateStorage with a threaded c++ implementation
- 1593. By Michael Zanetti on 2015-02-10
-
drop localstorage qml plugin dep
- 1594. By Michael Zanetti on 2015-02-10
-
improve debug print
| Michael Zanetti (mzanetti) wrote : | # |
> Saving the window geometry to the database on MouseArea.release causes
> jerkiness as you can see in this video:
> https:/
>
> The issue goes away if I comment-out the "onReleased:
> WindowStateStor
ok... this shouldn't happen any more...
- 1595. By Michael Zanetti on 2015-02-10
-
fix init code
| Daniel d'Andrada (dandrader) wrote : | # |
I'm getting that after every window movement or resize:
"""
QSqlQuery::exec: database not open
Error esecuting query "INSERT OR REPLACE INTO geometry (windowId, x, y, width, height) values ('test-window-id', '107', '210', '160', '160');" Driver error: "" Database error: ""
"""
| PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:1595
http://
Executed test runs:
SUCCESS: http://
UNSTABLE: http://
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
| Michael Zanetti (mzanetti) wrote : | # |
> I'm getting that after every window movement or resize:
>
> """
> QSqlQuery::exec: database not open
> Error esecuting query "INSERT OR REPLACE INTO geometry (windowId, x, y, width,
> height) values ('test-window-id', '107', '210', '160', '160');" Driver error:
> "" Database error: ""
> """
Can you please post the error that gets printed on the open() call?
- 1596. By Michael Zanetti on 2015-02-11
-
create cache dir if it doesn't exist
| Michael Zanetti (mzanetti) wrote : | # |
> I'm getting that after every window movement or resize:
>
> """
> QSqlQuery::exec: database not open
> Error esecuting query "INSERT OR REPLACE INTO geometry (windowId, x, y, width,
> height) values ('test-window-id', '107', '210', '160', '160');" Driver error:
> "" Database error: ""
> """
found it... should be fixed now
| Daniel d'Andrada (dandrader) wrote : | # |
> > I'm getting that after every window movement or resize:
> >
> > """
> > QSqlQuery::exec: database not open
> > Error esecuting query "INSERT OR REPLACE INTO geometry (windowId, x, y,
> width,
> > height) values ('test-window-id', '107', '210', '160', '160');" Driver
> error:
> > "" Database error: ""
> > """
>
> found it... should be fixed now
Yes, it's smooth now. Thanks!
But I have to say that this incident scared me off. I seems too wasteful to me to do IO like that on every single window drag or resize.
You might say this is a temporary solution but I think we will be using it for a quite some time still.
I won't block this MP but I also don't feel comfortable enough to approve it. So let's have a second opinion on the subject.
| Daniel d'Andrada (dandrader) wrote : | # |
I'm also slightly concerned that this is an ever-growing database
- 1597. By Michael Zanetti on 2015-02-11
-
only store when destroying the scene
| Michael Zanetti (mzanetti) wrote : | # |
> > > I'm getting that after every window movement or resize:
> > >
> > > """
> > > QSqlQuery::exec: database not open
> > > Error esecuting query "INSERT OR REPLACE INTO geometry (windowId, x, y,
> > width,
> > > height) values ('test-window-id', '107', '210', '160', '160');" Driver
> > error:
> > > "" Database error: ""
> > > """
> >
> > found it... should be fixed now
>
> Yes, it's smooth now. Thanks!
>
> But I have to say that this incident scared me off. I seems too wasteful to me
> to do IO like that on every single window drag or resize.
>
> You might say this is a temporary solution but I think we will be using it for
> a quite some time still.
>
> I won't block this MP but I also don't feel comfortable enough to approve it.
> So let's have a second opinion on the subject.
ok. changed it to only save it on destruction...
but seriously.. how often do you resize a window? I mean, I know it happens, but in average we're talking about multiple minutes between the events. I think it's worth it, in favour to not lose information on unclean shutdowns. Anyways... time will tell...
| Daniel d'Andrada (dandrader) wrote : | # |
> ok. changed it to only save it on destruction...
Thanks! Much appreciated!
>
> but seriously.. how often do you resize a window?
I find it hard to give you a number. But it hurts me to see resources being wasted like that when it's reasonably simple to avoid it. It's like seeing a faucet kept open, unattended, letting water go down the drain.
> I mean, I know it happens,
> but in average we're talking about multiple minutes between the events. I
> think it's worth it, in favour to not lose information on unclean shutdowns.
> Anyways... time will tell...
But then, how often do you get your window manager/compositor to crash and how bad is it to lose the windows' last position because of that?
I say rarely to the first and negligible to the second.
| PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:1597
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
7 + * Copyright (C) 2014 Canonical, Ltd.
s/2014/2015