| Message ID | 20220428231304.19472-4-dongwon.kim@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ui/gtk: new options, monitor and detach-all | expand |
On Thu, Apr 28, 2022 at 04:13:04PM -0700, Dongwon Kim wrote: > Specify location and size of detached window based on top level > window's location and size info when detachment happens. Can you explain what problem is being solved by this change ? What's wrong with default size/placement logic ? In terms of size at least, I would hope we are resizing windows any time the guest changes the resolution of the virtual video adapter. If there are 2 outputs, they can be at different resolution, so copying the size of the existing window feels wrong - we need to copy the guest resolution currently set. Why do we need to mess around with position at all ? > Cc: Philippe Mathieu-Daudé <philmd@redhat.com> > Cc: Paolo Bonzini <pbonzini@redhat.com> > Cc: Gerd Hoffmann <kraxel@redhat.com> > Cc: Vivek Kasireddy <vivek.kasireddy@intel.com> > Signed-off-by: Dongwon Kim <dongwon.kim@intel.com> > --- > ui/gtk.c | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/ui/gtk.c b/ui/gtk.c > index f1ca6a7275..7dadf3b588 100644 > --- a/ui/gtk.c > +++ b/ui/gtk.c > @@ -1338,6 +1338,8 @@ static void gd_menu_untabify(GtkMenuItem *item, void *opaque) > FALSE); > } > if (!vc->window) { > + gint x, y, w, h; > + int i; > gtk_widget_set_sensitive(vc->menu_item, false); > vc->window = gtk_window_new(GTK_WINDOW_TOPLEVEL); > #if defined(CONFIG_OPENGL) > @@ -1351,7 +1353,18 @@ static void gd_menu_untabify(GtkMenuItem *item, void *opaque) > } > #endif > gd_widget_reparent(s->notebook, vc->window, vc->tab_item); > + gtk_window_get_position(GTK_WINDOW(s->window), &x, &y); > + gtk_window_get_size(GTK_WINDOW(s->window), &w, &h); > + > + for (i = 0; i < s->nb_vcs; i++) { > + if (vc == &s->vc[i]) { > + break; > + } > + } > > + gtk_window_move(GTK_WINDOW(vc->window), > + x + w * (i % (s->nb_vcs/2) + 1), y + h * (i / (s->nb_vcs/2))); > + gtk_window_resize(GTK_WINDOW(vc->window), w, h); > g_signal_connect(vc->window, "delete-event", > G_CALLBACK(gd_tab_window_close), vc); > gtk_widget_show_all(vc->window); > -- > 2.30.2 > > With regards, Daniel
I saw windows, especially, third and fourth ones are 1/4 size of the first when detached regardless of resolutions. And the position is also pretty random and detached windows are usually placed somewhere on the previous window. This patch is to make the sizes same as the original window's and make sure all detached windows are not overlapped each other. On Tue, May 03, 2022 at 10:17:46AM +0100, Daniel P. Berrangé wrote: > On Thu, Apr 28, 2022 at 04:13:04PM -0700, Dongwon Kim wrote: > > Specify location and size of detached window based on top level > > window's location and size info when detachment happens. > > Can you explain what problem is being solved by this change ? > What's wrong with default size/placement logic ? > > In terms of size at least, I would hope we are resizing > windows any time the guest changes the resolution of the > virtual video adapter. If there are 2 outputs, they can > be at different resolution, so copying the size of the > existing window feels wrong - we need to copy the guest > resolution currently set. > > Why do we need to mess around with position at all ? > > > Cc: Philippe Mathieu-Daudé <philmd@redhat.com> > > Cc: Paolo Bonzini <pbonzini@redhat.com> > > Cc: Gerd Hoffmann <kraxel@redhat.com> > > Cc: Vivek Kasireddy <vivek.kasireddy@intel.com> > > Signed-off-by: Dongwon Kim <dongwon.kim@intel.com> > > --- > > ui/gtk.c | 13 +++++++++++++ > > 1 file changed, 13 insertions(+) > > > > diff --git a/ui/gtk.c b/ui/gtk.c > > index f1ca6a7275..7dadf3b588 100644 > > --- a/ui/gtk.c > > +++ b/ui/gtk.c > > @@ -1338,6 +1338,8 @@ static void gd_menu_untabify(GtkMenuItem *item, void *opaque) > > FALSE); > > } > > if (!vc->window) { > > + gint x, y, w, h; > > + int i; > > gtk_widget_set_sensitive(vc->menu_item, false); > > vc->window = gtk_window_new(GTK_WINDOW_TOPLEVEL); > > #if defined(CONFIG_OPENGL) > > @@ -1351,7 +1353,18 @@ static void gd_menu_untabify(GtkMenuItem *item, void *opaque) > > } > > #endif > > gd_widget_reparent(s->notebook, vc->window, vc->tab_item); > > + gtk_window_get_position(GTK_WINDOW(s->window), &x, &y); > > + gtk_window_get_size(GTK_WINDOW(s->window), &w, &h); > > + > > + for (i = 0; i < s->nb_vcs; i++) { > > + if (vc == &s->vc[i]) { > > + break; > > + } > > + } > > > > + gtk_window_move(GTK_WINDOW(vc->window), > > + x + w * (i % (s->nb_vcs/2) + 1), y + h * (i / (s->nb_vcs/2))); > > + gtk_window_resize(GTK_WINDOW(vc->window), w, h); > > g_signal_connect(vc->window, "delete-event", > > G_CALLBACK(gd_tab_window_close), vc); > > gtk_widget_show_all(vc->window); > > -- > > 2.30.2 > > > > > > With regards, > Daniel > -- > |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| > |: https://libvirt.org -o- https://fstop138.berrange.com :| > |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| >
On Tue, May 03, 2022 at 04:33:48PM -0700, Dongwon Kim wrote: > I saw windows, especially, third and fourth ones are 1/4 size of > the first when detached regardless of resolutions. > > And the position is also pretty random and detached windows are usually > placed somewhere on the previous window. > > This patch is to make the sizes same as the original window's and make > sure all detached windows are not overlapped each other. In terms of size, I think you need to just honour the surface size like this: @@ -1354,6 +1354,9 @@ static void gd_menu_untabify(GtkMenuItem *item, void *opaque) g_signal_connect(vc->window, "delete-event", G_CALLBACK(gd_tab_window_close), vc); + gtk_window_set_default_size(GTK_WINDOW(vc->window), + surface_width(vc->gfx.ds), + surface_height(vc->gfx.ds)); gtk_widget_show_all(vc->window); if (qemu_console_is_graphic(vc->gfx.dcl.con)) { for position, I don't think we should be overriding the window manager placement, as the logic applied could result in us placing windows off screen. > > On Tue, May 03, 2022 at 10:17:46AM +0100, Daniel P. Berrangé wrote: > > On Thu, Apr 28, 2022 at 04:13:04PM -0700, Dongwon Kim wrote: > > > Specify location and size of detached window based on top level > > > window's location and size info when detachment happens. > > > > Can you explain what problem is being solved by this change ? > > What's wrong with default size/placement logic ? > > > > In terms of size at least, I would hope we are resizing > > windows any time the guest changes the resolution of the > > virtual video adapter. If there are 2 outputs, they can > > be at different resolution, so copying the size of the > > existing window feels wrong - we need to copy the guest > > resolution currently set. > > > > Why do we need to mess around with position at all ? > > > > > Cc: Philippe Mathieu-Daudé <philmd@redhat.com> > > > Cc: Paolo Bonzini <pbonzini@redhat.com> > > > Cc: Gerd Hoffmann <kraxel@redhat.com> > > > Cc: Vivek Kasireddy <vivek.kasireddy@intel.com> > > > Signed-off-by: Dongwon Kim <dongwon.kim@intel.com> > > > --- > > > ui/gtk.c | 13 +++++++++++++ > > > 1 file changed, 13 insertions(+) > > > > > > diff --git a/ui/gtk.c b/ui/gtk.c > > > index f1ca6a7275..7dadf3b588 100644 > > > --- a/ui/gtk.c > > > +++ b/ui/gtk.c > > > @@ -1338,6 +1338,8 @@ static void gd_menu_untabify(GtkMenuItem *item, void *opaque) > > > FALSE); > > > } > > > if (!vc->window) { > > > + gint x, y, w, h; > > > + int i; > > > gtk_widget_set_sensitive(vc->menu_item, false); > > > vc->window = gtk_window_new(GTK_WINDOW_TOPLEVEL); > > > #if defined(CONFIG_OPENGL) > > > @@ -1351,7 +1353,18 @@ static void gd_menu_untabify(GtkMenuItem *item, void *opaque) > > > } > > > #endif > > > gd_widget_reparent(s->notebook, vc->window, vc->tab_item); > > > + gtk_window_get_position(GTK_WINDOW(s->window), &x, &y); > > > + gtk_window_get_size(GTK_WINDOW(s->window), &w, &h); > > > + > > > + for (i = 0; i < s->nb_vcs; i++) { > > > + if (vc == &s->vc[i]) { > > > + break; > > > + } > > > + } > > > > > > + gtk_window_move(GTK_WINDOW(vc->window), > > > + x + w * (i % (s->nb_vcs/2) + 1), y + h * (i / (s->nb_vcs/2))); > > > + gtk_window_resize(GTK_WINDOW(vc->window), w, h); > > > g_signal_connect(vc->window, "delete-event", > > > G_CALLBACK(gd_tab_window_close), vc); > > > gtk_widget_show_all(vc->window); > > > -- > > > 2.30.2 > > > > > > > > > > With regards, > > Daniel > > -- > > |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| > > |: https://libvirt.org -o- https://fstop138.berrange.com :| > > |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| > > > With regards, Daniel
On Fri, May 06, 2022 at 05:34:21PM +0100, Daniel P. Berrangé wrote: > On Tue, May 03, 2022 at 04:33:48PM -0700, Dongwon Kim wrote: > > I saw windows, especially, third and fourth ones are 1/4 size of > > the first when detached regardless of resolutions. > > > > And the position is also pretty random and detached windows are usually > > placed somewhere on the previous window. > > > > This patch is to make the sizes same as the original window's and make > > sure all detached windows are not overlapped each other. > > In terms of size, I think you need to just honour the surface > size like this: > > @@ -1354,6 +1354,9 @@ static void gd_menu_untabify(GtkMenuItem *item, void *opaque) > > g_signal_connect(vc->window, "delete-event", > G_CALLBACK(gd_tab_window_close), vc); > + gtk_window_set_default_size(GTK_WINDOW(vc->window), > + surface_width(vc->gfx.ds), > + surface_height(vc->gfx.ds)); Thanks for this. I will modify the code and test it. > gtk_widget_show_all(vc->window); > > if (qemu_console_is_graphic(vc->gfx.dcl.con)) { > > > for position, I don't think we should be overriding the window > manager placement, as the logic applied could result in us > placing windows off screen. Ok, I think what you are saying makes sense. > > > > > On Tue, May 03, 2022 at 10:17:46AM +0100, Daniel P. Berrangé wrote: > > > On Thu, Apr 28, 2022 at 04:13:04PM -0700, Dongwon Kim wrote: > > > > Specify location and size of detached window based on top level > > > > window's location and size info when detachment happens. > > > > > > Can you explain what problem is being solved by this change ? > > > What's wrong with default size/placement logic ? > > > > > > In terms of size at least, I would hope we are resizing > > > windows any time the guest changes the resolution of the > > > virtual video adapter. If there are 2 outputs, they can > > > be at different resolution, so copying the size of the > > > existing window feels wrong - we need to copy the guest > > > resolution currently set. > > > > > > Why do we need to mess around with position at all ? > > > > > > > Cc: Philippe Mathieu-Daudé <philmd@redhat.com> > > > > Cc: Paolo Bonzini <pbonzini@redhat.com> > > > > Cc: Gerd Hoffmann <kraxel@redhat.com> > > > > Cc: Vivek Kasireddy <vivek.kasireddy@intel.com> > > > > Signed-off-by: Dongwon Kim <dongwon.kim@intel.com> > > > > --- > > > > ui/gtk.c | 13 +++++++++++++ > > > > 1 file changed, 13 insertions(+) > > > > > > > > diff --git a/ui/gtk.c b/ui/gtk.c > > > > index f1ca6a7275..7dadf3b588 100644 > > > > --- a/ui/gtk.c > > > > +++ b/ui/gtk.c > > > > @@ -1338,6 +1338,8 @@ static void gd_menu_untabify(GtkMenuItem *item, void *opaque) > > > > FALSE); > > > > } > > > > if (!vc->window) { > > > > + gint x, y, w, h; > > > > + int i; > > > > gtk_widget_set_sensitive(vc->menu_item, false); > > > > vc->window = gtk_window_new(GTK_WINDOW_TOPLEVEL); > > > > #if defined(CONFIG_OPENGL) > > > > @@ -1351,7 +1353,18 @@ static void gd_menu_untabify(GtkMenuItem *item, void *opaque) > > > > } > > > > #endif > > > > gd_widget_reparent(s->notebook, vc->window, vc->tab_item); > > > > + gtk_window_get_position(GTK_WINDOW(s->window), &x, &y); > > > > + gtk_window_get_size(GTK_WINDOW(s->window), &w, &h); > > > > + > > > > + for (i = 0; i < s->nb_vcs; i++) { > > > > + if (vc == &s->vc[i]) { > > > > + break; > > > > + } > > > > + } > > > > > > > > + gtk_window_move(GTK_WINDOW(vc->window), > > > > + x + w * (i % (s->nb_vcs/2) + 1), y + h * (i / (s->nb_vcs/2))); > > > > + gtk_window_resize(GTK_WINDOW(vc->window), w, h); > > > > g_signal_connect(vc->window, "delete-event", > > > > G_CALLBACK(gd_tab_window_close), vc); > > > > gtk_widget_show_all(vc->window); > > > > -- > > > > 2.30.2 > > > > > > > > > > > > > > With regards, > > > Daniel > > > -- > > > |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| > > > |: https://libvirt.org -o- https://fstop138.berrange.com :| > > > |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| > > > > > > > With regards, > Daniel > -- > |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| > |: https://libvirt.org -o- https://fstop138.berrange.com :| > |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| >
diff --git a/ui/gtk.c b/ui/gtk.c index f1ca6a7275..7dadf3b588 100644 --- a/ui/gtk.c +++ b/ui/gtk.c @@ -1338,6 +1338,8 @@ static void gd_menu_untabify(GtkMenuItem *item, void *opaque) FALSE); } if (!vc->window) { + gint x, y, w, h; + int i; gtk_widget_set_sensitive(vc->menu_item, false); vc->window = gtk_window_new(GTK_WINDOW_TOPLEVEL); #if defined(CONFIG_OPENGL) @@ -1351,7 +1353,18 @@ static void gd_menu_untabify(GtkMenuItem *item, void *opaque) } #endif gd_widget_reparent(s->notebook, vc->window, vc->tab_item); + gtk_window_get_position(GTK_WINDOW(s->window), &x, &y); + gtk_window_get_size(GTK_WINDOW(s->window), &w, &h); + + for (i = 0; i < s->nb_vcs; i++) { + if (vc == &s->vc[i]) { + break; + } + } + gtk_window_move(GTK_WINDOW(vc->window), + x + w * (i % (s->nb_vcs/2) + 1), y + h * (i / (s->nb_vcs/2))); + gtk_window_resize(GTK_WINDOW(vc->window), w, h); g_signal_connect(vc->window, "delete-event", G_CALLBACK(gd_tab_window_close), vc); gtk_widget_show_all(vc->window);
Specify location and size of detached window based on top level window's location and size info when detachment happens. Cc: Philippe Mathieu-Daudé <philmd@redhat.com> Cc: Paolo Bonzini <pbonzini@redhat.com> Cc: Gerd Hoffmann <kraxel@redhat.com> Cc: Vivek Kasireddy <vivek.kasireddy@intel.com> Signed-off-by: Dongwon Kim <dongwon.kim@intel.com> --- ui/gtk.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)