Re: fixed item-height, item-spacing, max-elements-shown etc

Re: fixed item-height, item-spacing, max-elements-shown etc

[Vladimir Testov]

[-- Attachment #1: Type: text/plain, Size: 3447 bytes --]

>I am not sure whether it is appropriate fix. After your patch "box
>effect" on selected item is effectively lost (it makes
>selected_item_box the same height as selected item itself).
About "box effect" - there is no "box effect" in menu_box. The menu's box is 
drawn inside given area. So the idea of "box effect" was unclear to me. (Also, 
the height of selected element (with box) is item_height + box_pad_top + 
box_pad_bottom - 1... that "-1" makes the "box effect" totally unclear. My idea 
was to make GRUB theme be countable to pixel. Also, with my patch (when we can 
have a box for every unselected element) item_spacing loses it's meaning 
totally. It has uncertain meaning in current GRUB state too.

I thought that idea of menu's box was more valuable because it has influence to 
the code.

So, when we need to calculate exact values so the result will request our 
expectations, the idea of item_height as total height is more acceptable.

Also I have in mind some more changes:
 - width of scrollbar (similar but a little trickier)
 - move scrollbar (small fix, now scrollbar will be drawn only if we have east 
slice of the menu's bitmap.
 - if we move scrollbar then we should cut a little items' width (also if 
scrollbar's width is total scrollbar width (I mean if we use east and\or west 
slices of scrollbar_thumb or scrollbar_frame) then it will be easier to 
calculate)
 - also if we move scrollbar then the total idea of the scrollbar will become 
more intuitive (like if we work with something in a "window" and it's height 
is too big then scrollbar will appear in the same "window" and cut the 
"window's" width.) Mean the scrollbar will appear inside the "window" but not 
outside.
 - now the distance between right side of the selected item's box and the 
scrollbar is exactly 2 pixels.

I see two solutions for the last problem.
1) make item_spacing be really space between elements and also the space 
between menu_items and scrollbar
2) make additional option "scrollbar_spacing"

The first idea seems to me more adequate and simple.

So the idea of making item_height be total height is interconnected with other 
ideas.

>The actual fix looks much simpler - get_num_shown_items() should
>additionally account for self->selected_item_box->get_op_pad().

Also we will need:
1) update menu_box (throuhout code) so it also will have "boxing effect"
2) update height of the selected element

because of menu's box update fix won't look much simplier

>Could you make available theme that demonstrates this problem?
Yep, here's the example. Item's height is 20, item's spacing is 10.
so in 110px height menu there will be exactly 4 items (20*4+10*(4-1) == 110)

Let's suppose that we install grub2 into /boot/grub2 directory.

1. unpack test.tar.gz to /boot/grub2/test/
2. set GRUB_THEME="/boot/grub2/themes/test/theme.txt" in /etc/default/grub
3. run update-grub2 (or grub-mkconfig -o /boot/grub2/grub.cfg)
4. add "dummy" entries in /boot/grub2/grub.cfg if needed

so we see result on test_2.png - when we move to the bottom - the box of the 
selected item is not fully drawn



P.S.
Sorry, for the big message.

There are some more ideas how to make grub2 theming better. I really would 
like to discuss (the ones I've mentioned and the ones I did not).

I have working solutions for the ideas I had mentioned.

-- 
With best regards,
_______________________________
Vladimir Testov, ROSA Laboratory.
www.rosalab.ru

[-- Attachment #2: test_1.png --]
[-- Type: image/png, Size: 47072 bytes --]

[-- Attachment #3: test_2.png --]
[-- Type: image/png, Size: 46949 bytes --]

[-- Attachment #4: test.tar.gz --]
[-- Type: application/x-compressed-tar, Size: 10895 bytes --]

Re: fixed item-height, item-spacing, max-elements-shown etc

[Andrey Borzenkov]

Sorry for delay.

В Thu, 07 Feb 2013 14:07:08 +0400
Vladimir Testov <vladimir.testov@rosalab.ru> пишет:

> >I am not sure whether it is appropriate fix. After your patch "box
> >effect" on selected item is effectively lost (it makes
> >selected_item_box the same height as selected item itself).
> About "box effect" - there is no "box effect" in menu_box. The menu's box is 
> drawn inside given area. So the idea of "box effect" was unclear to me. (Also, 
> the height of selected element (with box) is item_height + box_pad_top + 
> box_pad_bottom - 1... that "-1" makes the "box effect" totally unclear. My idea 
> was to make GRUB theme be countable to pixel.

But it is. You know exactly which size you need. But in practice most
themes would probably use dynamic sizing by specifying screen
size percentage. I am not sure if grub2 currently dynamically adjusts
size to ensure whole number of items fit into it.  
 
>                                              Also, with my patch (when we can 
> have a box for every unselected element) item_spacing loses it's meaning 
> totally. It has uncertain meaning in current GRUB state too.
> 

It is pretty much certain meaning. Of course I agree that having more
detailed "style guide" for writing grub theme would definitely help.

As I understand current code, it lays out quite predictably

outer image top border
selected item box top border
item 1
inter-item space
item 2
iter-item space
...
item n
selected item box lower border
outer image lower border

selected item box itself is overlayed (or, rather, underlayed). It does
not change layout. So you, as theme author, has to ensure suitable
space to draw selected item box so that it fits without changing
existing layout.

Your patch changes text layout for selected item (by reducing available
space). As text is vertically aligned, this results in "jumping effect"
or part of text may be cut off.

In your example setting menu size to 118 provides for full 4 items +
selected item box.

> I thought that idea of menu's box was more valuable because it has influence to 
> the code.
> 
> So, when we need to calculate exact values so the result will request our 
> expectations, the idea of item_height as total height is more acceptable.
> 

selected item box is not really used to layout items. Only to adjust
where the first item is placed, to allow for full top border to be
displayed. Patch I just sent also adds space for lower border.

Anyway, this is just my 2c. Hopefully somebody with commit rights
or one of original theme authors can chime in :)

[PATCH] fix selected item's height

[Vladimir Testov]

[-- Attachment #1: Type: text/plain, Size: 525 bytes --]


> selected item box is not really used to layout items. Only to adjust
> where the first item is placed, to allow for full top border to be
> displayed. Patch I just sent also adds space for lower border.

By the way, your patch is not absolutely correct. We should correct selected 
item's height. The formula you have presented do not consider the fact that 
selected item's height is decreased by 1.

Patch included.

-- 
With best regards,
_______________________________
Vladimir Testov, ROSA Laboratory.
www.rosalab.ru

[-- Attachment #2: grub2-fix-selected-item-height.patch --]
[-- Type: text/x-patch, Size: 621 bytes --]

diff -Naur grub-2.00/grub-core/gfxmenu/gui_list.c grub-tmp/grub-core/gfxmenu/gui_list.c
--- grub-2.00/grub-core/gfxmenu/gui_list.c	2011-12-14 14:36:07.000000000 +0400
+++ grub-tmp/grub-core/gfxmenu/gui_list.c	2013-02-22 13:24:01.496967086 +0400
@@ -252,7 +252,7 @@
 	  int cwidth = oviewport.width - 2 * boxpad - 2;
 	  if (selbox->get_border_width)
 	    cwidth -= selbox->get_border_width (selbox);
-	  selbox->set_content_size (selbox, cwidth, item_height - 1);
+	  selbox->set_content_size (selbox, cwidth, item_height);
           selbox->draw (selbox, 0,
                         item_top - sel_toppad);
         }

Re: fixed item-height, item-spacing, max-elements-shown etc

[Vladimir Testov]

Thanks for the anwser! :)

On Thursday, February 21, 2013 09:56:07 PM Andrey Borzenkov wrote:
> It is pretty much certain meaning. Of course I agree that having more
> detailed "style guide" for writing grub theme would definitely help.

I'm just writing that kind of thing right now. With details and schemes. :) It 
will take some time to finish.

-- 
With best regards,
_______________________________
Vladimir Testov, ROSA Laboratory.
www.rosalab.ru

Re: fixed item-height, item-spacing, max-elements-shown etc

[Vladimir Testov]

[-- Attachment #1: Type: text/plain, Size: 152 bytes --]

Oh. One more white-space removed. That's better.
-- 
With best regards,
_______________________________
Vladimir Testov, ROSA Laboratory.
www.rosalab.ru

[-- Attachment #2: grub2-item-height-size-fix.patch --]
[-- Type: text/x-patch, Size: 1965 bytes --]

diff -Naur grub-2.00/grub-core/gfxmenu/gui_list.c grub-new/grub-core/gfxmenu/gui_list.c
--- grub-2.00/grub-core/gfxmenu/gui_list.c	2011-12-14 14:36:07.000000000 +0400
+++ grub-new/grub-core/gfxmenu/gui_list.c	2013-02-06 15:06:36.620696079 +0400
@@ -228,7 +228,9 @@
   grub_gfxmenu_box_t selbox = self->selected_item_box;
   int sel_leftpad = selbox->get_left_pad (selbox);
   int sel_toppad = selbox->get_top_pad (selbox);
-  int item_top = sel_toppad;
+  int sel_vertical_pad = sel_toppad
+                        + selbox->get_bottom_pad (selbox);
+  int item_top = 0;
   int menu_index;
   int visible_index;
   struct grub_video_rect oviewport;
@@ -252,9 +254,13 @@
 	  int cwidth = oviewport.width - 2 * boxpad - 2;
 	  if (selbox->get_border_width)
 	    cwidth -= selbox->get_border_width (selbox);
-	  selbox->set_content_size (selbox, cwidth, item_height - 1);
-          selbox->draw (selbox, 0,
-                        item_top - sel_toppad);
+          int cheight = item_height - sel_vertical_pad;
+          if (cwidth < 1)
+            cwidth = 1;
+          if (cheight < 1)
+            cheight = 1;
+          selbox->set_content_size (selbox, cwidth, cheight);
+          selbox->draw (selbox, 0, item_top);
         }
 
       icon = get_item_icon (self, menu_index);
@@ -391,9 +397,6 @@
       int box_bottom_pad = box->get_bottom_pad (box);
       unsigned width_s;
 
-      grub_gfxmenu_box_t selbox = self->selected_item_box;
-      int sel_toppad = selbox->get_top_pad (selbox);
-      
       *width = grub_font_get_string_width (self->item_font, "Typical OS");
       width_s = grub_font_get_string_width (self->selected_item_font,
 					    "Typical OS");
@@ -406,7 +409,7 @@
       *height = (item_height * num_items
                  + item_vspace * (num_items - 1)
                  + 2 * boxpad
-                 + box_top_pad + box_bottom_pad + sel_toppad);
+                 + box_top_pad + box_bottom_pad);
     }
   else
     {

Re: fixed item-height, item-spacing, max-elements-shown etc

[Vladimir Testov]

[-- Attachment #1: Type: text/plain, Size: 127 bytes --]

Here it is. :) (fixed)

-- 
With best regards,
_______________________________
Vladimir Testov, ROSA Laboratory.
www.rosalab.ru

[-- Attachment #2: grub2-item-height-size-fix.patch --]
[-- Type: text/x-patch, Size: 2107 bytes --]

diff -Naur grub-2.00/grub-core/gfxmenu/gui_list.c grub-new/grub-core/gfxmenu/gui_list.c
--- grub-2.00/grub-core/gfxmenu/gui_list.c	2011-12-14 14:36:07.000000000 +0400
+++ grub-new/grub-core/gfxmenu/gui_list.c	2013-02-06 14:11:24.536849714 +0400
@@ -228,7 +228,9 @@
   grub_gfxmenu_box_t selbox = self->selected_item_box;
   int sel_leftpad = selbox->get_left_pad (selbox);
   int sel_toppad = selbox->get_top_pad (selbox);
-  int item_top = sel_toppad;
+  int sel_vertical_pad = sel_toppad
+                        + selbox->get_bottom_pad (selbox);
+  int item_top = 0;
   int menu_index;
   int visible_index;
   struct grub_video_rect oviewport;
@@ -247,14 +249,19 @@
       int is_selected = (menu_index == self->view->selected);
       struct grub_video_bitmap *icon;
 
+
       if (is_selected)
         {
 	  int cwidth = oviewport.width - 2 * boxpad - 2;
 	  if (selbox->get_border_width)
 	    cwidth -= selbox->get_border_width (selbox);
-	  selbox->set_content_size (selbox, cwidth, item_height - 1);
-          selbox->draw (selbox, 0,
-                        item_top - sel_toppad);
+          int cheight = item_height - sel_vertical_pad;
+          if (cwidth < 1)
+            cwidth = 1;
+          if (cheight < 1)
+            cheight = 1;
+          selbox->set_content_size (selbox, cwidth, cheight);
+          selbox->draw (selbox, 0, item_top);
         }
 
       icon = get_item_icon (self, menu_index);
@@ -391,9 +398,6 @@
       int box_bottom_pad = box->get_bottom_pad (box);
       unsigned width_s;
 
-      grub_gfxmenu_box_t selbox = self->selected_item_box;
-      int sel_toppad = selbox->get_top_pad (selbox);
-      
       *width = grub_font_get_string_width (self->item_font, "Typical OS");
       width_s = grub_font_get_string_width (self->selected_item_font,
 					    "Typical OS");
@@ -406,7 +410,7 @@
       *height = (item_height * num_items
                  + item_vspace * (num_items - 1)
                  + 2 * boxpad
-                 + box_top_pad + box_bottom_pad + sel_toppad);
+                 + box_top_pad + box_bottom_pad);
     }
   else
     {

Re: fixed item-height, item-spacing, max-elements-shown etc

[Vladimir Testov]

Sorry, misprint. Should be "horizontal_pad" rather than "vertical_pad" in the patch included.

Re: fixed item-height, item-spacing, max-elements-shown etc

[Andrey Borzenkov]

В Tue, 5 Feb 2013 23:21:05 +0300 (MSK)
Vladimir Testov <vladimir.testov@rosalab.ru> пишет:

> Sorry, misprint. Should be "horizontal_pad" rather than "vertical_pad" in the patch included.
> 


Could you resend it without unrelated white space changes?

fixed item-height, item-spacing, max-elements-shown etc

[Vladimir Testov]

[-- Attachment #1: Type: text/plain, Size: 1484 bytes --]

I'm going to write some patches for GRUB2 - mainly for theme handling.

First one - I've faced some bug: maximum number of displayed boot elements is counted in presumption that element's height is item_height and distance between elements is item_spacing. Nevertheless, this conceptions does not work during menu drawing if we have bitmap box of the selected element. Real height of the element will be (item_height + box_pad_top + box_pad_bottom - 1), where box_pad_top and box_pad_bottom are top and bottom dimensions of bitmap box. So we will have strange difficulties with understanding of what will be really drawn and what menu height we should use.

The patch fixes these problemes. We will have item_height in every case. So, there will be no troubles with item_height, item_spacing and menu height. These three variables now mean what they suppose to mean.

Would be glad if this patch will be accepted. I also have many ideas how to improve GRUB theming functionality. Some of them are completed and I can send them as patches as soon as this patch is accepted.

Should I create update-request in bugzilla?

If something in the patch is not clear - you can ask me.

Also, some time ago I've sent a patch which adds new option - item_pixmap_style for bitmap box of unselected elements. It was not accepted but I didn't received any feedback. With whom should I discuss this patch (now I want to modify it, if the current patch will be accepted).

Thanks for your attention.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: grub2-item-height-size-fix.patch --]
[-- Type: text/x-patch; name=grub2-item-height-size-fix.patch, Size: 2267 bytes --]

diff -Naur grub-2.00/grub-core/gfxmenu/gui_list.c grub-new/grub-core/gfxmenu/gui_list.c
--- grub-2.00/grub-core/gfxmenu/gui_list.c	2011-12-14 14:36:07.000000000 +0400
+++ grub-new/grub-core/gfxmenu/gui_list.c	2013-02-03 13:51:17.038051628 +0400
@@ -227,8 +227,10 @@
 
   grub_gfxmenu_box_t selbox = self->selected_item_box;
   int sel_leftpad = selbox->get_left_pad (selbox);
+  int sel_vertical_pad = sel_leftpad
+                        + selbox->get_right_pad (selbox);
   int sel_toppad = selbox->get_top_pad (selbox);
-  int item_top = sel_toppad;
+  int item_top = 0;
   int menu_index;
   int visible_index;
   struct grub_video_rect oviewport;
@@ -247,14 +249,19 @@
       int is_selected = (menu_index == self->view->selected);
       struct grub_video_bitmap *icon;
 
+
       if (is_selected)
         {
-	  int cwidth = oviewport.width - 2 * boxpad - 2;
-	  if (selbox->get_border_width)
-	    cwidth -= selbox->get_border_width (selbox);
-	  selbox->set_content_size (selbox, cwidth, item_height - 1);
-          selbox->draw (selbox, 0,
-                        item_top - sel_toppad);
+          int cwidth = oviewport.width - 2 * boxpad - 2;
+          if (selbox->get_border_width)
+            cwidth -= selbox->get_border_width (selbox);
+          int cheight = item_height - sel_vertical_pad;
+          if (cwidth < 1)
+            cwidth = 1;
+          if (cheight < 1)
+            cheight = 1;
+          selbox->set_content_size (selbox, cwidth, cheight);
+          selbox->draw (selbox, 0, item_top);
         }
 
       icon = get_item_icon (self, menu_index);
@@ -391,9 +398,6 @@
       int box_bottom_pad = box->get_bottom_pad (box);
       unsigned width_s;
 
-      grub_gfxmenu_box_t selbox = self->selected_item_box;
-      int sel_toppad = selbox->get_top_pad (selbox);
-      
       *width = grub_font_get_string_width (self->item_font, "Typical OS");
       width_s = grub_font_get_string_width (self->selected_item_font,
 					    "Typical OS");
@@ -406,7 +410,7 @@
       *height = (item_height * num_items
                  + item_vspace * (num_items - 1)
                  + 2 * boxpad
-                 + box_top_pad + box_bottom_pad + sel_toppad);
+                 + box_top_pad + box_bottom_pad);
     }
   else
     {

Re: fixed item-height, item-spacing, max-elements-shown etc

[Andrey Borzenkov]

В Tue, 5 Feb 2013 19:44:14 +0300 (MSK)
Vladimir Testov <vladimir.testov@rosalab.ru> пишет:

> I'm going to write some patches for GRUB2 - mainly for theme handling.
> 
> First one - I've faced some bug: maximum number of displayed boot elements is counted in presumption that element's height is item_height and distance between elements is item_spacing. Nevertheless, this conceptions does not work during menu drawing if we have bitmap box of the selected element. Real height of the element will be (item_height + box_pad_top + box_pad_bottom - 1), where box_pad_top and box_pad_bottom are top and bottom dimensions of bitmap box. So we will have strange difficulties with understanding of what will be really drawn and what menu height we should use.
> 
> The patch fixes these problemes. We will have item_height in every case. So, there will be no troubles with item_height, item_spacing and menu height. These three variables now mean what they suppose to mean.
> 

I am not sure whether it is appropriate fix. After your patch "box
effect" on selected item is effectively lost (it makes
selected_item_box the same height as selected item itself).

The actual fix looks much simpler - get_num_shown_items() should
additionally account for self->selected_item_box->get_op_pad().

Could you make available theme that demonstrates this problem?

> Would be glad if this patch will be accepted. I also have many ideas how to improve GRUB theming functionality. Some of them are completed and I can send them as patches as soon as this patch is accepted.
> 
> Should I create update-request in bugzilla?
> 
> If something in the patch is not clear - you can ask me.
> 
> Also, some time ago I've sent a patch which adds new option - item_pixmap_style for bitmap box of unselected elements. It was not accepted but I didn't received any feedback. With whom should I discuss this patch (now I want to modify it, if the current patch will be accepted).
> 
> Thanks for your attention.