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