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

[Markus Armbruster <armbru@redhat.com>]

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

> On Thu, Jun 30, 2022 at 05:19:26PM +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
>> >
>> > v3: - Revised commit message
>> >     - Rewrote desription of the new parameter (Markus Armbruster)
>> >     - Replaced unnecessary 'for' loop with 'if' condition
>> >       (Markus Armbruster)
>> 
>> Again, patch history ...
>>
>> > Cc: Daniel P. Berrangé <berrange@redhat.com>
>> > Cc: Markus Armbruster <armbru@redhat.com>
>> > 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>
>> > ---
>> 
>> ... goes here.
>
> No problem moving down the version history but may I ask you if that
> is current rule? We don't want to include the history anymore in the
> git history?

Patch history is really valuable for reviewers, but once the patch is
done, it's rarely interesting anymore, so we keep it out of Git.

Sometimes, bits of history are still useful to understand why the patch
is done the way it is.  Such bits should be worked into the commit
message.

Don't:

    v2: A replaced by B [J. Reviewer]

Do:

    I initially tried A, but it turned out to be a bad idea because X,
    so I did B instead.

Makes sense?

> And FYI, the cover letter has the whole history already. I guess I can
> simply remove the history from individual patches then?

I like to keep detailed history in the cover letter.

Others like to keep it in each patch.

Still others like to keep an overview in the cover letter and details in
each patch.

All fine.

> Thanks!!
>
>> 
>> >  qapi/ui.json    |  9 ++++++++-
>> >  qemu-options.hx |  3 ++-
>> >  ui/gtk.c        | 31 +++++++++++++++++++++++++++++--
>> >  3 files changed, 39 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/qapi/ui.json b/qapi/ui.json
>> > index 413371d5e8..7b4c098bb4 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.
>> 
>> Drop the hyphen in "virtual-console".
>> 
>> Is the term "virtual console" obvious?  Gerd?
>> 
>
> I will do so.

Replace it by space, of course.

[...]