Compiz

Merge lp://staging/~sjakthol/compiz/fix-749084 into lp://staging/compiz/0.9.10

fix-749084
Merge into 0.9.10

Proposed by Sami Jaktholm on 2013-07-03

Status:

Merged

Approved by:

MC Return on 2013-07-08

Approved revision:

3755

Merged at revision:

3758

Proposed branch:

lp://staging/~sjakthol/compiz/fix-749084

Merge into:

lp://staging/compiz/0.9.10

Diff against target:

942 lines (+360/-462)

2 files modified

plugins/dbus/src/dbus.cpp (+305/-461)
plugins/dbus/src/dbus.h (+55/-1)

To merge this branch:

bzr merge lp://staging/~sjakthol/compiz/fix-749084

Medium

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
PS Jenkins bot (community)	continuous-integration		Approve on 2013-07-09
MC Return		2013-07-03	Approve on 2013-07-05
Review via email: mp+172846@code.staging.launchpad.net

Commit message

Port dbus introspection to compiz 0.9.
- move xml creation to a separate class (IntrospectionResponse) for easier
memory management (allocate buffer and writer in ctor, free in dtor).
- move duplicated response sending code to a separate method that takes
IntrospectionResponse and sends the resulting xml.
- Refactor handle*IntrospectMessage to work with compiz 0.9 interfaces.

This also fixes the broken list method which was a result of logic error. The
code to invoke list handler was never reached.

This fixes most of the issues noted in bug 749084.

Description of the change

The broken list method is fixed by change at line 834 of the diff. Cause was following code:
if (path.empty ()) ... else if (path.size () == 1) ... else if (path.size () < 2). If path was smaller than two, it either had a size of 1 or it was empty.

Please note that the plugin and option metadata is still unavailable.

Revision history for this message

MC Return (mc-return) wrote on 2013-07-03:

Wow, great job !!!
Thanks a lot, Sami, for bringing this back to life.

I will test this ASAP.

Revision history for this message

MC Return (mc-return) wrote on 2013-07-03:

First review (just a few style issues):

You could use prefix here:

65 + nArgs--;

88 + nArgs--;

The newline here could be removed:

493 +

lp://staging/~sjakthol/compiz/fix-749084 updated on 2013-07-04

3755. By Sami Jaktholm on 2013-07-04: Use prefix decrements and remove extra newline.

Revision history for this message

Sami Jaktholm (sjakthol) wrote on 2013-07-04:

Fixed.

Revision history for this message

MC Return (mc-return) wrote on 2013-07-04:

Looks good to me :)
Most stuff seems to show up again: http://uppix.com/f-D_Feet_D_Bus_deb51d5b87100134a82.png

Sami, 3 questions - you wrote:

"This fixes most of the issues noted in bug 749084."

Which are the open issues ?
Is there a bug report about those, or could you file one ?
Could you fix the remaining issues, or what do we need to do to fix them ?

Anyway, green light from me - top job, but we are currently waiting with merges as Ubuntu is testing 0.9.10 for Saucy and Sam is busy...
So the landing of this (and many other things) might still take a few days...

review: Approve

Revision history for this message

Sami Jaktholm (sjakthol) wrote on 2013-07-05:

> Which are the open issues ?
Plugin and option metadata are missing.

> Could you fix the remaining issues, or what do we need to do to fix them ?
Plugins and options no more carry their descriptions with them so we probably need to tap into libcompizconfig to get that information. That's all I know right now, I might investigate it when I have more time.

> Is there a bug report about those, or could you file one ?
Filed bug 1198024 for that one.

Revision history for this message

MC Return (mc-return) wrote on 2013-07-05:

> > Which are the open issues ?
> Plugin and option metadata are missing.
>
Sounds serious... :(

> > Could you fix the remaining issues, or what do we need to do to fix them ?
> Plugins and options no more carry their descriptions with them so we probably
> need to tap into libcompizconfig to get that information. That's all I know
> right now, I might investigate it when I have more time.
>
That would be great, thanks a lot for all your work. +1
IMHO you should get Compiz maintainer rights, Sami - are you interested ?

> > Is there a bug report about those, or could you file one ?
> Filed bug 1198024 for that one.

Thanks a lot, I've added this bug to the 0.9.10 milestone and have set it's importance to medium.

review: Approve

Revision history for this message

PS Jenkins bot (ps-jenkins) on 2013-07-09:

review: Approve (continuous-integration)