AW: Adding --set to videoinfo to retrieve current video resolution

[Markus Scholz]

Hi all,

* I checked the links by Daniel (thanks for providing).
* As I understand it Zhangs patches aim to add a high dpi scaling feature
for the fonts
* In contrast, the patch I submitted (albeit having a similar use-case) adds
the possibility
for grub "scripts" / config files to access & act upon the currently used
display resolution 
* for me both things would have their merit and independent use-cases

What is your opinion on having both features?

Besides, I am willing to help progress both features - if someone can guide
me /give a hint
how to proceed best?

Thank you
Markus


-----Ursprüngliche Nachricht-----
Von: Daniel Kiper <dkiper@net-space.pl> 
Gesendet: Donnerstag, 20. Oktober 2022 19:36
An: Markus Scholz <scholz@novelsense.com>
Cc: grub-devel@gnu.org; phcoder@gmail.com; pmenzel@molgen.mpg.de;
zhangboyang.id@gmail.com
Betreff: Re: Adding --set to videoinfo to retrieve current video resolution

Adding Paul, Vladimir, and Zhang...

On Wed, Oct 12, 2022 at 03:36:13PM +0200, Markus Scholz wrote:
> Dear Grub Developers,
>
> recently we faced the problem the author in this old thread faced:
> - https://lists.gnu.org/archive/html/grub-devel/2012-10/msg00009.html
>
> Ie. our theme wouldn't display properly due to different screen
resolutions.
> Hence, I wanted to change specifically the font size of our theme 
> given the current resolution.
> While the videoinfo module provides info about the current resolution 
> I wasn't able to discover any way to get the info from the booted 
> grub.
>
> Following the aforementioned thread, I therefore patched the videoinfo 
> mod to add the ability of a -set switch just like commands like 
> "smbinfo" or "search" have.
>
> Ie. the documentation of the videoinfo call could change as follows:
>
> Command: videoinfo [--set var | [WxH]xD]
>
>     Retrieve available video modes. If --set is given, assign the 
> currently active resolution to var.
>     If --set is not provided the available video modes are listed.
>     If resolution is given, show only matching modes.
>
> Attached is my patch proposal for the module; I probably made a lot 
> mistakes wrt.
> coding style and guidelines - please bear with me.
>
> I would be grateful regarding feedback what I could do / how I could 
> improve the patch to make it part of grub? If it possible at all with this
approach?

I think fix for similar problem has been proposed here [1] and I commented
it here [2]. Sadly it ended in limbo. Could you work with Zhang on this
issue?

Daniel

[1] https://lists.gnu.org/archive/html/grub-devel/2022-06/msg00143.html
[2] https://lists.gnu.org/archive/html/grub-devel/2022-07/msg00027.html

> Thank you for your reply & best,
> Markus
>
> ---
>
> --- grub-mod/grub-core/commands/videoinfo.c   2022-10-12
> 13:30:54.992451000 +0000
> +++ grub/grub-core/commands/videoinfo.c       2022-10-12
13:31:37.715512000 +0000
> @@ -30,7 +30,6 @@ GRUB_MOD_LICENSE ("GPLv3+");  struct hook_ctx  {
>    unsigned height, width, depth;
> -  unsigned char set_variable;
>    struct grub_video_mode_info *current_mode;  };
>
> @@ -39,9 +38,6 @@ hook (const struct grub_video_mode_info  {
>    struct hook_ctx *ctx = hook_arg;
>
> -  if (ctx->set_variable) /* if variable should be set don't print all 
> modes */
> -    return 0;
> -
>    if (ctx->height && ctx->width && (info->width != ctx->width ||
> info->height != ctx->height))
>      return 0;
>
> @@ -136,40 +132,31 @@ grub_cmd_videoinfo (grub_command_t cmd _
>    grub_video_adapter_t adapter;
>    grub_video_driver_id_t id;
>    struct hook_ctx ctx;
> -
> -  ctx.height = ctx.width = ctx.depth = ctx.set_variable = 0;
> -
> +
> +  ctx.height = ctx.width = ctx.depth = 0;
>    if (argc)
>      {
> -      if (argc == 2 && grub_memcmp(args[0], "--set", sizeof ("--set") -
1)
> == 0 )
> -  ctx.set_variable = 1;
> -      else
> -        {
>        const char *ptr;
>        ptr = args[0];
>        ctx.width = grub_strtoul (ptr, &ptr, 0);
>        if (grub_errno)
> -  return grub_errno;
> +     return grub_errno;
>        if (*ptr != 'x')
> -  return grub_error (GRUB_ERR_BAD_ARGUMENT,
> -        N_("invalid video mode specification `%s'"),
> -        args[0]);
> +     return grub_error (GRUB_ERR_BAD_ARGUMENT,
> +                        N_("invalid video mode specification `%s'"),
> +                        args[0]);
>        ptr++;
>        ctx.height = grub_strtoul (ptr, &ptr, 0);
>        if (grub_errno)
> -  return grub_errno;
> +     return grub_errno;
>        if (*ptr == 'x')
> -  {
> -    ptr++;
> -    ctx.depth = grub_strtoul (ptr, &ptr, 0);
> -    if (grub_errno)
> -      return grub_errno;
> -  }
> -        }
> -
> +     {
> +       ptr++;
> +       ctx.depth = grub_strtoul (ptr, &ptr, 0);
> +       if (grub_errno)
> +         return grub_errno;
> +     }
>      }
> -
> -
>
>  #ifdef GRUB_MACHINE_PCBIOS
>    if (grub_strcmp (cmd->name, "vbeinfo") == 0) @@ -177,23 +164,20 @@ 
> grub_cmd_videoinfo (grub_command_t cmd _  #endif
>
>    id = grub_video_get_driver_id ();
> -  if (!ctx.set_variable)
> -    {
> -grub_puts_ (N_("List of supported video modes:")); -grub_puts_ 
> (N_("Legend: mask/position=red/green/blue/reserved"));
> -    }
> -
> +
> +  grub_puts_ (N_("List of supported video modes:"));  grub_puts_ 
> + (N_("Legend: mask/position=red/green/blue/reserved"));
> +
>    FOR_VIDEO_ADAPTERS (adapter)
>    {
>      struct grub_video_mode_info info;
>      struct grub_video_edid_info edid_info;
> -    if (!ctx.set_variable)
> -  grub_printf_ (N_("Adapter `%s':\n"), adapter->name);
> +
> +    grub_printf_ (N_("Adapter `%s':\n"), adapter->name);
>
>      if (!adapter->iterate)
>        {
> -        if (!ctx.set_variable)
> -    grub_puts_ (N_("  No info available"));
> +     grub_puts_ (N_("  No info available"));
>       continue;
>        }
>
> @@ -202,18 +186,7 @@ grub_puts_ (N_("Legend: mask/position=re
>      if (adapter->id == id)
>        {
>       if (grub_video_get_info (&info) == GRUB_ERR_NONE)
> -    {
> -      ctx.current_mode = &info;
> -      if (ctx.set_variable)
> -        {
> -    /* set grub env var to current mode */
> -    char screen_wxh[20];
> -    unsigned int width = info.width;
> -    unsigned int height = info.height;
> -    grub_snprintf (screen_wxh, 20, "%ux%u", width, height);
> -    grub_env_set (args[1], screen_wxh);
> -        }
> -    }
> +       ctx.current_mode = &info;
>       else
>         /* Don't worry about errors.  */
>         grub_errno = GRUB_ERR_NONE;
> @@ -263,26 +236,19 @@ GRUB_MOD_INIT(videoinfo)
>                              /* TRANSLATORS: "x" has to be entered in,
>                                 like an identifier, so please don't
>                                 use better Unicode codepoints.  */
> -                            N_("[--set var | [WxH]xD]"),
> -                            N_("Retrieve available video modes. "
> -             "--set is given, assign the currently"
> -             "active resolution to var. If --set "
> -             "is not provided the available video "
> -             "modes are listed. If resolution is "
> -             "given, show only matching modes."));
> -
> +                            N_("[WxH[xD]]"),
> +                            N_("List available video modes. If "
> +                                  "resolution is given show only modes"
> +                                  " matching it."));
>  #ifdef GRUB_MACHINE_PCBIOS
>    cmd_vbe = grub_register_command ("vbeinfo", grub_cmd_videoinfo,
>                                  /* TRANSLATORS: "x" has to be entered in,
>                                     like an identifier, so please don't
>                                     use better Unicode codepoints.  */
> -                                N_("[--set var | [WxH]xD]"),
> -           N_("Retrieve available video modes. "
> -           "--set is given, assign the currently"
> -           "active resolution to var. If --set "
> -           "is not provided the available video "
> -           "modes are listed. If resolution is "
> -           "given, show only matching modes."));
> +                                N_("[WxH[xD]]"),
> +                                N_("List available video modes. If "
> +                                   "resolution is given show only modes"
> +                                   " matching it."));
>  #endif
>  }