Merge lp:~rodrigo-moya/unity/fix-740360 into lp:unity

[Rodrigo Moya]

[Alejandro Piñeiro (apinheiro)]

The code seems ok to me, so I will approve it.

Anyway, I still notice the crash. I will upload a backtrace log to the bug

review: Approve

[Alejandro Piñeiro (apinheiro)]

Reviewing the backtrace, the crash happens in a line added by this branch, so I think that it would be better to disapprove the merge.

Sorry for the noise

review: Disapprove

[Rodrigo Moya (rodrigo-moya)]

Can you provide a gdb's backtrace to see where it crashes?

[Alejandro Piñeiro (apinheiro)]

> Can you provide a gdb's backtrace to see where it crashes?

Yes, it is on the bug itself, I didn't find a way to upload the backtrace on the merge proposal.

BTW: moving from disapprove to need fixing.

review: Needs Fixing

[Alejandro Piñeiro (apinheiro)]

Ok, this seems to work now, as I was not able to reproduce the bug. Some brief comments:

* Little nitpick on lines 41-44: it seems that the indentation are not homogeneous

* Some things to take into account (for other bug, but writing here to not forget that)
   * When you select the volume indicator it exposes the level
      * But if you move down, when the volume bar is selected no sound is exposed
      * The same when you select the play buttons
      * In general: some of those entries are not exposing
   * On the calendar item: if you move down, and enter the calendar, I didn't find any way to close that window. Moving left-right moves on the calendar, and press Esc doesnt close it. In the same way you can't press alt+f1 or other unity keybinding.
      * In summary you "get trapped" on the calendar

But as I said, those are issues for other bugs. Approving the branch

review: Approve

[Rodrigo Moya (rodrigo-moya)]

Neil gave his +1 on IRC

review: Approve

=== modified file 'services/panel-service.c'
--- services/panel-service.c	2011-03-17 21:32:51 +0000
+++ services/panel-service.c	2011-03-29 15:28:47 +0000
@@ -923,13 +923,22 @@
 static gboolean
 should_skip_menu (IndicatorObjectEntry *entry)
 {
-  gboolean label_ok;
-  gboolean image_ok;
-
-  label_ok = gtk_widget_get_visible (GTK_WIDGET (entry->label))
-    && gtk_widget_is_sensitive (GTK_WIDGET (entry->label));
-  image_ok = gtk_widget_get_visible (GTK_WIDGET (entry->image))
-    && gtk_widget_is_sensitive (GTK_WIDGET (entry->image));
+  gboolean label_ok = FALSE;
+  gboolean image_ok = FALSE;
+
+  g_return_val_if_fail (entry != NULL, TRUE);
+
+  if (GTK_IS_LABEL (entry->label))
+    {
+      label_ok = gtk_widget_get_visible (GTK_WIDGET (entry->label))
+	      && gtk_widget_is_sensitive (GTK_WIDGET (entry->label));
+    }
+
+  if (GTK_IS_IMAGE (entry->image))
+    {
+      image_ok = gtk_widget_get_visible (GTK_WIDGET (entry->image))
+	      && gtk_widget_is_sensitive (GTK_WIDGET (entry->image));
+    }
 
   return !label_ok && !image_ok;
 }
@@ -979,13 +988,22 @@
         }
 
       new_entries = indicator_object_get_entries (new_object);
+      // if the indicator has no entries, move to the next/prev one until we find one with entries
+      while (new_entries == NULL)
+        {
+          gint cur_object_index = g_slist_index (indicators, new_object);
+          gint new_object_index = cur_object_index + (direction == GTK_MENU_DIR_CHILD ? 1 : -1);
+          new_object = g_slist_nth_data (indicators, new_object_index);
+          new_entries = indicator_object_get_entries (new_object);
+	}
+
       new_entry = g_list_nth_data (new_entries, direction == GTK_MENU_DIR_PARENT ? g_list_length (new_entries) - 1 : 0);
 
       g_list_free (entries);
       g_list_free (new_entries);
 
       if (should_skip_menu (new_entry))
-	{	  
+        {	  
 	  activate_next_prev_menu (self, new_object, new_entry, direction);
 	  return;
 	}
@@ -997,7 +1015,7 @@
       g_list_free (entries);
 
       if (should_skip_menu (new_entry))
-	{ 
+        { 
 	  activate_next_prev_menu (self, object, new_entry, direction);
 	  return;
 	}