Code review comment for lp:~unity-team/unity-api/add-surfacemanager-and-item

Revision history for this message
Gerry Boland (gerboland) wrote :

+++ include/unity/shell/application/SurfaceInterface.h
+ Q_PROPERTY(QString appId READ appId CONSTANT)
Why not a pointer to the application?

Would be useful to be able to directly read Application info in QML with something like: "surface.application.stage" as opposed to searching for the application with that appId.

It will require care to ensure the Application isn't used if it is deleted though (since a SurfaceItem will have QML ownership, but Application C++ ownership).

I expect the Application has some properties that we easily want to monitor per-surface, examples that come to mind are stage, supportedStages, state, hasFrozen (if app not responding) and maybe focus.

Note every Surface must have an Application behind it, else we make our lives hard IMO.

+class UNITY_API SurfaceItemInterface: public QQuickItem
Design is exactly what I had in mind.

I'm wondering though if instead of accessing the surface properties like:
"mySurfaceItem.surface.name" it might be handy to add the relevant setters/getters to the SurfaceItem so that can just do "mySurfaceItem.name"
What do you think? It's syntactic sugar really...

+ * @brief Get a Surface item (using stack index).
Stack or list?

Nitpick:
+ while (!m_list.empty())
+ {
brace on same line.

Overall this API is plenty to get us moving.
One thing I note is that getting info on the application which generates a surface is easy, but getting the surfaces that an application generates is hard. I can't come up with a good reason why the latter is needed however, so for now let's leave it as it is.

review: Needs Fixing

« Back to merge proposal