GNOME Bugzilla – Bug 450590
[toolbareditor] allow named icons with Gtk{Radio,Toggle}Actions
Last modified: 2007-06-30 15:15:22 UTC
(Noticed during an attempt to fix a bug in EOG: bug 374582 comment 9 ) Currently Gtk{Radio,Toggle}Actions show with a standard icon inside the toolbar editor if they don't have a stock icon set or even if they have a named icon set. The problem is that those "specialized" actions check first of what type the specified icon is and set only either the stock-icon or the icon-name property. Normal actions have the stock-icon always set (if an icon was specified) and set the named-icon property only if it actually is one.
Created attachment 90546 [details] [review] patch applied to EOG This is the patch I applied to EOG's toolbareditor copy. It checks the named-icon property first as that will catch named icons from all action types. If it's not set it takes the stock-id property.
Looks right to me. Please commit to libegg and the master copy in epiphany; thanks!
Felix The patch works well :-). I just tested in EOG. For clarity I think it's better to rename the stock_id variable to icon_name, because you now fill it with the icon_name and not the the stock_id. Kind Regards, Jaap PS I noticed if you drag an icon (no matter if it's a stock_id or named_icon) of the toolbar into the Toolbar Editor dialog it always shows a default icon instead of the real icon. As you seem to know this code pretty well, you might want to look into that as well
I for now checked the patch in without changing the variable name. But it should be no problem to change it.
Created attachment 90562 [details] [review] Patch adding icon_name variable Actually I think it's clearer to add an extra variable icon_name
Actually, I think you should just create the image right in the if (icon_name) branch instead of using the is_named variable. Could you also get rid of g_object_get_property and just use g_object_get at the same time?
Created attachment 90569 [details] [review] cleanup proposal So, this one pulls all needed properties at once and avoids that GValue stuff completely.
Looks good, lease commit to libegg and epiphany's master copy. I did spot this in the code (unchanged by you): item_name = strdup (name); Could you fix that to g_strdup() while you're at it (there are two occurrences of it in the code)? Thanks!
Created attachment 90573 [details] [review] Another small cleanup of the toolbar-editor Felix, Can you also apply this patch (assuming it is correct), while you're at it. I'll also attach shortly a patch so you also get to see the icons while dragging from the toolbar to the editor as I was asking you in the PS of comment 3. I could not stop my curiosity to see if I also could fix it. I hope you haven't started to code on that yet.
Missed me again! I just finished committing. :-D Christian, are you okay with that patch? Regarding your PS: I haven't done anything except for trying to find out what's happening. So feel free to give it a try. You should open a new bug though to keep things sorted.
(In reply to comment #10) > So feel free to give it a try. You should open a new bug though to keep things > sorted. > Attached it to Bug 450715
Comment on attachment 90573 [details] [review] Another small cleanup of the toolbar-editor Which icon size does this use for the drag icon when set like this?
(In reply to comment #12) > (From update of attachment 90573 [details] [review] [edit]) > Which icon size does this use for the drag icon when set like this? > GTK_ICON_SIZE_LARGE_TOOLBAR if I visually inspect them NOTE just below the patch the code checks for named icons and uses gtk_drag_source_set_icon_name. So also overthere you do not set the icon size explicitly.
You're right. So this is fine; please commit to libegg and epiphany. Thanks!
The named icon check was a patch by me. And there is actually a size difference. The named drag icons are larger than the other ones (didn't notice it before). Do we want the bigger ones or should I try to fix my patch to show toolbar sized icons (which is going to be a bit more expensive I think)?
Yes, I think that if possible, the drag-from-toolbar icon's size should be the origin toolbar's icon size.
Created attachment 90613 [details] [review] more fixup So this is basically a fix for my patch from bug 435614. This makes the toolbar editor set a GTK_ICON_SIZE_LARGE_TOOLBAR sized drag image for named icons as well. This would obsolete Jaap's patch as it sets GTK_ICON_SIZE_DND sized images.
Err, actually I meant bug 436684. (Setting committed flag on patch I committed yesterday)
Looks good, thanks for the patch! I wonder if the absence of a way to set the drag icon size when using named or stock icons (a gtk_drag_source_set_icon_size func) is an oversight or deliberate...
Christian, Felix, One consideration: I actually found that having a larger icon works quite intuitive. It's immediately clear that you picked up the icon. As a bonus the code is also much more compact, and you use the standard icon size for drag and drop.
Committed to eog, epiphany and libegg. (We two have some bad timing, Jaap! ;-) ) > I wonder if the absence of a way to set the drag icon size when using named or > stock icons (a gtk_drag_source_set_icon_size func) is an oversight or > deliberate... Good question. To be honest I couldn't find lots of usecases if you'd ask me. Maybe it has something to do with a global desktop look&feel. > I actually found that having a larger icon works quite intuitive. It's > immediately clear that you picked up the icon. Hmm, interesting point. Although, the current solution has the advantage that it looks like you are actually moving the toolbar entry around... > As a bonus the code is also much > more compact, and you use the standard icon size for drag and drop. > True; as long as GTK+ doesn't provide a simple API to get a named icon based on a GtkIconSize parameter (the implementation is actually based on the implementation in GtkCellRendererPixbuf and GtkDragContext).
I guess we can call this bug fixed :-)