Merge lp:~zsombi/ubuntu-ui-toolkit/statesaver-datafile-fix into lp:ubuntu-ui-toolkit/staging

Proposed by Zsombor Egri
Status: Merged
Approved by: Zsombor Egri
Approved revision: 1244
Merged at revision: 1261
Proposed branch: lp:~zsombi/ubuntu-ui-toolkit/statesaver-datafile-fix
Merge into: lp:ubuntu-ui-toolkit/staging
Diff against target: 67 lines (+17/-5)
3 files modified
modules/Ubuntu/Components/plugin/statesaverbackend_p.cpp (+13/-4)
modules/Ubuntu/Components/plugin/ucstatesaver.cpp (+3/-0)
tests/unit_x11/tst_statesaver/tst_statesaver.cpp (+1/-1)
To merge this branch: bzr merge lp:~zsombi/ubuntu-ui-toolkit/statesaver-datafile-fix
Reviewer Review Type Date Requested Status
PS Jenkins bot continuous-integration Approve
Cris Dywan Approve
Zsombor Egri Approve
Review via email: mp+232986@code.launchpad.net

Commit message

Fixing wrong path used in saving state file.

To post a comment you must log in.
Revision history for this message
Zsombor Egri (zsombi) wrote :

Not tested yet with camera-app.

review: Needs Fixing
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Zsombor Egri (zsombi) wrote :

Tested with the modified toolkit gallery app.

review: Approve
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Cris Dywan (kalikiana) wrote :

101 + * \note Application name must be set in order to have the property states saved.
102 + * The proeprty can be set either through \l MainView::applicationName property.
103 + * Remember, the application name must be the same as the application's package
104 + * name (e.g. com.ubuntu.calendar).
105 + *

How about:

\note The application name must be set correctly to the package name so that state saving
can work (e.g. com.ubuntu.calendar) through MainView::applicationName.

89 + m_archive = new QSettings(QString("%1/confined/%2/statesaver.appstate")

Shouldn't QStandardPaths::CacheLocation get this same result?

Or, if you're working around XDG_RUNTIME_DIR being set incorrectly, add a bug# and a comment.

review: Needs Fixing
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Zsombor Egri (zsombi) wrote :

> 101 + * \note Application name must be set in order to have the property
> states saved.
> 102 + * The proeprty can be set either through \l
> MainView::applicationName property.
> 103 + * Remember, the application name must be the same as the
> application's package
> 104 + * name (e.g. com.ubuntu.calendar).
> 105 + *
>
> How about:
>
> \note The application name must be set correctly to the package name so that
> state saving
> can work (e.g. com.ubuntu.calendar) through MainView::applicationName.

Ok, sounds good.
>
> 89 + m_archive = new
> QSettings(QString("%1/confined/%2/statesaver.appstate")
>
> Shouldn't QStandardPaths::CacheLocation get this same result?
>
> Or, if you're working around XDG_RUNTIME_DIR being set incorrectly, add a bug#
> and a comment.

I'm not working around XDG_RUNTIME_DIR, that points to the proper path. However there is no env var or standard path set for the confined folder...

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Cris Dywan (kalikiana) wrote :

> > Or, if you're working around XDG_RUNTIME_DIR being set incorrectly
>
> I'm not working around XDG_RUNTIME_DIR, that points to the proper path.
> However there is no env var or standard path set for the confined folder...

As discussed, we need to sort out our expectations. The code as-is has detailed knowledge of the confinement spec. If XDG_RUNTIME_DIR is changing or apparmor polices change this will break again. This is why I'm saying "working around", because the code side steps the platform abstraction.

I'd like to leave this open for a short while and have a chance to pull somebody from confinement into this discussion. We need a clear guideline not just in the UI toolkit. Whether it is to avoid certain API or add new abstractions or change the environment variables, we need to be on the same page.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Zsombor Egri (zsombi) wrote :

something had been messed up in staging: if I run qmlapicheck locally, I get this UbuntuColors -1.-1 out from it. However CI fails on that diff...

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Cris Dywan (kalikiana) wrote :

Please update tst_statesaver.cpp it still uses "confinded" in the path.

review: Needs Fixing
Revision history for this message
Cris Dywan (kalikiana) wrote :

Thanks!

review: Approve
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
1243. By Zsombor Egri

changes on toolkit gallery reverted

1244. By Zsombor Egri

staging sync

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) :
review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'modules/Ubuntu/Components/plugin/statesaverbackend_p.cpp'
2--- modules/Ubuntu/Components/plugin/statesaverbackend_p.cpp 2014-08-06 10:45:34 +0000
3+++ modules/Ubuntu/Components/plugin/statesaverbackend_p.cpp 2014-09-20 06:06:17 +0000
4@@ -43,11 +43,11 @@
5 this, &StateSaverBackend::reset);
6 QObject::connect(&QuickUtils::instance(), &QuickUtils::deactivated,
7 this, &StateSaverBackend::initiateStateSaving);
8+ // catch eventual app name changes so we can have different path for the states if needed
9+ QObject::connect(&UCApplication::instance(), &UCApplication::applicationNameChanged,
10+ this, &StateSaverBackend::initialize);
11 if (!qgetenv("APP_ID").isEmpty() || !UCApplication::instance().applicationName().isEmpty()) {
12 initialize();
13- } else {
14- QObject::connect(&UCApplication::instance(), &UCApplication::applicationNameChanged,
15- this, &StateSaverBackend::initialize);
16 }
17
18 UnixSignalHandler::instance().connectSignal(UnixSignalHandler::Terminate);
19@@ -65,11 +65,20 @@
20
21 void StateSaverBackend::initialize()
22 {
23+ if (m_archive) {
24+ // delete previous archive
25+ QFile archiveFile(m_archive.data()->fileName());
26+ archiveFile.remove();
27+ delete m_archive.data();
28+ m_archive.clear();
29+ }
30 QString applicationName(qgetenv("APP_ID"));
31 if (applicationName.isEmpty()) {
32 applicationName = UCApplication::instance().applicationName();
33 }
34- m_archive = new QSettings(QString("%1/%2.state")
35+ // make sure the path is in sync with https://wiki.ubuntu.com/SecurityTeam/Specifications/ApplicationConfinement
36+ // the file must be saved under XDG_RUNTIME_DIR/<APP_PKGNAME> path.
37+ m_archive = new QSettings(QString("%1/%2/statesaver.appstate")
38 .arg(QStandardPaths::writableLocation(QStandardPaths::RuntimeLocation))
39 .arg(applicationName), QSettings::NativeFormat);
40 m_archive->setFallbacksEnabled(false);
41
42=== modified file 'modules/Ubuntu/Components/plugin/ucstatesaver.cpp'
43--- modules/Ubuntu/Components/plugin/ucstatesaver.cpp 2014-04-23 08:50:20 +0000
44+++ modules/Ubuntu/Components/plugin/ucstatesaver.cpp 2014-09-20 06:06:17 +0000
45@@ -151,6 +151,9 @@
46 * time, as well as when the application is deactivated. Automatic serialization
47 * of a component can be turned off by simply setting false to \l enabled property.
48 *
49+ * \note The application name must be set correctly to the package name so that
50+ * state saving can work (e.g. com.ubuntu.calendar) through \l MainView::applicationName.
51+ *
52 * States saved are discarded when the application is closed properly. The state
53 * loading is ignored (but not discarded) when the application is launched through
54 * UriHandler.
55
56=== modified file 'tests/unit_x11/tst_statesaver/tst_statesaver.cpp'
57--- tests/unit_x11/tst_statesaver/tst_statesaver.cpp 2014-08-06 10:51:28 +0000
58+++ tests/unit_x11/tst_statesaver/tst_statesaver.cpp 2014-09-20 06:06:17 +0000
59@@ -77,7 +77,7 @@
60
61 QString stateFile(const QString &appId)
62 {
63- return QString("%1/%2.state")
64+ return QString("%1/%2/statesaver.appstate")
65 .arg(QStandardPaths::writableLocation(QStandardPaths::RuntimeLocation))
66 .arg(appId);
67 }

Subscribers

People subscribed via source and target branches