[PATCH 0/1] ui/cocoa: add full-screen-scaling display option

[PATCH 0/1] ui/cocoa: add full-screen-scaling display option

[carwynellis]

From: Carwyn Ellis <carwynellis@gmail.com>

The intention here is to allow fullscreen scaling of the display to be
enabled by setting the display option `full-screen-scaling` to on,
allowing this to be set from the command line without having to interact
with the ui menu.

Carwyn Ellis (1):
  ui/cocoa: add full-screen-scaling display option

 qapi/ui.json |  6 +++++-
 ui/cocoa.m   | 33 ++++++++++++++++++++-------------
 2 files changed, 25 insertions(+), 14 deletions(-)

-- 
2.42.0

[PATCH 1/1] ui/cocoa: add full-screen-scaling display option

[carwynellis]

From: Carwyn Ellis <carwynellis@gmail.com>

Provides a display option, full-screen-scaling, that enables scaling of
the display when full-screen mode is enabled.

Also ensures that the corresponding menu item is marked as enabled when
the option is set to on.
---
 qapi/ui.json |  6 +++++-
 ui/cocoa.m   | 33 ++++++++++++++++++++-------------
 2 files changed, 25 insertions(+), 14 deletions(-)

diff --git a/qapi/ui.json b/qapi/ui.json
index 006616aa77..9035b230ce 100644
--- a/qapi/ui.json
+++ b/qapi/ui.json
@@ -1409,13 +1409,17 @@
 #     codes match their position on non-Mac keyboards and you can use
 #     Meta/Super and Alt where you expect them.  (default: off)
 #
+# @full-screen-scaling: Scale display to fit when full-screen enabled.
+#     Defaults to "off".
+#
 # Since: 7.0
 ##
 { 'struct': 'DisplayCocoa',
   'data': {
       '*left-command-key': 'bool',
       '*full-grab': 'bool',
-      '*swap-opt-cmd': 'bool'
+      '*swap-opt-cmd': 'bool',
+      '*full-screen-scaling': 'bool'
   } }
 
 ##
diff --git a/ui/cocoa.m b/ui/cocoa.m
index d95276013c..7ddc4de174 100644
--- a/ui/cocoa.m
+++ b/ui/cocoa.m
@@ -1671,7 +1671,9 @@ static void create_initial_menus(void)
     // View menu
     menu = [[NSMenu alloc] initWithTitle:@"View"];
     [menu addItem: [[[NSMenuItem alloc] initWithTitle:@"Enter Fullscreen" action:@selector(doToggleFullScreen:) keyEquivalent:@"f"] autorelease]]; // Fullscreen
-    [menu addItem: [[[NSMenuItem alloc] initWithTitle:@"Zoom To Fit" action:@selector(zoomToFit:) keyEquivalent:@""] autorelease]];
+    menuItem = [[[NSMenuItem alloc] initWithTitle:@"Zoom To Fit" action:@selector(zoomToFit:) keyEquivalent:@""] autorelease];
+    [menuItem setState: (stretch_video) ? NSControlStateValueOn : NSControlStateValueOff];
+    [menu addItem: menuItem];
     menuItem = [[[NSMenuItem alloc] initWithTitle:@"View" action:nil keyEquivalent:@""] autorelease];
     [menuItem setSubmenu:menu];
     [[NSApp mainMenu] addItem:menuItem];
@@ -2041,18 +2043,6 @@ static void cocoa_display_init(DisplayState *ds, DisplayOptions *opts)
 
     [QemuApplication sharedApplication];
 
-    create_initial_menus();
-
-    /*
-     * Create the menu entries which depend on QEMU state (for consoles
-     * and removable devices). These make calls back into QEMU functions,
-     * which is OK because at this point we know that the second thread
-     * holds the iothread lock and is synchronously waiting for us to
-     * finish.
-     */
-    add_console_menu_entries();
-    addRemovableDevicesMenuItems();
-
     // Create an Application controller
     QemuCocoaAppController *controller = [[QemuCocoaAppController alloc] init];
     [NSApp setDelegate:controller];
@@ -2062,6 +2052,7 @@ static void cocoa_display_init(DisplayState *ds, DisplayOptions *opts)
         [NSApp activateIgnoringOtherApps: YES];
         [controller toggleFullScreen: nil];
     }
+
     if (opts->u.cocoa.has_full_grab && opts->u.cocoa.full_grab) {
         [controller setFullGrab: nil];
     }
@@ -2077,6 +2068,22 @@ static void cocoa_display_init(DisplayState *ds, DisplayOptions *opts)
         left_command_key_enabled = 0;
     }
 
+    if (opts->u.cocoa.has_full_screen_scaling && opts->u.cocoa.full_screen_scaling) {
+        stretch_video = true;
+    }
+
+    create_initial_menus();
+    /*
+     * Create the menu entries which depend on QEMU state (for consoles
+     * and removable devices). These make calls back into QEMU functions,
+     * which is OK because at this point we know that the second thread
+     * holds the iothread lock and is synchronously waiting for us to
+     * finish.
+     */
+    add_console_menu_entries();
+    addRemovableDevicesMenuItems();
+
+
     // register vga output callbacks
     register_displaychangelistener(&dcl);
 
-- 
2.42.0

Re: [PATCH 1/1] ui/cocoa: add full-screen-scaling display option

[Akihiko Odaki]

On 2023/10/25 23:04, carwynellis@gmail.com wrote:
> From: Carwyn Ellis <carwynellis@gmail.com>
> 
> Provides a display option, full-screen-scaling, that enables scaling of
> the display when full-screen mode is enabled.
> 
> Also ensures that the corresponding menu item is marked as enabled when
> the option is set to on.

Hi,

Thank you for your contribution.

Please drop qemu-trivial@nongnu.org. This change is not that trivial.
Instead add "Graphics maintainers" listed in MAINTAINERS file to CC.

Please also add Signed-off-by tag. See docs/devel/submitting-a-patch.rst 
to know what the tag means.

> ---
>   qapi/ui.json |  6 +++++-
>   ui/cocoa.m   | 33 ++++++++++++++++++++-------------
>   2 files changed, 25 insertions(+), 14 deletions(-)
> 
> diff --git a/qapi/ui.json b/qapi/ui.json
> index 006616aa77..9035b230ce 100644
> --- a/qapi/ui.json
> +++ b/qapi/ui.json
> @@ -1409,13 +1409,17 @@
>   #     codes match their position on non-Mac keyboards and you can use
>   #     Meta/Super and Alt where you expect them.  (default: off)
>   #
> +# @full-screen-scaling: Scale display to fit when full-screen enabled.
> +#     Defaults to "off".
> +#

I think it's better to name zoom-to-fit to align with DisplayGTK.
It should also have "(Since 8.2)".

>   # Since: 7.0
>   ##
>   { 'struct': 'DisplayCocoa',
>     'data': {
>         '*left-command-key': 'bool',
>         '*full-grab': 'bool',
> -      '*swap-opt-cmd': 'bool'
> +      '*swap-opt-cmd': 'bool',
> +      '*full-screen-scaling': 'bool'
>     } }
>   
>   ##
> diff --git a/ui/cocoa.m b/ui/cocoa.m
> index d95276013c..7ddc4de174 100644
> --- a/ui/cocoa.m
> +++ b/ui/cocoa.m
> @@ -1671,7 +1671,9 @@ static void create_initial_menus(void)
>       // View menu
>       menu = [[NSMenu alloc] initWithTitle:@"View"];
>       [menu addItem: [[[NSMenuItem alloc] initWithTitle:@"Enter Fullscreen" action:@selector(doToggleFullScreen:) keyEquivalent:@"f"] autorelease]]; // Fullscreen
> -    [menu addItem: [[[NSMenuItem alloc] initWithTitle:@"Zoom To Fit" action:@selector(zoomToFit:) keyEquivalent:@""] autorelease]];
> +    menuItem = [[[NSMenuItem alloc] initWithTitle:@"Zoom To Fit" action:@selector(zoomToFit:) keyEquivalent:@""] autorelease];
> +    [menuItem setState: (stretch_video) ? NSControlStateValueOn : NSControlStateValueOff];

nit: You don't need parentheses around strech_video.

> +    [menu addItem: menuItem];
>       menuItem = [[[NSMenuItem alloc] initWithTitle:@"View" action:nil keyEquivalent:@""] autorelease];
>       [menuItem setSubmenu:menu];
>       [[NSApp mainMenu] addItem:menuItem];
> @@ -2041,18 +2043,6 @@ static void cocoa_display_init(DisplayState *ds, DisplayOptions *opts)
>   
>       [QemuApplication sharedApplication];
>   
> -    create_initial_menus();
> -
> -    /*
> -     * Create the menu entries which depend on QEMU state (for consoles
> -     * and removable devices). These make calls back into QEMU functions,
> -     * which is OK because at this point we know that the second thread
> -     * holds the iothread lock and is synchronously waiting for us to
> -     * finish.
> -     */
> -    add_console_menu_entries();
> -    addRemovableDevicesMenuItems();
> -
>       // Create an Application controller
>       QemuCocoaAppController *controller = [[QemuCocoaAppController alloc] init];

QemuCocoaAppController also has code to initialize stretch_video; it's 
not OK to have code to initialize a same variable in two different places.

>       [NSApp setDelegate:controller];
> @@ -2062,6 +2052,7 @@ static void cocoa_display_init(DisplayState *ds, DisplayOptions *opts)
>           [NSApp activateIgnoringOtherApps: YES];
>           [controller toggleFullScreen: nil];
>       }
> +

Don't add a blank line here. It's irrelevant from this change.

Re: [PATCH 1/1] ui/cocoa: add full-screen-scaling display option

[Carwyn Ellis]

> On 26 Oct 2023, at 03:51, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
> 
> On 2023/10/25 23:04, carwynellis@gmail.com wrote:
>> From: Carwyn Ellis <carwynellis@gmail.com>
>> Provides a display option, full-screen-scaling, that enables scaling of
>> the display when full-screen mode is enabled.
>> Also ensures that the corresponding menu item is marked as enabled when
>> the option is set to on.
> 
> Hi,
> 
> Thank you for your contribution.
> 
> Please drop qemu-trivial@nongnu.org. This change is not that trivial.
> Instead add "Graphics maintainers" listed in MAINTAINERS file to CC.
> 
> Please also add Signed-off-by tag. See docs/devel/submitting-a-patch.rst to know what the tag means.

Hi,

No problem at all. Thanks for getting back to me so quickly! :) 

I guess I missed this on the email body so I’ll make sure this is present when I resubmit.

Thanks

> 
>> ---
>>  qapi/ui.json |  6 +++++-
>>  ui/cocoa.m   | 33 ++++++++++++++++++++-------------
>>  2 files changed, 25 insertions(+), 14 deletions(-)
>> diff --git a/qapi/ui.json b/qapi/ui.json
>> index 006616aa77..9035b230ce 100644
>> --- a/qapi/ui.json
>> +++ b/qapi/ui.json
>> @@ -1409,13 +1409,17 @@
>>  #     codes match their position on non-Mac keyboards and you can use
>>  #     Meta/Super and Alt where you expect them.  (default: off)
>>  #
>> +# @full-screen-scaling: Scale display to fit when full-screen enabled.
>> +#     Defaults to "off".
>> +#
> 
> I think it's better to name zoom-to-fit to align with DisplayGTK.
> It should also have "(Since 8.2)".

I did look for an existing param but missed this one. Thanks!

> 
>>  # Since: 7.0
>>  ##
>>  { 'struct': 'DisplayCocoa',
>>    'data': {
>>        '*left-command-key': 'bool',
>>        '*full-grab': 'bool',
>> -      '*swap-opt-cmd': 'bool'
>> +      '*swap-opt-cmd': 'bool',
>> +      '*full-screen-scaling': 'bool'
>>    } }
>>    ##
>> diff --git a/ui/cocoa.m b/ui/cocoa.m
>> index d95276013c..7ddc4de174 100644
>> --- a/ui/cocoa.m
>> +++ b/ui/cocoa.m
>> @@ -1671,7 +1671,9 @@ static void create_initial_menus(void)
>>      // View menu
>>      menu = [[NSMenu alloc] initWithTitle:@"View"];
>>      [menu addItem: [[[NSMenuItem alloc] initWithTitle:@"Enter Fullscreen" action:@selector(doToggleFullScreen:) keyEquivalent:@"f"] autorelease]]; // Fullscreen
>> -    [menu addItem: [[[NSMenuItem alloc] initWithTitle:@"Zoom To Fit" action:@selector(zoomToFit:) keyEquivalent:@""] autorelease]];
>> +    menuItem = [[[NSMenuItem alloc] initWithTitle:@"Zoom To Fit" action:@selector(zoomToFit:) keyEquivalent:@""] autorelease];
>> +    [menuItem setState: (stretch_video) ? NSControlStateValueOn : NSControlStateValueOff];
> 
> nit: You don't need parentheses around strech_video.

No problem - will change this. 

> 
>> +    [menu addItem: menuItem];
>>      menuItem = [[[NSMenuItem alloc] initWithTitle:@"View" action:nil keyEquivalent:@""] autorelease];
>>      [menuItem setSubmenu:menu];
>>      [[NSApp mainMenu] addItem:menuItem];
>> @@ -2041,18 +2043,6 @@ static void cocoa_display_init(DisplayState *ds, DisplayOptions *opts)
>>        [QemuApplication sharedApplication];
>>  -    create_initial_menus();
>> -
>> -    /*
>> -     * Create the menu entries which depend on QEMU state (for consoles
>> -     * and removable devices). These make calls back into QEMU functions,
>> -     * which is OK because at this point we know that the second thread
>> -     * holds the iothread lock and is synchronously waiting for us to
>> -     * finish.
>> -     */
>> -    add_console_menu_entries();
>> -    addRemovableDevicesMenuItems();
>> -
>>      // Create an Application controller
>>      QemuCocoaAppController *controller = [[QemuCocoaAppController alloc] init];
> 
> QemuCocoaAppController also has code to initialize stretch_video; it's not OK to have code to initialize a same variable in two different places.

Ok, I’ll take a look and tidy this up.

> 
>>      [NSApp setDelegate:controller];
>> @@ -2062,6 +2052,7 @@ static void cocoa_display_init(DisplayState *ds, DisplayOptions *opts)
>>          [NSApp activateIgnoringOtherApps: YES];
>>          [controller toggleFullScreen: nil];
>>      }
>> +
> 
> Don't add a blank line here. It's irrelevant from this change.

Ok.

I’ll aim to get this resubmitted as a V2 later today with the appropriate CCs.

Thanks again,
Carwyn