Created attachment 304208 [details] [review]
Proposed solution

I may have missed something but this just seems to work.

Review of attachment 304208 [details] [review]:

::: gtk/gtktextattributes.h
@@ +185,2 @@
   gint right_margin;
+  gint bottom_margin;

I'm afraid we can't do it this way - GtkTextAppearance is a (semi-)public struct, so we can't just add stuff in the middle. The only way this might work is to use up the two ints of padding at the end.

Review of attachment 304208 [details] [review]:

::: gtk/gtktextlayout.c
@@ +2241,3 @@
 
+  is_last = line == _gtk_text_btree_get_end_iter_line (
+      _gtk_text_buffer_get_btree (layout->buffer));

That line break needs work :-)

I tend to just do long lines these days, instead of awkward line breaks.

Any particular reason why the CSS attributes cannot be used? If this is the "internal margin" of the text from the widget's border, then this looks like a job for the CSS padding.

There's also the weird API angle, but that's a pre-existing condition — GtkWidget has set_margin_*(), and GtkTextView, which is-a GtkWidget, has set_*_margin().

I also have a patch for this but not only this :
it can use css paddings in a cumulative way with margins.

IIRC, i also have re-worked the gtk_text_view_scroll_to_iter logic.

Last time i have worked on it, there was still some minor display issues in the view and gsv gutters too, so it need work, pixel cache can be tricky to work with so maybe Christian can help with this...

Created attachment 304272 [details] [review]
gtktextview paddings and margins

Created attachment 304392 [details] [review]
Proposed solution after Matthias' review

Not the final solution because I should also merge Sébastien's patch.

This patch also adds some info about the new members of GtkTextAttributes added in 3.16.

Created attachment 304395 [details] [review]
gtktextview paddings and margins (by Sébastien Lafargue, with only minor changes)

I have updated this patch slightly to match the current HEAD, fixed some obvious errors and removed some obvious garbage. Sébastien, please check if I have not removed any valuable content by mistake.

With this update it should be easier to merge the patches.

Rafal:

thanks for cleanup and rebase of my old patch, i'll try to take a look at it in a few days

(In reply to Emmanuele Bassi (:ebassi) from comment #4)
> Any particular reason why the CSS attributes cannot be used? [...]

Sébastien's patch implements this feature.

(In reply to Emmanuele Bassi (:ebassi) from comment #5)
> There's also the weird API angle, but that's a pre-existing condition —
> GtkWidget has set_margin_*(), and GtkTextView, which is-a GtkWidget, has
> set_*_margin().

gtk_widget_set_margin_*() applies to the margin around the widget while gtk_text_view_set_*_margin() applies to the margin around the text, inside the widget. Weird but we can't change it.

(In reply to Rafal Luzynski from comment #11)
> (In reply to Emmanuele Bassi (:ebassi) from comment #4)
> > Any particular reason why the CSS attributes cannot be used? [...]
> 
> Sébastien's patch implements this feature.

More or less; it also adds API to change the margins programmatically, which is what I'd like to avoid.

> (In reply to Emmanuele Bassi (:ebassi) from comment #5)
> > There's also the weird API angle, but that's a pre-existing condition —
> > GtkWidget has set_margin_*(), and GtkTextView, which is-a GtkWidget, has
> > set_*_margin().
> 
> gtk_widget_set_margin_*() applies to the margin around the widget while
> gtk_text_view_set_*_margin() applies to the margin around the text, inside
> the widget. Weird but we can't change it.

I know, that also why I think using the term 'margin' is incorrect: it's really padding, since it changes the contents of the text box.

Personally, I think we should only use the CSS attributes and deprecate the existing API, without adding new API in the process.

OK, now I understand what you mean: so you want CSS padding as the only solution; you want no new gtk_text_view_(get|set)_(top|bottom)_margin() API and you want to deprecate gtk_text_view_(get|set)_(left|right)_margin(). I'd like to ask other people what they think about it.

I admit I have not followed closely this bug and I agree that the current "margin" API of textview has a very unfortunate naming. However I think it has different semantics from the css.

One thing we could do is to deprecate the old api and rename it to view_set_gutter_size (I'd appreciate if it was not set/get_gutter since that would stomp on existing gtksourceview api)

I don't see how this has anything to do with 'gutter', really.

I could see renaming it to padding, if that harmonizes things with css box model terminology. But this will have wider consequences, we also have GtkTextTag::left/right-margin, and GtkTextTag::accumulative-margin.

Not having api for this and 'just using css' will get us right back into the 'is it theme or application api' discussion, which I'd personally rather avoid.

(In reply to Matthias Clasen from comment #15)
> I don't see how this has anything to do with 'gutter', really.
> 

Well, if I remember correctly this is the "text" margin, so if you are using gtk_text_view_set_border_window to show e.g. line numbers, this margin will not map to the css box model, because it would not be on the outer edget of the widget. Unless you consider it the css of GtkTextView.view

in the most complete case you would have:

<widget margin><widget border><widget padding><line numbers><text margin><actual text>

That's why I was proposing to use a different term (gutter or whatever). Maybe even aliasing it to gtk_text_view_set_left_text_margin would help a bit?

I've tried the latest patch here, and found that after horizontal scrolling, the left css padding is lost.

Pushed a slightly fixed up version of the second patch.

Sorry for reopening, but the implementation of padding is broken and I would rather not have it than have a broken version.

 - Open gedit
 - Enable the line numbers
 - Open the inspector
 - type in the CSS tab GtkTextView { padding: 100; }

The padding is applied *inside* the widget, between the line numbers and the text.

If we cannot (yet) support proper padding around the widget, I think at least we should add the .view class when reading the padding from the style context, so that such padding is applied only if you specify GtkTextView.view { padding: 100; } in the css

mmm... actually that seems already be the case

+  gtk_style_context_save (style_context);
+  gtk_style_context_add_class (style_context, GTK_STYLE_CLASS_VIEW);

but the experiment with the inspector still seems to do what I described above...

Thinking more about it after a coffee, I guess that is normal since we just add a class for our "subwidgets" and so a style specified for the widget type is also applied to all subwidgets (if they support it).

This is a bit unfortunate and we will have to be careful in how we use this feature... Maybe we can document it somehow?

Anyway I am re-closing this.

(In reply to Paolo Borelli from comment #21)
> Thinking more about it after a coffee, I guess that is normal since we just
> add a class for our "subwidgets" and so a style specified for the widget
> type is also applied to all subwidgets (if they support it).
> 
> This is a bit unfortunate and we will have to be careful in how we use this
> feature... Maybe we can document it somehow?

We discussed this at the GTK BoF at GUADEC, and in the near future we're going to switch to "element names" instead of classes in our selectors; this should at least ensure that you don't get bit by inheritance.

We definitely need to improve the documentation in the CSS reference, to make it more explicit that selectors on type names will propagate to all the sub-types.

(In reply to Paolo Borelli from comment #19)
> Sorry for reopening, but the implementation of padding is broken and I would
> rather not have it than have a broken version.
> 
>  - Open gedit
>  - Enable the line numbers
>  - Open the inspector
>  - type in the CSS tab GtkTextView { padding: 100; }
> 
> The padding is applied *inside* the widget, between the line numbers and the
> text.

If you put the line numbers in GTK_TEXT_WINDOW_LEFT, that is exactly as intended, at least from my perspective. The text view internal structure with its 5 text windows is a bit too complex to map 1:1 to a single css box.

The intention here was to make it possible to do what margin-left/margin-right do today, but globally, from the theme, and make it work for top and bottom to.

As we get on with the css node transition, I expect that we may end up having separate css nodes for each text window, at which point the line numbers will get padding capabilities as well.