GNOME Bugzilla – Bug 471920
Expose the border width property
Last modified: 2009-12-03 17:55:57 UTC
At the moment vte hard codes a border (VTE_PAD_WIDTH) around the terminal contents. It would seem sensible to expose this either as a style property, cf GtkButton's "inner-border", or as part of the API, cf GtkContainer's border_width.
I was bitten by this nasty bug/feature. It took me some time to figure out why bitmap font with 16 pixel height won't fit on 800x480 screen even in fullscreen. See http://lists.maemo.org/pipermail/maemo-developers/2008-August/034534.html Same happens in Gnome terminal in Ubuntu. Due to current 1 pixel wide border you simply can't use whole screen height when in fullscreen and always get empty line on the bottom. This should be made configurable at runtime and preferably set to 0 not 1 as the border can be done in container if needed. Also making this settable at runtime allows to implement centering when font size does not match widget size exactly. Or at least splitting VTE_PAD_WIDTH into two constants (VTE_PAD_X, VTE_PAD_Y) would be nice. It would make vte.c more readable and allow setting border differently for left/right vs top/bottom. 1 pixel border is tolerable for left/right but not for top/bottom. See also http://www.internettablettalk.com/forums/showthread.php?t=17002&page=5
I see the downstream patch at http://fanoush.wz.cz/maemo/vte4.border.patch , hopefully upstream is also interested in that...
[Just for reference, the original reason the padding was added is bug 89048.] I'm not sure it makes sense to have different X and Y paddings... Since we're in API freeze right now, any solution must wait for gnome 2.25.
(In reply to comment #3) > [Just for reference, the original reason the padding was added is bug 89048.] > > I'm not sure it makes sense to have different X and Y paddings... > It makes sense because on sides it is less tolerable (also the original bug 89048 complained about left border, not top or bottom) and you often have scrollbar on the side so you can finetune its width to use whole 'wasted' space and can thus freely afford 1 pixel border on sides. OTOH with border on top/bottom you have ugly empty line since almost any font won't fit. For many resolutions display height-2 is not divisible by any reasonable font height. In my case 480-2 = 478 = 2 x 239. 239 is prime, you can try e.g. http://www.easycalculation.com/prime-factor.php for other resolutions. My first patch simply redefined VTE_PAD_WIDTH to 0 first but I immediatelly noticed similar thing reported in bug 89048 so I took the pain of searching sources and replacing one constant by two according to width/height/x/y context. As for top/bottom - at least the font I am using http://www.fixedsysexcelsior.com has proper padding there (maybe most fonts have it?) so top/bottom border is not needed at all in my case. That's why I ended with X set to 1 and Y set to 0 in final patch. Or as suggested previously the solution is also to remove the padding altogether and let the container embedding vte widget solve it as needed, it knows better what to do. Hardcoded border with no way around it is bad.
IMHO either an inner-border property like comment 0 proposes, or removing this altogether and relying on embedding the terminal in a container that sets the borders (e.g. GtkAlignment) makes the most sense. The disadvantage of the latter would be that the border wouldn't be (by default) in the background colour of the terminal.
(In reply to comment #5) > IMHO either an inner-border property like comment 0 proposes, or removing this > altogether and relying on embedding the terminal in a container that sets the > borders (e.g. GtkAlignment) makes the most sense. The disadvantage of the > latter would be that the border wouldn't be (by default) in the background > colour of the terminal. The terminal background color issue is real though. I actually like the separate X and Y padding. I can imagine one may want to have a zero X padding such that an exact number of chars fit in a small screen, but have some Y room for underline etc to show properly.
Behdad: so I'd like to just make this a GTK_TYPE_BORDER style property instead of 2 separate border-x, border-y INT properties. Any objections? :)
No. Go ahead.
Created attachment 148795 [details] [review] patch I'd appreciate if you double-checked that I converted each VTE_PAD_WIDTH to the right inner_border side. Esp. in _vte_invalidate_cells() which I don't really understand what exactly it's doing with the rectangles and the "2" and "3" constants. (The exaggerated default border values are just for testing; the final patch will use the default of 1, 1, 1, 1 of course.)
First one: @@ -3136,13 +3143,17 @@ vte_sequence_handler_window_manipulation (VteTerminal *terminal, GValueArray *pa /* Send window size, in pixels. */ g_snprintf(buf, sizeof(buf), _VTE_CAP_CSI "4;%d;%dt", - widget->allocation.height - 2 * VTE_PAD_WIDTH, - widget->allocation.width - 2 * VTE_PAD_WIDTH); + widget->allocation.height - + (terminal->pvt->inner_border.left + + terminal->pvt->inner_border.right), + widget->allocation.width - + (terminal->pvt->inner_border.top + + terminal->pvt->inner_border.bottom)); _vte_debug_print(VTE_DEBUG_PARSE,
@@ -6847,8 +6881,10 @@ vte_terminal_motion_notify(GtkWidget *widget, GdkEventMotion *event) x, y, FALSE, FALSE); /* Start scrolling if we need to. */ - if (event->y < VTE_PAD_WIDTH || - event->y >= terminal->row_count * height + VTE_PAD_WIDTH) + if (event->y < terminal->pvt->inner_border.top || + event->y >= terminal->row_count * height + + (terminal->pvt->inner_border.top + + terminal->pvt->inner_border.bottom)) Shouldn't add the .bottom here.
+ if (gtk_widget_get_direction(widget) == GTK_TEXT_DIR_RTL) { + int tmp; + + /* swap left/right border */ + tmp = inner_border.left; + inner_border.left = inner_border.right; + inner_border.right = tmp; + } Why? Not sure we should do this. Is this what Gtk+ does with GtkBorder? left/right are clearly absolute... I understand that we've changed that in many places in Gtk/Pango, but any reason to do that here? Vte doesn't really support RTL...
(In reply to comment #9) > Created an attachment (id=148795) [details] [review] > patch > > I'd appreciate if you double-checked that I converted each VTE_PAD_WIDTH to the > right inner_border side. Esp. in _vte_invalidate_cells() which I don't really > understand what exactly it's doing with the rectangles and the "2" and "3" > constants. (The exaggerated default border values are just for testing; the > final patch will use the default of 1, 1, 1, 1 of course.) Yeah, the -1, 2, and 3 there are all magic. The aim is simply to redraw adjacent cells too, such that overlapping glyphs (and underline) render properly. It's a PITA that we have to do that. But should work. Now why's it 2 and 3 and not 1, I have no idea. I think ickle did this. May have been put there to mask some other bug in the grid-cell extent calculations... But your patch looks good otherwise. Thanks.
Fixed; thanks! (In reply to comment #12) > Why? Not sure we should do this. Is this what Gtk+ does with GtkBorder? > left/right are clearly absolute... I understand that we've changed that in > many places in Gtk/Pango, but any reason to do that here? Vte doesn't really > support RTL... I thought that because the widgets are packed RTL that what is on the left in LTR should be on the right in RTL (and vv.) even though the widget contents themselves aren't mirrored. Looking at examples in gtk+ (the ones which don't just use the sum of left+right) this doesn't seem to be done this way. So it looks like this could use clarification in gtk+; for now I'll take this bit out of vte.
(In reply to comment #14) > Fixed; thanks! > > (In reply to comment #12) > > Why? Not sure we should do this. Is this what Gtk+ does with GtkBorder? > > left/right are clearly absolute... I understand that we've changed that in > > many places in Gtk/Pango, but any reason to do that here? Vte doesn't really > > support RTL... > > I thought that because the widgets are packed RTL that what is on the left in > LTR should be on the right in RTL (and vv.) even though the widget contents > themselves aren't mirrored. Looking at examples in gtk+ (the ones which don't > just use the sum of left+right) this doesn't seem to be done this way. So it > looks like this could use clarification in gtk+; for now I'll take this bit out > of vte. In general you're right. Historically things been named left/right, so keeping the name, we ended up switching the meaning in RTL environments. If the mapping is desired, it needs to happen somewhere indeed. But since vte doesn't deal with that stuff anywhere else, I'd rather keep left/right with their literal meaning.
Committed to master.