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 655884 - [New Call UI] Menus
[New Call UI] Menus
Status: RESOLVED FIXED
Product: empathy
Classification: Core
Component: VoIP
3.1.x
Other Linux
: Normal normal
: ---
Assigned To: empathy-maint
Depends on:
Blocks: 629902
 
 
Reported: 2011-08-03 08:21 UTC by Emilio Pozuelo Monfort
Modified: 2011-08-04 14:43 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
One Big Diff (12.39 KB, patch)
2011-08-03 08:27 UTC, Emilio Pozuelo Monfort
reviewed Details | Review
CallWindow: move 'Fullscreen' to the Call menu (2.32 KB, patch)
2011-08-03 08:42 UTC, Emilio Pozuelo Monfort
none Details | Review
CallWindow: remove the hangup menu item (1.88 KB, patch)
2011-08-03 08:42 UTC, Emilio Pozuelo Monfort
none Details | Review
CallWindow: add a Help->Contents menu (2.73 KB, patch)
2011-08-03 08:42 UTC, Emilio Pozuelo Monfort
none Details | Review
CallWindow: add a Help->About menu (3.16 KB, patch)
2011-08-03 08:42 UTC, Emilio Pozuelo Monfort
none Details | Review
CallWindow: add a 'Call log' menu (3.38 KB, patch)
2011-08-03 08:42 UTC, Emilio Pozuelo Monfort
none Details | Review
MainWindow: add API to show the preferences window (1.69 KB, patch)
2011-08-03 08:42 UTC, Emilio Pozuelo Monfort
none Details | Review
Add a command line option to Empathy to show the preferences (3.57 KB, patch)
2011-08-03 08:42 UTC, Emilio Pozuelo Monfort
none Details | Review
CallWindow: add a Settings menu item (3.40 KB, patch)
2011-08-03 08:42 UTC, Emilio Pozuelo Monfort
none Details | Review
CallWindow: move 'Fullscreen' to the Call menu (2.32 KB, patch)
2011-08-03 14:55 UTC, Emilio Pozuelo Monfort
accepted-commit_now Details | Review
CallWindow: remove the hangup menu item (1.88 KB, patch)
2011-08-03 14:55 UTC, Emilio Pozuelo Monfort
accepted-commit_now Details | Review
CallWindow: add a Help->Contents menu (2.84 KB, patch)
2011-08-03 14:55 UTC, Emilio Pozuelo Monfort
none Details | Review
CallWindow: add a Help->About menu (3.18 KB, patch)
2011-08-03 14:55 UTC, Emilio Pozuelo Monfort
accepted-commit_now Details | Review
CallWindow: add a 'Call log' menu (3.39 KB, patch)
2011-08-03 14:55 UTC, Emilio Pozuelo Monfort
accepted-commit_now Details | Review
MainWindow: add API to show the preferences window (1.69 KB, patch)
2011-08-03 14:55 UTC, Emilio Pozuelo Monfort
accepted-commit_now Details | Review
Add a command line option to Empathy to show the preferences (3.57 KB, patch)
2011-08-03 14:56 UTC, Emilio Pozuelo Monfort
accepted-commit_now Details | Review
CallWindow: add a Settings menu item (3.46 KB, patch)
2011-08-03 14:56 UTC, Emilio Pozuelo Monfort
accepted-commit_now Details | Review
Factor out empathy_launch_program() (9.06 KB, patch)
2011-08-03 14:56 UTC, Emilio Pozuelo Monfort
accepted-commit_now Details | Review
DebugWindow: add function to change the service to show (3.21 KB, patch)
2011-08-03 14:56 UTC, Emilio Pozuelo Monfort
reviewed Details | Review
empathy-debugger: add a command line option to show a service (2.47 KB, patch)
2011-08-03 14:56 UTC, Emilio Pozuelo Monfort
none Details | Review
CallWindow: show Empathy.Call when opening the debug window (834 bytes, patch)
2011-08-03 14:56 UTC, Emilio Pozuelo Monfort
none Details | Review
Preferences: add empathy_preferences_show_tab() (1.28 KB, patch)
2011-08-03 14:56 UTC, Emilio Pozuelo Monfort
reviewed Details | Review
Allow to specify the tab to open in the preferences (4.67 KB, patch)
2011-08-03 14:56 UTC, Emilio Pozuelo Monfort
none Details | Review
CallWindow: go to the Calls tab when opening the preferences (809 bytes, patch)
2011-08-03 14:56 UTC, Emilio Pozuelo Monfort
none Details | Review
Make the #defines an static array (4.64 KB, patch)
2011-08-04 09:00 UTC, Emilio Pozuelo Monfort
none Details | Review
Clear select_name after we select it (934 bytes, patch)
2011-08-04 09:00 UTC, Emilio Pozuelo Monfort
none Details | Review

Comment 1 Emilio Pozuelo Monfort 2011-08-03 08:27:21 UTC
Created attachment 193135 [details] [review]
One Big Diff
Comment 2 Emilio Pozuelo Monfort 2011-08-03 08:42:03 UTC
Created attachment 193136 [details] [review]
CallWindow: move 'Fullscreen' to the Call menu
Comment 3 Emilio Pozuelo Monfort 2011-08-03 08:42:07 UTC
Created attachment 193137 [details] [review]
CallWindow: remove the hangup menu item
Comment 4 Emilio Pozuelo Monfort 2011-08-03 08:42:11 UTC
Created attachment 193138 [details] [review]
CallWindow: add a Help->Contents menu
Comment 5 Emilio Pozuelo Monfort 2011-08-03 08:42:16 UTC
Created attachment 193139 [details] [review]
CallWindow: add a Help->About menu
Comment 6 Emilio Pozuelo Monfort 2011-08-03 08:42:20 UTC
Created attachment 193140 [details] [review]
CallWindow: add a 'Call log' menu
Comment 7 Emilio Pozuelo Monfort 2011-08-03 08:42:24 UTC
Created attachment 193141 [details] [review]
MainWindow: add API to show the preferences window
Comment 8 Emilio Pozuelo Monfort 2011-08-03 08:42:28 UTC
Created attachment 193142 [details] [review]
Add a command line option to Empathy to show the preferences
Comment 9 Emilio Pozuelo Monfort 2011-08-03 08:42:33 UTC
Created attachment 193143 [details] [review]
CallWindow: add a Settings menu item
Comment 10 Guillaume Desmottes 2011-08-03 08:43:14 UTC
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.
Comment 11 Emilio Pozuelo Monfort 2011-08-03 09:13:56 UTC
(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.
Comment 12 Emilio Pozuelo Monfort 2011-08-03 14:55:32 UTC
Created attachment 193171 [details] [review]
CallWindow: move 'Fullscreen' to the Call menu
Comment 13 Emilio Pozuelo Monfort 2011-08-03 14:55:37 UTC
Created attachment 193172 [details] [review]
CallWindow: remove the hangup menu item
Comment 14 Emilio Pozuelo Monfort 2011-08-03 14:55:42 UTC
Created attachment 193173 [details] [review]
CallWindow: add a Help->Contents menu
Comment 15 Emilio Pozuelo Monfort 2011-08-03 14:55:47 UTC
Created attachment 193174 [details] [review]
CallWindow: add a Help->About menu
Comment 16 Emilio Pozuelo Monfort 2011-08-03 14:55:52 UTC
Created attachment 193175 [details] [review]
CallWindow: add a 'Call log' menu
Comment 17 Emilio Pozuelo Monfort 2011-08-03 14:55:57 UTC
Created attachment 193176 [details] [review]
MainWindow: add API to show the preferences window
Comment 18 Emilio Pozuelo Monfort 2011-08-03 14:56:02 UTC
Created attachment 193177 [details] [review]
Add a command line option to Empathy to show the preferences
Comment 19 Emilio Pozuelo Monfort 2011-08-03 14:56:08 UTC
Created attachment 193178 [details] [review]
CallWindow: add a Settings menu item
Comment 20 Emilio Pozuelo Monfort 2011-08-03 14:56:12 UTC
Created attachment 193179 [details] [review]
Factor out empathy_launch_program()
Comment 21 Emilio Pozuelo Monfort 2011-08-03 14:56:17 UTC
Created attachment 193180 [details] [review]
DebugWindow: add function to change the service to show
Comment 22 Emilio Pozuelo Monfort 2011-08-03 14:56:23 UTC
Created attachment 193181 [details] [review]
empathy-debugger: add a command line option to show a service
Comment 23 Emilio Pozuelo Monfort 2011-08-03 14:56:28 UTC
Created attachment 193182 [details] [review]
CallWindow: show Empathy.Call when opening the debug window
Comment 24 Emilio Pozuelo Monfort 2011-08-03 14:56:33 UTC
Created attachment 193183 [details] [review]
Preferences: add empathy_preferences_show_tab()
Comment 25 Emilio Pozuelo Monfort 2011-08-03 14:56:37 UTC
Created attachment 193184 [details] [review]
Allow to specify the tab to open in the preferences
Comment 26 Emilio Pozuelo Monfort 2011-08-03 14:56:43 UTC
Created attachment 193185 [details] [review]
CallWindow: go to the Calls tab when opening the preferences
Comment 27 Emilio Pozuelo Monfort 2011-08-03 15:01:44 UTC
(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.
Comment 28 Guillaume Desmottes 2011-08-03 15:09:55 UTC
Review of attachment 193171 [details] [review]:

I'm still unconvinced but ok for now.
Comment 29 Guillaume Desmottes 2011-08-03 15:10:32 UTC
Review of attachment 193172 [details] [review]:

++
Comment 30 Guillaume Desmottes 2011-08-03 15:11:06 UTC
Review of attachment 193173 [details] [review]:

++
Comment 31 Guillaume Desmottes 2011-08-03 15:11:25 UTC
Review of attachment 193174 [details] [review]:

++
Comment 32 Guillaume Desmottes 2011-08-03 15:11:51 UTC
Review of attachment 193176 [details] [review]:

++
Comment 33 Guillaume Desmottes 2011-08-03 15:12:16 UTC
Review of attachment 193178 [details] [review]:

++
Comment 34 Guillaume Desmottes 2011-08-03 15:13:38 UTC
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.
Comment 35 Guillaume Desmottes 2011-08-03 15:16:00 UTC
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.
Comment 36 Guillaume Desmottes 2011-08-03 15:16:22 UTC
Review of attachment 193175 [details] [review]:

++
Comment 37 Guillaume Desmottes 2011-08-03 15:16:33 UTC
Review of attachment 193177 [details] [review]:

++
Comment 38 Guillaume Desmottes 2011-08-03 15:16:49 UTC
Review of attachment 193179 [details] [review]:

++
Comment 39 Emilio Pozuelo Monfort 2011-08-03 15:57:48 UTC
(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.
Comment 40 Emilio Pozuelo Monfort 2011-08-04 08:03:40 UTC
(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.
Comment 41 Guillaume Desmottes 2011-08-04 08:09:48 UTC
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.
Comment 42 Guillaume Desmottes 2011-08-04 08:14:16 UTC
+#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.
Comment 43 Emilio Pozuelo Monfort 2011-08-04 09:00:27 UTC
Created attachment 193230 [details] [review]
Make the #defines an static array
Comment 44 Emilio Pozuelo Monfort 2011-08-04 09:00:32 UTC
Created attachment 193231 [details] [review]
Clear select_name after we select it
Comment 45 Emilio Pozuelo Monfort 2011-08-04 09:04:02 UTC
(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).
Comment 46 Emilio Pozuelo Monfort 2011-08-04 09:13:01 UTC
Pushed!
Comment 47 André Klapper 2011-08-04 11:40:55 UTC
Emilio: Did I miss the UI and string change announcement, or did you forget to send one?
Comment 48 Emilio Pozuelo Monfort 2011-08-04 14:43:13 UTC
I forgot about it. I've sent it now.