ubuntu-system-settings

Merge lp://staging/~laney/ubuntu-system-settings/reset-api into lp://staging/ubuntu-system-settings

reset-api
Merge into trunk

Proposed by Iain Lane on 2014-02-27

Status:	Merged
Approved by:	Sebastien Bacher on 2014-08-04
Approved revision:	504
Merged at revision:	863
Proposed branch:	lp://staging/~laney/ubuntu-system-settings/reset-api
Merge into:	lp://staging/ubuntu-system-settings
Diff against target:	541 lines (+268/-30) 17 files modified debian/control (+2/-0) lib/SystemSettings/plugin-interface.h (+9/-0) plugins/battery/plugin/battery-plugin.h (+3/-3) plugins/brightness/plugin/brightness-plugin.h (+3/-3) plugins/reset/ResetAllSettings.qml (+4/-2) plugins/system-update/plugin/update-plugin.h (+4/-3) src/plugin-manager.cpp (+12/-0) src/plugin-manager.h (+1/-0) src/plugin.cpp (+63/-5) src/plugin.h (+2/-0) tests/CMakeLists.txt (+4/-0) tests/data/phone.settings (+9/-0) tests/test-plugin.cpp (+18/-10) tests/test-plugin.h (+3/-3) tests/test-plugin2.cpp (+44/-0) tests/test-plugin2.h (+41/-0) tests/tst_plugins.cpp (+46/-1)
To merge this branch:	bzr merge lp://staging/~laney/ubuntu-system-settings/reset-api
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
PS Jenkins bot	continuous-integration		Approve on 2014-07-31
Sebastien Bacher (community)			Needs Fixing on 2014-07-29
Alberto Mardegan		2014-02-27	Needs Fixing on 2014-03-03
Review via email: mp+208661@code.staging.launchpad.net

Commit message

Add a 'reset' API so that plugins can reset their settings to default. They can do this in one of two ways: by providing a function 'bool reset()' in their 'plugin' which returns true or by providing a top-level javascript function 'reset' in their page component.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-02-27:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntu-system-settings-ci/665/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-02-27:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntu-system-settings-ci/667/rebuild

review: Approve (continuous-integration)

Revision history for this message

Alberto Mardegan (mardy) wrote on 2014-02-28:

On 02/27/2014 07:55 PM, Iain Lane wrote:
> + // Try to use the plugin's reset method
> + if (d->m_plugin2 && d->m_plugin2->reset())
> + return;
> +
> + // Otherwise, try to use one from the page component
> + QQmlComponent *component = pageComponent();

I'd suggest adding this (to avoid the need of changing the Online
Accounts panel at the same time):

/* If the plugin implements version 1 of the interface but not version
* 2, we assume that we won't find a "reset()" method in the
* pageComponent either.
*/
if (d->m_plugin && !d->m_plugin2)
return;

Revision history for this message

Iain Lane (laney) wrote on 2014-02-28:

On Fri, Feb 28, 2014 at 08:15:32AM -0000, Alberto Mardegan wrote:
> On 02/27/2014 07:55 PM, Iain Lane wrote:
> > + // Try to use the plugin's reset method
> > + if (d->m_plugin2 && d->m_plugin2->reset())
> > + return;
> > +
> > + // Otherwise, try to use one from the page component
> > + QQmlComponent *component = pageComponent();
>
> I'd suggest adding this (to avoid the need of changing the Online
> Accounts panel at the same time):
>
> /* If the plugin implements version 1 of the interface but not version
> * 2, we assume that we won't find a "reset()" method in the
> * pageComponent either.
> */
> if (d->m_plugin && !d->m_plugin2)
> return;

I did consider that, but I came to the conclusion that I think it should
still be valid to continue to use interface version 1 and provide
resetting via the PageComponent.

--
Iain Lane [ <email address hidden> ]
Debian Developer [ <email address hidden> ]
Ubuntu Developer [ <email address hidden> ]

Revision history for this message

Iain Lane (laney) wrote on 2014-02-28:

Hold on, I'm going to write some tests / examples

Revision history for this message

Iain Lane (laney) wrote on 2014-02-28:

ok, tests added, see what you think now please

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-02-28:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntu-system-settings-ci/671/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-02-28:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntu-system-settings-ci/673/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Alberto Mardegan (mardy) wrote on 2014-03-03:

L149: better delete the component here (though it's not leaked because it has the Plugin as its parent, so it's not a big issue)

L284,L285: indent the QML code one space more

L291: is this really needed? If it is, then maybe we should have this code in src/plugin.cpp, and not just in the test plugin.

All the rest looks fine to me, though I'd really like to see the addition of the code I suggested in the previous comment. :-)

review: Needs Fixing

Revision history for this message

Iain Lane (laney) wrote on 2014-03-03:

On Mon, Mar 03, 2014 at 09:45:11AM -0000, Alberto Mardegan wrote:
> Review: Needs Fixing
>
> L149: better delete the component here (though it's not leaked because it has the Plugin as its parent, so it's not a big issue)
>
> L284,L285: indent the QML code one space more

Fixing, thanks - actually 284 is redundant now so I'll delete that.

> L291: is this really needed? If it is, then maybe we should have this code in src/plugin.cpp, and not just in the test plugin.

Something is broken here. I was trying to see if it worked on the CI,
but it doesn't as you can see; will try to fix some other way soon.

  QWARN : PluginsTest::testReset() QQmlComponent: Component is not ready
  INFO : PluginsTest::testReset() Did not receive message: "Hello"
  FAIL! : PluginsTest::testReset() Not all expected messages were received

& my last revision just makes it hang altogether. I guess it probably
/should/ be in plugin.cpp though, yeah, if it's generally possible that
the returned component is not ready (which I suppose is true).

>
> All the rest looks fine to me, though I'd really like to see the addition of the code I suggested in the previous comment. :-)

I'll let Seb or another reviewer adjudicate. I gave my reasons already,
but I'm happy to defer if I get outvoted. :-)

--
Iain Lane [ <email address hidden> ]
Debian Developer [ <email address hidden> ]
Ubuntu Developer [ <email address hidden> ]

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-03-03:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntu-system-settings-ci/674/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-03-03:

FAILED: Continuous integration, rev:502
http://jenkins.qa.ubuntu.com/job/ubuntu-system-settings-ci/675/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-trusty/3626
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-trusty-touch/3237/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-system-settings-trusty-amd64-ci/191
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-system-settings-trusty-armhf-ci/179
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-system-settings-trusty-i386-ci/178
    SUCCESS: http://jenkins.qa.ubuntu.com/job/autopilot-testrunner-otto-trusty/3190
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-trusty-amd64/3630
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-trusty-amd64/3630/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-trusty-armhf/3239
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-trusty-armhf/3239/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-runner-mako/5615/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/4425

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntu-system-settings-ci/675/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Sebastien Bacher (seb128) wrote on 2014-03-03:

> though I'd really like to see the addition of the code I suggested in the previous comment. :-)

what is going to happen without that code if we don't land -online at the same time? it seems the code is checking for a reset() and handle it not being there, so it shouldn't be an issue?

Revision history for this message

Iain Lane (laney) wrote on 2014-03-03:

On Mon, Mar 03, 2014 at 02:12:14PM -0000, Sebastien Bacher wrote:
> > though I'd really like to see the addition of the code I suggested in the previous comment. :-)
>
> what is going to happen without that code if we don't land -online at the same time? it seems the code is checking for a reset() and handle it not being there, so it shouldn't be an issue?

Because online accounts does something weird when you ask for its page
component - it unconditionally shows itself. So if you invoke the reset
action then the online accounts panel pops up.

Alberto's solution avoids this because online accounts doesn't implement
the '2' interface, so we would just skip resetting it completely.

--
Iain Lane [ <email address hidden> ]
Debian Developer [ <email address hidden> ]
Ubuntu Developer [ <email address hidden> ]

Revision history for this message

Sebastien Bacher (seb128) wrote on 2014-03-03:

thanks for the explanation, to me it looks like that check is low cost and makes things a bit more robust, so +1 for including it (though in practice it's likely only online-account is going to have the issue and we can batch those so it doesn't really matter much easier way)

Revision history for this message

Alberto Mardegan (mardy) wrote on 2014-03-03:

If that extra code bothers you, we can add it now and remove it later. :-)

Revision history for this message

Iain Lane (laney) wrote on 2014-03-03:

On Mon, Mar 03, 2014 at 02:46:35PM -0000, Sebastien Bacher wrote:
> thanks for the explanation, to me it looks like that check is low cost and makes things a bit more robust, so +1 for including it (though in practice it's likely only online-account is going to have the issue and we can batch those so it doesn't really matter much easier way)

OK then. I'll have to upgrade the plugins we provide to 2, and we need
to make sure that any future ones are 2, otherwise they won't be able to
be reset via qml/js.

I think it'll be a bit confusing and hard to debug if that ever happens,
so try to keep it in mind.

--
Iain Lane [ <email address hidden> ]
Debian Developer [ <email address hidden> ]
Ubuntu Developer [ <email address hidden> ]

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-03-03:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntu-system-settings-ci/676/rebuild

review: Approve (continuous-integration)

Revision history for this message

Jonas G. Drange (jonas-drange) wrote on 2014-07-29:

This looks great, thanks.

Revision history for this message

Sebastien Bacher (seb128) wrote on 2014-07-29:

needs to be rebased on trunk

review: Needs Fixing

lp://staging/~laney/ubuntu-system-settings/reset-api updated on 2014-07-31

504. By Iain Lane on 2014-07-31: Merge trunk

Revision history for this message

Iain Lane (laney) wrote on 2014-07-31:

merged, fyi

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-07-31:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntu-system-settings-ci/1111/rebuild

review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk

The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches

to all changes:

Iain Lane

Ubuntu Touch System Settings

ubuntu-system-settings

Merge lp://staging/~laney/ubuntu-system-settings/reset-api into lp://staging/ubuntu-system-settings

Commit message

Description of the change

Preview Diff

Subscribers