GNOME Bugzilla – Bug 579151
Adds an Adium theme parsing to have a nicer theme selector
Last modified: 2009-07-02 19:44:03 UTC
This branch needs review: http://git.collabora.co.uk/?p=user/pierlux/empathy.git;a=shortlog;h=refs/heads/pierlux-adium-theme-browser
Created attachment 132756 [details] [review] probably not the right diff configure.ac | 27 + data/Makefile.am | 8 +- data/Template.html | 159 +++ data/empathy.schemas.in | 14 + docs/libempathy-gtk/libempathy-gtk-docs.sgml | 3 +- docs/libempathy-gtk/libempathy-gtk.types | 5 +- libempathy-gtk/Makefile.am | 16 +- libempathy-gtk/empathy-chat-text-view.c | 1365 ++++++++++++++++++++++++ libempathy-gtk/empathy-chat-text-view.h | 86 ++ libempathy-gtk/empathy-chat-view.c | 1442 ++------------------------ libempathy-gtk/empathy-chat-view.h | 67 +- libempathy-gtk/empathy-chat.c | 54 +- libempathy-gtk/empathy-conf.h | 1 + libempathy-gtk/empathy-log-window.c | 5 +- libempathy-gtk/empathy-smiley-manager.c | 166 +++- libempathy-gtk/empathy-smiley-manager.h | 16 +- libempathy-gtk/empathy-theme-adium-manager.c | 223 ++++ libempathy-gtk/empathy-theme-adium-manager.h | 62 ++ libempathy-gtk/empathy-theme-adium.c | 795 ++++++++++++++ libempathy-gtk/empathy-theme-adium.h | 56 + libempathy-gtk/empathy-theme-boxes.c | 771 ++++----------- libempathy-gtk/empathy-theme-boxes.h | 15 +- libempathy-gtk/empathy-theme-irc.c | 293 +----- libempathy-gtk/empathy-theme-irc.h | 16 +- libempathy-gtk/empathy-theme-manager.c | 667 +++++++----- libempathy-gtk/empathy-theme-manager.h | 18 +- libempathy-gtk/empathy-theme.h | 98 -- libempathy-gtk/empathy-ui-utils.c | 71 +- libempathy-gtk/empathy-ui-utils.h | 11 +- libempathy/empathy-contact.c | 15 +- libempathy/empathy-contact.h | 4 +- libempathy/empathy-message.c | 20 - libempathy/empathy-message.h | 2 - python/pyempathy/pyempathy.defs | 10 +- python/pyempathygtk/pyempathygtk.defs | 322 ++---- python/pyempathygtk/pyempathygtk.override | 1 - python/update-binding.sh | 2 +- src/Makefile.am | 2 + src/empathy-chat-window.c | 36 +- src/empathy-preferences.c | 270 +++--- src/empathy-preferences.glade | 11 +- 41 files changed, 4066 insertions(+), 3159 deletions(-)
Is this scheduled for 2.28?
Should be rebased on top of master now the adium branch has been merged.
*** Bug 586289 has been marked as a duplicate of this bug. ***
Created attachment 136930 [details] mockup I've added a mockup to the other report, just in case.
Yes thanks for resubmitting. The idea looks like. It is better than the one in the attached patch. But you there are impossibilities with it: * Previous (non-adium) themes will remain in empathy too * We don't have access to the preview icon we can see on adiumxtras I think we can't do much more than having a combobox with themes listed, and just under a place where the theme is demoed.
I meant: The idea looks fine.
We could render a EmpathyThemeAdium widget in an offscreen cairo surface, get a pixbuf from cairo and use it to display a thumbnail... But I guess that will be horrible for performances. Otoh we could cache the thumbnail... Maybe we could make some experiments.
For the time being, even a simple list with the names would be ok I guess. The really important thing, IMO, is making it easy to install and remove themes.
I just reimplemented that patch against latest Empathy. http://git.collabora.co.uk/?p=user/pierlux/empathy.git;a=shortlog;h=refs/heads/adium-theme-selector It fills the Theme combobox with themes from /usr/share/adium and ~/.local/share/adium/ for now. Try it! It is probably not final.
Created attachment 137463 [details] [review] Proposed fix on branch adium-theme-selector http://git.collabora.co.uk/?p=user/pierlux/empathy.git;a=shortlog;h=refs/heads/adium-theme-selector libempathy-gtk/empathy-theme-adium.c | 16 ++- libempathy-gtk/empathy-theme-manager.c | 54 ++++++++++- libempathy-gtk/empathy-theme-manager.h | 1 + src/empathy-preferences.c | 173 +++++++++++++++----------------- src/empathy-preferences.ui | 52 +--------- 5 files changed, 147 insertions(+), 149 deletions(-)
cool! my comments: - empathy_adium_info_new: you can use tp_asv_set_string (info, g_strdup("path"), path); - find_themes: don't use g_warning for things that could happen. For example if the user add wrong dirs in its XDG env var. error is leaked. - find_themes: don't use do{}while, the dir could be empty so first g_dir_read_name could give NULL. while ((name = g_dir_read_name) != NULL){} should be fine. - find_themes: I think you should use empathy_adium_path_is_valid(). And probably that func should check if Info.pfile is present. - find_themes: Use g_list_prepend() to avoid going through the list each time to append to the end. - empathy_theme_manager_get_adium_themes: Use g_get_system_data_dirs. I'm not sure it's a good idea to use adium/ dir. What if adium client gets ported to linux? They most probably install stuff there and get mixed with themes we install. I prefer either "adium/themes/" or "adium-themes/" or something like that. - preferences_theme_notify_cb: Why not if (strcmp (name, "adium") != 0 || strcmp (path, conf_path) == 0) to avoid duplicating code? - preferences_theme_notify_cb: conf_name is leaked if it fail getting conf_path - get_string: You are reimplementing tp_asv_get_string - preferences_themes_setup: adium_themes = g_list_next (adium_themes); should be adium_themes = g_list_delete_link (adium_themes, adium_themes); so you can remove the g_list_free after the loop.
Oh, at last comment: - Would be great if you could reuse the info GHashTable from the theme selector to create EmpathyThemeAdium objects (empathy_theme_adium_new_with_info). To avoid parsing the pfile twice.
(In reply to comment #12) > - empathy_theme_manager_get_adium_themes: Use g_get_system_data_dirs. I'm not > sure it's a good idea to use adium/ dir. What if adium client gets ported to > linux? They most probably install stuff there and get mixed with themes we > install. I prefer either "adium/themes/" or "adium-themes/" or something like > that. I propose using adium/message-styles as Adium uses (Your Home Folder)/Library/Application Support/Adium 2.0/Message Styles. I doubt they'll still use spaces on linux... and they really call them messages styles as many other parts of Adium can be themed.
+1
(In reply to comment #14) > I propose using adium/message-styles as Adium uses (Your Home > Folder)/Library/Application Support/Adium 2.0/Message Styles. I doubt they'll > still use spaces on linux... and they really call them messages styles as many > other parts of Adium can be themed. > What about a more generic name, instead of adium. Something like: messaging/chat-styles/ This could potentially introduce also a common place for logs and other data that could be nice to share with other intant messaging clients (like kopete). Like: messaging/chat-logs/
I prefer having "adium" appearing in the path in case we have other theme systems.
Created attachment 137646 [details] [review] Update branch adium-theme-selector http://git.collabora.co.uk/?p=user/pierlux/empathy.git;a=shortlog;h=refs/heads/adium-theme-selector libempathy-gtk/empathy-location-manager.c | 36 ++++++++++++++-------------- src/empathy-preferences.ui | 2 +- 2 files changed, 19 insertions(+), 19 deletions(-)
Wrong patch
Created attachment 137647 [details] [review] Update branch adium-theme-selector http://git.collabora.co.uk/?p=user/pierlux/empathy.git;a=shortlog;h=refs/heads/adium-theme-selector libempathy-gtk/empathy-theme-adium.c | 21 ++++- libempathy-gtk/empathy-theme-manager.c | 63 ++++++++++++- libempathy-gtk/empathy-theme-manager.h | 1 + src/empathy-preferences.c | 161 +++++++++++++------------------- src/empathy-preferences.ui | 44 --------- 5 files changed, 145 insertions(+), 145 deletions(-)
- empathy_adium_path_is_valid: You should update the comment to tell info.plist is also required. - find_themes: You should add an empty line after gchar *path; - find_themes: if (empathy_adium_path_is_valid (path) == TRUE) --> ==TRUE is useless. - empathy_theme_manager_get_adium_themes: Your way to iterate on paths seems strange to me. Either you use an index and do "for (i = 0; paths[i] != NULL; i++){foo(path[i])}". Or you iterate on the pointer with "while (*paths) {foo(*paths); paths++;}". That's all :)
I merged the branch.