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 585474 - Support for CSS variants in adium themes
Support for CSS variants in adium themes
Status: RESOLVED FIXED
Product: empathy
Classification: Core
Component: Chat themes
unspecified
Other Linux
: Normal normal
: 3.2
Assigned To: empathy-maint
Depends on:
Blocks: 595318
 
 
Reported: 2009-06-11 19:13 UTC by Xavier Claessens
Modified: 2011-06-07 15:35 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
diff (471 bytes, patch)
2011-06-06 12:55 UTC, Guillaume Desmottes
none Details | Review
proper diff (33.83 KB, patch)
2011-06-06 12:58 UTC, Guillaume Desmottes
reviewed Details | Review

Description Xavier Claessens 2009-06-11 19:13:57 UTC
We should support adium themes with CSS variants and let the user choose which variant to use.
Comment 1 Xavier Claessens 2011-05-03 22:15:57 UTC
I started implementation here: http://cgit.collabora.co.uk/git/user/xclaesse/empathy.git/log/?h=variants

This is still WIP, it gives the UI to select the theme variants, but the variant is not yet applied on the views.
Comment 2 Xavier Claessens 2011-05-04 11:05:51 UTC
That branch is not complete, but it is hitting a webkit issue I've reported: https://bugs.webkit.org/show_bug.cgi?id=60155

It has the effect that changing variant on-the-fly for a view breaks completely its style.
Comment 3 Xavier Claessens 2011-05-04 11:16:20 UTC
(In reply to comment #2)
> That branch is not complete

I keep doing that error: s/not/now/ !
Comment 4 Gustavo Noronha (kov) 2011-05-04 13:24:38 UTC
FWIW, you can work around that bug by not using document fragments:

function setStylesheet( id, url ) {
    var head = document.getElementsByTagName( "head" ).item(0);

    var newStyle = document.createElement('style');
    newStyle.setAttribute('id', id);
    newStyle.setAttribute('type', 'text/css');
    newStyle.setAttribute('media', 'screen,print');

    if (url.length) 
        newStyle.innerHTML = '@import url("' + url + '");';

    head.removeChild( document.getElementById( id ) );
    head.appendChild( newStyle );
}
Comment 5 Xavier Claessens 2011-05-04 13:45:59 UTC
That workaround indeed fix the issue. However the JS function is shipped by each Theme, so that would require empathy to patch the html file before loading it... Not sure that's a good idea...
Comment 6 Jonny Lamb 2011-05-16 13:16:50 UTC
Seems to have odd indentation:

+ /* Create the model */
+ store = gtk_list_store_new (COL_VARIANT_COUNT,
+                             G_TYPE_STRING, /* name */
+                             G_TYPE_BOOLEAN); /* is default */
+ gtk_tree_sortable_set_sort_column_id (GTK_TREE_SORTABLE (store),
+         COL_VARIANT_NAME, GTK_SORT_ASCENDING);

The branch is pretty hard to review, but on a first look it seems okay.
Comment 7 Xavier Claessens 2011-05-16 15:20:53 UTC
What odd with the indentation? make sure to display tab as 8 spaces, that's the old gossip style.
Comment 8 Xavier Claessens 2011-06-06 04:48:51 UTC
Webkit 1.4.1 released with the needed fix. Ok to merge this?
Comment 9 Guillaume Desmottes 2011-06-06 12:55:32 UTC
Created attachment 189318 [details] [review]
diff
Comment 10 Guillaume Desmottes 2011-06-06 12:58:05 UTC
Created attachment 189319 [details] [review]
proper diff
Comment 11 Guillaume Desmottes 2011-06-06 13:25:11 UTC
Review of attachment 189319 [details] [review]:

I didn't test the branch yet.

::: libempathy-gtk/empathy-theme-adium.c
@@ +1688,3 @@
+
+	g_free (priv->variant);
+	gchar *script;

g_object_notify() ?

::: libempathy-gtk/empathy-theme-manager.c
@@ +60,3 @@
+	EmpathyAdiumData *adium_data;
+	gchar *adium_variant;
+	GList *adium_views;

Describe the content of the list.

@@ +578,3 @@
+	g_free (priv->adium_variant);
+	if (priv->adium_data != NULL) {
+				     &priv->adium_views);

tp_clear_pointer

::: src/empathy-preferences.c
@@ +884,3 @@
+	g_signal_connect (combo, "changed",
+			  G_CALLBACK (preferences_theme_variant_changed_cb),
+			gtk_combo_box_set_active_iter (combo, &default_iter);

Did you consider using g_settings_bind_with_mapping() ?

@@ +930,3 @@
 		g_free (path);
+		if (info != NULL) {
+			g_hash_table_unref (info);

You can use tp_clear_pointer()
Comment 12 Xavier Claessens 2011-06-07 08:37:05 UTC
(In reply to comment #11)
> ::: src/empathy-preferences.c
> @@ +884,3 @@
> +    g_signal_connect (combo, "changed",
> +              G_CALLBACK (preferences_theme_variant_changed_cb),
> +            gtk_combo_box_set_active_iter (combo, &default_iter);
> 
> Did you consider using g_settings_bind_with_mapping() ?

Can it work with a combobox to bind the value of the row selected? It is not used in the rest of the code, so I didn't care, tbh.

Fixed the rest.
Comment 13 Xavier Claessens 2011-06-07 15:35:13 UTC
Branch merged. Note that webkit 1.4.1 is required for variant to be updated correctly, otherwise you'll have to close and reopen chats.