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

[Markus Scholz]

Hi Zhang,

now I need to apologize for my very late reply.. sorry!
I saw that you also went ahead as you said with committing the highdpi patches.

Anyways, regarding the high DPI question:
until now we have mostly focused on different screen resolutions in our 
customer offerings and simply chose the font
size based on visual testing. So we did not attempt to solve this 
"mathematically".

E.g. right now use:
- x_res >= 1024; bootmenu font ubuntu regular 20 (large theme)
- x_res == 1204; bootmenu font ubuntu regular 17 (medium theme)
- x_res < 1024 & unknown ; bootmenu font ubuntu regular 15 (small theme) 

I don’t know if this helpful for you but that’s the current state we are using 
to keep
the text readable given different resolutions. Of course, for the different 
fonts/themes
(small, medium, large) the screens still look different i.e. the user 
experience is not consistent.
Maybe using your highDPI patch it will become much easier to
make the appearance consistent and more appealing across different screen 
resolutions.

Note that we exclusively use gfxmode for our application, avoiding text mode.
We believe that, for our application, this gives a much more polished and 
mature look&feel.

Also thank you for your recommendation for the patch submission.
I will attempt to send another reply to this thread with the help of git patch 
asap.

Best
Markus


-----Ursprüngliche Nachricht-----
Von: grub-devel-bounces+scholz=novelsense.com@gnu.org 
<grub-devel-bounces+scholz=novelsense.com@gnu.org> Im Auftrag von Zhang Boyang
Gesendet: Montag, 7. November 2022 08:56
An: Markus Scholz <scholz@novelsense.com>; grub-devel@gnu.org
Cc: phcoder@gmail.com; pmenzel@molgen.mpg.de; 'Daniel Kiper' 
<dkiper@net-space.pl>
Betreff: Re: AW: Adding --set to videoinfo to retrieve current video resolution

Hi,

Sorry for late reply... I think my patch is almost independent of Markus's, and 
it would be good to have both. However, I think there are some point we can 
share opinions. I'd like to ask your opinions of the mechanism of choosing font 
size automatically. In my patch (preliminary), it scales Unifont(16x16) M times 
on a N x 1080p screen, with M = pow(2,ceil(log2(N))), e.g. 2x when (1080p, 
2160p], 4x when (2160p, 4320p], 8x when (4320p, 8640p]. This approach seems not 
good enough, and it might not suitable for custom themes. What's your opinion 
on this from your perspective? I will probably go back to work on my HiDPI 
patch after few weeks.

By the way, if I understand correctly, you patch seems reversed, and your mail 
client seems corrupted your patch. You might find "git format-patch" and "git 
send-email" useful :)

Best Regards,
Zhang Boyang


On 2022/10/24 19:23, Markus Scholz wrote:
> 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
>>   }
> 

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel