[...] > 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). Sad. But OK with the code in your MR, then! [...] > 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). Correct -- but note that I'm not advocating against using DConf: my doubt is whether this fits better in the Unity-2d schema or in the applet's. For example: /com/canonical/unity-2d/panel/applet/clock: width = 200 expand = false > 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. This is also related to the previous point; I also think it can be changed later without too much effort, and later on we might have a better understanding on what measure this improved customizability is needed/helpful. For this MR, I agree that your approach is the most practical. :-) [...] > 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 ? createApplet() is much better indeed. > > 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 ? I don't know -- I just saw a merge request about changing all qDebug() and qWarnings() to use that macro, so I assumed it would make sense for this branch too. Indeed the name debug_p.h is suspicious... > 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 ? I'll see what other people think of it, too. Now, continuing with the review. I notice you changed the parent class of the applets from Unity2d::Applet to QWidget. While I don't see much code in the Applet class to make it useful right now, I think it's a good idea to keep it and actually have the applet interface's createApplet method return that, instead of QWidget. The reason is that it gives us the possibility of implementing some common applet behaviour in the future, and we can use the Applet class to define an interface for panel applets: suppose for instance that one day we want to have a way to ask applets to reload their configuration, we could have that as a slot in the Applet class. Or if in the future we need to pass some extra info to the Applet (for instance, a pointer the the Panel object), we could have it as a method there. And now I notice that the rest of the review is just about adding the interface implementation on the various widget. It's great to see how you implemented this important feature with so little changes to the code. Great work! (I'm setting the MR to need fixing state, mostly because of the latter point -- for the qDebug/qWarning thing, I'm not sure what the project's intention are, so you'd better ask Florian)