grub-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: AW: AW: Adding --set to videoinfo to retrieve current video resoluti


From: Markus Scholz
Subject: Re: AW: AW: Adding --set to videoinfo to retrieve current video resolution
Date: Mon, 12 Dec 2022 12:09:25 +0100

Hi, 

thank you again for your reply and explanation. Despite the different focus, I am still looking forward to try out your additions. 

Regarding my videoinfo patch I submitted them using git to the mailing list. What is the best way forward here?

Since its just a single file and no deep changes maybe the effort is no too much... Would you be so kind/able to review them or should I reach out to Daniel for this? 
Since I am new to the grub process I am not sure how to get the changes in. 

Thank you and best
Markus 


Zhang Boyang <zhangboyang.id@gmail.com> schrieb am Sa., 10. Dez. 2022, 12:32:
Hi,

On 2022/12/6 22:17, Markus Scholz wrote:
> 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.
>

Thanks for explaining your solution. My font scaling patches can't give
user consistent experience, because it only accept integer scaling factors.

By the way, Debian CDs use a fixed 800x600 resolution as far as I know,
and the user experience is consistent. However, its appearance is blurry
and this doesn't work on buggy firmwares which do not allow to switch to
800x600 resolution.

> 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.
>

Unfortunately my patches only work on gfxterm and there is no support
for gfxmenu yet (at least for now).

Nevertheless, I think it's good to have both my patches and your
patches, because I think we are focusing on different things.

Best Regards,
Zhang Boyang


> 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
>

reply via email to

[Prev in Thread] Current Thread [Next in Thread]