After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 767216 - style-schemes: use the style scheme for diagnostician underlining
style-schemes: use the style scheme for diagnostician underlining
Status: RESOLVED FIXED
Product: gnome-builder
Classification: Other
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: GNOME Builder Maintainers
GNOME Builder Maintainers
Depends on:
Blocks:
 
 
Reported: 2016-06-03 18:54 UTC by Matthew Leeds
Modified: 2016-06-06 19:34 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
style-schemes: use the style scheme for diagnostician underlining (13.08 KB, patch)
2016-06-03 18:54 UTC, Matthew Leeds
needs-work Details | Review
style-schemes: use the style scheme for diagnostics underlining (12.99 KB, patch)
2016-06-03 21:14 UTC, Matthew Leeds
committed Details | Review

Description Matthew Leeds 2016-06-03 18:54:11 UTC
Let me know if anyone can think of a cleaner way to write the fall-back code.
Comment 1 Matthew Leeds 2016-06-03 18:54:14 UTC
Created attachment 329088 [details] [review]
style-schemes: use the style scheme for diagnostician underlining

Builder underlines segments of code in different colors depending on the
condition reported by libclang (deprecated, error, note, or warning). As
of v3.17.2, GtkSourceView supports the underline-color attribute in style
schemes. So Builder now makes use of that, and falls back on hard coded
values in case our custom style scheme isn't installed.
Comment 2 Christian Hergert 2016-06-03 19:48:54 UTC
Review of attachment 329088 [details] [review]:

Logic/implementation looks good. Just some style nits for our codebase.

Thanks!

::: libide/ide-buffer.c
@@ +1032,3 @@
   if (style_scheme != NULL)
     {
+      // These are used as a fall-back if our style scheme isn't installed.

We are trying to be consistent with comment styling and use /* */ everywhere.

@@ +1038,3 @@
+      gdk_rgba_parse (&warning_rgba, WARNING_COLOR);
+
+      if (!ide_source_style_scheme_apply_style(style_scheme,

Space before open paren.

@@ +1039,3 @@
+
+      if (!ide_source_style_scheme_apply_style(style_scheme,
+                                          TAG_DEPRECATED,

Align multi-line parameter lists

@@ +1044,3 @@
+                     "underline", PANGO_UNDERLINE_ERROR,
+                     "underline-rgba", &deprecated_rgba,
+                     NULL);

Extra newline between unrelated conditional blocks

@@ +1108,3 @@
   IdeBuffer *self = (IdeBuffer *)object;
   IdeBufferPrivate *priv = ide_buffer_get_instance_private (self);
+  GtkTextTagTable *tag_table = gtk_text_buffer_get_tag_table (GTK_TEXT_BUFFER (self));

We try to only do assignment/casts or get_instance_private() calls in our declarations (which is not actually a call, its a pointer+offset so safe from accidental dereference). So move the assignments after the declaration block.

@@ +1110,3 @@
+  GtkTextTagTable *tag_table = gtk_text_buffer_get_tag_table (GTK_TEXT_BUFFER (self));
+  GtkSourceStyleScheme *style_scheme = gtk_source_buffer_get_style_scheme (GTK_SOURCE_BUFFER (self));
+  GtkTextTag *deprecated_tag;

Maybe g_autoptr(GtkTextTag) deprecated_tag = NULL; and drop the g_object_unref()s at the end of the function?

@@ +1121,3 @@
   G_OBJECT_CLASS (ide_buffer_parent_class)->constructed (object);
 
+  // These are used as a fall-back if our style scheme isn't installed.

/* */

@@ +1143,3 @@
+                   "underline", PANGO_UNDERLINE_ERROR,
+                   "underline-rgba", &deprecated_rgba,
+                   NULL);

Extra newline between unrelated blocks.

@@ +1160,3 @@
+                   NULL);
+
+  gtk_text_tag_table_add(tag_table, deprecated_tag);

Space before paren

::: libide/ide-source-style-scheme.c
@@ -92,2 +98,4 @@
-  if (underline_set && underline)
-    g_object_set (tag, "underline", PANGO_UNDERLINE_SINGLE, NULL);
+  if (underline_set)
+    g_object_set (tag, "underline", pango_underline, NULL);
+
+  if (underline_color_set && underline_color) {

We use gtk/gnu style curly braces (which can take a little time to get used to) and spaces before open parens. For pointers, we are trying to follow GNOME guidelines and check explicitly for NULL. For g_object_get() and g_object_set() we try to do a single key/value pair per-line to make it trivial to check for trailing NULL.

So altogether, that would look like:

  if (underline_color_set && underline_color != NULL)
    {
      gdk_rgba_parse (&underline_rgba, underline_color);
      g_object_set (tag,
                    "underline-rgba", &underline_rgba,
                    NULL);
    }
Comment 3 Matthew Leeds 2016-06-03 21:14:04 UTC
Created attachment 329092 [details] [review]
style-schemes: use the style scheme for diagnostics underlining

Builder underlines segments of code in different colors depending on the
condition reported by libclang (deprecated, error, note, or warning). As
of v3.17.2, GtkSourceView supports the underline-color attribute in
style schemes. So Builder now makes use of that, and falls back on hard-coded
values in case our custom style scheme isn't installed.
Comment 4 Christian Hergert 2016-06-04 21:28:23 UTC
Review of attachment 329092 [details] [review]:

LGTM, Thanks!
Comment 5 Christian Hergert 2016-06-06 19:34:02 UTC
Thanks for finding this old fixme and fixing it!

Attachment 329092 [details] pushed as 97607b9 - style-schemes: use the style scheme for diagnostics underlining