grub-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: AW: AW: Adding --set to videoinfo to retrieve current video resoluti


From: Zhang Boyang
Subject: Re: AW: AW: Adding --set to videoinfo to retrieve current video resolution
Date: Tue, 13 Dec 2022 19:06:51 +0800
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.5.1

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






reply via email to

[Prev in Thread] Current Thread [Next in Thread]