Unity API

Merge lp:~dandrader/unity-api/mirSurface into lp:unity-api

  1. mirSurface
  2. Merge into trunk
Proposed by Daniel d'Andrada
Status: Rejected
Rejected by: Daniel d'Andrada
Proposed branch: lp:~dandrader/unity-api/mirSurface
Merge into: lp:unity-api
Prerequisite: lp:~dandrader/unity-api/app-state-handling
Diff against target: 368 lines (+342/-1)
4 files modified
include/unity/shell/application/CMakeLists.txt (+1/-1)
include/unity/shell/application/Mir.h (+76/-0)
include/unity/shell/application/MirSurfaceInterface.h (+114/-0)
include/unity/shell/application/MirSurfaceItemInterface.h (+151/-0)
To merge this branch: bzr merge lp:~dandrader/unity-api/mirSurface
Reviewer Review Type Date Requested Status
Unity8 CI Bot continuous-integration Needs Fixing
PS Jenkins bot (community) continuous-integration Approve
Gerry Boland (community) Needs Fixing
Review via email: mp+264921@code.launchpad.net

Commit message

Added MirSurface and MirSurfaceItem interfaces

To post a comment you must log in.
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:180
http://jenkins.qa.ubuntu.com/job/unity-api-ci/313/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-api-wily-amd64-ci/12
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-api-wily-armhf-ci/12
        deb: http://jenkins.qa.ubuntu.com/job/unity-api-wily-armhf-ci/12/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-api-wily-i386-ci/12

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-api-ci/313/rebuild

review: Approve (continuous-integration)
Revision history for this message
Gerry Boland (gerboland) wrote :

+++ include/unity/shell/application/Mir.h
+class Mir : public QObject
This doesn't need to be a full QObject, it only has a bunch of enums. As we want access to the enums in QML, can make it a simple non-Qt class and use the Q_GADGET macro.
http://doc.qt.io/qt-5/qobject.html#Q_GADGET

+++ include/unity/shell/application/MirSurfaceInterface.h
All the methods on a MirSurface, you only add type to the interface? What's the point of having this, if you then have another MirSurfaceInterface in qtmir?

Also I will want to be able to inspect properties of the MirSurface (size, state, type, etc) to make decisions before creating a MirSurfaceItem.

+++ include/unity/shell/application/MirSurfaceItemInterface.h
Nit-pick, can you put the "surface" property at the top of the list - just because it is referred to in all comments.

+ // Only one item should have this property enabled for a given surface.
anything enforce this?

+ MirSurfaceItemInterface(QQuickItem *parent = 0) : QQuickItem(parent) {}
A constructor immediately setting the surface would be useful.

+ virtual bool consumesInput() const = 0;
(Discussion) Item has an "enabled" property to stop it processing input. I suspect "enabled" is a more consistent QML name than "consumesInput". If we used the existing property, it has the bonus of updating activeFocus correctly:
http://doc.qt.io/qt-5/qquickitem.html#enabled-prop

+ void typeChanged(Mir::Type);
I didn't think QML has any support for using the argument of a signal in any way, so passing it around has no benefit. WDYT?

You need to bump the debian version too.

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

Hmm, looks like my formatting slightly lost, I tend to double-linefeed after comments for each file. As a test, here should be 2 linebreaks

and just one

Let's see...

Revision history for this message
Daniel d'Andrada (dandrader) wrote :
Download full text (4.5 KiB)

On 16/07/15 07:42, Gerry Boland wrote:
> Review: Needs Fixing
>
> +++ include/unity/shell/application/Mir.h
> +class Mir : public QObject
> This doesn't need to be a full QObject, it only has a bunch of enums. As we want access to the enums in QML, can make it a simple non-Qt class and use the Q_GADGET macro.
> http://doc.qt.io/qt-5/qobject.html#Q_GADGET

Ok, I will give it a spin.

>
>
> +++ include/unity/shell/application/MirSurfaceInterface.h
> All the methods on a MirSurface, you only add type to the interface? What's the point of having this, if you then have another MirSurfaceInterface in qtmir?

The MirSurfaceInterface in qtmir defines the *internal* interface
between qtmir::MirSurface and other qtmir components, like
qtmir::Session, qtmir::Application, qtmir::SurfaceManager and
qtmir::MirSurfaceItem. They're qtmir implementation details.
Unity.Application users (ie, unity8) are oblivious to all of the methods
there. All unity8 cares about is taking a
unity::shell::application::MirSurface and putting it in a
unity::shell::application::MirSurfaceItem.

>
> Also I will want to be able to inspect properties of the MirSurface (size, state, type, etc) to make decisions before creating a MirSurfaceItem.

This is just a conjecture at this point and does not reflect how unity8
uses it currently. I'm being pragmatic, only adding the API that is
*actually needed*. Once unity8 actually needs all the properties you
mentioned in MirSurface, we just add them. No big deal. This is a
constantly evolving API. I'm not setting in stone that
unity::shell::application::MirSurface will only have a type property for
all eternity and it does seems odd, but that's the current reality. We
should keep APIs as lean as possible. The best API is no API. You have
to prove that you need an API before you create it and untiy8 is our proof.

I, on the other hand, see MirSurface being used mostly as an opaque type
that we put in a MirSurfaceItem, from where we do all the manipulation.
Ie, MirSurface being an opaque model and MirSurfaceItem being the
view+controller. This is what's happening currently. Let's see how
future UIs (like desktop unity8) takes us.

>
>
> +++ include/unity/shell/application/MirSurfaceItemInterface.h
> Nit-pick, can you put the "surface" property at the top of the list - just because it is referred to in all comments.
>
> + // Only one item should have this property enabled for a given surface.
> anything enforce this?

No.

> + MirSurfaceItemInterface(QQuickItem *parent = 0) : QQuickItem(parent) {}
> A constructor immediately setting the surface would be useful.

I don't think so. MirSurfaceItems are created declaratively in QML code.
You can't pass parameters to constructors in QML.

>
> + virtual bool consumesInput() const = 0;
> (Discussion) Item has an "enabled" property to stop it processing input. I suspect "enabled" is a more consistent QML name than "consumesInput". If we used the existing property, it has the bonus of updating activeFocus correctly:
> http://doc.qt.io/qt-5/qquickitem.html#enabled-prop

I thought about doing it, but found safer to use a separate property.
enabled() already has several meanings and consequences and I'...

Read more...

Revision history for this message
Lukáš Tinkl (lukas-kde) wrote :

> >
> > + void typeChanged(Mir::Type);
> > I didn't think QML has any support for using the argument of a signal in any way, so passing it > > around has no benefit. WDYT?
>
> Yeah, I was also unsure about whether to have arguments in signals or
> not. In the end I decided to keep the arguments since this seems to be
> standard Qt way of doing it and specially because QQuickItem's signals
> also have parameters. But no strong opinion here. But maybe the internal
> QML implementation uses signal parameters? I've no idea.

Definitely keep the signal arguments declared _and_ also the name if you intend to use it from QML.

void typeChanged(Mir::Type foo);

onTypeChanged: {
   var type = foo;
}

Revision history for this message
Daniel d'Andrada (dandrader) wrote :

On 16/07/15 11:23, Lukáš Tinkl wrote:
>>> + void typeChanged(Mir::Type);
>>> I didn't think QML has any support for using the argument of a signal in any way, so passing it > > around has no benefit. WDYT?
>> Yeah, I was also unsure about whether to have arguments in signals or
>> not. In the end I decided to keep the arguments since this seems to be
>> standard Qt way of doing it and specially because QQuickItem's signals
>> also have parameters. But no strong opinion here. But maybe the internal
>> QML implementation uses signal parameters? I've no idea.
> Definitely keep the signal arguments declared _and_ also the name if you intend to use it from QML.
>
> void typeChanged(Mir::Type foo);
>
> onTypeChanged: {
> var type = foo;
> }

Good point.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:181
http://jenkins.qa.ubuntu.com/job/unity-api-ci/314/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-api-wily-amd64-ci/13
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-api-wily-armhf-ci/13
        deb: http://jenkins.qa.ubuntu.com/job/unity-api-wily-armhf-ci/13/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-api-wily-i386-ci/13

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-api-ci/314/rebuild

review: Approve (continuous-integration)
Revision history for this message
Gerry Boland (gerboland) wrote :
Download full text (4.8 KiB)

> > +++ include/unity/shell/application/MirSurfaceInterface.h
> > All the methods on a MirSurface, you only add type to the interface? What's
> the point of having this, if you then have another MirSurfaceInterface in
> qtmir?
>
> The MirSurfaceInterface in qtmir defines the *internal* interface
> between qtmir::MirSurface and other qtmir components, like
> qtmir::Session, qtmir::Application, qtmir::SurfaceManager and
> qtmir::MirSurfaceItem. They're qtmir implementation details.
> Unity.Application users (ie, unity8) are oblivious to all of the methods
> there. All unity8 cares about is taking a
> unity::shell::application::MirSurface and putting it in a
> unity::shell::application::MirSurfaceItem.
>
>
> >
> > Also I will want to be able to inspect properties of the MirSurface (size,
> state, type, etc) to make decisions before creating a MirSurfaceItem.
>
> This is just a conjecture at this point and does not reflect how unity8
> uses it currently. I'm being pragmatic, only adding the API that is
> *actually needed*. Once unity8 actually needs all the properties you
> mentioned in MirSurface, we just add them. No big deal. This is a
> constantly evolving API. I'm not setting in stone that
> unity::shell::application::MirSurface will only have a type property for
> all eternity and it does seems odd, but that's the current reality. We
> should keep APIs as lean as possible. The best API is no API. You have
> to prove that you need an API before you create it and untiy8 is our proof.

I see where you're coming from. It's a fair argument. Unity8 is our main consumer.

But a position I don't want to be in is for us to require Unity8 justify every API addition to QtMir. Especially for obvious properties a mir surface will definitely have, like width/height, state. Adding these things has a small cost if done right now, but will most likely make our future lives easier.

I think of us getting properties added to Mir classes and how long it takes. I'd rather take the hit to make the Unity8 team's job faster.

"The best API is no API." is totally untrue from an API consumer's point of view. We don't know who other future consumers might be and what they'd want, but they certainly will review an API before trying it. If it's missing obvious stuff, it's not inviting.

QtMir isn't just for Unity8. If it is flexible and powerful, other shells may start using it, which would be a great win.

> > + MirSurfaceItemInterface(QQuickItem *parent = 0) : QQuickItem(parent) {}
> > A constructor immediately setting the surface would be useful.
>
> I don't think so. MirSurfaceItems are created declaratively in QML code.
> You can't pass parameters to constructors in QML.
Ok

> > + virtual bool consumesInput() const = 0;
> > (Discussion) Item has an "enabled" property to stop it processing input. I
> suspect "enabled" is a more consistent QML name than "consumesInput". If we
> used the existing property, it has the bonus of updating activeFocus
> correctly:
> > http://doc.qt.io/qt-5/qquickitem.html#enabled-prop
>
> I thought about doing it, but found safer to use a separate property.
> enabled() already has several meanings and consequences and I'm not sur...

Read more...

Revision history for this message
Daniel d'Andrada (dandrader) wrote :
Download full text (5.1 KiB)

On 16/07/15 13:50, Gerry Boland wrote:
>>> +++ include/unity/shell/application/MirSurfaceInterface.h
>>> All the methods on a MirSurface, you only add type to the interface? What's
>> the point of having this, if you then have another MirSurfaceInterface in
>> qtmir?
>>
>> The MirSurfaceInterface in qtmir defines the *internal* interface
>> between qtmir::MirSurface and other qtmir components, like
>> qtmir::Session, qtmir::Application, qtmir::SurfaceManager and
>> qtmir::MirSurfaceItem. They're qtmir implementation details.
>> Unity.Application users (ie, unity8) are oblivious to all of the methods
>> there. All unity8 cares about is taking a
>> unity::shell::application::MirSurface and putting it in a
>> unity::shell::application::MirSurfaceItem.
>>
>>
>>> Also I will want to be able to inspect properties of the MirSurface (size,
>> state, type, etc) to make decisions before creating a MirSurfaceItem.
>>
>> This is just a conjecture at this point and does not reflect how unity8
>> uses it currently. I'm being pragmatic, only adding the API that is
>> *actually needed*. Once unity8 actually needs all the properties you
>> mentioned in MirSurface, we just add them. No big deal. This is a
>> constantly evolving API. I'm not setting in stone that
>> unity::shell::application::MirSurface will only have a type property for
>> all eternity and it does seems odd, but that's the current reality. We
>> should keep APIs as lean as possible. The best API is no API. You have
>> to prove that you need an API before you create it and untiy8 is our proof.
> I see where you're coming from. It's a fair argument. Unity8 is our main consumer.
>
> But a position I don't want to be in is for us to require Unity8 justify every API addition to QtMir. Especially for obvious properties a mir surface will definitely have, like width/height, state. Adding these things has a small cost if done right now, but will most likely make our future lives easier.

It's like SurfaceManager being a model (list of surfaces). Yeah, it
makes sense, might be useful and all. But it's not, we never used it.
It's just dead code and I'm now finally removing it.

> I think of us getting properties added to Mir classes and how long it takes. I'd rather take the hit to make the Unity8 team's job faster.
>
> "The best API is no API." is totally untrue from an API consumer's point of view. We don't know who other future consumers might be and what they'd want, but they certainly will review an API before trying it. If it's missing obvious stuff, it's not inviting.
>
> QtMir isn't just for Unity8. If it is flexible and powerful, other shells may start using it, which would be a great win.

You run the risk of having multiple ways/APIs to achieve the same thing,
of having redundant APIs.

>>> + virtual bool consumesInput() const = 0;
>>> (Discussion) Item has an "enabled" property to stop it processing input. I
>> suspect "enabled" is a more consistent QML name than "consumesInput". If we
>> used the existing property, it has the bonus of updating activeFocus
>> correctly:
>>> http://doc.qt.io/qt-5/qquickitem.html#enabled-prop
>> I thought about doing it, but found safer to use a separate prope...

Read more...

Revision history for this message
Daniel d'Andrada (dandrader) wrote :

Anyway, added some properties to MirSurface.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:182
http://jenkins.qa.ubuntu.com/job/unity-api-ci/315/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-api-wily-amd64-ci/14
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-api-wily-armhf-ci/14
        deb: http://jenkins.qa.ubuntu.com/job/unity-api-wily-armhf-ci/14/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-api-wily-i386-ci/14

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-api-ci/315/rebuild

review: Approve (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:183
http://jenkins.qa.ubuntu.com/job/unity-api-ci/316/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-api-wily-amd64-ci/15
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-api-wily-armhf-ci/15
        deb: http://jenkins.qa.ubuntu.com/job/unity-api-wily-armhf-ci/15/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-api-wily-i386-ci/15

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-api-ci/316/rebuild

review: Approve (continuous-integration)
Revision history for this message
Gerry Boland (gerboland) wrote :

Thanks for all your changes so far, it's looking good. The consumesInput property will be fine, I understand it now.

+ enum Type {
+ UnknownType,
...
+ enum State {
+ UnknownState,

Do we need the name of the enum appended to the name of each entry in the enum?

We'll be using it like
surface.type == Mir.UnknownType
and repeating "type" feels a bit redundant to me.

Is it the duplicate "Unknown" that inspired it?

Oh and do please bump the debian version.

Don't we need to add some qml test to test/qmltest/unity/shell/application to check these objects actually contain these properties? Which means this interface has to be mocked. /me unsure

review: Needs Fixing
Revision history for this message
Lukáš Tinkl (lukas-kde) wrote :

> Thanks for all your changes so far, it's looking good. The consumesInput
> property will be fine, I understand it now.
>
>
> + enum Type {
> + UnknownType,
> ...
> + enum State {
> + UnknownState,
>
> Do we need the name of the enum appended to the name of each entry in the
> enum?
>
> We'll be using it like
> surface.type == Mir.UnknownType
> and repeating "type" feels a bit redundant to me.
>
> Is it the duplicate "Unknown" that inspired it?

Just wondering, wouldn't it be better to use C++11 strong enums, like:

enum class State {
Unknown,
...
}

Revision history for this message
Daniel d'Andrada (dandrader) wrote :

On 17/07/15 07:19, Lukáš Tinkl wrote:
>> Thanks for all your changes so far, it's looking good. The consumesInput
>> property will be fine, I understand it now.
>>
>>
>> + enum Type {
>> + UnknownType,
>> ...
>> + enum State {
>> + UnknownState,
>>
>> Do we need the name of the enum appended to the name of each entry in the
>> enum?
>>
>> We'll be using it like
>> surface.type == Mir.UnknownType
>> and repeating "type" feels a bit redundant to me.
>>
>> Is it the duplicate "Unknown" that inspired it?
> Just wondering, wouldn't it be better to use C++11 strong enums, like:
>
> enum class State {
> Unknown,
> ...
> }
>
 It would be even worse, like Mir::State::Maximized. And I don't even
known if QML supports it.

Revision history for this message
Daniel d'Andrada (dandrader) wrote :

On 16/07/15 19:14, Gerry Boland wrote:
> Review: Needs Fixing
>
> Thanks for all your changes so far, it's looking good. The consumesInput property will be fine, I understand it now.
>
>
> + enum Type {
> + UnknownType,
> ...
> + enum State {
> + UnknownState,
>
> Do we need the name of the enum appended to the name of each entry in the enum?
>
> We'll be using it like
> surface.type == Mir.UnknownType
> and repeating "type" feels a bit redundant to me.
>
> Is it the duplicate "Unknown" that inspired it?

That's convention for naming enum values. You have to mention somewhere
in the name value the enum it belongs to. It's like all those enums in
Qt namespace.

>
> Oh and do please bump the debian version.
>
> Don't we need to add some qml test to test/qmltest/unity/shell/application to check these objects actually contain these properties? Which means this interface has to be mocked. /me unsure

Can't really bump the debian version right now as it builds on top of a
yet unreleased MP (the app focus stuff). The debian/changelog would
conflict big time. So I will do it once the prerequisite gets released.

lp:~dandrader/unity-api/mirSurface updated
175. By Michi Henning

Remove dependency on gcc 4.9. Fixes: #1452342
Approved by: Marcus Tomlinson, PS Jenkins bot, Matthias Klose

176. By CI Train Bot Account

Releasing 7.97+15.10.20150721-0ubuntu1

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

Ok, no further objections. Will ack after you can bump deb version

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

Actually no, one other thing:
+ Q_PROPERTY(QSize size READ size WRITE setSize NOTIFY sizeChanged)
having this property writable is confusing, as when we set the size, we're only asking the client to resize, and that may not be allowed, or the resized buffer will only be returned after up to 2 frame swaps.

This API suggests the resize is immediate, and claims the pre-resize frame
is of the desired size, which is wrong.

Having a separate resize(QSize) method helps reinforce this, just like you did in qtmir. So please make the property non-writable, and add the resize() method to this interface.

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

+ * @brief Width of the contained MirSurface, if any
+ * If set, it means the desired width for the contained MirSurface. Otherwise it has
+ * the current width of the contained MirSurface.
+ Q_PROPERTY(int surfaceWidth READ surfaceWidth

Kind confusing. What if I set a new width, but then want to read the current width?

Did you get anywhere looking into implicitHeight/Width to be readonly values which always reflect the current actual size of the surface?

If so, then the purpose of surfaceWidth/Height can be made more clear: it sets the surfaceWidth/Height we desire. A rename might be needed, e.g. requestedSurfaceWidth/Height

review: Needs Information
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:184
http://jenkins.qa.ubuntu.com/job/unity-api-ci/320/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-api-wily-amd64-ci/19
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-api-wily-armhf-ci/19
        deb: http://jenkins.qa.ubuntu.com/job/unity-api-wily-armhf-ci/19/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-api-wily-i386-ci/19

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-api-ci/320/rebuild

review: Approve (continuous-integration)
Revision history for this message
Daniel d'Andrada (dandrader) wrote :

On 22/07/15 08:08, Gerry Boland wrote:
> Review: Needs Information
>
> + * @brief Width of the contained MirSurface, if any
> + * If set, it means the desired width for the contained MirSurface. Otherwise it has
> + * the current width of the contained MirSurface.
> + Q_PROPERTY(int surfaceWidth READ surfaceWidth
>
> Kind confusing. What if I set a new width, but then want to read the current width?
>
> Did you get anywhere looking into implicitHeight/Width to be readonly values which always reflect the current actual size of the surface?
>
> If so, then the purpose of surfaceWidth/Height can be made more clear: it sets the surfaceWidth/Height we desire. A rename might be needed, e.g. requestedSurfaceWidth/Height

Fixed.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:185
http://jenkins.qa.ubuntu.com/job/unity-api-ci/321/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-api-wily-amd64-ci/20
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-api-wily-armhf-ci/20
        deb: http://jenkins.qa.ubuntu.com/job/unity-api-wily-armhf-ci/20/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-api-wily-i386-ci/20

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-api-ci/321/rebuild

review: Approve (continuous-integration)
Revision history for this message
Daniel d'Andrada (dandrader) wrote :

On 22/07/15 08:02, Gerry Boland wrote:
> Review: Needs Fixing
>
> Actually no, one other thing:
> + Q_PROPERTY(QSize size READ size WRITE setSize NOTIFY sizeChanged)
> having this property writable is confusing, as when we set the size, we're only asking the client to resize, and that may not be allowed, or the resized buffer will only be returned after up to 2 frame swaps.
>
> This API suggests the resize is immediate, and claims the pre-resize frame
> is of the desired size, which is wrong.
>
> Having a separate resize(QSize) method helps reinforce this, just like you did in qtmir. So please make the property non-writable, and add the resize() method to this interface.

Done.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:186
http://jenkins.qa.ubuntu.com/job/unity-api-ci/322/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-api-wily-amd64-ci/21
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-api-wily-armhf-ci/21
        deb: http://jenkins.qa.ubuntu.com/job/unity-api-wily-armhf-ci/21/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-api-wily-i386-ci/21

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-api-ci/322/rebuild

review: Approve (continuous-integration)
Revision history for this message
Gerry Boland (gerboland) wrote :

Doxygen complaining some things not documented:

/home/gerry/dev/projects/unity-api/mirSurface/include/unity/shell/application/MirSurfaceInterface.h:27: warning: Member name (property) of class unity::shell::application::MirSurfaceInterface is not documented.
/home/gerry/dev/projects/unity-api/mirSurface/include/unity/shell/application/MirSurfaceInterface.h:27: warning: Member state (property) of class unity::shell::application::MirSurfaceInterface is not documented.
/home/gerry/dev/projects/unity-api/mirSurface/include/unity/shell/application/MirSurfaceInterface.h:27: warning: Member orientationAngle (property) of class unity::shell::application::MirSurfaceInterface is not documented.

Otherwise, looks good!

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

I still think we need to add the mock implementation stuff to test/qmltest/unity/shell/Application

review: Needs Information
Revision history for this message
Daniel d'Andrada (dandrader) wrote :

On 23/07/15 11:20, Gerry Boland wrote:
> Review: Needs Information
>
> I still think we need to add the mock implementation stuff to test/qmltest/unity/shell/Application

Why? In my opinion they just add to the overhead of using unity-api for
no real benefit.

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

+++ include/unity/shell/application/MirSurfaceInterface.h
+ virtual void resize(int width, int height) = 0;
QSize please, for consistency

review: Needs Fixing
Revision history for this message
Daniel d'Andrada (dandrader) wrote :

On 23/07/15 13:47, Gerry Boland wrote:
> Review: Needs Fixing
>
> +++ include/unity/shell/application/MirSurfaceInterface.h
> + virtual void resize(int width, int height) = 0;
> QSize please, for consistency

Which is simply inconvenient. I'll have to change qtmir MirSurfaceItem
code from:
m_surface->resize(width, height);
to:
m_surface->resize(QSize(width, height));

Which makes me think there's no advantage in having surface dimensions
expressed as a MirSuface.size instead of a MirSurface.width and a
MirSurface.height

Revision history for this message
Daniel d'Andrada (dandrader) wrote :

On 23/07/15 11:16, Gerry Boland wrote:
> Doxygen complaining some things not documented:
>
> /home/gerry/dev/projects/unity-api/mirSurface/include/unity/shell/application/MirSurfaceInterface.h:27: warning: Member name (property) of class unity::shell::application::MirSurfaceInterface is not documented.
> /home/gerry/dev/projects/unity-api/mirSurface/include/unity/shell/application/MirSurfaceInterface.h:27: warning: Member state (property) of class unity::shell::application::MirSurfaceInterface is not documented.
> /home/gerry/dev/projects/unity-api/mirSurface/include/unity/shell/application/MirSurfaceInterface.h:27: warning: Member orientationAngle (property) of class unity::shell::application::MirSurfaceInterface is not documented.
>
 Fixed

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:187
http://jenkins.qa.ubuntu.com/job/unity-api-ci/323/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-api-wily-amd64-ci/22
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-api-wily-armhf-ci/22
        deb: http://jenkins.qa.ubuntu.com/job/unity-api-wily-armhf-ci/22/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-api-wily-i386-ci/22

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-api-ci/323/rebuild

review: Approve (continuous-integration)
lp:~dandrader/unity-api/mirSurface updated
177. By Mirco Müller

Added alert-API to LauncherItemInterface and LauncherModelInterface to allow applications to trigger a wiggle/peeking-animation on their launcher-icon to draw users attention.
Approved by: PS Jenkins bot, Michael Zanetti

178. By CI Train Bot Account

Releasing 7.98+15.10.20150724-0ubuntu1

179. By Daniel d'Andrada

Merge lp:~dandrader/unity-api/app-state-handling

* Remove ApplicationManagerInterface.forceDashActive
* Remove ApplicationManagerInterface.suspended
* Add ApplicationInfoInterface.requestedState
  ScopeInterface
* This project forked from lp:unity/phablet. Updating the versioning to

180. By Daniel d'Andrada

Added MirSurface and MirSurfaceItem interfaces

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

> On 23/07/15 13:47, Gerry Boland wrote:
> > Review: Needs Fixing
> >
> > +++ include/unity/shell/application/MirSurfaceInterface.h
> > + virtual void resize(int width, int height) = 0;
> > QSize please, for consistency
>
> Which is simply inconvenient. I'll have to change qtmir MirSurfaceItem
> code from:
> m_surface->resize(width, height);
> to:
> m_surface->resize(QSize(width, height));

So it's not a big deal here, as it's setting width & height in a single function call. I just prefer the consistency of using QSize everywhere to refer to size. That's what it is for. Making an author do an extra line of code, to get some basic type safety, is a win in my book.

> Which makes me think there's no advantage in having surface dimensions
> expressed as a MirSuface.size instead of a MirSurface.width and a
> MirSurface.height

I'm trying to avoid invalid intermediate states appearing. Surface resize of 100x100->101x101 should not suggest to shell a 100x101 or 101x100 middle-state - that's just incorrect. Having individual getters & signals for width & height will result in these invalid intermediate states.

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

> On 23/07/15 11:20, Gerry Boland wrote:
> > Review: Needs Information
> >
> > I still think we need to add the mock implementation stuff to
> test/qmltest/unity/shell/Application
>
> Why? In my opinion they just add to the overhead of using unity-api for
> no real benefit.

That's debatable, but I'm struggling to find someone who will partake in that debate! As a result, I suggest we just go for this, and deal with the heat if it happens later.

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

> > Oh and do please bump the debian version.
> Can't really bump the debian version right now as it builds on top of a
> yet unreleased MP (the app focus stuff). The debian/changelog would
> conflict big time. So I will do it once the prerequisite gets released.

Think now you can

lp:~dandrader/unity-api/mirSurface updated
181. By Daniel d'Andrada

Add MirSurfaceInterface::resize(QSize)

Revision history for this message
Daniel d'Andrada (dandrader) wrote :

Addressed it all here:
https://code.launchpad.net/~dandrader/unity-api/mirSurface-15.04/+merge/268484

Revision history for this message
Unity8 CI Bot (unity8-ci-bot) wrote :

FAILED: Continuous integration, rev:181
https://unity8-jenkins.ubuntu.com/job/lp-unity-api-1-ci/4/
Executed test runs:
    None: https://unity8-jenkins.ubuntu.com/job/lp-generic-2-update-mp/4/console

Click here to trigger a rebuild:
https://unity8-jenkins.ubuntu.com/job/lp-unity-api-1-ci/4/rebuild

review: Needs Fixing (continuous-integration)

Unmerged revisions

181. By Daniel d'Andrada

Add MirSurfaceInterface::resize(QSize)

180. By Daniel d'Andrada

Added MirSurface and MirSurfaceItem interfaces

179. By Daniel d'Andrada

Merge lp:~dandrader/unity-api/app-state-handling

* Remove ApplicationManagerInterface.forceDashActive
* Remove ApplicationManagerInterface.suspended
* Add ApplicationInfoInterface.requestedState
  ScopeInterface
* This project forked from lp:unity/phablet. Updating the versioning to

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'include/unity/shell/application/CMakeLists.txt'
2--- include/unity/shell/application/CMakeLists.txt 2015-06-19 12:02:05 +0000
3+++ include/unity/shell/application/CMakeLists.txt 2015-08-19 13:12:34 +0000
4@@ -7,7 +7,7 @@
5
6 set(UNITY_API_LIB_HDRS ${UNITY_API_LIB_HDRS} ${headers} ${internal_headers} PARENT_SCOPE)
7
8-set(VERSION 7)
9+set(VERSION 8)
10 set(PKGCONFIG_NAME "unity-shell-application")
11 set(PKGCONFIG_DESCRIPTION "Unity shell Application APIs")
12 set(PKGCONFIG_REQUIRES "Qt5Core")
13
14=== added file 'include/unity/shell/application/Mir.h'
15--- include/unity/shell/application/Mir.h 1970-01-01 00:00:00 +0000
16+++ include/unity/shell/application/Mir.h 2015-08-19 13:12:34 +0000
17@@ -0,0 +1,76 @@
18+/*
19+ * Copyright (C) 2015 Canonical, Ltd.
20+ *
21+ * This program is free software; you can redistribute it and/or modify
22+ * it under the terms of the GNU General Public License as published by
23+ * the Free Software Foundation; version 3.
24+ *
25+ * This program is distributed in the hope that it will be useful,
26+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
27+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
28+ * GNU General Public License for more details.
29+ *
30+ * You should have received a copy of the GNU General Public License
31+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
32+ */
33+
34+#ifndef UNITY_SHELL_APPLICATION_MIR_H
35+#define UNITY_SHELL_APPLICATION_MIR_H
36+
37+#include <QObject>
38+
39+/**
40+ @brief Acting as a namespace to hold enums and such for use in QML
41+ */
42+class Mir
43+{
44+ Q_GADGET
45+ Q_ENUMS(Type)
46+ Q_ENUMS(State)
47+ Q_ENUMS(OrientationAngle)
48+
49+public:
50+ /**
51+ @brief Surface type
52+ */
53+ enum Type {
54+ UnknownType,
55+ NormalType,
56+ UtilityType,
57+ DialogType,
58+ GlossType,
59+ FreeStyleType,
60+ MenuType,
61+ InputMethodType,
62+ SatelliteType,
63+ TipType,
64+ };
65+
66+ /**
67+ @brief Surface state
68+ */
69+ enum State {
70+ UnknownState,
71+ RestoredState,
72+ MinimizedState,
73+ MaximizedState,
74+ VertMaximizedState,
75+ FullscreenState,
76+ HorizMaximizedState,
77+ HiddenState,
78+ };
79+
80+ /**
81+ @brief Surface orientation angle
82+ */
83+ enum OrientationAngle {
84+ Angle0 = 0,
85+ Angle90 = 90,
86+ Angle180 = 180,
87+ Angle270 = 270
88+ };
89+};
90+
91+Q_DECLARE_METATYPE(Mir::OrientationAngle)
92+
93+#endif // UNITY_SHELL_APPLICATION_MIR_H
94
95=== added file 'include/unity/shell/application/MirSurfaceInterface.h'
96--- include/unity/shell/application/MirSurfaceInterface.h 1970-01-01 00:00:00 +0000
97+++ include/unity/shell/application/MirSurfaceInterface.h 2015-08-19 13:12:34 +0000
98@@ -0,0 +1,114 @@
99+/*
100+ * Copyright (C) 2015 Canonical, Ltd.
101+ *
102+ * This program is free software; you can redistribute it and/or modify
103+ * it under the terms of the GNU General Public License as published by
104+ * the Free Software Foundation; version 3.
105+ *
106+ * This program is distributed in the hope that it will be useful,
107+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
108+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
109+ * GNU General Public License for more details.
110+ *
111+ * You should have received a copy of the GNU General Public License
112+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
113+ */
114+
115+#ifndef UNITY_SHELL_APPLICATION_MIRSURFACE_H
116+#define UNITY_SHELL_APPLICATION_MIRSURFACE_H
117+
118+#include <QObject>
119+#include <QSize>
120+
121+#include "Mir.h"
122+
123+namespace unity
124+{
125+namespace shell
126+{
127+namespace application
128+{
129+
130+/**
131+ @brief Holds a Mir surface. Pretty much an opaque class.
132+
133+ All surface manipulation is done by giving it to a MirSurfaceItem and then
134+ using MirSurfaceItem's properties.
135+ */
136+class MirSurfaceInterface : public QObject
137+{
138+ Q_OBJECT
139+
140+ /**
141+ * @brief The surface type
142+ */
143+ Q_PROPERTY(Mir::Type type READ type NOTIFY typeChanged)
144+
145+ /**
146+ * @brief Name of the surface, given by the client application
147+ */
148+ Q_PROPERTY(QString name READ name CONSTANT)
149+
150+ /**
151+ * @brief Size of the current surface buffer, in pixels.
152+ */
153+ Q_PROPERTY(QSize size READ size NOTIFY sizeChanged)
154+
155+ /**
156+ * @brief State of the surface
157+ */
158+ Q_PROPERTY(Mir::State state READ state WRITE setState NOTIFY stateChanged)
159+
160+ /**
161+ * @brief True if it has a mir client bound to it.
162+ * A "zombie" (live == false) surface never becomes alive again.
163+ */
164+ Q_PROPERTY(bool live READ live NOTIFY liveChanged)
165+
166+ /**
167+ * @brief Orientation angle of the surface
168+ *
169+ * How many degrees, clockwise, the UI in the surface has to rotate to match shell's UI orientation
170+ */
171+ Q_PROPERTY(Mir::OrientationAngle orientationAngle READ orientationAngle WRITE setOrientationAngle
172+ NOTIFY orientationAngleChanged DESIGNABLE false)
173+
174+public:
175+ /// @cond
176+ MirSurfaceInterface(QObject *parent = nullptr) : QObject(parent) {}
177+ virtual ~MirSurfaceInterface() {}
178+
179+ virtual Mir::Type type() const = 0;
180+
181+ virtual QString name() const = 0;
182+
183+ virtual QSize size() const = 0;
184+ virtual void resize(int width, int height) = 0;
185+ virtual void resize(const QSize &size) = 0;
186+
187+ virtual Mir::State state() const = 0;
188+ virtual void setState(Mir::State qmlState) = 0;
189+
190+ virtual bool live() const = 0;
191+
192+ virtual Mir::OrientationAngle orientationAngle() const = 0;
193+ virtual void setOrientationAngle(Mir::OrientationAngle angle) = 0;
194+ /// @endcond
195+
196+Q_SIGNALS:
197+ /// @cond
198+ void typeChanged(Mir::Type value);
199+ void liveChanged(bool value);
200+ void stateChanged(Mir::State value);
201+ void orientationAngleChanged(Mir::OrientationAngle value);
202+ void sizeChanged(const QSize &value);
203+ /// @endcond
204+};
205+
206+} // namespace application
207+} // namespace shell
208+} // namespace unity
209+
210+Q_DECLARE_METATYPE(unity::shell::application::MirSurfaceInterface*)
211+
212+#endif // UNITY_SHELL_APPLICATION_MIRSURFACE_H
213
214=== added file 'include/unity/shell/application/MirSurfaceItemInterface.h'
215--- include/unity/shell/application/MirSurfaceItemInterface.h 1970-01-01 00:00:00 +0000
216+++ include/unity/shell/application/MirSurfaceItemInterface.h 2015-08-19 13:12:34 +0000
217@@ -0,0 +1,151 @@
218+/*
219+ * Copyright (C) 2015 Canonical, Ltd.
220+ *
221+ * This program is free software; you can redistribute it and/or modify
222+ * it under the terms of the GNU General Public License as published by
223+ * the Free Software Foundation; version 3.
224+ *
225+ * This program is distributed in the hope that it will be useful,
226+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
227+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
228+ * GNU General Public License for more details.
229+ *
230+ * You should have received a copy of the GNU General Public License
231+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
232+ */
233+
234+#ifndef UNITY_SHELL_APPLICATION_MIRSURFACEITEM_H
235+#define UNITY_SHELL_APPLICATION_MIRSURFACEITEM_H
236+
237+#include "Mir.h"
238+
239+#include <QQuickItem>
240+
241+namespace unity
242+{
243+namespace shell
244+{
245+namespace application
246+{
247+
248+class MirSurfaceInterface;
249+
250+/**
251+ @brief Renders a MirSurface in a QML scene and forwards the input events it receives to it.
252+
253+ You can have multiple MirSurfaceItems displaying the same MirSurface. But care must
254+ be taken that only one of them feeds the MirSurface with input events and also only
255+ one resizes it.
256+ */
257+class MirSurfaceItemInterface : public QQuickItem
258+{
259+ Q_OBJECT
260+
261+ /**
262+ * @brief The surface to be displayed
263+ */
264+ Q_PROPERTY(unity::shell::application::MirSurfaceInterface* surface READ surface WRITE setSurface NOTIFY surfaceChanged)
265+
266+ /**
267+ * @brief Type of the given surface or Mir.UnknownType if no surface is set
268+ */
269+ Q_PROPERTY(Mir::Type type READ type NOTIFY typeChanged)
270+
271+ /**
272+ * @brief State of the given surface or Mir.UnknownState if no surface is set
273+ */
274+ Q_PROPERTY(Mir::State surfaceState READ surfaceState WRITE setSurfaceState NOTIFY surfaceStateChanged)
275+
276+ /**
277+ * @brief Name of the given surface or an empty string if no surface is set
278+ */
279+ Q_PROPERTY(QString name READ name CONSTANT)
280+
281+ /**
282+ * @brief True if the item has a surface and that surface has a mir client bound to it.
283+ * A "zombie" (live == false) surface never becomes alive again.
284+ */
285+ Q_PROPERTY(bool live READ live NOTIFY liveChanged)
286+
287+ /**
288+ * @brief Orientation angle of the given surface
289+ *
290+ * How many degrees, clockwise, the UI in the surface has to rotate to match shell's UI orientation
291+ */
292+ Q_PROPERTY(Mir::OrientationAngle orientationAngle READ orientationAngle WRITE setOrientationAngle
293+ NOTIFY orientationAngleChanged DESIGNABLE false)
294+
295+
296+ /**
297+ * @brief Whether the item will forward activeFocus, touch events, mouse events and key events to its surface.
298+ * It's false by default.
299+ * Only one item should have this property enabled for a given surface.
300+ */
301+ Q_PROPERTY(bool consumesInput READ consumesInput
302+ WRITE setConsumesInput
303+ NOTIFY consumesInputChanged)
304+
305+ /**
306+ * @brief The desired width for the contained MirSurface.
307+ * It's ignored if set to zero or a negative number
308+ * The default value is zero
309+ */
310+ Q_PROPERTY(int surfaceWidth READ surfaceWidth
311+ WRITE setSurfaceWidth
312+ NOTIFY surfaceWidthChanged)
313+
314+ /**
315+ * @brief The desired height for the contained MirSurface.
316+ * It's ignored if set to zero or a negative number
317+ * The default value is zero
318+ */
319+ Q_PROPERTY(int surfaceHeight READ surfaceHeight
320+ WRITE setSurfaceHeight
321+ NOTIFY surfaceHeightChanged)
322+
323+public:
324+ /// @cond
325+ MirSurfaceItemInterface(QQuickItem *parent = 0) : QQuickItem(parent) {}
326+ virtual ~MirSurfaceItemInterface() {}
327+
328+ virtual Mir::Type type() const = 0;
329+ virtual QString name() const = 0;
330+ virtual bool live() const = 0;
331+
332+ virtual Mir::State surfaceState() const = 0;
333+ virtual void setSurfaceState(Mir::State) = 0;
334+
335+ virtual Mir::OrientationAngle orientationAngle() const = 0;
336+ virtual void setOrientationAngle(Mir::OrientationAngle angle) = 0;
337+
338+ virtual MirSurfaceInterface* surface() const = 0;
339+ virtual void setSurface(MirSurfaceInterface*) = 0;
340+
341+ virtual bool consumesInput() const = 0;
342+ virtual void setConsumesInput(bool value) = 0;
343+
344+ virtual int surfaceWidth() const = 0;
345+ virtual void setSurfaceWidth(int value) = 0;
346+
347+ virtual int surfaceHeight() const = 0;
348+ virtual void setSurfaceHeight(int value) = 0;
349+ /// @endcond
350+
351+Q_SIGNALS:
352+ /// @cond
353+ void typeChanged(Mir::Type);
354+ void surfaceStateChanged(Mir::State);
355+ void liveChanged(bool live);
356+ void orientationAngleChanged(Mir::OrientationAngle angle);
357+ void surfaceChanged(MirSurfaceInterface*);
358+ void consumesInputChanged(bool value);
359+ void surfaceWidthChanged(int value);
360+ void surfaceHeightChanged(int value);
361+ /// @endcond
362+};
363+
364+} // namespace application
365+} // namespace shell
366+} // namespace unity
367+
368+#endif // UNITY_SHELL_APPLICATION_MIRSURFACEITEM_H

Subscribers

People subscribed via source and target branches

to all changes: