Created attachment 193135 [details] [review]
One Big Diff

Created attachment 193136 [details] [review]
CallWindow: move 'Fullscreen' to the Call menu

Created attachment 193137 [details] [review]
CallWindow: remove the hangup menu item

Created attachment 193138 [details] [review]
CallWindow: add a Help->Contents menu

Created attachment 193139 [details] [review]
CallWindow: add a Help->About menu

Created attachment 193140 [details] [review]
CallWindow: add a 'Call log' menu

Created attachment 193141 [details] [review]
MainWindow: add API to show the preferences window

Created attachment 193142 [details] [review]
Add a command line option to Empathy to show the preferences

Created attachment 193143 [details] [review]
CallWindow: add a Settings menu item

Review of attachment 193135 [details] [review]:

CallWindow: move 'Fullscreen' to the Call menu
Really. Shouldn't that be in a "View" menu like in any GNOME app?

::: src/empathy-call-window.c
@@ +787,3 @@
+
+static void
+      goto out;

We should select Empathy.Call by default.

@@ +797,3 @@
+
+  /* Try to run from source directory if possible */
+

We should factor out this code and share it with empathy.

::: src/empathy-call-window.ui
@@ +34,3 @@
+          <object class="GtkAction" id="menusettings">
+            <property name="name">menusettings</property>
+            <property name="label" translatable="yes">Settings</property>

Should use a stock id.

@@ +46,3 @@
+          <object class="GtkAction" id="menucontents">
+            <property name="name">menucontents</property>
+          </object>

It should be "_Content" have the F1 accelerator and the help icon, as in the contact list menu. We should use the stock id, see src/empathy-main-window.ui

@@ +52,3 @@
+          <object class="GtkAction" id="menudebug">
+            <property name="name">menudebug</property>
+          <object class="GtkAction" id="menucontents">

Why not "_Debug" as in the contact list?

@@ +58,3 @@
+          <object class="GtkAction" id="menuabout">
+            <property name="name">menuabout</property>
+          </object>

Should be a stock id as well.

::: src/empathy-main-window.h
@@ +54,3 @@
 GtkWidget *empathy_main_window_dup (void);
 
+void empathy_main_window_show_preferences (EmpathyMainWindow *window);

We should be able to select the page displayed by default so empathy-call will select the "Calls" page.

(In reply to comment #10)
> Review of attachment 193135 [details] [review]:
> 
> CallWindow: move 'Fullscreen' to the Call menu
> Really. Shouldn't that be in a "View" menu like in any GNOME app?

That's where the new Call UI puts it. I've commented on #629902

> 
> ::: src/empathy-call-window.c
> @@ +787,3 @@
> +
> +static void
> +      goto out;
> 
> We should select Empathy.Call by default.

Indeed. I'll do that in a subsequent commit.

> @@ +797,3 @@
> +
> +  /* Try to run from source directory if possible */
> +
> 
> We should factor out this code and share it with empathy.
> 
> ::: src/empathy-call-window.ui
> @@ +34,3 @@
> +          <object class="GtkAction" id="menusettings">
> +            <property name="name">menusettings</property>
> +            <property name="label" translatable="yes">Settings</property>
> 
> Should use a stock id.

Done.

> @@ +46,3 @@
> +          <object class="GtkAction" id="menucontents">
> +            <property name="name">menucontents</property>
> +          </object>
> 
> It should be "_Content" have the F1 accelerator and the help icon, as in the
> contact list menu. We should use the stock id, see src/empathy-main-window.ui

Done.

> @@ +52,3 @@
> +          <object class="GtkAction" id="menudebug">
> +            <property name="name">menudebug</property>
> +          <object class="GtkAction" id="menucontents">
> 
> Why not "_Debug" as in the contact list?

The mockup has 'Call log', though Debug sounds better, otherwise it may be confused with History. I have commented on the new call ui bug. I've commented on #629902.

> @@ +58,3 @@
> +          <object class="GtkAction" id="menuabout">
> +            <property name="name">menuabout</property>
> +          </object>
> 
> Should be a stock id as well.

Done.

> ::: src/empathy-main-window.h
> @@ +54,3 @@
>  GtkWidget *empathy_main_window_dup (void);
> 
> +void empathy_main_window_show_preferences (EmpathyMainWindow *window);
> 
> We should be able to select the page displayed by default so empathy-call will
> select the "Calls" page.

Yeah, I'll do that in another commit.

Created attachment 193171 [details] [review]
CallWindow: move 'Fullscreen' to the Call menu

Created attachment 193172 [details] [review]
CallWindow: remove the hangup menu item

Created attachment 193173 [details] [review]
CallWindow: add a Help->Contents menu

Created attachment 193174 [details] [review]
CallWindow: add a Help->About menu

Created attachment 193175 [details] [review]
CallWindow: add a 'Call log' menu

Created attachment 193176 [details] [review]
MainWindow: add API to show the preferences window

Created attachment 193177 [details] [review]
Add a command line option to Empathy to show the preferences

Created attachment 193178 [details] [review]
CallWindow: add a Settings menu item

Created attachment 193179 [details] [review]
Factor out empathy_launch_program()

Created attachment 193180 [details] [review]
DebugWindow: add function to change the service to show

Created attachment 193181 [details] [review]
empathy-debugger: add a command line option to show a service

Created attachment 193182 [details] [review]
CallWindow: show Empathy.Call when opening the debug window

Created attachment 193183 [details] [review]
Preferences: add empathy_preferences_show_tab()

Created attachment 193184 [details] [review]
Allow to specify the tab to open in the preferences

Created attachment 193185 [details] [review]
CallWindow: go to the Calls tab when opening the preferences

(In reply to comment #11)
> > ::: src/empathy-call-window.c
> > @@ +787,3 @@
> > +
> > +static void
> > +      goto out;
> > 
> > We should select Empathy.Call by default.
> 
> Indeed. I'll do that in a subsequent commit.

Done

> > @@ +797,3 @@
> > +
> > +  /* Try to run from source directory if possible */
> > +
> > 
> > We should factor out this code and share it with empathy.

Done

> > ::: src/empathy-main-window.h
> > @@ +54,3 @@
> >  GtkWidget *empathy_main_window_dup (void);
> > 
> > +void empathy_main_window_show_preferences (EmpathyMainWindow *window);
> > 
> > We should be able to select the page displayed by default so empathy-call will
> > select the "Calls" page.
> 
> Yeah, I'll do that in another commit.

Done

The other two things (Call logs vs Debug, and Call->Fullscreen vs View->Fullscreen) can be changed later when we discuss them in the other bug.

Review of attachment 193171 [details] [review]:

I'm still unconvinced but ok for now.

Review of attachment 193172 [details] [review]:

++

Review of attachment 193173 [details] [review]:

++

Review of attachment 193174 [details] [review]:

++

Review of attachment 193176 [details] [review]:

++

Review of attachment 193178 [details] [review]:

++

Review of attachment 193180 [details] [review]:

::: src/empathy-debug-window.c
@@ +1798,3 @@
+    empathy_debug_window_select_name (self, name);
+  else
+{

leak if already set.

Review of attachment 193183 [details] [review]:

::: src/empathy-preferences.h
@@ +56,3 @@
 
+void empathy_preferences_show_tab (EmpathyPreferences *self,
+    gint tab);

I think we should have an enum for tabs instead of relying on magic number. Also, it would be more robust to pass "calls" rather than 3. SO we'll have a table mapping string -> enum.

Review of attachment 193175 [details] [review]:

++

Review of attachment 193177 [details] [review]:

++

Review of attachment 193179 [details] [review]:

++

(In reply to comment #34)
> Review of attachment 193180 [details] [review]:
> 
> ::: src/empathy-debug-window.c
> @@ +1798,3 @@
> +    empathy_debug_window_select_name (self, name);
> +  else
> +{
> 
> leak if already set.

Oops. Fixed (amended).(In reply to comment #35)
> Review of attachment 193183 [details] [review]:
> 
> ::: src/empathy-preferences.h
> @@ +56,3 @@
> 
> +void empathy_preferences_show_tab (EmpathyPreferences *self,
> +    gint tab);
> 
> I think we should have an enum for tabs instead of relying on magic number.
> Also, it would be more robust to pass "calls" rather than 3. SO we'll have a
> table mapping string -> enum.

Right. Jonny fixed a bug caused by this the other day (see f28483d938a0291f167b2e8f53e11c4d2ae3a6ec). Done in the last two commits.

(In reply to comment #27)
> The other two things (Call logs vs Debug, and Call->Fullscreen vs
> View->Fullscreen) can be changed later when we discuss them in the other bug.

And now it says Help->Debug too.

Review of attachment 193180 [details] [review]:

::: src/empathy-debug-window.c
@@ +703,3 @@
+      if (priv->select_name != NULL &&
+          !tp_strdiff (name, priv->select_name))
+        gtk_combo_box_set_active_iter (GTK_COMBO_BOX (priv->chooser), &iter);

Shouldn't we set select_name to NULL here? If not we'll reselect it if a new client is started later.

+#define EMPATHY_PREFERENCES_STR_TAB_GENERAL "general"

You could use a static const gchar * [] containing the different strings in the order of the enum. Just put a comment saying that the enum and this array have to stay sync.

Created attachment 193230 [details] [review]
Make the #defines an static array

Created attachment 193231 [details] [review]
Clear select_name after we select it

(In reply to comment #41)
> Review of attachment 193180 [details] [review]:
> 
> ::: src/empathy-debug-window.c
> @@ +703,3 @@
> +      if (priv->select_name != NULL &&
> +          !tp_strdiff (name, priv->select_name))
> +        gtk_combo_box_set_active_iter (GTK_COMBO_BOX (priv->chooser), &iter);
> 
> Shouldn't we set select_name to NULL here? If not we'll reselect it if a new
> client is started later.

You're right, fixed.

(In reply to comment #42)
> +#define EMPATHY_PREFERENCES_STR_TAB_GENERAL "general"
> 
> You could use a static const gchar * [] containing the different strings in the
> order of the enum. Just put a comment saying that the enum and this array have
> to stay sync.

I've done it that way, though now empathy-call needs to link to empathy-preferences (not a big deal anyway).

Pushed!

Emilio: Did I miss the UI and string change announcement, or did you forget to send one?

I forgot about it. I've sent it now.