Code review comment for lp:~uriboni/unity-2d/unity-2d-panel-pluggable

> data/com.canonical.Unity2d.gschema.xml:
>
> 24 + <schema path="/com/canonical/unity-2d/panel/"
> id="com.canonical.Unity2d.Panel" gettext-domain="unity-2d">
> 25 + <key type="as" name="applets">
> 26 + <default>["!homebutton", "!separator", "appname", "!legacytray",
> "indicator"]</default>
> 27 + <summary>Applets to display in the panel</summary>
>
> GSettings schema allows you to write this in a simpler way: you could use
> type="a(sb)" where "s" is the name and "b" is the boolean for "only on first
> screen". And if you need it, you can also add the expanding here, having the
> type as "a(sbb)". I see that there's also a "child" element for GSettings
> schema, but I don't know much about GSettings myself, so I'm not sure how
> suitable it is for our case.

I thought about it but then I read the documentation for QConf, which basically says that not all complex types are supported by the QT bindings. In fact the only non-basic types supported are "as" (QStringList) and "ay" (QByteArray).
For more details read https://gitorious.org/dconf-qt/dconf-qt/blobs/master/lib/qconftypes.cpp

> 34 + <key type="s" name="expanding">
> 35 + <default>"appname"</default>
> 36 + <summary>Applet that should expand to fill all unused
> space</summary>
>
> Why does it need to be specified here? I'd rather let the applet itself set
> the desired policy on its widget; also, in that way you could have more than
> one applet expanding, and not just one.

I guess it's essentially a matter of choice if it's up to the applet or if it has to be a configuration policy.
The reason why I chose it to be a configuration policy is related to why I am proposing this change: I need to alter the panel layout (add new applets, remove stock applets) for an OEM project and want to modify the stock unity-2d as little as possible.
In this context, having the expanding widget configured in dconf allows me to change which one it is without having to patch the unity-2d widgets (or my own widgets for that matter).

Regarding the choice of having just one expanding applet, I did that to reproduce what was going on in unity-2d before this merge request: only one applet (appname) was expanding.
It's clearly possible to turn that property in an "as" property and have more than one expanding applet, but I think it's outside of the scope of this MR.

> libunity-2d-private/src/appletproviderinterface.h:
> 88 + virtual QString getAppletName() const = 0;
> 89 + virtual QWidget* getApplet() const = 0;
>
> Qt coding style is appletName() and applet(), with no "get".
> (it would also be "Type *variable", but I see that's never used in unity-2d so
> I won't interfere :-) )

I agree with you on appletName(). For applet() now that I think of it, it would actually be much more clear to call it createApplet() since it create a new one every time it's called. What do you think ?

> panel/CMakeLists.txt:
> 104 -add_subdirectory(tests)
> 105 +#add_subdirectory(tests)
>
> Looks suspicious. :-)

This should be fixed now.

> panel/app/panelmanager.cpp:
> 162 -#include <QLabel>
> 163 +#include <QProcessEnvironment>
> 164 +#include <QFileInfo>
> 165 +#include <QDir>
> 166 +#include <QHash>
> 167 +#include <QPluginLoader>
> 168 +#include <QDebug>
> 169 +#include <QVariant>
>
> Please sort include files alphabetically (and generally, any list where order
> is not otherwise important).

I'll do that, even though it's not in our code guidelines (and I don't think I have ever seen it done in unity-2d code in general)
Updates to that document (CODING in the root of the repo) are generally proposed and approved by means of mini-referendums in the #unity-qt channel.

> 206 + if (!pluginFileInfo.exists() || !pluginFileInfo.isDir()) {
> 207 + qWarning() << "Panel plugin path does not exist or it is not a
> directory:" << pluginPath;
> 208 + return plugins;
> 209 + }
>
> The "exists" test is unnecessary, and the error message can also simply be:
> "The plugin directory does not exist:".

I'll fix the test and message.

> BTW, I see the rest of the code is using UQ_WARNING and UQ_DEBUG instead of
> qWarning and qDebug.

I'm a bit unsure if they should be used so widely in unity-2d code because they are defined in a private header debug_p.h in libunity-2d-private. This makes me think they are internal utilities to the library.
If this is not the case, why don't we get the file renamed to something else or even better merged with the public unity2ddebug.h ?

> 226 + if (provider == NULL) {
>
> Qt style prefers 0 to NULL.

Our internal guidelines don't seem to prefer anything, and I personally find the explicit comparison with NULL to be clearer. Unity-2d code seems to use both.
What about I change them to == 0 and we do something to put that rule into the coding conventions too ?

> 296 + if (!(onlyLeftmost && screen != leftmost)) {
> 297 + QWidget *applet = provider->getApplet();
>
> This condition's logic is a bit twisted. :-)
> I suggest:
> if (screen == leftmost || !onlyLeftmost)

Sounds like a good idea.

> Got to leave now. I'll continue the review on Monday. :-)

I'll be on vacation on Monday but I'll get to your comments when I come back on Tuesday.