GNOME Bugzilla – Bug 655884
[New Call UI] Menus
Last modified: 2011-08-04 14:43:13 UTC
http://cgit.collabora.com/git/user/pochu/empathy.git/log/?h=call-menus
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.