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: Sat, 10 Dec 2022 19:32:17 +0800
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.5.1

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]