[virt-tools-list] [PATCH virt-viewer] Avoid warning when loading initial monitor mapping

[Pavel Grunt]

On Tue, 2017-04-04 at 10:40 -0500, Jonathon Jongsma wrote:
> On Mon, 2017-04-03 at 08:58 +0200, Pavel Grunt wrote:
> > Hi Jonathon,
> > 
> > On Wed, 2017-03-29 at 15:24 -0500, Jonathon Jongsma wrote:
> > > When started in fullscreen mode with a monitor-mapping
> > > configuration
> > > option, we are getting the following warnings on the terminal:
> > 
> > I haven't seen it. wayland ?
> 
> Yes. Wayland on Fedora 25.
> 
> > 
> > > 
> > >     (process:27428): Gdk-CRITICAL **: gdk_screen_get_n_monitors:
> > > assertion 'GDK_IS_SCREEN (screen)' failed
> > > 
> > >     ** (process:27428): WARNING **: Invalid monitor-mapping
> > > configuration: monitor #1 for display #1 does not exist
> > > 
> > > This is apparently because we were processing the fallback
> > > monitor
> > > mapping before the graphic server display was opened, so
> > > gdk_screen_get_default() returned NULL. This resulted in
> > > gdk_screen_get_n_monitors() reporting that we had 0 monitors.
> > > 
> > > This patch moves the fallback monitor mapping parsing from
> > > virt_viewer_app_init() to
> > > virt_viewer_app_on_application_startup(),
> > > after chaining up to the base class startup() vfunc. The graphic
> > > server
> > > display is opened in the base class vfunc, so by the time that
> > > returns,
> > > we should be able to query the number of monitors.
> > 
> > sure, it makes a sense. Ack
> > 
> > > 
> > > The patch also adds a check in
> > > virt_viewer_parse_monitor_mappings()
> > > to
> > > ensure that we pass a sane value for nmonitors.
> > 
> > Sounds like testable
> 
> Can you clarify what you mean here?

we have a test-monitor-mapping.c, we can trigger/check for that the
GCritical there.


> 
> 
> > 
> > Thanks,
> > Pavel
> > 
> > 
> > > ---
> > >  src/virt-viewer-app.c  | 2 +-
> > >  src/virt-viewer-util.c | 7 +++++--
> > >  2 files changed, 6 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/src/virt-viewer-app.c b/src/virt-viewer-app.c
> > > index 2e7e193..8c5c3f5 100644
> > > --- a/src/virt-viewer-app.c
> > > +++ b/src/virt-viewer-app.c
> > > @@ -1730,7 +1730,6 @@ virt_viewer_app_init(VirtViewerApp *self)
> > >  
> > >      g_clear_error(&error);
> > >  
> > > -    self->priv->initial_display_map =
> > > virt_viewer_app_get_monitor_mapping_for_section(self,
> > > "fallback");
> > >      g_signal_connect(self, "notify::guest-name",
> > > G_CALLBACK(title_maybe_changed), NULL);
> > >      g_signal_connect(self, "notify::title",
> > > G_CALLBACK(title_maybe_changed), NULL);
> > >      g_signal_connect(self, "notify::guri",
> > > G_CALLBACK(title_maybe_changed), NULL);
> > > @@ -1807,6 +1806,7 @@
> > > virt_viewer_app_on_application_startup(GApplication *app)
> > >      self->priv->main_window = virt_viewer_app_window_new(self,
> > >                                                           virt_v
> > > iew
> > > e
> > > r_app_get_first_monitor(self));
> > >      self->priv->main_notebook =
> > > GTK_WIDGET(virt_viewer_window_get_notebook(self->priv-
> > > > main_window));
> > > 
> > > +    self->priv->initial_display_map =
> > > virt_viewer_app_get_monitor_mapping_for_section(self,
> > > "fallback");
> > >  
> > >      virt_viewer_app_set_kiosk(self, opt_kiosk);
> > >      virt_viewer_app_set_hotkeys(self, opt_hotkeys);
> > > diff --git a/src/virt-viewer-util.c b/src/virt-viewer-util.c
> > > index 0491f73..6b63133 100644
> > > --- a/src/virt-viewer-util.c
> > > +++ b/src/virt-viewer-util.c
> > > @@ -641,11 +641,14 @@
> > > virt_viewer_shift_monitors_to_origin(GHashTable *displays)
> > >  GHashTable*
> > >  virt_viewer_parse_monitor_mappings(gchar **mappings, const
> > > gsize
> > > nmappings, const gint nmonitors)
> > >  {
> > > -    GHashTable *displaymap = g_hash_table_new(g_direct_hash,
> > > g_direct_equal);
> > > -    GHashTable *monitormap = g_hash_table_new(g_direct_hash,
> > > g_direct_equal);
> > > +    GHashTable *displaymap;
> > > +    GHashTable *monitormap;
> > >      gint i, max_display_id = 0;
> > >      gchar **tokens = NULL;
> > >  
> > > +    g_return_val_if_fail(nmonitors != 0, NULL);
> > > +    displaymap = g_hash_table_new(g_direct_hash,
> > > g_direct_equal);
> > > +    monitormap = g_hash_table_new(g_direct_hash,
> > > g_direct_equal);
> > >      if (nmappings == 0) {
> > >          g_warning("Empty monitor-mapping configuration");
> > >          goto configerror;