Code review comment for lp://staging/~laney/ubuntu-system-settings/reset-api

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> ]