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