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....@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.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 _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
