Notify OSD

Merge lp:~macslow/notify-osd/conditional-bubble-refresh into lp:notify-osd/karmic

  1. conditional-bubble-refresh
  2. Merge into karmic
Proposed by Mirco Müller
Status: Merged
Merge reported by: Mirco Müller
Merged at revision: not available
Proposed branch: lp:~macslow/notify-osd/conditional-bubble-refresh
Merge into: lp:notify-osd/karmic
Diff against target: 126 lines
1 file modified
src/bubble.c (+53/-9)
To merge this branch: bzr merge lp:~macslow/notify-osd/conditional-bubble-refresh
Reviewer Review Type Date Requested Status
Robert Collins (community) Approve
Ted Gould (community) Approve
Review via email: mp+13545@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Mirco Müller (macslow) wrote :

This branch adds checks to avoid superfluous tile-/blur-cache regeneration for icon, background and indicator/value. This optimization is meant to help solve LP: #367049

Revision history for this message
Ted Gould (ted) wrote :

I don't understand why you're only comparing the basename of the icons. It seems like there could be an odd case where there is the same basename, but different paths to an icon. It seems like you already have the path, just use that.

Everything else looks good.

review: Needs Information
lp:~macslow/notify-osd/conditional-bubble-refresh updated
405. By Mirco Müller

use the full filename (+path) for watching not just the basename of the icon

Revision history for this message
Mirco Müller (macslow) wrote :

> I don't understand why you're only comparing the basename of the icons. It
> seems like there could be an odd case where there is the same basename, but
> different paths to an icon. It seems like you already have the path, just use
> that.

Oh, that was obviously the wrong thing to survive my experiments with this branch. Switched back to use the full filename (+path).

Revision history for this message
Ted Gould (ted) :
review: Approve
Revision history for this message
Robert Collins (lifeless) wrote :

Looks ok to me. One note:
you treat "" and NULL as different. I don't know how folk don't pass in icons, but if they pass them in as NULL, then perhaps you should init the cache to NULL?

It would be really good to have a test that shows when a refresh is triggered, as this code is split over several places and as such it could well get regressed unobviously.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/bubble.c'
2--- src/bubble.c 2009-10-19 22:05:05 +0000
3+++ src/bubble.c 2009-10-19 22:37:09 +0000
4@@ -84,7 +84,7 @@
5 gint future_height;
6 gboolean prevent_fade;
7
8- // these will be replaced by notification_t* later on
9+ // these will be replaced by class Notification later on
10 GString* title;
11 GString* message_body;
12 guint id;
13@@ -94,6 +94,10 @@
14 guint timeout;
15 guint urgency;
16 //notification_t* notification;
17+
18+ // used to prevent unneeded updates of the tile-cache, for append-,
19+ // update or replace-cases, needs to move into class Notification
20+ GString* old_icon_filename;
21 };
22
23 enum
24@@ -2076,6 +2080,12 @@
25 priv->tile_indicator = NULL;
26 }
27
28+ if (priv->old_icon_filename)
29+ {
30+ g_string_free ((gpointer) priv->old_icon_filename, TRUE);
31+ priv->old_icon_filename = NULL;
32+ }
33+
34 // chain up to the parent class
35 G_OBJECT_CLASS (bubble_parent_class)->dispose (gobject);
36 }
37@@ -2282,6 +2292,7 @@
38 this->priv->tile_body = NULL;
39 this->priv->tile_indicator = NULL;
40 this->priv->prevent_fade = FALSE;
41+ this->priv->old_icon_filename = g_string_new ("");
42
43 update_input_shape (window, 1, 1);
44
45@@ -2422,6 +2433,17 @@
46
47 priv = GET_PRIVATE (self);
48
49+ //basename = g_path_get_basename (filename);
50+
51+ // check if an app tries to set the same file as icon again, this check
52+ // avoids superfluous regeneration of the tile/blur-cache for the icon,
53+ // thus it improves performance in update- and append-cases
54+ if (!g_strcmp0 (priv->old_icon_filename->str, filename))
55+ return;
56+
57+ // store the new icon-basename
58+ g_string_assign (priv->old_icon_filename, filename);
59+
60 if (priv->icon_pixbuf)
61 {
62 g_object_unref (priv->icon_pixbuf);
63@@ -2554,14 +2576,25 @@
64 bubble_set_value (Bubble* self,
65 gint value)
66 {
67+ BubblePrivate* priv;
68+
69 if (!self || !IS_BUBBLE (self))
70 return;
71
72- GET_PRIVATE (self)->value = value;
73-
74- g_signal_emit (self, g_bubble_signals[VALUE_CHANGED], 0, value);
75-
76- _refresh_indicator (self);
77+ priv = GET_PRIVATE (self);
78+
79+ // only really cause a refresh (blur, recreating tile-/blur-cache) if
80+ // a different value has been set, this helps improve performance when
81+ // a user e.g. presses and holds down keys for Volume-Up/Down or
82+ // Brightness-Up/Down and apps like gnome-settings-daemon or
83+ // gnome-power-daemon fire continues notification-updates due to the
84+ // key-repeat events
85+ if (priv->value != value)
86+ {
87+ priv->value = value;
88+ g_signal_emit (self, g_bubble_signals[VALUE_CHANGED], 0, value);
89+ _refresh_indicator (self);
90+ }
91 }
92
93 gint
94@@ -3306,6 +3339,8 @@
95 void
96 bubble_recalc_size (Bubble *self)
97 {
98+ gint old_bubble_width = 0;
99+ gint old_bubble_height = 0;
100 gint new_bubble_width = 0;
101 gint new_bubble_height = 0;
102 Defaults* d;
103@@ -3484,11 +3519,20 @@
104 break;
105 }
106 priv->future_height = new_bubble_height;
107+
108+ bubble_get_size (self, &old_bubble_width, &old_bubble_height);
109 bubble_set_size (self, new_bubble_width, new_bubble_height);
110
111- _refresh_background (self);
112- _refresh_title (self);
113- _refresh_body (self);
114+ // don't refresh tile/blur-caches for background, title or body if the
115+ // size of the bubble has not changed, this improves performance for the
116+ // update- and replace-cases
117+ if (old_bubble_width != new_bubble_width ||
118+ old_bubble_height != new_bubble_height)
119+ {
120+ _refresh_background (self);
121+ _refresh_title (self);
122+ _refresh_body (self);
123+ }
124
125 update_shape (self);
126 }

Subscribers

People subscribed via source and target branches

to all changes: