[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
AW: AW: AW: Adding --set to videoinfo to retrieve current video resoluti
|
From: |
Markus Scholz |
|
Subject: |
AW: AW: AW: Adding --set to videoinfo to retrieve current video resolution |
|
Date: |
Wed, 14 Dec 2022 13:54:13 +0100 |
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.id@gmail.com>
Gesendet: Dienstag, 13. Dezember 2022 12:07
An: Markus Scholz <scholz@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.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.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
>>>
>>
>