Created attachment 223860 [details] [review]
The patch

A properly formatted patch

Created attachment 228754 [details] [review]
The patch

Fix documentation, fix patch formatting (now I hope it's really properly formatted).

(In reply to comment #0)
> Also fixes non-breaking
> spaces still being shown as normal spaces if NBSP flag was off but SPACE flag
> was on.

Please provide a separate patch for this bug fix, if it is still relevant for the current code.

Review of attachment 228754 [details] [review]:

I wonder if we can instead combine different flags. Have an enum for leading, internal and trailing spaces. And have another enum for the different types of spaces. And be able to e.g. combine leading spaces with tabs, so only leading tabs are shown.

With such a solution, what to do with line breaks? Should they always be combined with the trailing flag?

Is it possible to implement this solution in a compatible way with the current API? If not, it's not a big problem to deprecate the current API, if the new API is better.

Combine enums how? Like this?

typedef enum
{
  SPACE   = 0,
  NBSP    = 1,
  TAB     = 2,
  NEWLINE = 3
} SpaceType;

typedef enum
{
  LEADING  = 0,
  TRAILING = 1,
  INTERNAL = 2,
  N_SPACE_TYPES
} SpaceLocation;

#define COMBINE(s, w)  ((1 << ((s) * N_SPACE_TYPES)) << (w))

int flags = COMBINE(TAB, LEADING) | COMBINE(NEWLINE, TRAILING);

No, what I mean is to have other functions, like:

> void
> gtk_source_view_set_draw_spaces_for_type (GtkSourceView *view,
>                                           GtkSourceSpaceType space_type,
>                                           GtkSourceSpaceLocation space_location);
>
> GtkSourceSpaceLocation
> gtk_source_view_get_draw_spaces_for_type (GtkSourceView *view,
>                                           GtkSourceSpaceType space_type);

Or inversely, set or get the visible space types in function of the space location.

With 'ALL' values available in the SpaceType and SpaceLocation enums, for convenience.

This idea has the advantage to easily add a space type or space location, without having an explosion of flags in GtkSourceDrawSpacesFlags.

I think it is worth exploring the idea. But it's less convenient to use when we don't care about the location (that's why I propose a 'ALL' value in the enum).

And the stored settings would not be easily available with properties. One property per location or per space type would be required.

I wanted to implement it like that in the first place, but people said that it's bad and I should add flags instead. With new enums in place, what should the old function do?

If possible, it's better to extend the current API instead of creating a new one and deprecate the other. But having only one enum is not convenient, and is less extensible (above 32 flags I think the enum overflows).

In the future, it would be nice if Pango and GtkTextView can draw the spaces directly (see bug #721014), instead of calling gtk_text_view_get_iter_location() to locate the spaces and draw the symbols afterwards.

The API in GtkTextView is free. What we can do is to add directly the API in GtkTextView, deprecate the current GtkSourceView API, and implement the Pango space drawing later. Or we can also wait the Pango stuff before doing anything in GtkTextView.

(In reply to comment #8)
> With new enums in place, what should the old function do?

The 'set' function uses internally the new API, and also store the GtkSourceDrawSpacesFlags.
The 'get' function simply returns the GtkSourceDrawSpacesFlags stored with the last 'set' function. If the 'set' function from the new API is used, it is not taken into account for the old 'get' function.

Created attachment 287787 [details] [review]
Fine-grained control for drawing whitespaces. https://bugzilla.gnome.org/show_bug.cgi?id=683678

* Added new enums GtkSourceSpaceLocation and GtkSourceSpaceType.
* Added new functions for setting the location where a given whitespace should be drawn
* The old function set_draw_spaces uses the new function internally
* Added convenience values for ALL locations ans ALL space types
This will have several benefits, including the possibility to add a Selection location
to only show the whitespaces in selected text.

@Sébastien: Find attached my first try for the fine-grained whitespace drawing control, a review would be welcome. Dev-related question: how can I use (see the results) the PROFILE macro used in drawing function?

(In reply to comment #12)
> Dev-related question: how can I use (see
> the results) the PROFILE macro used in drawing function?

At the top of gtksourceview.c, uncomment "#define ENABLE_PROFILE" and comment "#undef ENABLE_PROFILE".

I'll have a closer look at the patch tomorrow, I haven't thought myself about how to implement this, so at first I was a bit surprised by the hash table, but yes, it probably makes sense.

For the coding style, please add curly braces around one-line blocks too. Some old code doesn't always do that, but for new code it's better (see the file HACKING for the rationale).

In the header, the function parameters should be aligned.

First, there should be at least one use case where the new API would be useful. For example:
- for spaces inside the text, draw all space types except normal spaces;
- for trailing spaces, draw all spaces;
- for spaces inside selection, draw all spaces.

This is not possible with the current API.

I think it's more convenient to specify the types of spaces to draw for each location. So having the functions set_draw_spaces_for_location() and get_draw_spaces_for_location(). Think also about a configuration tool: you can have a combo box to select the location, and below have check boxes for each space type.

With this solution, GtkSourceSpaceLocation can be a normal enum (0, 1, 2, etc) with GTK_SOURCE_SPACE_LOCATION_ALL as 0 so the enum can be expanded at the end. But I'm not sure it's a good solution. It's probably safer to have flags for both enums, so the flags can be combined in any order. Also, with flags it's possible to set several locations at once with set_draw_spaces_for_location(). With a normal enum for the locations, the implementation is simpler (an array can store the space types for the different locations). But the API is more important than the implementation details of course, so flags for both enums is better.

Another thing, GTK_SOURCE_SPACE_LOCATION_SELECTION could be combined with the other locations, for example trailing spaces inside a selection. But it becomes too complicated, taking the selection as one location is easier.

Review of attachment 287787 [details] [review]:

::: gtksourceview/gtksourceview.c
@@ +148,2 @@
 	GtkSourceDrawSpacesFlags draw_spaces;
+	GHashTable      *draw_spaces_at_location;

Add a comment to explain what are the types of the keys and the values.
It should be SpaceLocation -> SpaceType (with gint <-> gpointer conversions).

@@ +1075,3 @@
+	if (view->priv->draw_spaces_at_location)
+	{
+		g_hash_table_destroy (view->priv->draw_spaces_at_location);

like a gobject, use g_hash_table_unref().

@@ +2119,3 @@
+	/* tab */
+	if (c == '\t')
+		result = GTK_SOURCE_SPACE_TYPE_TAB;

curly braces around one-like blocks.

@@ +3973,3 @@
+
+	GHashTableIter iter;
+	gpointer key;

Variable declarations must be at the beginning of the block.

@@ +4008,3 @@
+	{
+		if (GPOINTER_TO_INT (key) | type)
+			result = result | GPOINTER_TO_INT (value);

I'm not sure it's a good solution to add all the results.
There should maybe be a check to verify that only one @type (or one location) is given. For example when one type (or location) matches, store the result and continue the iteration, if another type/location matches, do a g_return_val.

::: gtksourceview/gtksourceview.h
@@ +137,3 @@
+ * @GTK_SOURCE_SOURCE_SPACE_TYPE_ALL: wheter all kind of spaces should be drawn.
+ *
+ * GtkSourceSpaceType determine what kind of spaces whould be drawn.

For the documentation of the enums, don't say that it is for the drawing. A SpaceType can be used for other features. Also, "whould" is a typo (but it was there before).

@@ +243,3 @@
+							(GtkSourceView   *view,
+							 GtkSourceSpaceType type,
+							 GtkSourceSpaceLocation location);

Align the parameters here.
Also, change the function as set_draw_spaces_for_location().

Also, do not forget the "Since: 3.16" in the GTK-Doc comments. And in another commit, deprecate the old enum, functions and property.

Thanks for the time for following up on this. It's mostly clear what I have to do based on the review, but I'm not sure I understand the tasks from your comment before the review, comment 15: Let us clarify if I did get it right, the proposed implementation uses enums with flags, is that OK? Should I add the *spaces_for_location methods instead of the *spaces_for_type methods?

Regarding configuration, I have been thinking of an interface to test this, the test-widget test seems a nice place to have this in, I just wanted to make sure the API is fine before extending the test (in another commit).

Will follow up with a next set of patches soon anyway, hopefully closer to the implementation you expect for this.

Sorry if I was not clear, when writing the comment I was at the same time thinking about the API/implementation.

I think flags for both enums is better, yes.
And spaces_for_location() seems more useful and more natural than spaces_for_type().

For the configuration, there is also a gedit plugin. It's probably easier to modify just the gedit plugin, but if you want to update test-widget, add for example a button that open a dialog window containing the combo box + check boxes, so it doesn't clutter the main window.

Created attachment 288253 [details] [review]
Fine-grained control for drawing whitespaces. https://bugzilla.gnome.org/show_bug.cgi?id=683678

* Added new enums GtkSourceSpaceLocation and GtkSourceSpaceType.
* Added new functions for setting the types of whitespaces to show
per location.
* The old function set_draw_spaces uses the new function internally.
* Added convenience values for ALL locations ans ALL space types
This will have several benefits, including the possibility to add a Selection location
to only show the whitespaces in selected text.

Attached the second version of the patch, hopefully with all the review requests fulfilled. (Deprecation for the old functions will come after this one gets a positive review). Please comment, ask, If there's anything else you would like to see. I just realized that I forgot the SELECTION location enum value there, the implementation for that will come in another patch, and in the context of another bug, fairly soon after this part gets accepted.

Review of attachment 288253 [details] [review]:

I didn't do a thorough review for the first patch, here are more details, mainly for the code style.

::: gtksourceview/gtksourceview.c
@@ +148,3 @@
 	GtkSourceDrawSpacesFlags draw_spaces;
+	/* Mapping for GtkSourceSpaceLocation -> GtkSourceSpaceType */
+	/* with gint <-> gpointer conversions */

A single comment is better in my opinion. Something like:
/* blah
 * foobar
 */

@@ +984,3 @@
 	view->priv->right_margin_overlay_color = NULL;
 	view->priv->spaces_color = NULL;
+	view->priv->draw_spaces_at_location = g_hash_table_new (NULL,

With all the inserts() below it's better to have a function init_draw_spaces_at_location().

@@ +2121,2 @@
+	/* tab */
+	if (c == '\t')

Not really a useful comment. Reading the code is sufficient here, also for the next comments in the same function.

@@ +2151,3 @@
+	/* not a whitespace */
+	if (type == 0)
+		return FALSE;

Curly braces.

@@ +2154,3 @@
+
+	stored_for_location =  GPOINTER_TO_INT (g_hash_table_lookup (view->priv->draw_spaces_at_location,
+							    GINT_TO_POINTER (location)));

There are too spaces after the = (yes I have good eyes ;)
But more importantly, the parameters should be aligned on the parenthesis (or is it not well displayed in bugzilla?).

@@ +2156,3 @@
+							    GINT_TO_POINTER (location)));
+	/* check if the given whitespace should be drawn at the given location */
+	return type & stored_for_location;

I would remove this comment too, there is no given whitespace, just a whitespace type. And the name of the function is enough to know all what the function does.

@@ +2163,3 @@
+			    GtkTextIter        *iter,
+			    GtkTextIter        *leading,
+			    GtkTextIter        *trailing)

Nitpick, there should be only one column separation between types and parameter names.

@@ +2183,2 @@
 	}
+	return (GtkSourceSpaceLocation) flags;

Declare the variable with the GtkSourceSpaceLocation type directly.

@@ -2472,3 @@
-		if (view->priv->draw_spaces != 0)
-		{
-			draw_tabs_and_spaces (view, cr);

The "if" has been remove here. Is there a particular reason to not adapt the condition? It seems that draw_tabs_and_spaces() does the actual drawing, so if space drawing is disabled it's better to avoid calling this function I think (since it is quite slow).

@@ +3958,3 @@
+ * gtk_source_view_set_draw_spaces_for_location:
+ * @view: a #GtkSourceView.
+ * @location: a #GtkSourceSpaceLocation so set the rule for

There should maybe be a description of the function (not only documenting the parameters) to explain that @location can contain several locations. The "location(s)" at the next parameter gives a hint about that, but it's not sufficiently explicit.

@@ +3982,3 @@
+		}
+	}
+	gtk_widget_queue_draw (GTK_WIDGET (view));

Add a blank line after the end of the while block. It's important to well space out the code for better readability. Remember: optimize the time to _read_ the code, not to _write_ it, because a code is read far more often than it is written.

@@ +3989,3 @@
+ * @view: a #GtkSourceView.
+ * @location: the #GtkSourceSpaceLocation to get the drawable spaces for
+ * Returns a #GtkSourceSpaceType specifying which types of spaces will get drawn

Add a blank line to separate the parameters from the description.

@@ +3996,3 @@
+ * Since: 3.16
+ *-
+ * Return value: a #GtkSourceSpaceType value.

"Return value:" is deprecated in GTK-Doc I think, use "Returns:". It may conflict with the "Returns" at the beginning of the description, so check the result.
Only, remove the "-" (probably a leftover).

@@ +4006,3 @@
+	GtkSourceSpaceType result = 0;
+	GHashTableIter iter;
+	gpointer key, value;

Already said that in the previous review, please declare variables at the beginning of a block, here the beginning of the function, above g_return_if_fail.
Also, it's generally better to declare one variable per line, it's more convenient to remove/move/comment/change a single variable declaration, and it's better also for readability.

@@ +4010,3 @@
+	while (g_hash_table_iter_next (&iter, &key, &value))
+	{
+		if ( GPOINTER_TO_INT (key) | location)

There should be no spaces after an opening parentheses.

@@ +4015,3 @@
+		}
+	}
+	return result;

blank line after the block.

@@ +4046,3 @@
+		{
+			GtkSourceSpaceType type = 0;
+			if (flags != GTK_SOURCE_DRAW_SPACES_ALL)

space out the code in this function too.

@@ +4069,3 @@
+				type = GTK_SOURCE_SPACE_TYPE_ALL;
+			}
+			g_hash_table_insert (view->priv->draw_spaces_at_location,

blank line missing.

::: gtksourceview/gtksourceview.h
@@ +137,3 @@
+ * @GTK_SOURCE_SOURCE_SPACE_TYPE_ALL: wheter all kind of spaces should be drawn.
+ *
+ * GtkSourceSpaceType determine what kind of spaces will be shown.

Again, please to not explain that the purpose is for the drawing. Those flags could be useful for other features.

@@ +156,3 @@
+ * @GTK_SOURCE_SPACE_LOCATION_TEXT: whether whitespaces inside text should be drawn.
+ * @GTK_SOURCE_SPACE_LOCATION_TRAILING: whether trailing whitespaces should be drawn.
+ * @GTK_SOURCE_SPACE_LOCATION_SELECTION: whether whitespaces in selected text should be drawn.

Please add the SELECTION flag in the later commit that will add also the implementation. So that this commit can be merged even if the next part is not done.

::: tests/test-widget.c
@@ +343,3 @@
 	{
+		gtk_source_view_set_draw_spaces_for_location (self->priv->view,
+							  GTK_SOURCE_SPACE_LOCATION_ALL,

Align parameters on the parenthesis.

Created attachment 288264 [details] [review]
Fine-grained control for drawing whitespaces. https://bugzilla.gnome.org/show_bug.cgi?id=683678

* Added new enums GtkSourceSpaceLocation and GtkSourceSpaceType.
* Added new functions for setting the types of whitespaces to show
per location.
* The old function set_draw_spaces uses the new function internally.
* Added convenience values for ALL locations ans ALL space types
This will have several benefits, including the possibility to add a Selection
location
to only show the whitespaces in selected text.

Next iteration of the patch attached.
I couldn't get the docs to show the new symbols, tried with 
 ./autogen.sh --enable-gtk-doc
 make

Is there anything else I have to do to make the docs?

(In reply to comment #24)
> I couldn't get the docs to show the new symbols, tried with 
>  ./autogen.sh --enable-gtk-doc
>  make

You can add --enable-gtk-doc in your jhbuildrc, see for example:
https://people.gnome.org/~swilmet/jhbuildrc-latexila

> Is there anything else I have to do to make the docs?

Yes, I forgot that, in the docs/reference/ directory, see the files gtksourceview-3.0-sections.txt and gtksourceview-3.0-unused.txt (the latter is generated by GTK-Doc).

If you need more details, see the GTK-Doc manual:
https://developer.gnome.org/gtk-doc-manual/unstable/

Review of attachment 288264 [details] [review]:

Still a few comments. From the previous review you also forgot to add blank lines at several places (e.g. before a "return" at the end of a function, or after a block).

::: gtksourceview/gtksourceview.c
@@ +149,3 @@
+	/* Mapping for GtkSourceSpaceLocation -> GtkSourceSpaceType
+	* with gint <-> gpointer conversions
+	*/

The *'s should be aligned, of course.

@@ +968,3 @@
+{
+	view->priv->draw_spaces_at_location = g_hash_table_new (NULL,
+								NULL);

For small parameters, you can have them all in one line.
Add also a blank line after this one.

@@ +1003,3 @@
+	init_draw_spaces_at_location (view);
+
+

Only one blank line is enough, see the HACKING file.

@@ +2127,3 @@
+		result = GTK_SOURCE_SPACE_TYPE_TAB;
+	}
+	if (g_unichar_break_type (c) == G_UNICODE_BREAK_NON_BREAKING_GLUE)

A "else if" is better, also for the next conditions.

::: gtksourceview/gtksourceview.h
@@ +132,3 @@
+ * GtkSourceSpaceType:
+ * @GTK_SOURCE_SOURCE_SPACE_TYPE_SPACE: whether the space character should be drawn.
+ * @GTK_SOURCE_SOURCE_SPACE_TYPE_TAB: whether the tab character should be drawn.

Hrm, please describe those symbols as simply:
"space character"
"tab character"
etc

So the enum can be used for other purposes than drawing.

@@ +163,3 @@
+typedef enum
+{
+        GTK_SOURCE_SPACE_LOCATION_ALL        = 0x7f,

Please put "ALL" at the end, since it is a sum of all the previous flags.
Also, is 0x7f the correct value? I think it was not updated when removing SELECTION.

Created attachment 288271 [details] [review]
Fine-grained control for drawing whitespaces. https://bugzilla.gnome.org/show_bug.cgi?id=683678

* Added new enums GtkSourceSpaceLocation and GtkSourceSpaceType.
* Added new functions for setting the types of whitespaces to show
per location.
* The old function set_draw_spaces uses the new function internally.
* Added convenience values for ALL locations ans ALL space types
This will have several benefits, including the possibility to add a Selection
location
to only show the whitespaces in selected text.

Yet another iteration, fixes based on last review.

(In reply to comment #26)
> Also, is 0x7f the correct value? I think it was not updated when removing
> SELECTION.
0x7f is 1111111, so it does not have to be updated until we have more than 7 location flags.

Created attachment 288273 [details] [review]
Fine-grained control for drawing whitespaces. https://bugzilla.gnome.org/show_bug.cgi?id=683678

* Added new enums GtkSourceSpaceLocation and GtkSourceSpaceType.
* Added new functions for setting the types of whitespaces to show
per location.
* The old function set_draw_spaces uses the new function internally.
* Added convenience values for ALL locations ans ALL space types
* Added gtk-doc
This will have several benefits, including the possibility to add a Selection
location
to only show the whitespaces in selected text.

Review of attachment 288273 [details] [review]:

Still a few comments… If you don't understand something, you can ask questions, but I think it's clear enough.
That said, those comments are really small problems.

::: gtksourceview/gtksourceview.c
@@ +3992,3 @@
+
+	GHashTableIter iter;
+	gpointer key;

Hum, please read _all_ comments in a review, I've already said at least TWO times to put variable declarations at the beginning of a block!

@@ +4003,3 @@
+		}
+	}
+	gtk_widget_queue_draw (GTK_WIDGET (view));

And please add a blank line after the "while" block, I've already said that several times…

(In reply to comment #30)
> 
> Hum, please read _all_ comments in a review, I've already said at least TWO
> times to put variable declarations at the beginning of a block!
Sorry for testing your nerves, that's not my intention. I always read all the comments and fix them one by one as reading through, and I am absolutely sure I have fixed at least two of these. I must have missed some.
> 

> And please add a blank line after the "while" block, I've already said that
> several times…
Yes, and I have also done that several times, after my last patch submit I have even resubmitted it in a matter of minutes to fix two such issues.

I will check the resulting diff twice now to make sure no such issues will slip in anymore. Sorry for the trouble, I will pay more attention in the future.

Created attachment 288277 [details] [review]
Fine-grained control for drawing whitespaces. https://bugzilla.gnome.org/show_bug.cgi?id=683678

* Added new enums GtkSourceSpaceLocation and GtkSourceSpaceType.
* Added new functions for setting the types of whitespaces to show
per location.
* The old function set_draw_spaces uses the new function internally.
* Added convenience values for ALL locations ans ALL space types
* Added gtk-doc entries
This will have several benefits, including the possibility to add a Selection
location to only show the whitespaces in selected text.

When I add a comment in a review, sometimes I suppose that the surrounding context is updated accordingly, or that the problem is fixed at other similar places.

Review of attachment 288277 [details] [review]:

Finally ready to be merged (after the freeze).
Just a minor problem that can be fixed just before pushing the patch: the commit message must have a blank line after the title. And the link to bugzilla can be at the end (it's done automatically with git-bz for example).

(In reply to comment #34)
> Finally ready to be merged (after the freeze).
As we're after the GNOME 3.14 release, which freeze would that be?

> Just a minor problem that can be fixed just before pushing the patch: the
> commit message must have a blank line after the title. And the link to bugzilla
> can be at the end (it's done automatically with git-bz for example).

I've pushed it with git bz attach, so I'll remove the link next time, that way the blank line will come right after the title.

Created attachment 288281 [details] [review]
Fine-grained control for drawing whitespaces.

* Added new enums GtkSourceSpaceLocation and GtkSourceSpaceType.
* Added new functions for setting the types of whitespaces to show
per location.
* The old function set_draw_spaces uses the new function internally.
* Added convenience values for ALL locations ans ALL space types
* Added gtk-doc entries
This will have several benefits, including the possibility to add a Selection
location to only show the whitespaces in selected text.

Review of attachment 288281 [details] [review]:

Marking as commit after freeze as per review from comment #34, the only change here is the change in the commit text.

The master branch is still for GtkSourceView 3.14. After the release of 3.14.1 this monday, we'll create the branch gnome-3-14 so the freeze ends.

I am sorry if I jump in so late after all this work has been done and I should probably read more carefully the whole bug discussion, but before checking the code or the APi, can someone explain me what is the use case we are trying to address here? What is the feature we are trying to expose to the final users? How would the UI look like?

(In reply to comment #39)
> I am sorry if I jump in so late after all this work has been done and I should
> probably read more carefully the whole bug discussion, but before checking the
> code or the APi, can someone explain me what is the use case we are trying to
> address here?
> What is the feature we are trying to expose to the final users?
Paolo, I wanted to tackle bug #737796 (based on a downstream requirement) and have as much as possible upstreamed. Sébastien pointed me to this bug. So the final use case I would like to handle is showing whitespaces only in selected text.
This bug shows that other people would find it useful too (e.g. I find myself most of the time checking whitespaces just to see whether there's a trailing whitespace or whether the leading whitespaces are all tabs/spaces)

> How would the UI look like?
For the basic setup the current user interface (checkbox to show/hide all whitespaces) is enough, for most advanced setups (some of which have been possible with the old API too) most editors didn't provide an UI, but if required, editors can implement it.

See also Comment 15 for a use-case and a possible UI. I think it makes sense, and with the ALL flags it's still convenient to use for a basic usage.

Robert: can you explain in bug #737796 what's your use case for the selection?

So it seems to me that we have this use case:

1) show white spaces in the selection

which could be addressed by simply adding a flag to the current API.


However there is a further use case that would not be addressed by the flag:

2) draw only trailing spaces in the normal view, but show all spaces in the selection


It seems to me that we could just have set/get_draw_spaces_in_selection() that takes the same flags.
I think it would be more self explaining, it would not require deprecating what we already have, and it would not introduce two new enum types.

One of the reasons why I do not like the Location enum approach is that using the getter would be awkward: you just have to try to get the value for all possible locations

Created attachment 288311 [details]
Sublime screenshot

Users can see if the selected text uses spaces or tabs and how
many levels of indentation is present.

In the attached screenshot you can see that in action, how easy it makes spotting incorrect indentation characters, without cluttering the whole interface with whitespace chars.

If we use LOCATION_ALL for the setter, for convenience the getter should also work with LOCATION_ALL. Either by doing an AND of all space types for each provided location, or doing an OR. The OR is easier and gives the same result if only LOCATION_ALL is used.

In my opinion having two enums is better, to clearly separate locations and space types. Combining them is more powerful.

So, bad news, Paolo wants to close this bug as WONTFIX, but I don't agree. Here are the reasons he gives (on IRC):

> it does not map properly to gobject properties

Not every state can be represented as a single property. To find a similar problem already solved in another library, we don't need to look far: GtkWidget has 4 properties for the margins, for each location. Plus one property "margin" that sets the same value to all margins, and that gets the maximum margin.

Here it is similar. For each location we want to be able to store a different value. It's not rocket science!

> the getter is broken
> it is broken because it is a getter without an underlying property
> it makes dealing with settings a lot harder

Do you prefer one property for each location? That way the gsettings binding works.

But what are the advantages of properties?
- the notify signal: we can replace that with a normal signal, if needed (but I think it's useless).
- property bindings: a text editor doesn't need that for space drawing. The setter is called normally at only one place.
- gsettings bindings: it can easily be replaced by a small function, it wouldn't be a lot of code, and there can be only one gsetting that stores a list of values, instead of one gsetting for each location (so the four properties is not really convenient in that case).
- GtkBuilder/glade: you can not build an entire app with glade… Some code is always needed. The GtkSourceView object is anyway useful to retrieve in the code, so calling a setter function in init() is not the end of the world.

> it is aesthetically unpleasing

It's subjective. I prefer the two enums, they are smaller and clearer. Defining the space types for each location is natural to me.

> it adds a lot of api (2 new types) and even if the old gets deprecated, it still means having 3 types + 4 functions in the public api for a while

GtkWidget has 5 properties and 8 functions just for margins. A deprecated API doesn't count.

I think having just a setter and getter is sufficient. Dealing with space drawing in an application is not difficult. An application not interested in the full flexibility can anyway use ALL on one or both axis.

And, as I've explained, there are real use cases where defining different space types for each location is desirable:

- for C code following the GNU coding style: draw all spaces except normal spaces for leading spaces + spaces inside text. For trailing spaces: draw all kind of spaces. The purpose is to draw spaces only when they are not allowed. Lots of people don't like having all spaces drawn everywhere all the time. And if all spaces are always drawn, it adds more noise for locating space errors.

- for LaTeX: draw only non-breaking spaces for leading spaces + spaces inside text. For trailing spaces: draw all kind of spaces. Non-breaking spaces can lead to strange errors when generating the PDF, and it is really hard to diagnose the first time. If only non-breaking spaces are drawn inside the text, the error is much more visible.

And that is just two use cases for me. Other people have probably other use cases in other languages and other coding styles.

(In reply to comment #46)
> So, bad news, Paolo wants to close this bug as WONTFIX

This is not correct. I said I do not like the api that uses the two enums because it leads to a matrix-like property, and I consider that bad for many reasons.


I am ok with implementing the support for selection, and I think the API I proposed in comment #42 addresses that specific use case which I think is relevant

This bug is about "Visibility of leading, internal and trailing spaces for individual types of whitespaces", and you don't want to fix that. See bug #737796 for the selection.

What we agree on is: for trailing spaces and selection, it's not needed to specify which types of spaces to draw; all kind of spaces should be drawn for those locations. So we could have two boolean properties for those locations.

But for leading and internal spaces, it should be possible to specify which types of spaces to draw, independently of each other. So we can add two SpaceType properties for that.

But having four properties just for drawing spaces is a bit too much. With only two functions it's possible to achieve the same, but with even more flexibility.

So, which way should we go? How do we get to a consensus here? The API from comment 42 is a lot easier to implement than the current proposed patch, but not as flexible. Are the usecases for this flexibility phantom requirements?

> Are the usecases for this flexibility phantom requirements?

I think I've given real use-cases. Maybe Paolo doesn't want this flexibility for gedit (especially for the UI), but there are other text editors and IDEs in the wild that uses GtkSourceView.

If we use a GVariant property everybody should be happy. g_param_spec_variant() exists. We must find a variant type that works fine when the GtkSourceSpaceLocation enum is expanded in the future. For example "ai", an array of integers. Each int would store the SpaceType flags for a specific location.

Paolo, do you agree with swilmet's proposed solution of having an "ai" g_variant setting for the source space flags?

Done!

With a "au" GVariant property.

commit bfffea8beb9bd9e6c1d35f2b52d504e9dd9eaa93 and previous commits.