Dear Zhang, thank you a lot for your advice and review! I will address the pointed out improvements and then ask Daniel (as he suggested) to take a look.
Thanks again & best Markus -----Ursprüngliche Nachricht----- Von: Zhang Boyang <zhangboyang...@gmail.com> Gesendet: Dienstag, 13. Dezember 2022 12:07 An: Markus Scholz <sch...@novelsense.com> Cc: The development of GNU GRUB <grub-devel@gnu.org> Betreff: Re: AW: AW: Adding --set to videoinfo to retrieve current video resolution Hi, On 2022/12/12 19:09, Markus Scholz wrote: > 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? > Maybe the only thing what can do now is wait... If you think the maintainers forget you patches, you can resend them like [PATCH RESEND 1/1] after few days/weeks. > 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? I'm not a maintainer of GRUB, so I'm not sure whether I have rights to send Reviewed-By tags. Although I think I can give some advice for these patches. I think you can reach out to Daniel if you think these patches are forgotten. > Since I am new to the grub process I am not sure how to get the changes in. I'm also new to GRUB (and other open source projects).... So please forgive me if I said something misleading. Best Regards, Zhang Boyang > > Thank you and best > Markus > > > Zhang Boyang <zhangboyang...@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....@gnu.org >> <grub-devel-bounces+scholz=novelsense....@gnu.org> Im Auftrag von >> Zhang Boyang >>> Gesendet: Montag, 7. November 2022 08:56 >>> An: Markus Scholz <sch...@novelsense.com>; grub-devel@gnu.org >>> Cc: phco...@gmail.com; pmen...@molgen.mpg.de; 'Daniel Kiper' < >> dki...@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 <dki...@net-space.pl> >>>> Gesendet: Donnerstag, 20. Oktober 2022 19:36 >>>> An: Markus Scholz <sch...@novelsense.com> >>>> Cc: grub-devel@gnu.org; phco...@gmail.com; pmen...@molgen.mpg.de; >>>> zhangboyang...@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.htm >>>>> l >>>>> >>>>> 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 >>> >> > _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
