(In reply to comment #0)
> Created an attachment (id=167183) [details] [review]
> patch
> 
> I've attached a patch that provides this functionality.

Please use more descriptive names (like the patch subject) instead of just "path" to make it easier to see what the patch is about when viewing the patch list.

You might want to use git-bz which should make submitting patches to bugzilla a lot easier ;)

Created attachment 167972 [details] [review]
Change widgets' css rules on the fly

A first draft for editing css on the fly.
The css rules are added to the info displayed by the inspector tool of lookingGlass. The info is rendered in an St.Entry which becomes editable by pressing Shift+E. The changes are saved in a file (/data/theme/test-css.css) and are loaded using st_theme_load_stylesheet. Shift+R restores the original theme and the changes of the last session can be found in /data/theme/test-css.css.

I need to find a way to avoid creating a file for loading the css changes every time. I just used this approach for now, to prove that it can be done.

Review of attachment 167972 [details] [review]:

Thanks for working on this - having some way to display/change the CSS from looking glass will be super useful!

First, the patch needs to be rebased on master - the conflicts in st-theme-node.[ch] are trivial, so I reviewed those files. I skipped the javascript part for now, as resolving the conflicts there requires more code changes - nothing too scary though :)

Some observations before skipping into the code:

 - You cannot assume that the global theme directory is writable by the user
   (normal people don't use jhbuild)

 - I'm not really sure that the Inspector is the best place for displaying CSS;
   lots of extreme size changes ... not that I'd have a convincing alternative
   though ...

 - Any particular reason for picking 'shift' as modifier? Is that what Firebug uses?

I should also add that I have not been able to actually modify the CSS (likely my mistake when resolving conflicts), so I can't comment on that part.

::: src/st/st-theme-node.c
@@ +2787,3 @@
+ *
+ * Uses a node's properties in order to reconstruct its css rules.
+ * The string returned has css identation.

Missing documentation for the return value (including transfer annotation). Something along the lines of

Returns: (transfer full): newly allocated string holding the result.

It's also nice to add something like "The returned string should be freed with g_free() when no longer needed." to the description.

@@ +2790,3 @@
+ */
+char*
+st_theme_node_get_css_to_string (StThemeNode *node)

Hmm, that name sounds weird - something like st_theme_node_print_css() or a simple st_theme_node_to_string() would make more sense to me.

Also, the return type should be written as "char *" instead of "char*". Oh, and you should use that style consistently :-)

@@ +2794,3 @@
+  char* result = NULL;
+  gchar *str = NULL;
+  char *previous_sel = "";

Eeek - char*, gchar * and char *!

I really dislike using "" to mean "unset", this should be NULL.

@@ +2797,3 @@
+  GString *answer = NULL;
+  answer = g_string_new (NULL);
+  int i;

Mixing code and variable declarations is not allowed in C89.

It's odd to initialize answer to NULL right before an assignment.

@@ +2801,3 @@
+  ensure_properties (node);
+  if(node->properties)
+    {

You can get rid of one indentation level if you do:

if (!node->properties)
  return g_strdup ("");

answer = g_string_new (NULL);

(alternatively you could return NULL and update the method documentation accordingly)

Note that there should be a space between 'if' and '('.

@@ +2806,3 @@
+          CRSelector *sel = decl->parent_statement->kind.ruleset->sel_list;
+          if (sel)
+

You can get rid of another indentation level by doing

if (!sel)
  continue;

@@ +2808,3 @@
+          if (sel)
+            {
+              if (strcmp(previous_sel, (char *)cr_selector_to_string (sel)) != 0)

cr_selector_to_string() returns newly allocated memory, which you leak here and with any subsequent call - you need to store the result in a variable which is freed after use (just like you do with cr_declaration_to_string()). You can reuse str here, but separate variables are probably cleaner.

Oh, and you missed a space after strcmp.

If you initialize previous_sel to NULL, you can just replace strcmp() with g_strcmp0() here ...

@@ +2810,3 @@
+              if (strcmp(previous_sel, (char *)cr_selector_to_string (sel)) != 0)
+                {
+                  if(strcmp(previous_sel, "") != 0)

... and this would become a simple

if (previous_sel != NULL)

@@ +2823,3 @@
+            }
+        }
+      g_string_append_printf (answer, "}");

I doubt that this will be a problem in practice, but if sel is undefined for every property you will get a return value of "}".

@@ +2828,3 @@
+  result = (char*) answer->str;
+  g_string_free (answer, FALSE);
+  answer = NULL;

Hmmm, libcroco does it that way, but I'd just do

return g_string_free (answer, FALSE);

Note that this will allow you to get rid of result - one string variable less!

(I would then rename answer to result, but hey, up to you!)

*** Bug 658684 has been marked as a duplicate of this bug. ***

Created attachment 202265 [details] [review]
Change widgets' css rules on the fly

Adds st_theme_node_css_to_string function that reconstructs
a node's css through its properties. The css rules are added
to the info displayed by the inspector tool of lookingGlass.
A CSS button is added to the item inserted in lookingGlass
by the inspector. This button open an editor window.
The changes take effect by using the "Apply" button.
A Reset button is availabe which restores the original theme
(FIXME: it removes all changes not just the current edited
item). The changes of the last session are saved in
the userdatadir/gnomie-shell/theme/test-css.css with a copy
of the theme resources (FIXME: the copy is only done once).

Signed-off-by: Alban Browaeys <prahal@yahoo.com>

this patch does not take into account all reviewed issues. It is merely a forward.

Comment on attachment 202265 [details] [review]
Change widgets' css rules on the fly

(In reply to comment #6)
> this patch does not take into account all reviewed issues. It is merely a
> forward.

Getting it off the review list then.

GNOME is going to shut down bugzilla.gnome.org in favor of  gitlab.gnome.org.
As part of that, we are mass-closing older open tickets in bugzilla.gnome.org
which have not seen updates for a longer time (resources are unfortunately
quite limited so not every ticket can get handled).

If you can still reproduce the situation described in this ticket in a recent
and supported software version, then please follow
  https://wiki.gnome.org/GettingInTouch/BugReportingGuidelines
and create a new ticket at
  https://gitlab.gnome.org/GNOME/gnome-shell/-/issues/

Thank you for your understanding and your help.