After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 612759 - Fix app icon fading
Fix app icon fading
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
: 612697 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2010-03-13 02:24 UTC by Colin Walters
Modified: 2010-03-13 17:51 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Fix app icon fading (13.93 KB, patch)
2010-03-13 02:24 UTC, Colin Walters
none Details | Review
Fix app icon fading (14.09 KB, patch)
2010-03-13 02:48 UTC, Colin Walters
none Details | Review
Fix app icon fading (13.24 KB, patch)
2010-03-13 02:52 UTC, Colin Walters
committed Details | Review

Description Colin Walters 2010-03-13 02:24:05 UTC
The way we were loading data into a CoglTexture, then pulling it out
and manipulating it on the CPU, then loading it back into a texture
was a bit lame.

Clean things up a bit here by loading directly into the CPU.

This patch also changes things so we cache the faded texture data
inside ShellApp, which is conceptually slightly unclean, but should
be workable in practice.
Comment 1 Colin Walters 2010-03-13 02:24:07 UTC
Created attachment 156036 [details] [review]
Fix app icon fading
Comment 2 Colin Walters 2010-03-13 02:48:49 UTC
Created attachment 156039 [details] [review]
Fix app icon fading
Comment 3 Colin Walters 2010-03-13 02:52:16 UTC
Created attachment 156040 [details] [review]
Fix app icon fading
Comment 4 drago01 2010-03-13 09:17:27 UTC
Review of attachment 156040 [details] [review]:

Looks good, even though we still don't know what was going on, pulling data from the video memory, processing it on the CPU and uploading it back to the GPU is not very efficient anyway.
And having it cached now is an added bonus.

Just some minor comments:

::: js/ui/panel.js
@@ +275,3 @@
             this._label.setText(this._focusedApp.get_name());
         } else if (this._activeSequence != null) {
             icon = this._activeSequence.create_icon(AppDisplay.APPICON_SIZE);

Looks inconsistent .. don't we want to create a faded icon here too?

::: src/shell-app.c
@@ +112,3 @@
+    + gdk_pixbuf_get_width (pixbuf) * ((gdk_pixbuf_get_n_channels (pixbuf)* gdk_pixbuf_get_bits_per_sample (pixbuf) + 7) / 8);
+
+  pixels = g_malloc0 (rowstride * height);

Should be pixbuf_byte_size right?

@@ +121,3 @@
+      for (j = 0; j < height; j++)
+        {
+          guchar *pixel = &pixels[j * rowstride + i * 4];

Instead of hard coding 4 the result of gdk_pixbuf_get_n_channels () should be used.
Seems odd to assume 4 here and check for an alpha channel later when creating the texture.
Comment 5 drago01 2010-03-13 12:18:05 UTC
*** Bug 612697 has been marked as a duplicate of this bug. ***
Comment 6 Colin Walters 2010-03-13 17:49:09 UTC
(In reply to comment #4)
> 
> Looks inconsistent .. don't we want to create a faded icon here too?

I'd prefer not to actually for now - when I land my startup-notification work, we'll much more consistently have an app in this case, and not having it will make the untracked case stand out more.

> ::: src/shell-app.c
> @@ +112,3 @@
> +    + gdk_pixbuf_get_width (pixbuf) * ((gdk_pixbuf_get_n_channels (pixbuf)*
> gdk_pixbuf_get_bits_per_sample (pixbuf) + 7) / 8);
> +
> +  pixels = g_malloc0 (rowstride * height);
> 
> Should be pixbuf_byte_size right?

Hmmm...I'm not entirely sure if cogl is going to handle that and, I don't think it matters much in the big picture; it's not going to be very many bytes, and we free it quickly anyways.  

> 
> @@ +121,3 @@
> +      for (j = 0; j < height; j++)
> +        {
> +          guchar *pixel = &pixels[j * rowstride + i * 4];
> 
> Instead of hard coding 4 the result of gdk_pixbuf_get_n_channels () should be
> used.
> Seems odd to assume 4 here and check for an alpha channel later when creating
> the texture.

Fixed, thanks.
Comment 7 Colin Walters 2010-03-13 17:51:05 UTC
Attachment 156040 [details] pushed as 3aea09b - Fix app icon fading