Merge into trunk : indicator-root-state-icons-fix : Code : Unity 8

Status:	Merged
Approved by:	Antti Kaijanmäki on 2014-05-13
Approved revision:	800
Merged at revision:	893
Proposed branch:	lp:~kaijanmaki/unity8/indicator-root-state-icons-fix
Merge into:	lp:unity8
Diff against target:	85 lines (+44/-8) 2 files modified plugins/Unity/Indicators/rootactionstate.cpp (+7/-8) tests/plugins/Unity/Indicators/rootactionstatetest.cpp (+37/-0)
To merge this branch:	bzr merge lp:~kaijanmaki/unity8/indicator-root-state-icons-fix
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
PS Jenkins bot (community)	continuous-integration		Needs Fixing on 2014-05-13
Nick Dedekind (community)		2014-04-01	Approve on 2014-05-13
Michał Sawicz			Abstain on 2014-05-13
Ted Gould (community)			Approve on 2014-05-08
Antti Kaijanmäki (community)			Approve on 2014-05-08
Review via email: mp+213727@code.launchpad.net

Revision history for this message

Antti Kaijanmäki (kaijanmaki) wrote on 2014-04-01:

#

Proposing this change as I noticed I was unable to get multiple icons working with my new indicator-network code base. Although the "icons" element in the root state is not documented in the "Indicators and Settings Protocol" document I figured it out that the variant in the VARDICT should contain an array of GVariants coming from g_icon_serialize(). Now, when my code did exactly that (see the dump below), the icon loading failed. For some reason the g_variant_get_child_value() returned a variant which contained another variant (the one coming from _serialize).

So the old code would have required an additional call:
child = g_variant_get_variant(child);

Using iter_loop() gets the correct results and is also the way "GVariant Format Strings" manual shows as an example how to unpack arrays.

===

$ dbus-send --print-reply --session --dest=com.canonical.indicator.network /com/canonical/indicator/network org.gtk.Actions.Describe string:phone.network-status

method return sender=:1.430 -> dest=:1.468 reply_serial=2
struct {
    boolean true
    signature ""
    array [
        variant array [
            dict entry(
                string "icon"
                variant struct {
                    string "themed"
                    variant array [
                        string "nm-signal-50-secure"
                    ]
                }
            )
            dict entry(
                string "icons"
                variant array [
                    variant struct {
                        string "themed"
                        variant array [
                            string "nm-signal-75"
                        ]
                    }
                    variant struct {
                        string "themed"
                        variant array [
                            string "nm-signal-25"
                        ]
                    }
                ]
            )
            dict entry(
                string "title"
                variant string "Network"
            )
            dict entry(
                string "visible"
                variant boolean true
            )
        ]
    ]
}

Proposing this change as I noticed I was unable to get multiple icons working with my new indicator-network code base. Although the "icons" element in the root state is not documented in the "Indicators and Settings Protocol" document I figured it out that the variant in the VARDICT should contain an array of GVariants coming from g_icon_serialize(). Now, when my code did exactly that (see the dump below), the icon loading failed. For some reason the g_variant_get_child_value() returned a variant which contained another variant (the one coming from _serialize).

So the old code would have required an additional call:
    child = g_variant_get_variant(child);

Using iter_loop() gets the correct results and is also the way "GVariant Format Strings" manual shows as an example how to unpack arrays.

===

$ dbus-send --print-reply --session --dest=com.canonical.indicator.network /com/canonical/indicator/network org.gtk.Actions.Describe string:phone.network-status

method return sender=:1.430 -> dest=:1.468 reply_serial=2
struct {
    boolean true
    signature ""
    array [
        variant array [
            dict entry(
                string "icon"
                variant struct {
                    string "themed"
                    variant array [
                        string "nm-signal-50-secure"
                    ]
                }
            )
            dict entry(
                string "icons"
                variant array [
                    variant struct {
                        string "themed"
                        variant array [
                            string "nm-signal-75"
                        ]
                    }
                    variant struct {
                        string "themed"
                        variant array [
                            string "nm-signal-25"
                        ]
                    }
                ]
            )
            dict entry(
                string "title"
                variant string "Network"
            )
            dict entry(
                string "visible"
                variant boolean true
            )
        ]
    ]
}

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-04-01:

#

FAILED: Continuous integration, rev:797
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~kaijanmaki/unity8/indicator-root-state-icons-fix/+merge/213727/+edit-commit-message

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity8-ci/2707/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-04-01:

#

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity8-ci/2708/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Antti Kaijanmäki (kaijanmaki) wrote on 2014-04-02:

#

ted pointed me to this more up to date wiki page:
https://wiki.ubuntu.com/SystemComponents#Root_Item

icons : av
If there is more than one icon shown, this should be an array of serialized GIcons in the order they should appear. This item should take precedence over 'icon'.

AFAIK the dbus-send printout confirms that my code in the indicator-service side is exporting exactly this.

Here's a dump from the vala code base currently on phone images:
struct {
    boolean true
    signature ""
    array [
        variant array [
            dict entry(
                string "title"
                variant string "Network"
            )
            dict entry(
                string "icon"
                variant struct {
                    string "themed"
                    variant array [
                        string "network-cellular-3g"
                    ]
                }
            )
            dict entry(
                string "accessibility-desc"
                variant string "Network (cellular, 3g)"
            )
            dict entry(
                string "icons"
                variant array [
                    struct {
                        string "themed"
                        variant array [
                            string "gsm-3g-medium"
                        ]
                    }
                    struct {
                        string "themed"
                        variant array [
                            string "network-cellular-3g"
                        ]
                    }
                ]
            )
            dict entry(
                string "visible"
                variant boolean true
            )
        ]
    ]
}

To me that looks like the signature of "icons" is a(sv).

Revision history for this message

Nick Dedekind (nick-dedekind) wrote on 2014-04-07:

#

My indicator-network is giving a variant of type: a(sv)
Multi-icon is not working for me.

review: Needs Information

Revision history for this message

Antti Kaijanmäki (kaijanmaki) wrote on 2014-04-07:

#

yes, as I said the old vala code is sending a(sv) which is not what the wikipage specifies.

This MP is part of a landing slot that fixes indicator-network side also to send a(v).

Also, by sending a(v) we can also provide plain strings inside a variant to implement the FIXME: should send URL.

Revision history for this message

Nick Dedekind (nick-dedekind) wrote on 2014-04-09:

#

Ok. In that case it looks ok; but cannot approve until the indicator-network code is available to test alongside it.

Revision history for this message

Antti Kaijanmäki (kaijanmaki) wrote on 2014-04-09:

#

> Ok. In that case it looks ok; but cannot approve until the indicator-network
> code is available to test alongside it.

I will link to a silo once we have it available.

Revision history for this message

Albert Astals Cid (aacid) wrote on 2014-04-24:

#

Antti, we'll also need you to copy&paste the top part of https://wiki.ubuntu.com/Process/Merges/Checklists/Unity8 as description and answer the questions.

Revision history for this message

Michał Sawicz (saviq) wrote on 2014-05-06:

#

Please strip tags: http://people.canonical.com/~msawicz/unity8/strip-u8-tags.sh

Do the same on your local checkout(s), too!

review: Needs Fixing

Revision history for this message

Antti Kaijanmäki (kaijanmaki) wrote on 2014-05-07:

#

and stripped tags.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-05-07:

#

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity8-ci/2932/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Nick Dedekind (nick-dedekind) wrote on 2014-05-08:

#

LGTM

review: Approve

Revision history for this message

Nick Dedekind (nick-dedekind) wrote on 2014-05-08:

#

needs test

review: Needs Fixing

Revision history for this message

Antti Kaijanmäki (kaijanmaki) wrote on 2014-05-08:

#

merged tests and deleted the old tags from this remote branch as well for real.

review: Approve

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-05-08:

#

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity8-ci/2937/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Ted Gould (ted) on 2014-05-08:

#

review: Approve

Revision history for this message

Michał Sawicz (saviq) on 2014-05-13:

#

review: Abstain

Revision history for this message

Antti Kaijanmäki (kaijanmaki) wrote on 2014-05-13:

#

I ran the whole unite8-autopilot tests on n4:

Ran 49 tests in 1782.995s
OK

Revision history for this message

Nick Dedekind (nick-dedekind) on 2014-05-13:

#

review: Approve

Revision history for this message

Nick Dedekind (nick-dedekind) wrote on 2014-05-13:

#

* Did you perform an exploratory manual test run of the code change and any related functionality?
Yes.

* Did CI run pass? If not, please explain why.
Unrelated failure.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-05-13:

#

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity8-ci/2951/rebuild

review: Needs Fixing (continuous-integration)

lp:~kaijanmaki/unity8/indicator-root-state-icons-fix updated on 2014-05-14

801. By Antti Kaijanmäki on 2014-05-14: merge upstream

 === modified file 'plugins/Unity/Indicators/rootactionstate.cpp'
 --- plugins/Unity/Indicators/rootactionstate.cpp	2013-10-11 15:51:46 +0000
 +++ plugins/Unity/Indicators/rootactionstate.cpp	2014-05-14 05:50:30 +0000
@@ -240,19 +240,18 @@
                  QStringList icons;
--                if (g_variant_type_is_array(g_variant_get_type(vvalue))) {
--
--
--                    for (int i = 0, iMax = g_variant_n_children(vvalue); i < iMax; i++) {
--                        GVariant *child = g_variant_get_child_value(vvalue, i);
--
++                if (g_variant_is_of_type(vvalue, G_VARIANT_TYPE("av"))) {
++                    GVariantIter iter;
++                    GVariant *val = 0;
++                    g_variant_iter_init (&iter, vvalue);
++                    while (g_variant_iter_loop (&iter, "v", &val))
++                    {
                          // FIXME - should be sending a url.
--                        GIcon *gicon = g_icon_deserialize (child);
++                        GIcon *gicon = g_icon_deserialize (val);
                          if (gicon) {
                              icons << iconUri(gicon);
                              g_object_unref (gicon);
+                         }
--                        g_variant_unref(child);
+                     }
+                 }
                  // will overwrite icon.
 === modified file 'tests/plugins/Unity/Indicators/rootactionstatetest.cpp'
 --- tests/plugins/Unity/Indicators/rootactionstatetest.cpp	2013-10-11 15:06:09 +0000
 +++ tests/plugins/Unity/Indicators/rootactionstatetest.cpp	2014-05-14 05:50:30 +0000
@@ -21,6 +21,7 @@
  #include <unitymenumodel.h>
  #include <QtTest>
++#include <gio/gio.h>
  class RootActionStateTest : public QObject
+ {
@@ -52,6 +53,42 @@
          QVERIFY(rootState->menu() == NULL);
          delete rootState;
+     }
++
++    void testToQVariantIcons()
++    {
++        GVariantBuilder builderIcons;
++        g_variant_builder_init(&builderIcons, G_VARIANT_TYPE("av"));
++        for (int i = 0; i < 3; i ++) {
++
++            GIcon* icon = NULL;
++            icon = g_icon_new_for_string (QString("testIcon%1").arg(i).toUtf8().constData(), NULL);
++
++            g_variant_builder_add(&builderIcons,
++                                  "v",
++                                  g_icon_serialize (icon));
++            g_object_unref(icon);
++        }
++
++        GVariantBuilder builderParams;
++        g_variant_builder_init(&builderParams, G_VARIANT_TYPE_ARRAY);
++        GVariant* icons = g_variant_builder_end (&builderIcons);
++        g_variant_ref_sink (icons);
++        g_variant_builder_add (&builderParams, "{sv}", g_strdup ("icons"), icons, NULL);
++
++        GVariant* params = g_variant_builder_end (&builderParams);
++
++        RootActionState rootState;
++        QVariant result = rootState.toQVariant(params);
++        g_variant_unref(params);
++
++        QVariantMap paramResult = result.toMap();
++        QVERIFY(paramResult.contains("icons"));
++
++        QStringList serializedIcons = paramResult["icons"].toStringList();
++        QVERIFY(serializedIcons[0] == "image://theme/testIcon0");
++        QVERIFY(serializedIcons[1] == "image://theme/testIcon1");
++        QVERIFY(serializedIcons[2] == "image://theme/testIcon2");
++    }
  };
  QTEST_GUILESS_MAIN(RootActionStateTest)

Unity 8

Merge lp:~kaijanmaki/unity8/indicator-root-state-icons-fix into lp:unity8

Commit message

Description of the change

Preview Diff

Subscribers