Re: [virt-tools-list] [PATCH virt-viewer v3 00/10] Replace oVirt foreign menu with dedicated dialog

2016-09-08 Thread Christophe Fergeau 
On Mon, Sep 05, 2016 at 02:15:18PM +0200, Christophe Fergeau wrote:
> On Tue, Aug 02, 2016 at 06:17:31PM +0200, Christophe Fergeau wrote:
> > > Eduardo Lima (Etrunko) (10):
> > >   ovirt-foreign-menu: Remove timer used to refresh iso list
> > >   ovirt-foreign-menu: Add accessors for current iso and iso list
> > >   ovirt-foreign-menu: Remove GtkMenu related functions
> > >   ovirt-foreign-menu: Notify of new files even if nothing changed
> > >   UI: Make 'Change CD' menu item a submenu under 'File' toplevel menu
> > >   Introduce ISO List dialog
> > >   Run iso-dialog when 'Change CD' menu is activated
> > >   remote-viewer: Make ovirt-foreign-menu a property
> > >   iso-dialog: Implement functionality provided by oVirt foreign menu
> > >   iso-dialog: Use header bar for buttons
> > 
> > I'm not sure how to approach this series as I don't think
> > 'ovirt-foreign-menu: Add accessors for current iso and iso list'
> 
> See the attached patch for some variation around this which avoids
> relying on notify::file and add an async method to do it.
> 
> Christophe

> From 910272343409332953fe9a91b8e6d8398290c380 Mon Sep 17 00:00:00 2001
> From: Christophe Fergeau 
> Date: Mon, 5 Sep 2016 12:13:42 +0200
> Subject: [virt-viewer] Add ovirt_foreign_menu_set_current_iso_name_async
> 
> ---
>  src/ovirt-foreign-menu.c| 42 
> +
>  src/ovirt-foreign-menu.h|  9 +++-
>  src/remote-viewer-iso-list-dialog.c | 14 +++--
>  3 files changed, 45 insertions(+), 20 deletions(-)
> 
> diff --git a/src/ovirt-foreign-menu.c b/src/ovirt-foreign-menu.c
> index 8320552..2923554 100644
> --- a/src/ovirt-foreign-menu.c
> +++ b/src/ovirt-foreign-menu.c
> @@ -47,7 +47,7 @@ static void 
> ovirt_foreign_menu_fetch_storage_domain_async(OvirtForeignMenu *menu
>  static void ovirt_foreign_menu_fetch_vm_cdrom_async(OvirtForeignMenu *menu);
>  static void ovirt_foreign_menu_refresh_cdrom_file_async(OvirtForeignMenu 
> *menu);
>  static void ovirt_foreign_menu_fetch_iso_list_async(OvirtForeignMenu *menu);
> -static void updated_cdrom_cb(GObject *source_object, GAsyncResult *result, 
> gpointer user_data);
> +static void iso_name_set_cb(GObject *source_object, GAsyncResult *result, 
> gpointer user_data);
>  
>  G_DEFINE_TYPE (OvirtForeignMenu, ovirt_foreign_menu, G_TYPE_OBJECT)
>  
> @@ -102,8 +102,14 @@ ovirt_foreign_menu_get_current_iso_name(OvirtForeignMenu 
> *foreign_menu)
>  
>  
>  void
> -ovirt_foreign_menu_set_current_iso_name(OvirtForeignMenu *foreign_menu, char 
> *name)
> +ovirt_foreign_menu_set_current_iso_name_async(OvirtForeignMenu *foreign_menu,
> +  char *name,
> +  GCancellable *cancellable,
> +  GAsyncReadyCallback callback,
> +  gpointer user_data)
>  {
> +GTask *task = g_task_new(foreign_menu, cancellable, callback, user_data);
> +
>  g_return_if_fail(foreign_menu->priv->cdrom != NULL);
>  g_return_if_fail(foreign_menu->priv->next_iso_name == NULL);
>  
> @@ -120,9 +126,17 @@ ovirt_foreign_menu_set_current_iso_name(OvirtForeignMenu 
> *foreign_menu, char *na
>   NULL);
>  ovirt_cdrom_update_async(foreign_menu->priv->cdrom, TRUE,
>   foreign_menu->priv->proxy, NULL,
> - updated_cdrom_cb, foreign_menu);
> + iso_name_set_cb, task);
>  }
>  
> +gboolean ovirt_foreign_menu_set_current_iso_name_finish(OvirtForeignMenu 
> *foreign_menu,
> +GAsyncResult *result,
> +GError **error)
> +{
> +g_return_val_if_fail(OVIRT_IS_FOREIGN_MENU(foreign_menu), FALSE);
> +
> +return g_task_propagate_boolean(G_TASK(result), error);
> +}
>  
>  GList*
>  ovirt_foreign_menu_get_iso_names(OvirtForeignMenu *foreign_menu)
> @@ -359,15 +373,16 @@ ovirt_foreign_menu_start(OvirtForeignMenu *menu)
>  }
>  
>  
> -static void updated_cdrom_cb(GObject *source_object,
> - GAsyncResult *result,
> - gpointer user_data)
> +static void iso_name_set_cb(GObject *source_object,
> +GAsyncResult *result,
> +gpointer user_data)
>  {
>  GError *error = NULL;
>  OvirtForeignMenu *foreign_menu;
>  gboolean updated;
> +GTask *task = G_TASK(user_data);
>  
> -foreign_menu = OVIRT_FOREIGN_MENU(user_data);
> +foreign_menu = OVIRT_FOREIGN_MENU(g_task_get_source_object(task));
>  updated = ovirt_cdrom_update_finish(OVIRT_CDROM(source_object),
>  result, );
>  g_debug("Finished updating cdrom content");
> @@ -375,14 +390,19 @@ static void updated_cdrom_cb(GObject *source_object,
>

Re: [virt-tools-list] [PATCH virt-viewer v3 00/10] Replace oVirt foreign menu with dedicated dialog

2016-09-05 Thread Christophe Fergeau 
On Tue, Aug 02, 2016 at 06:17:31PM +0200, Christophe Fergeau wrote:
> > Eduardo Lima (Etrunko) (10):
> >   ovirt-foreign-menu: Remove timer used to refresh iso list
> >   ovirt-foreign-menu: Add accessors for current iso and iso list
> >   ovirt-foreign-menu: Remove GtkMenu related functions
> >   ovirt-foreign-menu: Notify of new files even if nothing changed
> >   UI: Make 'Change CD' menu item a submenu under 'File' toplevel menu
> >   Introduce ISO List dialog
> >   Run iso-dialog when 'Change CD' menu is activated
> >   remote-viewer: Make ovirt-foreign-menu a property
> >   iso-dialog: Implement functionality provided by oVirt foreign menu
> >   iso-dialog: Use header bar for buttons
> 
> I'm not sure how to approach this series as I don't think
> 'ovirt-foreign-menu: Add accessors for current iso and iso list'

See the attached patch for some variation around this which avoids
relying on notify::file and add an async method to do it.

Christophe
From 910272343409332953fe9a91b8e6d8398290c380 Mon Sep 17 00:00:00 2001
From: Christophe Fergeau 
Date: Mon, 5 Sep 2016 12:13:42 +0200
Subject: [virt-viewer] Add ovirt_foreign_menu_set_current_iso_name_async

---
 src/ovirt-foreign-menu.c| 42 +
 src/ovirt-foreign-menu.h|  9 +++-
 src/remote-viewer-iso-list-dialog.c | 14 +++--
 3 files changed, 45 insertions(+), 20 deletions(-)

diff --git a/src/ovirt-foreign-menu.c b/src/ovirt-foreign-menu.c
index 8320552..2923554 100644
--- a/src/ovirt-foreign-menu.c
+++ b/src/ovirt-foreign-menu.c
@@ -47,7 +47,7 @@ static void 
ovirt_foreign_menu_fetch_storage_domain_async(OvirtForeignMenu *menu
 static void ovirt_foreign_menu_fetch_vm_cdrom_async(OvirtForeignMenu *menu);
 static void ovirt_foreign_menu_refresh_cdrom_file_async(OvirtForeignMenu 
*menu);
 static void ovirt_foreign_menu_fetch_iso_list_async(OvirtForeignMenu *menu);
-static void updated_cdrom_cb(GObject *source_object, GAsyncResult *result, 
gpointer user_data);
+static void iso_name_set_cb(GObject *source_object, GAsyncResult *result, 
gpointer user_data);
 
 G_DEFINE_TYPE (OvirtForeignMenu, ovirt_foreign_menu, G_TYPE_OBJECT)
 
@@ -102,8 +102,14 @@ ovirt_foreign_menu_get_current_iso_name(OvirtForeignMenu 
*foreign_menu)
 
 
 void
-ovirt_foreign_menu_set_current_iso_name(OvirtForeignMenu *foreign_menu, char 
*name)
+ovirt_foreign_menu_set_current_iso_name_async(OvirtForeignMenu *foreign_menu,
+  char *name,
+  GCancellable *cancellable,
+  GAsyncReadyCallback callback,
+  gpointer user_data)
 {
+GTask *task = g_task_new(foreign_menu, cancellable, callback, user_data);
+
 g_return_if_fail(foreign_menu->priv->cdrom != NULL);
 g_return_if_fail(foreign_menu->priv->next_iso_name == NULL);
 
@@ -120,9 +126,17 @@ ovirt_foreign_menu_set_current_iso_name(OvirtForeignMenu 
*foreign_menu, char *na
  NULL);
 ovirt_cdrom_update_async(foreign_menu->priv->cdrom, TRUE,
  foreign_menu->priv->proxy, NULL,
- updated_cdrom_cb, foreign_menu);
+ iso_name_set_cb, task);
 }
 
+gboolean ovirt_foreign_menu_set_current_iso_name_finish(OvirtForeignMenu 
*foreign_menu,
+GAsyncResult *result,
+GError **error)
+{
+g_return_val_if_fail(OVIRT_IS_FOREIGN_MENU(foreign_menu), FALSE);
+
+return g_task_propagate_boolean(G_TASK(result), error);
+}
 
 GList*
 ovirt_foreign_menu_get_iso_names(OvirtForeignMenu *foreign_menu)
@@ -359,15 +373,16 @@ ovirt_foreign_menu_start(OvirtForeignMenu *menu)
 }
 
 
-static void updated_cdrom_cb(GObject *source_object,
- GAsyncResult *result,
- gpointer user_data)
+static void iso_name_set_cb(GObject *source_object,
+GAsyncResult *result,
+gpointer user_data)
 {
 GError *error = NULL;
 OvirtForeignMenu *foreign_menu;
 gboolean updated;
+GTask *task = G_TASK(user_data);
 
-foreign_menu = OVIRT_FOREIGN_MENU(user_data);
+foreign_menu = OVIRT_FOREIGN_MENU(g_task_get_source_object(task));
 updated = ovirt_cdrom_update_finish(OVIRT_CDROM(source_object),
 result, );
 g_debug("Finished updating cdrom content");
@@ -375,14 +390,19 @@ static void updated_cdrom_cb(GObject *source_object,
 g_free(foreign_menu->priv->current_iso_name);
 foreign_menu->priv->current_iso_name = 
foreign_menu->priv->next_iso_name;
 foreign_menu->priv->next_iso_name = NULL;
-goto end;
+g_task_return_boolean(task, TRUE);
+return;
 }
 
 /* Reset old state back as we were

Re: [virt-tools-list] [PATCH virt-viewer v3 00/10] Replace oVirt foreign menu with dedicated dialog

2016-08-03 Thread Xiaodai Wang 
- Original Message -
> From: "Fabiano Fidêncio" <fiden...@redhat.com>
> To: "Eduardo Lima (Etrunko)" <etru...@redhat.com>
> Cc: "Christophe Fergeau" <cferg...@redhat.com>, "virt" 
> <virt-tools-list@redhat.com>
> Sent: Wednesday, August 3, 2016 10:36:07 PM
> Subject: Re: [virt-tools-list] [PATCH virt-viewer v3 00/10] Replace oVirt 
> foreign menu with dedicated dialog
> 
> On Wed, Aug 3, 2016 at 3:04 PM, Eduardo Lima (Etrunko)
> <etru...@redhat.com> wrote:
> > On 08/02/2016 01:17 PM, Christophe Fergeau wrote:
> >> On Fri, Jul 29, 2016 at 06:40:23PM -0300, Eduardo Lima (Etrunko) wrote:
> >>> I have pushed the first two patches of the series because they were
> >>> already acknowledged and were pretty much self-contained.
> >>>
> >>> I tried to replace GtkTreeView in favor of new GtkListBox, but after some
> >>> painful work, I decided to drop it because it is not possible to
> >>> reproduce the exact same behavior of the former with the latter. For
> >>> instance, once we have a GtkRadioButton activated, it is not possible to
> >>> deactivate it.
> >>
> >> For what it's worth, I'm not sure we should keep GtkRadioButtons at all
> >> cost in the list, imo they look not so nice. Dunno if it would be
> >> possibe to have either empty column or check mark instead of a
> >> GtkRadioButton.
> >>
> >
> > Well, I am no UI expert, but it seems to me that radio buttons are the
> > best choice to express the behavior of the list, exactly 1 or 0 item can
> > be active at any given time. With check buttons, user is given the
> > impression that more than once can be selected, which is not the case.
> 
> I do agree with Eduardo here.
> I have the feeling that radio buttons are the most suitable option here.
> 

How about the UI for "Attach CD" option in ovirt portal.

You can find it in "Edit Virtual Machine" dialog, then select "Boot Options".

Or How about the "Change CD" dialog? 


> [snip]
> 
> Best Regards,
> --
> Fabiano Fidêncio
> 
> ___
> virt-tools-list mailing list
> virt-tools-list@redhat.com
> https://www.redhat.com/mailman/listinfo/virt-tools-list

___
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 v3 00/10] Replace oVirt foreign menu with dedicated dialog

2016-08-03 Thread Fabiano Fidêncio 
On Wed, Aug 3, 2016 at 3:04 PM, Eduardo Lima (Etrunko)
 wrote:
> On 08/02/2016 01:17 PM, Christophe Fergeau wrote:
>> On Fri, Jul 29, 2016 at 06:40:23PM -0300, Eduardo Lima (Etrunko) wrote:
>>> I have pushed the first two patches of the series because they were
>>> already acknowledged and were pretty much self-contained.
>>>
>>> I tried to replace GtkTreeView in favor of new GtkListBox, but after some
>>> painful work, I decided to drop it because it is not possible to
>>> reproduce the exact same behavior of the former with the latter. For
>>> instance, once we have a GtkRadioButton activated, it is not possible to
>>> deactivate it.
>>
>> For what it's worth, I'm not sure we should keep GtkRadioButtons at all
>> cost in the list, imo they look not so nice. Dunno if it would be
>> possibe to have either empty column or check mark instead of a
>> GtkRadioButton.
>>
>
> Well, I am no UI expert, but it seems to me that radio buttons are the
> best choice to express the behavior of the list, exactly 1 or 0 item can
> be active at any given time. With check buttons, user is given the
> impression that more than once can be selected, which is not the case.

I do agree with Eduardo here.
I have the feeling that radio buttons are the most suitable option here.

[snip]

Best Regards,
--
Fabiano Fidêncio

___
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 v3 00/10] Replace oVirt foreign menu with dedicated dialog

2016-08-03 Thread Eduardo Lima (Etrunko) 
On 08/02/2016 01:17 PM, Christophe Fergeau wrote:
> On Fri, Jul 29, 2016 at 06:40:23PM -0300, Eduardo Lima (Etrunko) wrote:
>> I have pushed the first two patches of the series because they were
>> already acknowledged and were pretty much self-contained.
>>
>> I tried to replace GtkTreeView in favor of new GtkListBox, but after some
>> painful work, I decided to drop it because it is not possible to
>> reproduce the exact same behavior of the former with the latter. For
>> instance, once we have a GtkRadioButton activated, it is not possible to
>> deactivate it. 
> 
> For what it's worth, I'm not sure we should keep GtkRadioButtons at all
> cost in the list, imo they look not so nice. Dunno if it would be
> possibe to have either empty column or check mark instead of a
> GtkRadioButton.
> 

Well, I am no UI expert, but it seems to me that radio buttons are the
best choice to express the behavior of the list, exactly 1 or 0 item can
be active at any given time. With check buttons, user is given the
impression that more than once can be selected, which is not the case.

All in all it is a one line change to show check buttons instead of
radio buttons.

>>
>> In this version:
>>
>>  * Removed leftover enum when changed from GtkNotebook to GtkStack
>>  * Some cosmetic fixes (indentation, renaming, etc)
>>  * UI Tweaks:
>>- Added some padding between items of the list.
>>- Set tree view selection to current ISO when the list is first loaded
>>  or refreshed.
>>- Handle tree view "activate" signal the same way as radio button
>>  toggle
>>- Removed "Select ISO" label and alignment in header bar patch.
>>
>> Eduardo Lima (Etrunko) (10):
>>   ovirt-foreign-menu: Remove timer used to refresh iso list
>>   ovirt-foreign-menu: Add accessors for current iso and iso list
>>   ovirt-foreign-menu: Remove GtkMenu related functions
>>   ovirt-foreign-menu: Notify of new files even if nothing changed
>>   UI: Make 'Change CD' menu item a submenu under 'File' toplevel menu
>>   Introduce ISO List dialog
>>   Run iso-dialog when 'Change CD' menu is activated
>>   remote-viewer: Make ovirt-foreign-menu a property
>>   iso-dialog: Implement functionality provided by oVirt foreign menu
>>   iso-dialog: Use header bar for buttons
> 
> I'm not sure how to approach this series as I don't think
> 'ovirt-foreign-menu: Add accessors for current iso and iso list'
> 'ovirt-foreign-menu: Notify of new files even if nothing changed'
> are needed if you want to have proper error handling in the dialog.
> So I don't think I'd have much more to add compared to the first
> iteration of the review. Or are there specific changes you want me to
> look at?

I have been busy with other things at the moment, and I could not finish
the work here. The idea was to gather comments about UI and behavior in
general, as the new changes will not affect the appearance of the
dialog. So it seems it is looking good, apart from the Radio/Check
buttons decision.

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



signature.asc
Description: OpenPGP digital 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 v3 00/10] Replace oVirt foreign menu with dedicated dialog

2016-07-29 Thread Eduardo Lima (Etrunko) 
I have pushed the first two patches of the series because they were
already acknowledged and were pretty much self-contained.

I tried to replace GtkTreeView in favor of new GtkListBox, but after some
painful work, I decided to drop it because it is not possible to
reproduce the exact same behavior of the former with the latter. For
instance, once we have a GtkRadioButton activated, it is not possible to
deactivate it. We also need to create a new GObject that would be used
as the model, while with a tree view, things can be automatically done
with Glade. Finally, it would be necessary to raise the requirements for
both gtk+ (3.16) and glib (2.44). If some one wants to take a look on
the work in progress, check out the 'dialog' branch in my github clone
(http://github.com/etrunko/virt-viewer).

In this version:

 * Removed leftover enum when changed from GtkNotebook to GtkStack
 * Some cosmetic fixes (indentation, renaming, etc)
 * UI Tweaks:
   - Added some padding between items of the list.
   - Set tree view selection to current ISO when the list is first loaded
 or refreshed.
   - Handle tree view "activate" signal the same way as radio button
 toggle
   - Removed "Select ISO" label and alignment in header bar patch.

Eduardo Lima (Etrunko) (10):
  ovirt-foreign-menu: Remove timer used to refresh iso list
  ovirt-foreign-menu: Add accessors for current iso and iso list
  ovirt-foreign-menu: Remove GtkMenu related functions
  ovirt-foreign-menu: Notify of new files even if nothing changed
  UI: Make 'Change CD' menu item a submenu under 'File' toplevel menu
  Introduce ISO List dialog
  Run iso-dialog when 'Change CD' menu is activated
  remote-viewer: Make ovirt-foreign-menu a property
  iso-dialog: Implement functionality provided by oVirt foreign menu
  iso-dialog: Use header bar for buttons

 configure.ac   |   4 +-
 po/POTFILES.in |   2 +
 src/Makefile.am|   3 +
 src/ovirt-foreign-menu.c   | 182 +
 src/ovirt-foreign-menu.h   |   5 +-
 src/remote-viewer-iso-list-dialog.c| 316 +
 src/remote-viewer-iso-list-dialog.h|  58 ++
 src/remote-viewer.c|  89 
 src/resources/ui/remote-viewer-iso-list.ui | 135 
 src/resources/ui/virt-viewer.ui|  19 +-
 src/resources/virt-viewer.gresource.xml|   1 +
 src/virt-viewer-window.c   |  37 
 12 files changed, 660 insertions(+), 191 deletions(-)
 create mode 100644 src/remote-viewer-iso-list-dialog.c
 create mode 100644 src/remote-viewer-iso-list-dialog.h
 create mode 100644 src/resources/ui/remote-viewer-iso-list.ui

-- 
2.7.4

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