Closed
Bug 562506
Opened 14 years ago
Closed 13 years ago
Process icon are builtin, while launcher icon uses the system icon theme
Categories
(Core :: Widget: Gtk, defect)
Tracking
()
RESOLVED
FIXED
mozilla11
People
(Reporter: jagw40k, Assigned: jagw40k)
References
Details
Attachments
(5 files, 18 obsolete files)
766 bytes,
patch
|
karlt
:
review-
khuey
:
feedback+
|
Details | Diff | Splinter Review |
1.45 KB,
patch
|
karlt
:
review-
|
Details | Diff | Splinter Review |
1.08 KB,
patch
|
karlt
:
review-
|
Details | Diff | Splinter Review |
6.35 KB,
patch
|
karlt
:
review+
mounir
:
checkin+
|
Details | Diff | Splinter Review |
6.42 KB,
patch
|
Details | Diff | Splinter Review |
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2.3) Gecko/20100402 Lightningphoenix/3.6.3 (Namoroka/3.6.3 polymorphe) Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2.3) Gecko/20100402 Lightningphoenix/3.6.3 (Namoroka/3.6.3 polymorphe) The process icons are taken from the /usr/lib/firefox-3.6/chrome/icons/default directory, while the launcher icon is taken from the system icon theme, which is chosen by the user, and might provide a custom icon for Firefox. This can create a lot visual inconsistencies, especially when using a dock that will use the launcher icon. See screenshot for more details : http://i12.photobucket.com/albums/a243/OxayotlTheGreat/Capture-1-16.png Reproducible: Always Actual Results: http://i12.photobucket.com/albums/a243/OxayotlTheGreat/Capture-1-16.png Expected Results: The process icon should be taken from the system icon theme.
Assignee | ||
Updated•14 years ago
|
Severity: normal → major
Assignee | ||
Comment 1•14 years ago
|
||
Comment 3•14 years ago
|
||
FYI : http://www.omgubuntu.co.uk/2010/09/cmon-mozilla-show-me-my-firefox-icon.html
Assignee | ||
Comment 4•14 years ago
|
||
I forgot to mention that the patch is for the file widget/src/gtk2/nsWindows.cpp
Comment 5•14 years ago
|
||
I am going to mark as New since Yi mentioned in forums that he is seeing the bug with our recompiled firefox code on archlinux and not with a distro version of Firefox. Yi, here is the documentation to make a patch on mozilla code that can be reviewed: https://developer.mozilla.org/En/Developer_Guide/How_to_Submit_a_Patch CCing Paul Rouget, from the engagement team since this bug derives from a thread in French Ubuntu Forums on how to get into our bug reporting process (http://forum.ubuntu-fr.org/viewtopic.php?pid=3702792#p3702792)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee | ||
Comment 6•14 years ago
|
||
Attachment #470187 -
Attachment is obsolete: true
Updated•14 years ago
|
Attachment #471473 -
Flags: review?(roc)
Comment 7•14 years ago
|
||
(Yi had problems with bugzilla, I asked the review to roc for him)
Comment 8•14 years ago
|
||
Comment on attachment 471473 [details] [diff] [review] Same as the first patch, but built the canonical way Hi Yi, thanks for your contribution! >+ list = g_list_append(list, gtk_icon_theme_load_icon (gtk_icon_theme_get_default (),"firefox",16,GTK_ICON_LOOKUP_FORCE_SIZE,NULL)); We don't put an extra white space after function name >+ list = g_list_append(list, gtk_icon_theme_load_icon (gtk_icon_theme_get_default (),"firefox",32,GTK_ICON_LOOKUP_FORCE_SIZE,NULL)); >+ list = g_list_append(list, gtk_icon_theme_load_icon (gtk_icon_theme_get_default (),"firefox",48,GTK_ICON_LOOKUP_FORCE_SIZE,NULL)); >+ list = g_list_append(list, gtk_icon_theme_load_icon (gtk_icon_theme_get_default (),"firefox",64,GTK_ICON_LOOKUP_FORCE_SIZE,NULL)); >+ list = g_list_append(list, gtk_icon_theme_load_icon (gtk_icon_theme_get_default (),"firefox",128,GTK_ICON_LOOKUP_FORCE_SIZE,NULL)); You could put get_icon_theme_get_default out of the cycle as: GtkIconTheme *theme = gtk_icon_theme_get_default();
Assignee | ||
Comment 9•14 years ago
|
||
Attachment #471473 -
Attachment is obsolete: true
Attachment #471619 -
Flags: review?(roc)
Attachment #471473 -
Flags: review?(roc)
Assignee | ||
Comment 10•14 years ago
|
||
Thanks, Zbigniew. I updated the patch.
Comment 11•14 years ago
|
||
Great, sorry I didn't catch this earlier, but as you're waiting for roc to give a review, another nit - add spaces after each arg., like: g_list_append(list, gtk_icon_theme_load_icon(theme, "firefox", 128, GTK_ICON_LOOKUP_FORCE_SIZE, NULL)); Also, I'm not sure if "firefox" should be hardcoded here (we have the brandShortName, which is different for Minefield, Firefox, Seamonkey etc.)
Assignee | ||
Comment 12•14 years ago
|
||
Well, I don't know if minefield's .desktop use a specific icon. But if it does, then you're right. If it doesn't, we need another way to adapt it.
With this patch, we ignore aIconList. Perhaps we should be asking the theme first and then, if the theme didn't give us anything, use aIconList with the existing code? Gandalf is right that we shouldn't hardcode "firefox" here. That would definitely break Thunderbird for example. You could get the brandShortName like we do here for example: http://mxr.mozilla.org/mozilla-central/source/browser/components/shell/src/nsGNOMEShellService.cpp#242
Also instead of hand-writing several calls to gtk_icon_theme_load_icon to build this list, why can't we just call gtk_window_set_icon_name? We could call gtk_icon_theme_has_icon first, if the theme has an icon then call gtk_window_set_icon_name, otherwise take the existing code path.
Hmm, there's also the question of whether it's right to give all windows the same icons. Don't some XUL apps have different icons for different windows of the same application?
Comment 16•14 years ago
|
||
(In reply to comment #15) > Don't some XUL apps have different icons for different windows of > the same application? Neither Firefox not Thunderbird does that, but maybe Seamonkey? CC'ing Kairo.
Comment 17•14 years ago
|
||
SeaMonkey has different icons for different windows, yes, and we really should keep it that way. We're even working on having different kinds of windows grouped differently on Win7 (the platform supports that!), would be a shame if other platforms would go in the contrary way...
Comment 18•14 years ago
|
||
sure. So roc's suggestion from comment 13 makes all the sense in the world, right?
Comment 19•14 years ago
|
||
(In reply to comment #18) > sure. So roc's suggestion from comment 13 makes all the sense in the world, > right? That's something someone with code knowledge needs to reply to, I have no idea what e.g. aIconList is, nor should I need to care, right?
If we just use my suggestion in comment #13, that will break apps like Seamonkey that want different icons for different windows. To fix this properly, SetWindowIconList needs more information. We need a string we can use to look up the icon theme. nsWindow::SetIcon gets a string, though. Can we move the code to look up the icon them into SetIcon and extract a string to use from the parameter?
Assignee | ||
Comment 21•14 years ago
|
||
Ok, so, just to check if I understand right how it works : all the code in nsWindows.cpp is shared between all XUL applications. The functions that are called by application-specific code are nsWindow::SetIcon and nsWindow::SetDefaultIcon . So I think we should change nsWindow::SetDefaultIcon in such a way that it looks in the current GTK icon theme for an icon called like brandShortName. If it does found one, it calls SetWindowIconList on them. To build the path list we must provide to SetWindowIconList, we can use the gtk_icon_info_get_filename. If it does not found one, we revert to the current behaviour. How does that sounds to you ? Should I try to write a patch that works this way ?
Assignee | ||
Comment 22•14 years ago
|
||
One good improvement would be to make the string completely lowercase.
Attachment #471619 -
Attachment is obsolete: true
Attachment #478004 -
Flags: review?(roc)
Attachment #471619 -
Flags: review?(roc)
This patch is pretty good. I don't think you need those new #includes, though.
Assignee | ||
Comment 24•14 years ago
|
||
You were right, the first include was a leftover of my first attempt to get the brandname, and the second was here only for debugging purpose, when I added a printf… Now the brandname is converted to lowercase, because gtk_icon_theme_load_icon is case-sensitive, and iconnames are always lowercase-only.
Attachment #478004 -
Attachment is obsolete: true
Attachment #478212 -
Flags: review?(roc)
Attachment #478004 -
Flags: review?(roc)
Assignee | ||
Comment 25•14 years ago
|
||
I forgot to free the string I used in my previous patch. And cBrand.get() returns a null-terminated string, so I don't need to use strlen.
Attachment #478212 -
Attachment is obsolete: true
Attachment #478216 -
Flags: review?(roc)
Attachment #478212 -
Flags: review?(roc)
Comment on attachment 478216 [details] [diff] [review] Improved patch Thanks!
Attachment #478216 -
Flags: review?(roc) → review+
Assignee | ||
Comment 27•14 years ago
|
||
Added a fallback mechanism that uses the old way to get the icon if it is not present in the GTK theme. Useful for some brandname that are not included in most GTK theme, like minefield.
Attachment #478216 -
Attachment is obsolete: true
Attachment #478755 -
Flags: review+
Assignee | ||
Updated•14 years ago
|
Keywords: checkin-needed
Comment 28•14 years ago
|
||
(In reply to comment #27) > Created attachment 478755 [details] [diff] [review] > Improved patch I think you actually meant to ask for a new review, instead of reviewing the patch yourself? You should probably edit your attachment to set the review field to "?" and put roc as requestee again.
Assignee | ||
Updated•14 years ago
|
Attachment #478755 -
Flags: review+ → review?(roc)
Assignee | ||
Comment 29•14 years ago
|
||
Yes, you are right. Thanks.
We use camelCase. So use currentIcon and iconName. + if(currenticon) + {list = g_list_append(list, currenticon);} Our style requires if (currentIcon) { list = g_list_append(list, currentIcon); } Also, instead of repeating the same code 5 times, use a loop. E.g. static const int sizes[5] = { 16, 32, 48, 64, 128 }; for (int i = 0; i < NS_ARRAY_LENGTH(sizes); ++i) {
Assignee | ||
Comment 31•14 years ago
|
||
Thanks ! I also camelCased iconname to iconName.
Attachment #478755 -
Attachment is obsolete: true
Attachment #481502 -
Flags: review?(roc)
Attachment #478755 -
Flags: review?(roc)
+ currentIcon = gtk_icon_theme_load_icon(theme,iconName,16,GTK_ICON_LOOKUP_FORCE_SIZE,NULL); You shouldn't be using "16" here + if (currentIcon) { + list = g_list_append(list, currentIcon); + } Fix indent + if (!list) + { { on the same line as the if
Assignee | ||
Comment 33•14 years ago
|
||
Fixed.
Attachment #481502 -
Attachment is obsolete: true
Attachment #481854 -
Flags: review?(roc)
Attachment #481502 -
Flags: review?(roc)
OK, the code looks correct now, but there are just a few more style nits :-) + gchar* iconName = g_utf8_strdown(cBrand.get(),-1); space after , + currentIcon = gtk_icon_theme_load_icon(theme,iconName,sizes[i],GTK_ICON_LOOKUP_FORCE_SIZE,NULL); space after commas + if (!list){ Space between ) and {
Assignee | ||
Comment 35•14 years ago
|
||
Done.
Attachment #481854 -
Attachment is obsolete: true
Attachment #481973 -
Flags: review?(roc)
Attachment #481854 -
Flags: review?(roc)
Comment on attachment 481973 [details] [diff] [review] Improved patch + currentIcon = gtk_icon_theme_load_icon(theme, iconName,sizes[i], GTK_ICON_LOOKUP_FORCE_SIZE, NULL); You missed a space here. Please fix that, attach the final patch and request checkin-needed. Thanks!!!
Attachment #481973 -
Flags: review?(roc) → review+
Assignee | ||
Comment 37•14 years ago
|
||
Attachment #481973 -
Attachment is obsolete: true
Attachment #482018 -
Flags: review?
Assignee | ||
Comment 38•14 years ago
|
||
Err, I had already put checkin-needed in the keyword of the bug. Is it the right way ?
Assignee | ||
Updated•14 years ago
|
Attachment #482018 -
Flags: review? → review?(roc)
Attachment #482018 -
Flags: review?(roc) → review+
Comment 39•14 years ago
|
||
Comment on attachment 482018 [details] [diff] [review] Improved patch Roc, could you approve the patch? It looks like it can't land now...
Attachment #482018 -
Flags: approval2.0?
Updated•14 years ago
|
Keywords: checkin-needed
Attachment #482018 -
Flags: approval2.0? → approval2.0+
Updated•14 years ago
|
Assignee: nobody → jagw40k
Status: NEW → ASSIGNED
Version: unspecified → Trunk
Comment 40•14 years ago
|
||
Unfortunately, GTK_ICON_LOOKUP_FORCE_SIZE needs gtk2-2.14.0, see: http://library.gnome.org/devel/gtk/unstable/GtkIconTheme.html#GTK-ICON-LOOKUP-FORCE-SIZE:CAPS So we need to update configure.in but also our build machines (this seems to be the hard part)
Attachment #482486 -
Flags: review?(khuey)
Comment 41•14 years ago
|
||
Jean-Alexandre, I refreshed your patch while trying to push it (and fixed a warning).
Attachment #482018 -
Attachment is obsolete: true
Comment on attachment 482486 [details] [diff] [review] Update configure.in This is fine by me, but I'd prefer that the gtk2 widget people approved this.
Attachment #482486 -
Flags: review?(khuey)
Attachment #482486 -
Flags: review?(karlt)
Attachment #482486 -
Flags: feedback+
Comment 43•14 years ago
|
||
Comment on attachment 482487 [details] [diff] [review] Refreshed patch >+ currentIcon = gtk_icon_theme_load_icon(theme, iconName, sizes[i], GTK_ICON_LOOKUP_FORCE_SIZE, NULL); GTK_ICON_LOOKUP_FORCE_SIZE doesn't seem right here "gtk_window_set_icon_list() allows you to pass in the same icon in several hand-drawn sizes. The list should contain the natural sizes your icon is available in; that is, don't scale the image before passing it to GTK+. Scaling is postponed until the last minute, when the desired final size is known, to allow best quality." and GtkWindow does not use GTK_ICON_LOOKUP_FORCE_SIZE. AFAIK there is no theme-change handling here (unless SetDefault is going to be called every time the theme changes). I think it would be best to let GTK do the lookup, set the icon list, and handle theme changes. This can be done by using gtk_window_set_icon_name(). The slightly tricky bit then is determining whether GTK will find icons for that name. It looks like it should be reasonable to assume that, if gtk_icon_theme_get_icon_sizes() returns a non-empty array, then GTK will have some icons to use. >+ gtk_window_set_icon_list(GTK_WINDOW(mShell), list); This function will need an early return when mShell is NULL.
Attachment #482487 -
Flags: review-
Comment 44•14 years ago
|
||
Comment on attachment 482486 [details] [diff] [review] Update configure.in >-GTK2_VERSION=2.10.0 >+GTK2_VERSION=2.14.0 It is about time we updated out minimum requirements but that should follow broader discussion in newsgroups and will require updating build machines. GTK_ICON_LOOKUP_FORCE_SIZE is not a good reason to make this change, though.
Attachment #482486 -
Flags: review?(karlt) → review-
Comment 45•14 years ago
|
||
Jean-Alexandre, is it possible to use something else than GTK_ICON_LOOKUP_FORCE_SIZE which is available in 2.10 and open a follow-up to use GTK_ICON_LOOKUP_FORCE_SIZE when gtk-2.14.0 will be required?
Assignee | ||
Comment 46•14 years ago
|
||
I think we can just remove the GTK_ICON_LOOKUP_FORCE_SIZE. But I am completely unable to figure out how to call gtk_icon_theme_load_icon without a GtkIconLookupFlags argument… That's actually why I put GTK_ICON_LOOKUP_FORCE_SIZE in the first place.
Comment 47•14 years ago
|
||
(In reply to comment #46) > I think we can just remove the GTK_ICON_LOOKUP_FORCE_SIZE. But I am completely > unable to figure out how to call gtk_icon_theme_load_icon without a > GtkIconLookupFlags argument… > That's actually why I put GTK_ICON_LOOKUP_FORCE_SIZE in the first place. Would another flag from this list working? http://library.gnome.org/devel/gtk/unstable/GtkIconTheme.html#GtkIconLookupFlags Otherwise, you can try to pass 0 and test if that's working as expected?
Comment 48•14 years ago
|
||
(In reply to comment #47) > Otherwise, you can try to pass 0 and test if that's working as expected? Actually, GTK_ICON_LOOKUP_NO_SVG is equal to 0... I guess you might know (or at least test) which flag should be used.
Assignee | ||
Comment 49•14 years ago
|
||
We could try GTK_ICON_LOOKUP_GENERIC_FALLBACK. I've already tried passing 0, it won't compile. I also tried NULL, or just removing the argument…
Comment 50•14 years ago
|
||
(In reply to comment #49) > We could try GTK_ICON_LOOKUP_GENERIC_FALLBACK. I've already tried passing 0, it > won't compile. I also tried NULL, or just removing the argument… Passing 0 isn't valid because it's expecting a GtkIconLookupFlags type. NULL is often equivalent to 0 is C++ (might be (void*)0 sometimes). Removing the argument can't work because, in C++, the argument need to be explicitly optional (which is not the case). So, we have to pick a value. Unfortunately, GTK_ICON_LOOKUP_GENERIC_FALLBACK is 2.12 and we need something that is available in 2.10. Can we use GTK_ICON_LOOKUP_NO_SVG? Karl, would that be fine?
Assignee | ||
Comment 51•14 years ago
|
||
I think GTK_ICON_LOOKUP_USE_BUILTIN would be better. We know there are no builtin icons for Firefox, Thunderbird, Seamonkey or any other Xul application, so this flag doesn't do anything, I guess.
Comment 52•14 years ago
|
||
There's no need to use gtk_icon_theme_load_icon. Only call gtk_window_set_icon_name(), and GTK will handle finding the available sizes, etc. It looks like a gtk_window_set_icon_name(window, NULL) will also be needed in nsWindow::SetIcon(). Otherwise, if SetDefaultIcon is subsequently called again, gtk_window_set_icon_name() won't notice that a change is required. (In reply to comment #43) > The slightly tricky bit then is determining whether GTK will find icons for > that name. It looks like it should be reasonable to assume that, if > gtk_icon_theme_get_icon_sizes() returns a non-empty array, then GTK will have > some icons to use. Easier/better than that is to check gtk_window_get_icon after calling gtk_window_set_icon_name() from SetDefaultIcon.
Comment 53•14 years ago
|
||
Jean-Alexandre, can you update your patch with recommendations from comment 52?
Assignee | ||
Comment 54•14 years ago
|
||
Err, I don't know how to handle theme change. If the user switches from a theme with a brandName icon to a theme without one, how can we go back to the default behavior ? I think one workaround could be to add a brandName icon to the hicolor icon theme during install, but I don't know how to do that…
Comment 55•14 years ago
|
||
(In reply to comment #54) > Err, I don't know how to handle theme change. If the user switches from a theme > with a brandName icon to a theme without one, how can we go back to the default > behavior ? Oh, thanks, I hadn't thought about that. > I think one workaround could be to add a brandName icon to the hicolor icon > theme during install, but I don't know how to do that… I don't know enough about that to comment. One thing I thought about was using gtk_window_set_default_icon_list from nsNativeAppSupportUnix, which would provide a default for windows without an icon_name and windows where there were no icons for the icon_name. In some ways that's tidy because nsWindow::SetDefaultIcon wouldn't need to worry about checking that there are icons for the icon_name. However, it is also unfortunate to load an icon that may not be used, which might have an impact on startup time. And it feels like it would be abusing the API a little. Possibly we should be using gtk_window_set_default_icon_name and falling back to gtk_window_set_default_icon_list when there are no icons for the name. Another possibility is to connect to the "changed" signal on the GtkIconTheme for each window that is using a themed icon (those for which SetDefaultIcon was called and found a brandName icon). GtkWindow uses this to update the icon on theme changes.: g_signal_connect (icon_theme, "changed", G_CALLBACK (update_themed_icon), window); It might be easiest to simply call SetDefaultIcon from the our signal handler. I expect that would handle switches both to and from themes with/without brandName icons. Such a signal handler should be removed for windows with non-themed icons (when SetIcon is called).
Assignee | ||
Comment 56•14 years ago
|
||
Well, why not just adding the icons we want to use to hicolor, like I proposed in comment #54 ? It's the way most programs does it, and it seems much simpler, since theme change will be handled properly by gtk_window_set_icon_name, I guess.
Comment 57•14 years ago
|
||
I'm assuming you are asking me, but I can't tell you whether that is better or not because I don't know enough about that to comment. I was mentioning another possible option, because it sounded like neither of us knew how to add an icon to hicolor. What you say makes it sound good. Would the icons be added to hicolor if not already present during startup? That might be an option. How do most programs do it? (I don't know of any installation phase with Gecko apps. It's more an untar and run situation, but I guess there must be some app-specific storage available if necessary as updates must use some kind of similar storage.) Would GTK fallback to using the hicolor icon if the selected theme did not have an icon with matching name?
Assignee | ||
Comment 58•14 years ago
|
||
Actually, all icon theme inherits from hicolor, so yes, GTK always fallback to the hicolor icon if the current icon theme doesn't provide an icon. So, there are actually two ways we can proceed. The first one is to put a firefox/whatever icon in /usr/share/icons/hicolor/sizexsize/apps, the second is to create a directory like /usr/share/firefox/icons/hicolor/sizexsize/apps, put a firefox/whatever icon in it, and add /usr/share/firefox/icons to the icon path. Many applications (brasero, empathy, evolution, …) uses both solutions : they put their application icon in /usr/share/icons/hicolor, and all the icons they use in their interface in /usr/share/applicationname/icons/hicolor. This is because the launcher (i.e. the .desktop file which describe how the program should appear in the different menu) give the icon name, but can't modify the icon search path. Currently, the firefox icon used in it's launcher is stored in /usr/share/pixmaps. I think the best way to do would be to move this icon to /usr/share/icons/hicolor/sizexsize/apps (and maybe provide different icon size). But if we do it this way, people who just untar and run won't get the right window icon. There is a way to bypass that by checking that the icon is present, and if it isn't, just adding it to a user-specific addition to the hicolor theme (we copy the icon in ~/.icon/hicolor/sizexsize/apps and we add a right index.theme in ~/.icon/hicolor/), but this seems dirty.
Comment 59•14 years ago
|
||
(In reply to comment #58) > Actually, all icon theme inherits from hicolor, so yes, GTK always fallback to > the hicolor icon if the current icon theme doesn't provide an icon. Thanks. Sounds good then. > So, there are actually two ways we can proceed. The first one is to put a > firefox/whatever icon in /usr/share/icons/hicolor/sizexsize/apps, the second is > to create a directory like /usr/share/firefox/icons/hicolor/sizexsize/apps, put > a firefox/whatever icon in it, and add /usr/share/firefox/icons to the icon > path. > > [...] > > Currently, the firefox icon used in it's launcher is stored in > /usr/share/pixmaps. > I think the best way to do would be to move this icon to > /usr/share/icons/hicolor/sizexsize/apps (and maybe provide different icon > size). I don't know whether or not mozilla-central has an installer script that can be modified to do this. It seems that the file in /usr/share/pixmaps is copied there by scripts in the distribution packages. > But if we do it this way, people who just untar and run won't get the > right window icon. There is a way to bypass that by checking that the icon is > present, and if it isn't, just adding it to a user-specific addition to the > hicolor theme (we copy the icon in ~/.icon/hicolor/sizexsize/apps and we add a > right index.theme in ~/.icon/hicolor/), but this seems dirty. It looks like gtk_icon_theme_add_builtin_icon() can be used to add default icons during startup without needing to touch ~/.icon/hicolor. It's unfortunate to load the files during startup when they may not be used, but it's got to be better than what we currently do, which is to load the files for every new window. Perhaps, a better option would be to rearrange the file in chrome/icons/default/ (or add links) so that the directory can be added with gtk_icon_theme_append_search_path(), but that might be more work.
Comment 60•14 years ago
|
||
We need to be able to run correctly (and with correct icons) when _not_ running any installation script. The way to "install" our official downloads is to to just unpack the .tar.bz2 and run the "firefox" (or "thunderbird", or "seamonkey) script in it. No other steps needed, and the application is supposed to act correctly in every way with just that.
Assignee | ||
Comment 61•14 years ago
|
||
I was trying to test a new patch using gtk_window_set_icon_name combined with gtk_icon_theme_append_search_path, but I can't get Firefox to compile anymore, since it is looking for Python 2.6, and I upgraded to Python 2.7… Any suggestion ?
Comment 62•14 years ago
|
||
(In reply to comment #61) > it is looking for Python 2.6, and I upgraded to Python 2.7… > Any suggestion ? Yes, either clobber your objdir or at least delete config.* in there and re-run configure.
Assignee | ||
Comment 63•14 years ago
|
||
Thanks, worked perfectly ! Now, I have something that works perfectly, I just need to be able to find where the icons are, to use it as the argument for gtk_icon_theme_append_search_path. That is, - if it is an official pre-compiled version unpacked without any installation script, I must use /path/to/the/extracted/files/icons (and there must be a icons folder in the archive, with the right icons are the right place, but that should be too hard) - if it is an installation from source with make install, I must use /usr/share/firefox/icons (or $prefix/share/firefox/icons, I guess), and we should also change make install so that it creates the right folder at the right place. (I used the specification given here : http://live.gnome.org/ThemableAppSpecificIcons . BTW, in the long-term, it would provide a much better integration if all the icons from the GTK ui were put in an icons folder added to the hicolor icon theme. It would allow theme-makers to provide custom version of every icon of Firefox. For example, my icon theme provides a new window icon, and also a new tab icon, but they are not used by Firefox. With those changes, I could fix it in my theme.)
Comment 64•14 years ago
|
||
Well, all the icons/ folder stuff basically breaks our cross-platform support probably significantly. Not sure if that's ideal.
Assignee | ||
Comment 65•14 years ago
|
||
Could you be more specific ? How does creating a specific directory for the gtk interface breaks cross-platform support ?
Comment 66•14 years ago
|
||
Well, we're supposed to be able to have some kind of pluggable structure under chrome/icons that allows us to put in icons with the window names and have them used by the windows in some way across platforms. I never personally used this much, but I skimmed posts explaining this to people, not sure the details of the workings of it. When it comes to icons being used in themes, that's even worse in that regard, but please let's put that discussion somewhere else than this bug.
Assignee | ||
Comment 67•14 years ago
|
||
I guess you are speaking of the SetIcon method. My patch only modifies the SetDefaultIcon method, that used to just call SetIcon with "default" as the argument. So I guess you can still put icons with the window name and have them used by the windows in some way across the platform.
Comment 68•14 years ago
|
||
(In reply to comment #66) > Well, we're supposed to be able to have some kind of pluggable structure under > chrome/icons that allows us to put in icons with the window names and have them > used by the windows in some way across platforms. Looking at http://hg.mozilla.org/mozilla-central/file/39de140a8bdd/other-licenses/branding/firefox/Makefile.in#l74 it seems that the filenames are already quite different across platforms. However, I had mis-assumed that there was only one directory structure that would need rearranging (or supplementing with a new link structure). As I think Robert was alluding to, XUL extensions can add their own themes with their own directories of icons and these are would also need to be considered. Making gtk_icon_theme_append_search_path() work with icons from extensions would pretty much require extensions to adapt to the new structure and also require adding extension directories to the search path. I suspect the easiest way to get something working here is going to be by using the existing icon file finding code and gtk_icon_theme_add_builtin_icon(). Given the existing ResolveIconName code in nsBaseWidget, this is probably most easily done in widget/gtk2 code when the icon is first required.
Assignee | ||
Comment 69•14 years ago
|
||
Do you mean that the problem comes from third-party theme that want to modify the window icon ? Or for interface icons like the big red one from AdBlock+ ? Let's stick to window icon here, I'll open a bug for interface icon later. Since I guess third-party theme uses SetIcon and not SetDefaultIcon, I think my method is still valid.
Comment 70•14 years ago
|
||
(In reply to comment #69) > Do you mean that the problem comes from third-party theme that want to modify > the window icon ? I did mean the window icon. I don't know for certain that there are XUL themes that change the default window icon, but the code (currently executed by SetDefaultIcon) looks like it provides for this: http://hg.mozilla.org/mozilla-central/annotate/eaf6f9566f2e/widget/src/xpwidgets/nsBaseWidget.cpp#l1133 http://hg.mozilla.org/mozilla-central/annotate/eaf6f9566f2e/toolkit/xre/nsXREDirProvider.cpp#l680
Assignee | ||
Comment 71•14 years ago
|
||
Well, I'm sorry, I just don't understand anything about the code you are linking to. I never learned C++, I don't know how much about XUL, and if someone with better knowledge could take care of that bug, it would be great.
Comment 72•13 years ago
|
||
Comment on attachment 482018 [details] [diff] [review] Improved patch (bookkeeping)
Attachment #482018 -
Flags: approval2.0+
Updated•13 years ago
|
Whiteboard: not-ready-for-cedar
Assignee | ||
Comment 73•13 years ago
|
||
Ok, I wish to make sure I understand the problem with the current patch. The problem is that a theme might change the folders in which the XUL application looks for it default icon, and if we apply the current patch, then this modification of the search path won't have any effect. Is that it ?
Comment 74•13 years ago
|
||
(In reply to Jean-Alexandre Anglès d'Auriac from comment #73) I guess you mean your patch referenced in comment 61 and comment 63. I haven't seen that patch so I'm guessing about how it might be working, but yes, a Gecko theme/extension might add folders in which the app looks for its default and non-default icons. I have trouble imagining how that would continue to work if nsBaseWidget::ResolveIconName() is no longer used. However, I see nsCocoaWindow (for Mac OS) doesn't implement nsIWidget::SetIcon() at all, and Mac windows don't have icons. Gecko extensions therefore can't modify window icons in a cross-platform manner. On Mac, Thunderbird modifies its launcher icon with the message count, but there must be some other code that does this. There may be very little value in continuing to support changing the default icon through Gecko extensions. Maybe we should try switching from nsBaseWidget::ResolveIconName() to gtk_icon_theme_append_search_path(), if that is what you have in mind.
Assignee | ||
Comment 75•13 years ago
|
||
Yes, I was. If a patch that works even though not supporting changing the default icon the old way can be accepted, that's good news ! I'll resume working on working on a patch using gtk_icon_theme_append_search_path() and gtk_window_set_icon_name() then.
Assignee | ||
Comment 76•13 years ago
|
||
I don't know if I should put the icons in NS_APP_CHROME_DIR, or in one of the directories in NS_APP_CHROME_DIR_LIST, or elsewhere. Currently, it seems to me that the code search for icons in both NS_APP_CHROME_DIR and NS_APP_CHROME_DIR_LIST. Cf http://hg.mozilla.org/mozilla-central/annotate/eaf6f9566f2e/widget/src/xpwidgets/nsBaseWidget.cpp#l1112 Which one makes more sense ?
Comment 77•13 years ago
|
||
The default icons are currently in $(DIST)/bin/chrome/icons/default. Without any other influencing factors, I'd aim for something similar. http://hg.mozilla.org/mozilla-central/annotate/086dea3f0bad/browser/app/Makefile.in#l194
Assignee | ||
Comment 78•13 years ago
|
||
Actually, we need to put them in $(DIST)/bin/chrome/hicolor/sizexsize/apps. And there is also another issue I didn't think about. The icon names should be changed to match GetBrandName put in lowercase. Is it a problem ?
Component: Shell Integration → Private Browsing
Assignee | ||
Comment 79•13 years ago
|
||
Attachment #575978 -
Flags: review?(roc)
Assignee | ||
Updated•13 years ago
|
Assignee: jagw40k → nobody
Component: Private Browsing → Shell Integration
Updated•13 years ago
|
Component: Shell Integration → Widget: Gtk
Product: Firefox → Core
QA Contact: shell.integration → gtk
Whiteboard: not-ready-for-cedar
Attachment #575978 -
Flags: review?(roc) → review?(karlt)
Assignee | ||
Comment 80•13 years ago
|
||
This patch works without any change to the Makefile or in the place or name of the icons. I did make it also provide GTK theming for non-default icon, however we can remove that, or add a moz- prefix to the icon name, if it causes problem.
Attachment #576886 -
Flags: review?(karlt)
Comment 81•13 years ago
|
||
Comment on attachment 575978 [details] [diff] [review] Patch that uses gtk_icon_theme_append_search_path and gtk_window_set_default_icon_name This code is setting up GTK to look for default icons under NS_APP_CHROME_DIR. Such an approach should be fine if the appropriate Makefile changes can be made to alter the icon install locations so that GTK will find the distributed icons. However, the purpose of nsWindow::SetDefaultIcon() is to make that particular window have the default icon. With such an approach, gtk_icon_theme_append_search_path and gtk_window_set_default_icon_name should be called only once instead of for each window. There should be some code to ensure that SetDefaultIcon() undoes the effects of a previous SetIcon() call. I assume gtk_window_set_icon_list(GTK_WINDOW(mShell), NULL) would do that. But I suspect SetIcon() should also be modified to use gtk_window_set_icon_name() so that all icons will be installed in the same kind of directory structure. Note also that nsXULWindow expects SetIcon("default") to select the default icon. http://hg.mozilla.org/mozilla-central/annotate/cb7297c7297c/xpfe/appshell/src/nsXULWindow.cpp#l1403 >+ nsXPIDLString brandName; >+ GetBrandName(brandName); >+ NS_ConvertUTF16toUTF8 cBrand(brandName); >+ gchar* iconName = g_utf8_strdown(cBrand.get(), -1); iconName would need to be freed, but instead use ToLowerCase() from nsREadableUtils to change case in place. >+ gtk_window_set_default_icon_name("iconName"); ITYM the variable iconName instead of the literal string.
Attachment #575978 -
Flags: review?(karlt) → review-
Comment 82•13 years ago
|
||
Comment on attachment 576886 [details] [diff] [review] Patch that uses gtk_icon_theme_add_builtin_icon and gtk_window_set_default_icon_name This gtk_icon_theme_add_builtin_icon approach is also fine if that is easier. It should be possible to share the code rather than copying SetIcon code to SetDefaultIcon. Many of my comments on the gtk_icon_theme_append_search_path patch also apply here.
Updated•13 years ago
|
Attachment #576886 -
Flags: review?(karlt) → review-
Assignee | ||
Comment 83•13 years ago
|
||
Attachment #576886 -
Attachment is obsolete: true
Attachment #579689 -
Flags: review?(karlt)
Assignee | ||
Comment 84•13 years ago
|
||
(In reply to Karl Tomlinson (:karlt) from comment #81) > However, the purpose of nsWindow::SetDefaultIcon() is to make that particular > window have the default icon. With such an approach, > gtk_icon_theme_append_search_path and gtk_window_set_default_icon_name should > be called only once instead of for each window. When should it be done ? I couldn't find where gtk_init() is called. I tried to do it in moz_gtk_init, but it is in gtk2drawing.c, and I couldn't use the C++ fonction needed to get the NS_APP_CHROME_DIR path.
Comment 85•13 years ago
|
||
(In reply to Jean-Alexandre Anglès d'Auriac from comment #84) > (In reply to Karl Tomlinson (:karlt) from comment #81) > > With such an approach, > > gtk_icon_theme_append_search_path and gtk_window_set_default_icon_name should > > be called only once instead of for each window. > When should it be done ? The first time SetIcon or SetDefaultIcon is called should be fine. You can use a static variable to record whether or not this has already been done.
Comment 86•13 years ago
|
||
Comment on attachment 579689 [details] [diff] [review] Patch that uses gtk_icon_theme_add_builtin_icon and gtk_window_set_default_icon_name Similarly with this approach, gtk_icon_theme_add_builtin_icon should be called only once for each iconName. I don't see a reasonable way to find out from GTK what we have added, so we'll have to keep track of that ourselves. I suspect it would be easiest to do this only for the default icon, using a single static variable. It would be possible to do this for all icons by keeping a set of icons that have been added. >+ if (!g_strcmp0("default",iconName.get())){ iconName.EqualsLiteral("default") (g_strcmp0 is not available in the GLib against which we build.) >+ gboolean foundicon = FALSE; Use the C++ bool/true/false where practical, saving gboolean just for interaction with GLib. bool is a real boolean and its use follows Gecko convention. gboolean is actually an integer. >+ GdkPixbuf *icon = gdk_pixbuf_new_from_file(path.get(), NULL); Check for null icon before adding it. Need to unref after adding, I expect.
Attachment #579689 -
Flags: review?(karlt) → review-
Assignee | ||
Comment 87•13 years ago
|
||
(In reply to Karl Tomlinson (:karlt) from comment #85) > The first time SetIcon or SetDefaultIcon is called should be fine. You can > use a static variable to record whether or not this has already been done. Well, if I want something that could be reusable for bug #705070, it would need to be done beforehand, during the GTK initialization, I guess I think that maybe, instead of using a static boolean, we could use gtk_icon_theme_has_icon to check if adding a builtin is required, what do you think about that ? Therefore, we could do it for every icon, and we only do it if really needed, which should be very rare (almost every distribution adds a firefox and thunderbird icon to the icon theme, so that it can be used for the .desktop launcher). Updated patch coming tomorow when I've got the time to test-compile it.
Assignee | ||
Comment 88•13 years ago
|
||
Attachment #579689 -
Attachment is obsolete: true
Attachment #579932 -
Flags: review?(karlt)
Assignee | ||
Comment 89•13 years ago
|
||
I updated the wrong patch.
Attachment #579932 -
Attachment is obsolete: true
Attachment #579933 -
Flags: review?(karlt)
Attachment #579932 -
Flags: review?(karlt)
Comment 90•13 years ago
|
||
Comment on attachment 579933 [details] [diff] [review] Patch that uses gtk_icon_theme_add_builtin_icon and gtk_window_set_default_icon_name I like this approach, thank you. It doesn't address the situation with existing windows when the user switches from a theme with a brandName icon to a theme without one (comment 54), but this approach is better in other ways, and more code can be added in the future if that situation turns out to be important. >+static void GetBrandName(nsXPIDLString& brandName); Can you move this to the top of the file with other forward declarations, please. >+ NS_ConvertUTF16toUTF8 iconName(aIconSpec); >+ >+ if (iconName.EqualsLiteral("default")){ >+ nsXPIDLString brandName; >+ GetBrandName(brandName); >+ NS_ConvertUTF16toUTF8 brandName8(brandName); >+ ToLowerCase(brandName8); >+ iconName = brandName8; >+ } I think we can save a copy and do this slightly more efficiently: nsCAutoString iconName if (iconName.EqualsLiteral("default")){ nsXPIDLString brandName; GetBrandName(brandName); AppendUTF16toUTF8(brandName, iconName); ToLowerCase(iconName); } else { AppendUTF16toUTF8(aIconSpec, iconName); } >+ bool foundicon = false; Normally Gecko uses camelCase for variables, so this would be "foundIcon". >+ if(!gtk_icon_theme_has_icon(gtk_icon_theme_get_default(),iconName.get())){ Need a space after "if", after "," and before "{". But how about doing this to make the "else" unnecessary: bool foundIcon = gtk_icon_theme_has_icon(gtk_icon_theme_get_default(), iconName.get()); if (!foundIcon) { ... } >- iconList.AppendElement(path); >- return SetWindowIconList(iconList); iconList and SetWindowIconList are now unused and so can be removed. >+ if(icon){ >+ gtk_icon_theme_add_builtin_icon(iconName.get(),gdk_pixbuf_get_height(icon),icon); Spaces needed to follow file style here too, and split the argument list across lines to keep within 80 columns. >+ g_free(icon); GdkPixbuf is a GObject, so use g_object_unref instead of g_free.
Attachment #579933 -
Flags: review?(karlt)
Attachment #579933 -
Flags: review-
Attachment #579933 -
Flags: feedback+
Assignee | ||
Comment 91•13 years ago
|
||
I think I should move g_signal_connect(gtk_icon_theme_get_default(), "changed", G_CALLBACK(checkIconOnIconThemeChange), mShell) somewhere else, in some part of the code that is executed only once, but I have no idea where.
Attachment #579933 -
Attachment is obsolete: true
Attachment #580190 -
Flags: review?(karlt)
Comment 92•13 years ago
|
||
Comment on attachment 580190 [details] [diff] [review] Patch that uses gtk_icon_theme_add_builtin_icon and gtk_window_set_default_icon_name (In reply to Jean-Alexandre Anglès d'Auriac from comment #91) > I think I should move g_signal_connect(gtk_icon_theme_get_default(), > "changed", G_CALLBACK(checkIconOnIconThemeChange), mShell) somewhere else, > in some part of the code that is executed only once, but I have no idea > where. This may be tricky to get right and I don't think it is important, so I suggest leaving this out of this patch. We can consider a subsequent patch if someone has tested that it works. > >+static void GetBrandName(nsXPIDLString& brandName); >+void checkIconOnIconThemeChange(GtkIconTheme *icontheme, GtkWidget *window); > /* callbacks from widgets */ Leave the blank line before the "callbacks from widgets" section, please. >+ if (aIconSpec.EqualsLiteral("default")){ I see I left out the space between ")" and "{" in my own copy-paste. Can you add that in, please? >+ if (!foundIcon) { > // Look for icons with the following suffixes appended to the base name. Can you indent the comment lines too, please? The now-unused SetWindowIconList() method should be removed.
Attachment #580190 -
Flags: review?(karlt) → review-
Assignee | ||
Comment 93•13 years ago
|
||
Attachment #580190 -
Attachment is obsolete: true
Attachment #580359 -
Flags: review?(karlt)
Assignee | ||
Comment 94•13 years ago
|
||
I guess the right place to add this signal is in the fonction that create the window, just after the default icon has been set up. I actually tested the patch. I had to manually remove all the firefox icons from the hicolor theme, and remove /usr/share/icons/hicolor/icon-theme.cache (finding that took me quite some time), to test it. At the beginning, it was causing firefox to segfault on exit, that's why I replaced “if(!(iconTheme && window))” by “if(!(GTK_IS_ICON_THEME(iconTheme) && GTK_IS_WINDOW(window)))”. I switched themes a few dozen times, and it seemed to work pretty much fine. If you have any suggestion on how to test it further, tell me.
Attachment #580431 -
Flags: review?(karlt)
Comment 95•13 years ago
|
||
Comment on attachment 580359 [details] [diff] [review] Patch that uses gtk_icon_theme_add_builtin_icon and gtk_window_set_default_icon_name Looks good. Thank you for tidying this up.
Attachment #580359 -
Flags: review?(karlt) → review+
Assignee | ||
Comment 96•13 years ago
|
||
Ok, so referring to this article : https://developer.mozilla.org/en/Hacking_Mozilla I should now either ask you to push the patch, or add checkin-needed to the keyword of the bug. Is that right ? Which one should I do ? Should I wait to see if the other patch is also alright, to combine them into one patch before pushing them ?
Comment 97•13 years ago
|
||
Comment on attachment 580431 [details] [diff] [review] Patch that add the builtin icon if needed on theme change (In reply to Jean-Alexandre Anglès d'Auriac from comment #94) > I guess the right place to add this signal is in the fonction that create > the window, just after the default icon has been set up. The Create method is a suitable place, yes. SetIcon() was probably a suitable place too, but here it is clear that the call happens before gtk_widget_realize(). (I don't think it matters whether it is before or after the SetDefaultIcon(), but I assume it is important that it is before gtk_widget_realize - see below.) >+checkIconOnIconThemeChange(GtkIconTheme *iconTheme, GtkWidget *window) Gecko uses CapitalizedCamelCase for function names and uncapitalizedCamelCase for variable names, so please capitalize the first "C" of check. Please use "static" to give this function internal linkage. >+ const gchar* uiconName = gtk_window_get_icon_name(GTK_WINDOW(window)); >+ if (!gtk_icon_theme_has_icon(iconTheme,uiconName)){ >+ nsWindow *nsWindow = get_window_for_gtk_widget(window); >+ nsWindow->SetIcon(NS_LITERAL_STRING(iconName)); >+ } Apologies for jumping to the conclusion that this wasn't tested. I guess the "u" before "iconName" was to make NS_LITERAL_STRING(iconName) work for you, though I'm still not clear what this actually does because this is not the intended purpose of NS_LITERAL_STRING. NS_LITERAL_STRING is actually intended for strings that are known at compile time. Here you want to convert the GTK icon_name from utf-8 to utf-16. However, it is more complicated than that because the default icons are stored under the name "default", while the GTK icon_name is the brand name. > At the beginning, it was > causing firefox to segfault on exit, that's why I replaced “if(!(iconTheme > && window))” by “if(!(GTK_IS_ICON_THEME(iconTheme) && > GTK_IS_WINDOW(window)))”. It's actually mostly getting lucky that this resolved the crash. When the nsWindow is destroyed, window is deleted and so the memory at which |window| points is being recycled for other use. It may be a new window; it could even get marked as inaccessible. The signal handler needs to be removed before the window is deleted. See calls to g_signal_handlers_disconnect_by_func in nsWindow.cpp for examples of how that is done. Interestingly, it looks like the subsequent the gtk_window_set_icon_name() with the same name behaves like a no-op. It does not cause GTK to search again for icons. GTK also connects to the "changed" signal and searches again for icons when it receives this signal. Because the signal handler in this patch is registered before the signal handler that GTK registers, it runs first and then GTK's signal handler picks up the changes. > I switched themes a few dozen times, and it seemed to work pretty much fine. > If you have any suggestion on how to test it further, tell me. Did you start with a theme that has a "firefox" icon installed, and then switch to a theme that does not have a "firefox" icon?
Attachment #580431 -
Flags: review?(karlt) → review-
Assignee | ||
Comment 98•13 years ago
|
||
(In reply to Karl Tomlinson (:karlt) from comment #97) > Comment on attachment 580431 [details] [diff] [review] > Patch that add the builtin icon if needed on theme change > > The Create method is a suitable place, yes. SetIcon() was probably a > suitable > place too, but here it is clear that the call happens before > gtk_widget_realize(). (I don't think it matters whether it is before or > after the SetDefaultIcon(), but I assume it is important that it is before > gtk_widget_realize - see below.) SetIcon() seemed bad because we don't want to call g_signal_connect 4 or 5 times for each new window, and then call it once more every time the user changed his icon theme. > However, it is more complicated than that because the default icons are > stored under the name "default", while the GTK icon_name is the brand name. You are right. If the window icon is brandname, there is actually no way to find out if it was set initially set as "default" or as "brandname". This is problematic. I think a different approach can be better. What do you think about creating a completely empty GTK theme using gtk_icon_theme_set_search_path, to check if we already added builtin icons ? I tried emptypath[0]=NULL; GtkIconTheme *virgin = gtk_icon_theme_new(); gtk_icon_theme_set_search_path(virgin,emptypath,1); and it effectively gives an icon theme containing only the builtin icons. > Did you start with a theme that has a "firefox" icon installed, and then > switch to a theme that does not have a "firefox" icon? Yes, and it seemed to work. But has you have pointed, it shouldn't. I'm getting confused here. I must have made a mistake somewhere.
Assignee | ||
Comment 99•13 years ago
|
||
Tested, with extra care this time. Seemed to work fine.
Attachment #580431 -
Attachment is obsolete: true
Attachment #580983 -
Flags: review?(karlt)
Comment 100•13 years ago
|
||
(In reply to Jean-Alexandre Anglès d'Auriac from comment #98) > I think a different approach can be better. What do you think about creating > a completely empty GTK theme using gtk_icon_theme_set_search_path, to check > if we already added builtin icons ? > I tried > emptypath[0]=NULL; > GtkIconTheme *virgin = gtk_icon_theme_new(); > gtk_icon_theme_set_search_path(virgin,emptypath,1); > and it effectively gives an icon theme containing only the builtin icons. This approach should work. I think the n_elements parameter for gtk_icon_theme_set_search_path should be 0, which should mean that the theme itself adds no filesystem reading/scanning. However, this approach misses out on the advantage that the previous patch had that it skipped loading the files if they were not used. Assuming that most distribution installs add icons to the hicolor theme, I expect most people will not need these files. There are some people who will only install from another source (e.g. mozilla.com), but even for them, the issue that this is addressing is not going to happen so frequently or be so major that it is worth the cost to everyone.
Comment 101•13 years ago
|
||
Comment on attachment 580359 [details] [diff] [review] Patch that uses gtk_icon_theme_add_builtin_icon and gtk_window_set_default_icon_name This is clear win IMO, so let's try to get this in ASAP.
Attachment #580359 -
Flags: checkin?
Updated•13 years ago
|
Keywords: checkin-needed
Whiteboard: [not part of WINNT build]
Assignee | ||
Comment 102•13 years ago
|
||
(In reply to Karl Tomlinson (:karlt) from comment #100) > However, this approach misses out on the advantage that the previous patch > had > that it skipped loading the files if they were not used. Well, yes, but they are only added once. I think the time needed to look for the file on the filesystem is not important (the previous code was looking for those files 3 or 4 times for each window, it seems to me). If it was important, then maybe this will even improve the performance. But you are right that it is not addressing a common issue, especially XUL users switching theme already have bigger issues since the icons in gnomestripe don't change. So, the question is : do we want to make icon theme change “on the fly” possible, or is it fine if it requires to restart XUL ?
Assignee | ||
Comment 103•13 years ago
|
||
Just fixed gtk_icon_theme_set_search_path(virgin,emptypath,1); to gtk_icon_theme_set_search_path(virgin,emptypath,0);
Attachment #580983 -
Attachment is obsolete: true
Attachment #580983 -
Flags: review?(karlt)
Updated•13 years ago
|
Assignee: nobody → jagw40k
Comment 104•13 years ago
|
||
Comment on attachment 580359 [details] [diff] [review] Patch that uses gtk_icon_theme_add_builtin_icon and gtk_window_set_default_icon_name Next time, it would be great to have a commit message :)
Attachment #580359 -
Flags: checkin? → checkin+
Updated•13 years ago
|
Keywords: checkin-needed
Whiteboard: [not part of WINNT build]
Comment 106•13 years ago
|
||
(In reply to Jean-Alexandre Anglès d'Auriac from comment #102) > do we want to make icon > theme change “on the fly” possible, or is it fine if it requires to restart > XUL ? Sorry, I missed this. For some reason, I don't seem to be getting email notifications for your (and only your) comments. On the fly changes are nice, and AIUI will actually work for most users/situations with the patch that has landed. The patch that has landed has fixed this bug as originally reported and is scheduled to ship in Firefox 11, so I'll close this bug. If you want to address the issue for people without an icon in the hicolor theme, that can be addressed in new bug report. Perhaps an approach similar to attachment 580431 [details] [diff] [review] but with the default icons renamed to match the brand names ("firefox", "nightly", and whatever is appropriate for beta and aurora) would work without requiring loading at startup any files that are not needed. However, I suspect there are more important things that could be fixed.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
I think this change broke the icon entirely when --enable-official-branding is used. With an --enable-official-branding build, I'm seeing the default process icon rather than a Firefox icon. It's probably worth checking that it works with each of: ac_add_options --enable-official-branding ac_add_options --with-branding=browser/branding/aurora ac_add_options --with-branding=browser/branding/nightly ac_add_options --with-branding=browser/branding/official ac_add_options --with-branding=browser/branding/unofficial
Assignee | ||
Comment 108•13 years ago
|
||
My testing build was with --enable-official-branding, and it worked. Does your icon theme include a firefox icon ? Does the problem happens in both situations (with and without a firefox icon in the theme) ? Does it also happens when applying the patch that add the builtin icon once and only once ?
Comment 109•13 years ago
|
||
I'm seeing the nightly and aurora icons in official builds. Branding configure arguments are not listed in about:buildconfig, but I assume they are used in these builds.
(In reply to Jean-Alexandre Anglès d'Auriac from comment #108) > Does your icon theme include a firefox icon ? How would I tell? I'm running GNOME 2 on Ubuntu 11.04. And shouldn't things still work when it doesn't?
Assignee | ||
Comment 111•13 years ago
|
||
Yes, it definitely should, as you could have tell if you had looked at the previous comments or at the code. I'm trying to track down where the bug is, that's why I asked you this question. Can you please compile that, launch it, and tell me the result (i.e. does it say that there is a firefox icon in your theme, and does the window use it ?) #include <gtk/gtk.h> void destroy (void) { gtk_main_quit (); } int main (int argc, char *argv[]) { GtkWidget *window; gtk_init (&argc, &argv); if (gtk_icon_theme_has_icon(gtk_icon_theme_get_default(), "firefox")) printf("Default theme has an firefox icon\n"); else printf("Default theme has no firefox icon\n"); window = gtk_window_new (GTK_WINDOW_TOPLEVEL); gtk_window_set_icon_name (GTK_WINDOW(window), "firefox"); gtk_widget_show (window); g_signal_connect (window, "destroy",G_CALLBACK (destroy), NULL); gtk_main (); return 0; }
Comment 112•13 years ago
|
||
This seems to cause bug 714068
Assignee | ||
Comment 113•13 years ago
|
||
(In reply to Chris Coulson from comment #112) > This seems to cause bug 714068 Thanks, I'll look for what I can do about bug 714068
You need to log in
before you can comment on or make changes to this bug.
Description
•