Re: [virt-tools-list] [PATCH virt-viewer 05/11] ovirt-foreign-menu: Remove GtkMenu related functions

2016-07-19 Thread Eduardo Lima (Etrunko) 
On 07/19/2016 12:43 PM, Christophe Fergeau wrote:
> On Sun, Jul 17, 2016 at 11:13:05PM -0300, Eduardo Lima (Etrunko) wrote:
>> With this commit, we finish cleaning up ovirt foreign menu code, which
>> only deals with data, leaving the UI bits to be handled properly in the
>> new ISO list dialog.
> 
> I'm not clear exactly what this is doing from the commit log. This
> removes the old toplevel "change cd" menu, replaces it with a
> File/Change CD menu entry, which is going to be not doing anything for
> now?
> 

Yes, that's it. It is strange, but I avoided removing old code and
replacing with lots of new code only to keep the functionality working.
Same happens with the dialog code, which is introduced with no
functionality at all, and only plugged into oVirt foreign menu later on.

>> -ovirt_foreign_menu_start(foreign_menu);
>> -}
>>  
>> +ovirt_foreign_menu_updated(self);
> 
> Why do we use _updated() instead of _start()?
> 

_updated() will only make the menu option visible, while I moved the
call to _start() when dialog is shown. This whole naming seems wrong
btw, because those functions are static to remote-viewer.c and not
present in ovirt-foreign-menu.c.

-- 
Eduardo de Barros Lima (Etrunko)
Software Engineer - RedHat
etru...@redhat.com

___
virt-tools-list mailing list
virt-tools-list@redhat.com
https://www.redhat.com/mailman/listinfo/virt-tools-list

Re: [virt-tools-list] [PATCH virt-viewer 05/11] ovirt-foreign-menu: Remove GtkMenu related functions

2016-07-19 Thread Christophe Fergeau 
On Sun, Jul 17, 2016 at 11:13:05PM -0300, Eduardo Lima (Etrunko) wrote:
> With this commit, we finish cleaning up ovirt foreign menu code, which
> only deals with data, leaving the UI bits to be handled properly in the
> new ISO list dialog.

I'm not clear exactly what this is doing from the commit log. This
removes the old toplevel "change cd" menu, replaces it with a
File/Change CD menu entry, which is going to be not doing anything for
now?

> 
> This patch also updates remote-viewer to reflect the change.
> 

> diff --git a/src/remote-viewer.c b/src/remote-viewer.c
> index 6d29bf2..95130dc 100644
> --- a/src/remote-viewer.c
> +++ b/src/remote-viewer.c
> @@ -734,33 +734,11 @@ authenticate_cb(RestProxy *proxy, G_GNUC_UNUSED 
> RestProxyAuth *auth,
>  static void
>  ovirt_foreign_menu_update(GtkApplication *gtkapp, GtkWindow *gtkwin, 
> G_GNUC_UNUSED gpointer data)
>  {
> -RemoteViewer *app = REMOTE_VIEWER(gtkapp);
> +RemoteViewer *self = REMOTE_VIEWER(gtkapp);
>  VirtViewerWindow *win = g_object_get_data(G_OBJECT(gtkwin), 
> "virt-viewer-window");
> -GtkWidget *menu = g_object_get_data(G_OBJECT(win), "foreign-menu");
> -GtkWidget *submenu;
> -
> -if (app->priv->ovirt_foreign_menu == NULL) {
> -/* nothing to do */
> -return;
> -}
> -
> -submenu = ovirt_foreign_menu_get_gtk_menu(app->priv->ovirt_foreign_menu);
> -if (submenu == NULL) {
> -/* No items to show, no point in showing the menu */
> -if (menu != NULL)
> -   gtk_widget_set_visible(menu, FALSE);
> -g_object_set_data(G_OBJECT(win), "foreign-menu", NULL);
> -return;
> -}
> -
> -if (menu == NULL) {
> -menu = 
> GTK_WIDGET(gtk_builder_get_object(virt_viewer_window_get_builder(win), 
> "menu-change-cd"));
> -g_object_set_data(G_OBJECT(win), "foreign-menu", menu);
> -gtk_widget_set_visible(menu, TRUE);
> -}
> -
> -gtk_menu_item_set_submenu(GTK_MENU_ITEM(menu), submenu);
> -gtk_widget_show_all(menu);
> +GtkBuilder *builder = virt_viewer_window_get_builder(win);
> +GtkWidget *menu = GTK_WIDGET(gtk_builder_get_object(builder, 
> "menu-change-cd"));
> +gtk_widget_set_visible(menu, self->priv->ovirt_foreign_menu != NULL);
>  }
>  
>  static void
> @@ -783,36 +761,22 @@ ovirt_foreign_menu_updated(RemoteViewer *self)
>  }
>  
>  static void
> -ovirt_foreign_menu_changed(OvirtForeignMenu *foreign_menu G_GNUC_UNUSED,
> -   GParamSpec *pspec G_GNUC_UNUSED,
> -   VirtViewerApp *app)
> -{
> -ovirt_foreign_menu_updated(REMOTE_VIEWER(app));
> -}
> -
> -
> -static void
>  virt_viewer_app_set_ovirt_foreign_menu(VirtViewerApp *app,
> OvirtForeignMenu *foreign_menu)
>  {
>  RemoteViewer *self;
> +
>  g_return_if_fail(REMOTE_VIEWER_IS(app));
>  g_return_if_fail(OVIRT_IS_FOREIGN_MENU(foreign_menu));
>  
>  self = REMOTE_VIEWER(app);
> -if (self->priv->ovirt_foreign_menu != NULL) {
> -g_object_unref(G_OBJECT(self->priv->ovirt_foreign_menu));
> -}
> +g_clear_object(>priv->ovirt_foreign_menu);
>  self->priv->ovirt_foreign_menu = foreign_menu;
> -g_signal_connect(G_OBJECT(foreign_menu), "notify::file",
> - (GCallback)ovirt_foreign_menu_changed, app);
> -g_signal_connect(G_OBJECT(foreign_menu), "notify::files",
> - (GCallback)ovirt_foreign_menu_changed, app);
>  g_signal_connect(G_OBJECT(app), "window-added",
>   (GCallback)ovirt_foreign_menu_update, NULL);
> -ovirt_foreign_menu_start(foreign_menu);
> -}
>  
> +ovirt_foreign_menu_updated(self);

Why do we use _updated() instead of _start()?

Christophe


signature.asc
Description: PGP signature
___
virt-tools-list mailing list
virt-tools-list@redhat.com
https://www.redhat.com/mailman/listinfo/virt-tools-list

[virt-tools-list] [PATCH virt-viewer 05/11] ovirt-foreign-menu: Remove GtkMenu related functions

2016-07-17 Thread Eduardo Lima (Etrunko) 
With this commit, we finish cleaning up ovirt foreign menu code, which
only deals with data, leaving the UI bits to be handled properly in the
new ISO list dialog.

This patch also updates remote-viewer to reflect the change.

Signed-off-by: Eduardo Lima (Etrunko) 
---
 src/ovirt-foreign-menu.c | 79 
 src/remote-viewer.c  | 52 +--
 2 files changed, 8 insertions(+), 123 deletions(-)

diff --git a/src/ovirt-foreign-menu.c b/src/ovirt-foreign-menu.c
index 2446239..24b5af2 100644
--- a/src/ovirt-foreign-menu.c
+++ b/src/ovirt-foreign-menu.c
@@ -361,22 +361,6 @@ ovirt_foreign_menu_start(OvirtForeignMenu *menu)
 }
 
 
-static void
-ovirt_foreign_menu_activate_item_cb(GtkMenuItem *menuitem, gpointer user_data);
-
-
-static void
-menu_item_set_active_no_signal(GtkMenuItem *menuitem,
-   gboolean active,
-   GCallback callback,
-   gpointer user_data)
-{
-g_signal_handlers_block_by_func(menuitem, callback, user_data);
-gtk_check_menu_item_set_active(GTK_CHECK_MENU_ITEM(menuitem), active);
-g_signal_handlers_unblock_by_func(menuitem, callback, user_data);
-}
-
-
 static void updated_cdrom_cb(GObject *source_object,
  GAsyncResult *result,
  gpointer user_data)
@@ -412,69 +396,6 @@ static void updated_cdrom_cb(GObject *source_object,
 }
 
 
-static void
-ovirt_foreign_menu_activate_item_cb(GtkMenuItem *menuitem, gpointer user_data)
-{
-OvirtForeignMenu *foreign_menu;
-const char *iso_name = NULL;
-gboolean checked;
-
-checked = gtk_check_menu_item_get_active(GTK_CHECK_MENU_ITEM(menuitem));
-foreign_menu = OVIRT_FOREIGN_MENU(user_data);
-g_return_if_fail(foreign_menu->priv->cdrom != NULL);
-g_return_if_fail(foreign_menu->priv->next_iso_name == NULL);
-
-g_debug("'%s' clicked", gtk_menu_item_get_label(menuitem));
-
-/* We only want to move the check mark for the currently selected ISO
- * when ovirt_cdrom_update_async() is successful, so for now we move
- * the check mark back to where it was before
- */
-menu_item_set_active_no_signal(menuitem, !checked,
-   
(GCallback)ovirt_foreign_menu_activate_item_cb,
-   foreign_menu);
-
-if (checked) {
-iso_name = gtk_menu_item_get_label(menuitem);
-}
-
-ovirt_foreign_menu_set_current_iso_name(foreign_menu, iso_name);
-}
-
-
-GtkWidget *ovirt_foreign_menu_get_gtk_menu(OvirtForeignMenu *foreign_menu)
-{
-GtkWidget *gtk_menu;
-GList *it;
-char *current_iso;
-
-if (foreign_menu->priv->iso_names == NULL) {
-g_debug("ISO list is empty, no menu to show");
-return NULL;
-}
-g_debug("Creating GtkMenu for foreign menu");
-current_iso = ovirt_foreign_menu_get_current_iso_name(foreign_menu);
-gtk_menu = gtk_menu_new();
-for (it = foreign_menu->priv->iso_names; it != NULL; it = it->next) {
-GtkWidget *menuitem;
-
-menuitem = gtk_check_menu_item_new_with_label((char *)it->data);
-if (g_strcmp0((char *)it->data, current_iso) == 0) {
-g_warn_if_fail(g_strcmp0(current_iso, 
foreign_menu->priv->current_iso_name) == 0);
-gtk_check_menu_item_set_active(GTK_CHECK_MENU_ITEM(menuitem),
-   TRUE);
-}
-g_signal_connect(menuitem, "activate",
- G_CALLBACK(ovirt_foreign_menu_activate_item_cb),
- foreign_menu);
-gtk_menu_shell_append(GTK_MENU_SHELL(gtk_menu), menuitem);
-}
-g_free(current_iso);
-
-return gtk_menu;
-}
-
-
 static void ovirt_foreign_menu_set_files(OvirtForeignMenu *menu,
  const GList *files)
 {
diff --git a/src/remote-viewer.c b/src/remote-viewer.c
index 6d29bf2..95130dc 100644
--- a/src/remote-viewer.c
+++ b/src/remote-viewer.c
@@ -734,33 +734,11 @@ authenticate_cb(RestProxy *proxy, G_GNUC_UNUSED 
RestProxyAuth *auth,
 static void
 ovirt_foreign_menu_update(GtkApplication *gtkapp, GtkWindow *gtkwin, 
G_GNUC_UNUSED gpointer data)
 {
-RemoteViewer *app = REMOTE_VIEWER(gtkapp);
+RemoteViewer *self = REMOTE_VIEWER(gtkapp);
 VirtViewerWindow *win = g_object_get_data(G_OBJECT(gtkwin), 
"virt-viewer-window");
-GtkWidget *menu = g_object_get_data(G_OBJECT(win), "foreign-menu");
-GtkWidget *submenu;
-
-if (app->priv->ovirt_foreign_menu == NULL) {
-/* nothing to do */
-return;
-}
-
-submenu = ovirt_foreign_menu_get_gtk_menu(app->priv->ovirt_foreign_menu);
-if (submenu == NULL) {
-/* No items to show, no point in showing the menu */
-if (menu != NULL)
-   gtk_widget_set_visible(menu, FALSE);
-g_object_set_data(G_OBJECT(win), "foreign-menu", NULL);
-return;
-}
-
-