Re: [PATCH v4 2/2] ui/gtk: a new array param monitor to specify the target displays

[Markus Armbruster]

Dongwon Kim <dongwon.kim@intel.com> writes:

> On Tue, Jul 12, 2022 at 08:11:08AM +0200, Markus Armbruster wrote:
>> Dongwon Kim <dongwon.kim@intel.com> writes:
>> 
>> > New integer array parameter, 'monitor' is for specifying the target
>> > monitors where individual GTK windows are placed upon launching.
>> >
>> > Monitor numbers in the array are associated with virtual consoles
>> > in the order of [VC0, VC1, VC2 ... VCn].
>> >
>> > Every GTK window containing each VC will be placed in the region
>> > of corresponding monitors.
>> >
>> > Usage: -display gtk,monitor.<id of VC>=<target monitor>,..
>> >        ex)-display gtk,monitor.0=1,monitor.1=0
>> >
>> > Cc: Daniel P. Berrangé <berrange@redhat.com>
>> > Cc: Markus Armbruster <armbru@redhat.com>
>> > Cc: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> > 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>
>> > ---
>> >  qapi/ui.json    |  9 ++++++++-
>> >  qemu-options.hx |  3 ++-
>> >  ui/gtk.c        | 30 ++++++++++++++++++++++++++++--
>> >  3 files changed, 38 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/qapi/ui.json b/qapi/ui.json
>> > index 413371d5e8..ee0f9244ef 100644
>> > --- a/qapi/ui.json
>> > +++ b/qapi/ui.json
>> > @@ -1195,12 +1195,19 @@
>> >  #               assuming the guest will resize the display to match
>> >  #               the window size then.  Otherwise it defaults to "off".
>> >  #               Since 3.1
>> > +# @monitor:     Array of numbers, each of which represents physical
>> > +#               monitor where GTK window containing a given VC will be
>> > +#               placed. Each monitor number in the array will be
>> > +#               associated with a virtual console starting from VC0.
>> > +#
>> > +#               since 7.1
>> 
>> I dislike repeating the type (here: array of numbers) in the
>> description.
>> 
>> Suggest something like
>> 
>>    # @monitor:     List of physical monitor numbers where the GTK windows
>>    #               containing the virtual consoles VC0, VC1, ... are to be
>>    #               placed.  (Since 7.1)
>> 
>> Missing: what happens when there are more VCs than list elements.  Can
>> you tell us?
>
>     # @monitor:     List of physical monitor numbers where the GTK windows
>     #               containing the virtual consoles VC0, VC1, ... are to be
>     #               placed. If a mapping exists for a VC, then it'd be
>     #               placed on that specific physical monitor; otherwise,
>     #               it'd default to the monitor from where it was launched
>     #               since 7.1
>
> How does this look?

Pretty good!

Nitpicks: replace "id'd" by "it will" or "it is to be", end your second
sentence with a period, and format "since" like we do elsewhere.
Together:

      # @monitor:     List of physical monitor numbers where the GTK windows
      #               containing the virtual consoles VC0, VC1, ... are to be
      #               placed. If a mapping exists for a VC, then it is to be
      #               placed on that specific physical monitor; otherwise,
      #               it defaults to the monitor from where it was launched.
      #               (Since 7.1)

>> 
>> >  #
>> >  # Since: 2.12
>> >  ##
>> >  { 'struct'  : 'DisplayGTK',
>> >    'data'    : { '*grab-on-hover' : 'bool',
>> > -                '*zoom-to-fit'   : 'bool'  } }
>> > +                '*zoom-to-fit'   : 'bool',
>> > +                '*monitor'       : ['uint16']  } }
>> >  
>> >  ##
>> >  # @DisplayEGLHeadless:
>> 
>> [...]
>>