GNOME Bugzilla – Bug 433275
GNOME Goal: make application icon theme-friendly
Last modified: 2007-07-12 22:44:54 UTC
Please describe the problem: http://live.gnome.org/GnomeGoals/AppIcon Ekiga's application icon should follow the icon themes naming specs, so it can be replaced in themes. Steps to reproduce: Actual results: Expected results: Does this happen every time? Other information:
Hmmmm... I just read the page, and something bothers me : a program is run from the makefile. Are we sure this is portable? More specifically, wouldn't such a change break our cross-building for win32? Yes, ekiga is a gnome application, but we would like it to stay portable.
If I remember well, we do run other things from Makefile : - some .h from .png (the smileys) ; - glib signal marshallers ; - DBUS stub code. So perhaps it's not that problematic.
Created attachment 90592 [details] [review] Proposed patch Here is a stab at this. Things to note: * I made up a 72x72 icon, created from pixmaps/text_logo.xpm. I'm no graphical artist and I may have screwed it up. If you don't want to use it, ignore the 72x72 stuff below and go without it (though if you don't have a 72x72 icon, the Call tab will be grosser than necessary). * This *should* work on Windows, though I haven't tried it. I removed a WIN32 #ifdef around part of some icon code in this belief. If it doesn't work in Windows, I can create a new patch that works around that (by using bundled backup icons). Let me know. * I'm interested in converting the rest of Ekiga to use the icon theme (instead of all those inline png icons). I would preserve the current inline system because Windows probably needs it anyway. Would this be a patch you all would be interested in? After applying this, the following need to happen: cd pixmaps mkdir 16x16/apps 22x22 22x22/apps 32x32 32x32/apps 48x48 48x48/apps 72x72 72x72/apps cp ekiga_logo_16x16.png 16x16/apps/ekiga.png cp ekiga_logo_22x22.png 22x22/apps/ekiga.png cp ekiga_logo_32x32.png 32x32/apps/ekiga.png cp ekiga_logo_48x48.png 48x48/apps/ekiga.png cp FILE_ABOUT_TO_BE_ATTACHED 72x72/apps/ekiga.png svn add 16x16/apps 22x22 32x32 48x48 72x72 svn remove ekiga_logo* ekiga.png text_logo.xpm
Created attachment 90593 [details] 72x72 app icon Promised 72x72 icon.
Adding me to cc list and reopening (because I think the previous concern about portability due to running programs was overcome and any new concerns about portability of the patch can be addressed as an open bug).
Well, if you both plan on doing the work yourself and are committed to do so while keeping the portability in mind, then welcome on board ;-) I already have several remarks about your patch : 1) in pixmaps/Makefile.am, it removes the pixmap_stuff in favor of icon_stuff... but aren't the smileys in the fist category ? 2) in src/devices/fakevideoinput.cpp, I see a "static const gint DEFAULT_WIDTH = 72"; why not a #define at the top of the file ? 3) in src/devices/fakevideoinput.cpp, I'd rather see the GdkPixbuf *icon at the beginning of the function, and initialized to NULL (and reset to NULL after use, if it is to be used again later down the function) ; 4) I don't exactly get the code removal in src/gui/tools.cpp, src/gui/history.cpp, src/gui/druid.cpp and src/gui/chat.cpp ; 5) in src/gui/main.cpp, you remove the code which was centering the logo in the view... is that normal?
#1: Sure, the smileys are 'pixmaps,' but they are handled via the EMOTICONS_FILES variable and made inline. They don't use pixmapdir or pixmap_DATA. #2: Eh, just a style thing to minimize the scope of the symbol. I can change it to a #define to match the rest of ekiga's style. #3: OK. #4: The code removal is because in main.cpp, "gtk_window_set_default_icon_name (PACKAGE);" is called which will set the default icon name for all future windows. So setting the same icon on individual windows is pointless. But upon reflection, that line should be moved to early on in main() to guarantee that it gets called before any windows are instantiated. #5: Yes. GtkImage will center its image inside of its size request. I'll submit a new patch that deals with these issues. Also, I'll go ahead and use the bundled backup icon method I mentioned to make sure that either install problems or Windows goofiness doesn't cause us to end up with a blank icon.
Nice. Thanks.
Created attachment 90719 [details] [review] Fixes nits and adds bulitin icons Here's a new and improved version. Fixes the stuff you mentioned and adds the icons back as inline pixmaps. Which are only used if no theme contains them (install problem or on Windows most likely). I will go ahead and work on a much-wider-scope patch for the rest of the icons, but that will be a different bug/patch.
Hmmm... I see you put GM_STOCK_LOGO (which is defined as "PACKAGE") in places where PACKAGE_NAME is used. We have frequent cvs snapshots, and that PACKAGE_NAME is what allows to it possible to install those snapshots together with the stable version. For example, there will be ekiga.png and ekiga-snapshot.png, to avoid collisions. Are you sure your patch doesn't break this?
Depends. I couldn't see how you built your snapshots (the source snapshot seemed different than the binary ones), but PACKAGE is supposed to be the all-lowercase simple name of the program ("ekiga"). PACKAGE_NAME is (if supplied in configure.in) the pretty name of the program ("Ekiga"). If not provided, it's the same as PACKAGE. So unless you're explicitly overloading the semantics of PACKAGE_NAME, my patch won't break it. But if you're just swapping the PACKAGE string, it's safe.
Kilian, can you comment on this? Would this mean we erroneously use PACKAGE_NAME everywhere?!
http://www.gnu.org/software/autoconf/manual/html_node/Initializing-configure.html Oddly, it doesn't mention how it defines PACKAGE itself, but it does mention that one is supposed to call AC_INIT with the pretty version of your program name, and that PACKAGE_NAME will be an exact copy of that. In my experience, PACKAGE is defined as a canonical lowercase version of PACKAGE_NAME (possibly equivalent to PACKAGE_TARNAME). I mean, it doesn't really matter for Ekiga, as you call AC_INIT with the lowercase version to start.
Created attachment 90915 [details] [review] Use PACKAGE_NAME instead of PACKAGE I have another question : is GM_STOCK_LOGO that useful?
For realz? A modified patch with no explanation of why you prefer PACKAGE_NAME? See some examples in the autoconf manual [1], do a grep ' PACKAGE_NAME ' */config.h in your code directory vs a grep ' PACKAGE ' */config.h, or read the section I linked to before. I mean, sure, no big deal for Ekiga since it doesn't set PACKAGE_NAME correctly anyway, but I don't understand why you're going out of your way to use it like you are. As for GM_STOCK_LOGO, perhaps not strictly necessary. But it's consistent with the other stock lookups and will be more so when I submit a patch for further icon-themification. Plus, it already made your life easier to switch from PACKAGE to PACKAGE_NAME! ;) [1] http://www.gnu.org/software/autoconf/manual/autoconf-2.57/html_chapter/autoconf_6.html#SEC74
I don't know if PACKAGE or PACKAGE_NAME is correct ; all I know is PACKAGE_NAME is used everywhere in ekiga. If that is wrong, then let's open another bug about it and fix it all the way through. In the meantime, it's more coherent to stick to what is done in the current codebase. This beeing said, should I apply this first patch or should I wait for further patches and apply them in one go?
Oh, sorry, I missed notification of your response. :( I plan to submit the big ol' icon patch today, so it might be reasonable to wait.
*** Bug 453979 has been marked as a duplicate of this bug. ***
Whoops. OK, patch and description are in the dup bug 453979.
Looks pretty neat, but I'd like a second pair of eyes (and a brain, if possible ;-) ) to get over it, since it's pretty big.
Well, I don't really think I'll be helpful on this... I haven't been able to have a working ekiga for ages. Since ekiga now is delivered as a tarballs to jhbuild, I tried to switch to the svn trunk, but had no luck. As you asked in the bug 453979, I gave an eye to the patch, everything seemed fine. The only problem with the patch is that it does a bit more than the scope of the bug. It's generally better to strictly avoid changes unrelated to a bug. The part I found tricky was the smiley regexp generation, and didn't dig deeply into it. I just checked if there were way to get out of bounds, but saw none. There was a part I didn't understand: the #define GTK_MENU_* changes... Snark, can you check if it's ok ? I just saw a coding convention mistake: + pixbuf = gtk_icon_theme_load_icon(theme, tmp->icon_name, 22, 0, NULL); should have a space after function name + pixbuf = gtk_icon_theme_load_icon (theme, tmp->icon_name, 22, 0, NULL); Just move the patch from bug 453979 to this bug, so it's easy to find. So it's a very nice work, indeed. Congrats, Michael :-D
The smiley regex change is a good idea : I had a hard time coming up with a nice regex -- it's better if it's auto-generated. That being said, I have a new round of questions : (1) will I have to do the file deletions myself? (2) how does one do the mime-type changes you made? (3) could you rework a little your gtk_text_buffer_get_smiley_regex function, so that it initialize its pointer to NULL when declaring them? (4) while I'm at it, wouldn't it be wiser to make regex a static variable, and do the computation only once by using an organization like if(regex == NULL) { computation } return regex ? I think we leak the string everytime the function is called -- with a static we leak it only once in the application lifetime. (5) now that Luis points it out and now that I reread it, I don't get the GTK_MENU magic anymore ; could you explain? Marking as high, because it's getting near :-) Thanks!
Created attachment 91591 [details] [review] Static regex OK, this fixes the coding style issue and makes the change to a static const char regex string. It also updates the #define names for the new-style icons to GM_ICON_* instead of GM_STOCK_* to help avoid confusion between the two. (1) No, I can do them once I have approval. It's all in my tree. (2) I don't know. That's something svn did when I 'svn add'ed the file. (5) The change is needed to tell gmmenuaddon.c to use the correct function to find the icon. These new-style icons need a different lookup function, even though stock icons and theme both use string keys. So I needed some flag off which gmmenuaddon.c would branch.
Wonderful : please commit! :-) Thanks!
Committed as revision 5247.