libindicator

Merge lp://staging/~3v1n0/libindicator/reduce-image-serialization into lp://staging/libindicator/14.04

reduce-image-serialization
Merge into trunk.14.04

Proposed by Marco Trevisan (Treviño) on 2014-02-28

Status:

Merged

Merged at revision:

526

Proposed branch:

lp://staging/~3v1n0/libindicator/reduce-image-serialization

Merge into:

lp://staging/libindicator/14.04

Diff against target:

136 lines (+44/-40)

1 file modified

libindicator/indicator-image-helper.c (+44/-40)

To merge this branch:

bzr merge lp://staging/~3v1n0/libindicator/reduce-image-serialization

Related bugs:

Bug #784055: CPU usage increases dramatically (and visible stuttering) when running indicator-multiload	Low	Fix Committed
Bug #1000785: unity-panel-service cause high cpu usage during wifi-connection	Medium	Fix Committed
Bug #1285989: Unity should use high-quality SVG icons for the indicators in HiDPI mode	Medium	Fix Committed

Link a bug report

Reviewer	Review Type	Date Requested	Status
PS Jenkins bot (community)	continuous-integration		Approve on 2014-03-03
Lars Karlitski (community)		2014-02-28	Approve on 2014-03-03
Review via email: mp+208736@code.staging.launchpad.net

Commit message

IndicatorImageHelper: always try to use a GIcon or the filename as source of the GdkImage

We don't need to fallback to pure pixbuf, unless an indicator provided a custom icon that is not in the current theme path.
This helps a lot in reducing the Unity7 workload as this decreases the cases where
we need to encode the pixbuf contents, send them via dbus to unity, encode them
back, reload to a new pixbuf...

Also thanks to this, the library clients can load the actual icon, scaled at the value they want.

Description of the change

Try to avoid image loading to a pixbuf, unless we can't export a GImage or a
filename

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-02-28:

PASSED: Continuous integration, rev:527
http://jenkins.qa.ubuntu.com/job/libindicator-ci/51/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/libindicator-trusty-amd64-ci/4
    SUCCESS: http://jenkins.qa.ubuntu.com/job/libindicator-trusty-armhf-ci/4
        deb: http://jenkins.qa.ubuntu.com/job/libindicator-trusty-armhf-ci/4/artifact/work/output/*zip*/output.zip

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

review: Approve (continuous-integration)

Revision history for this message

Lars Karlitski (larsu) wrote on 2014-02-28:

Yeah, I'd love to have this as well, but some of the icons aren't square and need different rescaling.

For example, this patch breaks indicator-power's icon. It's much smaller and blurry.

Revision history for this message

Marco Trevisan (Treviño) (3v1n0) wrote on 2014-02-28:

> Yeah, I'd love to have this as well, but some of the icons aren't square and
> need different rescaling.
>
> For example, this patch breaks indicator-power's icon. It's much smaller and
> blurry.

Well, not in unity [with lp:~3v1n0/unity/hidpi-better-scaling]... As we handle this thing at UI level (where it should be done imho), btw I've pushed also a change to the indicator-loader to show how to deal with this case.

I know it's something it would be nice to have at library level, but the gain in handling just image references is too high for us...
We could probably do the same thing I did here at unity-panel-service level (reading the "private" INDICATOR_NAMES_DATA, but I'd prefer to depend on an unstable data element like that).

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-02-28:

PASSED: Continuous integration, rev:528
http://jenkins.qa.ubuntu.com/job/libindicator-ci/52/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/libindicator-trusty-amd64-ci/5
    SUCCESS: http://jenkins.qa.ubuntu.com/job/libindicator-trusty-armhf-ci/5
        deb: http://jenkins.qa.ubuntu.com/job/libindicator-trusty-armhf-ci/5/artifact/work/output/*zip*/output.zip

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

review: Approve (continuous-integration)

Revision history for this message

Lars Karlitski (larsu) wrote on 2014-03-03:

> Well, not in unity [with lp:~3v1n0/unity/hidpi-better-scaling]... As we handle
> this thing at UI level (where it should be done imho), btw I've pushed also a
> change to the indicator-loader to show how to deal with this case.

I totally agree that this is where it should be handled. I didn't know you already fixed it. Nice work!

There's a bigger margin around the menu item in the loader now, but the size of the pixbuf seems to be correct. (Doesn't need to be fixed, I'm just mentioning it.)

I'm fine with landing this after the hidpi unity branch landed. Thanks!

review: Approve

lp://staging/~3v1n0/libindicator/reduce-image-serialization updated on 2014-03-03

528. By Marco Trevisan (Treviño) on 2014-03-03

IndicatorLoader: Make sure that we load the icon at its original size, if not higher than IMAGE_SIZE

In this way icons such as the battery one won't be shrunk.

Revision history for this message

Marco Trevisan (Treviño) (3v1n0) wrote on 2014-03-03:

So, I've pushed a new revision that handles the things in a different way: basically setting the pixel size to 0 makes gtk to load the icon at its full size; so we can safely do that if the actual icon size isn't bigger than the ICON_SIZE value.

In this way also gtk apps (such as the greeter) won't need any change to support these kinds of icons.

Revision history for this message

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

PASSED: Continuous integration, rev:528
http://jenkins.qa.ubuntu.com/job/libindicator-ci/53/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/libindicator-trusty-amd64-ci/6
    SUCCESS: http://jenkins.qa.ubuntu.com/job/libindicator-trusty-armhf-ci/6
        deb: http://jenkins.qa.ubuntu.com/job/libindicator-trusty-armhf-ci/6/artifact/work/output/*zip*/output.zip