GNOME Bugzilla – Bug 85292
add an icon to gtkentry
Last modified: 2008-12-19 17:46:28 UTC
navigation apps like moxilla usually include drag handles for combo boxes. It would be nice if gtk made this easy so that galeon and perhaps even nautilus could add drag handles inside of a combo box.
I'm not sure what you mean here. Do you mean allowing the combo boxes contents to be torn off (like gnumeric? I haven't seen this in Mozilla.)
no, no i mean to have a url drag icon. So favicons can be easily be put into the combo box in galeon. In nautilus it would also be nice to have a combo box for text entry which has an icon in the box that can be used to drag a folder to the desktop etc. (there are feature requests in nautilus bugzilla for such behavior). Apparently this is very hard to do right now, but such features are expected by users of other browser type apps like mozilla and Ie, and i can imagine other apps finding this functionality useful as well, so making the implementation easy would be nice.
Just to add some infos about current situation. I managed to get a gtkentry subclass doing it, a bit hacky, but seem to work correctly. But still I cant put it in a combo box, I'd have to rewrite all the combo code and the GnomeEntry code. Not sure what would be the better way for gtk to make this easier.
Retitling this to mean what I meant. Basically for the epiphany address entry it would be nice if we could put an icon into the gtkentry we use as in internet explorer/mozilla etc.
This has actually been implemented outside gtk+: http://freedesktop.org/cgi-bin/viewcvs.cgi/galago/libgalago-gtk/libgalago-gtk/galago-gtk-icon-entry.c?rev=1.9&view=markup The problem with that implementation is that the text flickers every time the icon is changed. I couldnt find any obvious ways to fix that ...
Yeah, some work needs to go into it still, and it is a bit of a hack, although less so than the previous implementation :) I have plans for a hopefully better one, but haven't had the time, since I'm trying to get the rest of Galago working smoothly and integrated with Evolution.
*** Bug 508809 has been marked as a duplicate of this bug. ***
Created attachment 106411 [details] [review] Merged SexyIconEnry Thanks Johan. Importing my patch from bug #508809 here. I haven't looked at this recently, so I don't remember what the state of it is yet. My plan was to revisit this during GTK Hackfest and finish up anything that's missing during that time.
Created attachment 107076 [details] [review] Fixed patch Sorry, I forgot to svn add before I generated the previous patch. This is the correct patch.
some comments on the patch: + * @file libsexy/sexy-icon-entry.c Entry widget + * + * @Copyright (C) 2004-2006 Christian Hammond. mmh, should we change this? acknowledge Christian work, but remove what looks like annotations? + entry->priv = g_new0 (GtkIconEntryPriv, 1); use g_type_class_add_private() in class_init and the G_TYPE_INSTANCE_GET_PRIVATE() in the instance init function. + if (G_OBJECT_CLASS (parent_class)->finalize) + G_OBJECT_CLASS (parent_class)->finalize (obj); finalize is guaranteed to be there, so there's no need to check. +static void +gtk_icon_entry_destroy (GtkObject *obj) +{ + GtkIconEntry *entry; + + entry = GTK_ICON_ENTRY (obj); + + gtk_icon_entry_set_icon (entry, GTK_ICON_ENTRY_PRIMARY, NULL); + gtk_icon_entry_set_icon (entry, GTK_ICON_ENTRY_SECONDARY, NULL); + + if (GTK_OBJECT_CLASS (parent_class)->destroy) + GTK_OBJECT_CLASS (parent_class)->destroy (obj); +} should really use ::dispose, not ::destroy. I also think that we should probably add two properties, :icon-primary and :icon-secondary, so that UI builders and GtkBuilder can define the icons directly in XML.
Created attachment 107106 [details] [review] Improved patch I believe this addresses all of ebassi's comments.
Some more review: - The way in which GtkImages are used here is not very well aligned with GTK+ api conventions. These widgets are not children of the icon entry, they are only used as vehicles for the pixbuf - and the code does not even handle all possible types of GtkImage. If the image contains e.g. an animation, things just break. This should be redone by using pixbufs and/or icon-names/stock-ids. - Looking at the firefox location entry, do we need to allow more than one icon on each side ? - What about keynav ? That is one of my main concerns about this whole ui 'innovation'. At the very least, the documentation needs to recommend that every action that can be triggered by clicking on an embedded icon should also be available in an accessible way, e.g. by adding a context menu item with the same icon. - colorshift_pixbuf should probably be unified with the similar function gtkcellrendererpixbuf.c:create_colorized_pixbuf - gtk_icon_entry_set_icon_highlight() should be turned into properties as well: highlight-primary, highlight-secondary.
Created attachment 107297 [details] [review] Unfinished, but posting anyway This doesn't yet address all Matthias's comments, but I'm posting anyway.
So, I like the idea of supporting more than one icon on each side. I'm not sure what the ideal API would look like. Here's what I'm thinking at the moment: int gtk_icon_entry_add (GtkIconEntryPosition pos, GdkPixbuf *pixbuf); int gtk_icon_entry_add_from_stock (GtkIconEntryPosition pos, const gchar* stock_id); The return values would be the index of the image on that side of the entry (primary or secondary). Then functions like gtk_icon_entry_set_icon_highlight() would add an extra parameter to account for the index, such as: void gtk_icon_entry_set_icon_highlight (GtkIconEntryPosition pos, gint index, gboolean highlight); I imagine that the index would be relative to either the side or the entry, so the first icon you add is index 0, second is index 1, etc. But then the application developer needs to be aware that they can't just store an index as a unique integer, because index 1 could refer to the second icon on each side. If that's not desirable, the index could still be numbered uniquely and in a way that is obvious to application developers by using the sign as the position. Using this way, icons on the left could use indexes -1, -2, -3.. and icons on the right could use 1, 2, 3.. I don't know how well-aligned that is with other places in the GTK API, but it still seems reasonable to me. With the signed numbering, I would probably prefer to remove the GtkIconEntryPosition enum and just have functions such as: int gtk_icon_entry_add_primary (GdkPixbuf *pixbuf); int gtk_icon_entry_add_secondary (GdkPixbuf *pixbuf); Anyway, this is what I've been thinking about in terms of end user API and just wanted to ping on here and get some thoughts. In the mean time, I'll work on Matthias's other comments from his previous post.
Could you also add functions to set the icon from an icon name? Stock IDs are nice, but icon names are better.
I took another look at this kind of ui in browsers (firefox, epiphany), and notice that there are some more things: - allow the icon to act as a drag source - set a different cursor on the icon - allow a tooltip on the icon - allow a diffent context menu on the icon
Thanks guys! I'll try to get a new patch up soon the adds Ross's and Matthias's suggestions.
This might be a pipedream at this point, but gtk_icon_entry_add_gicon() would be nice... probably depends on bug 522084 being fixed first.
I have to look at what GIcon is. To be honest, I don't even know what that is yet. :)
For all practical purposes, a GIcon is an abstract icon name. You can pass it to GtkIconTheme and get an icon back. Not very important, and will be trivial to add support for later.
Created attachment 111781 [details] [review] Adds icon names and GIcon Updated patch to add ross's and awalton's suggestions. Marking as needs-work initially because I haven't yet added mclasen's suggestions.
Created attachment 112076 [details] [review] Add support for tooltips on icons
"The way in which GtkImages are used here is not very well aligned with GTK+ api conventions. These widgets are not children of the icon entry, they are only used as vehicles for the pixbuf - and the code does not even handle all possible types of GtkImage. If the image contains e.g. an animation, things just break." I was thinking about this, but I'm not sure what is the best way to fix it. Is it possible to use the GtkImage as a child widget, or should I continue using a GdkWindow child as it currently does but simply switch the icon store to a gdk-pixbuf instead of a a GtkImage that doesn't behave like a widget?
"Looking at the firefox location entry, do we need to allow more than one icon on each side ?" We were talking briefly about this widget in the GTK+ meeting on IRC, and the consensus seemed to be pretty unanimous (kris, awalton, ebassi, me) that we should not support multiple widgets on each side. If anyone disagrees with this, let's start up a discussion here and maybe get some use cases or something.
Why exactly did you replace the GtkImage* as means of providing icons with a number of accessors? As far as I can follow comments, the problem was that not all types of images GtkImage supports were supported by GtkIconEntry. But imho it seems more useful to actually fix that instead of changing the interface. For one, I would find it very useful to be able to use an animated icon, and also was the original interface much easier and less complex.
(In reply to comment #25) > Why exactly did you replace the GtkImage* as means of providing icons with a > number of accessors? see comment 12 - the GtkImages wre not children of the entry as one would expect with them being GtkWidgets. > For one, I would find it very useful to be able to use an animated icon then an accessor for GdkPixbufAnimation should be added. using GtkImage widgets just to hold the data type is expensive and pointless if they are not used as children of the entry.
> see comment 12 - the GtkImages wre not children of the entry as one would > expect with them being GtkWidgets. If you say children, wouldn't it suffice to implement the container interface and providing them in gtk_container_get_children? > then an accessor for GdkPixbufAnimation should be added. using GtkImage widgets > just to hold the data type is expensive and pointless if they are not used as > children of the entry. I think GtkImage actually has an advantage, being able to hold different kinds of images, which can simplify the logic of your application code if you make use of that, i.e. mixing stock items, named icons and pixbufs.
There is no container interface. Entries are not containers.
Created attachment 112349 [details] [review] Dump GtkImage for image storage This changes the GtkImage to GdkPixbuf. Does not yet take into account animated pixbufs. Also fixed an issue when you gtk_icon_entry_set_cursor() before the widget is realized.
So I think the main things remaining are drag-and-drop functionality and context menus.
I think you should probably check these cases: > screen = gtk_widget_get_screen (GTK_WIDGET (entry)); This may lead to bad situations if the widget has no screen. For popup menus I suggest adding icon-populate-popup and/ or icon-popup-menu. The interesting question is then what key bindings to use here.
Thanks, Christian. Just curious, what kind of situations can cause the widget to have no screen?
> This may lead to bad situations if the widget has no screen. Indeed, whats missing here is a style_set handler (compare to GtkImage). I don't like how the icon-name and gicon are write-only properties. I think we need to do something similar to what GtkImage does. + /** + * GtkIconEntry:icon-primary: + * + * An image to use as the primary icon for the entry. + */ + g_object_class_install_property (gobject_class, + PROP_PIXBUF_PRIMARY, + g_param_spec_object ("pixbuf-primary", The documentation doesn't match the property name, same for pixbuf-secondary. update_icon doesn't make much sense to me. It seems to only ever be called as update_icon (NULL, NULL, entry); except for the one place where it is connected to the notify signal on a pixbuf - which will never happen, I think. Also, it checks for property names which are not pixbuf properties. + * displayed instead. If the current icon theme is changed, the icon will be + * updated appropriately. I don't see code to do that...see above wrt to style_set +void +gtk_icon_entry_add_clear_button (GtkIconEntry *icon_entry) That name is a bit confusing, I think, since we are just adding an icon to the entry, not a button. Also, the implementation has a leftover widget that is leaked. Comparing to the evolution search entry, this clear 'button' should appear insensitive when the entry is empty. Maybe 'sensitive/insensitive' would be a useful hi-level attribute to set on the icons. an insensitive icon would use the default arrow cursor instead of a custom cursor, appear greyed out, not highlight on mouseover, and not react on mouse clicks. > The interesting question is then what key bindings to use here. I could imagine adding the icon menus as submenus to the main entry context menu.
For DND, we need to be able to associate a list of targets with each icon, plus a set of actions, a la: void gtk_tree_view_enable_model_drag_source (GtkTreeView *tree_view, GdkModifierType start_button_mask, const GtkTargetEntry *targets, gint n_targets, GdkDragAction actions);
Also need a way to use the gtk_target_list_add_*_targets functions on these drag sources/dests, so we don't get the same problem as on treeview in bug 506853.
I was thinking earlier, is there any particular reason to make this a separate widget rather than integrating the functionality into GtkEntry?
(In reply to comment #36) > I was thinking earlier, is there any particular reason to make this a separate > widget rather than integrating the functionality into GtkEntry? I find that an interesting point. I'm not sure if the now still missing functionality will involve incompatible parts, but it certainly would be useful. Thinking of for example existing subclasses that add spell checking features that would make it possible to use icons on these widgets as well.
Imo, one criterium for deciding 'should this go in the main class or a subclass' is: do we want all entries to benefit from the new functionality, transparently ? While that is clearly the case for spell-checking, it is probably not the case for icons. Another criterium is are entries used in lots of ways that could would transparently gain useful functionality if the icon support is in the main class ? For that, the answer is yes, I think: e.g. spin buttons, editable cell renderers.
Created attachment 112523 [details] [review] Updated patch This fixes some, but not all, of the issues noted by Matthias. Just figured I'd go ahead and post an updated patch in case anyone is interested, but I'm not requesting a new review yet or anything.
Created attachment 113755 [details] [review] Added sensitivity Just another incremental update. Added sensitivity to this one.
When the GtkEntry widget is resized in a container (as can be seen with the GtkComboBoxEntry widget) the text is rendered in the vertical center of the entry. However, i'm seeing that with the GtkIconEntry widget, the icon/text stays at the top left of the entry. Also if i resize the widget i see the text flickers; it appears briefly vertically centered before being drawn at the top left corner of the entry. I can get the icon/text to be drawn in the vertical center by adding the following lines to the place_windows() function, before the first call to get_icon_allocation: gdk_window_get_geometry (GTK_ENTRY (icon_entry)->text_area, NULL, &y, NULL, NULL, NULL); text_area_alloc.y = y; I'm still trying to work out how to fix the flicker problem. Is it possible to make a similar change to gtkiconentry? Thanks.
I noticed that stock icons are merely rendered to pixbufs to then replace the internal pixbuf. This means that they won't be updated when the theme changes. It would be nice if that could be improved in that regard. And there seems to be a bug. The situation is this, I'm having a GtkIconEntry, and I want to show a secondary stock icon as an indicator, that is set when a condition is TRUE, and unset with a NULL pixbuf, when the condition becomes FALSE. For some reason I must set a stock icon when creating the entry, even though I don't want one initially, because if I don't, any later attempts at setting an icon will fail silently. FYI, the Midori Web Browser is now using GtkIconEntry as in comment #40, instead of SexyIconEntry, in three places. This is where my comments are coming from.
> I noticed that stock icons are merely rendered to pixbufs to then replace the > internal pixbuf. This means that they won't be updated when the theme changes. > It would be nice if that could be improved in that regard. Yeah, this clearly needs some more copying from GtkImage Some more things to notice: set_cursor should preferably take a GdkCursor, not a GdkCursorType What about markup in tooltips ? If we decide to fold this into GtkEntry itself, care needs to be taken to only allocate the extra data when we actually use the functionality. We don't want all the worlds entries to suddenly take up twice the space...
Created attachment 121992 [details] [review] another patch
This patch addresses a number of the issues outlined in earlier comments. Still open: - new tooltip api - cursors - subclass or not
Comment on attachment 121992 [details] [review] another patch >+ /** >+ * GtkIconEntry::icon-pressed: >+ * @entry: The entry on which the signal is emitted >+ * @icon_pos: The position of the clicked icon >+ * @button: The mouse button clicked >+ * >+ * The ::icon-pressed signal is emitted when an icon is clicked. >+ */ Just so it's not forgotten: all of the signals and properties need Since tags and those which lack them need doc comments. >+static void >+gtk_icon_entry_finalize (GObject *obj) >+{ >+ GtkIconEntry *entry; >+ >+ g_return_if_fail (obj != NULL); >+ g_return_if_fail (GTK_IS_ICON_ENTRY(obj)); >+ >+ entry = GTK_ICON_ENTRY (obj); >+ >+ G_OBJECT_CLASS (parent_class)->finalize (obj); >+} Those _return_if_fail are wrong. The _finalize doesn't currently serve a purpose in the first place, it better be removed. >+static void >+gtk_icon_entry_size_allocate (GtkWidget *widget, >+ GtkAllocation *allocation) >+{ >+ g_return_if_fail (GTK_IS_ICON_ENTRY (widget)); >+ g_return_if_fail (allocation != NULL); >+ >+ widget->allocation = *allocation; >+ >+ GTK_WIDGET_CLASS (parent_class)->size_allocate (widget, allocation); >+ >+ if (GTK_WIDGET_REALIZED (widget)) >+ place_windows (GTK_ICON_ENTRY (widget), allocation); >+} Those _return_if_fail are also wrong. >+static gint >+gtk_icon_entry_expose (GtkWidget *widget, >+ GdkEventExpose *event) >+{ >+ GtkIconEntry *entry; >+ GtkIconEntryPrivate *priv; >+ >+ g_return_val_if_fail (GTK_IS_ICON_ENTRY (widget), FALSE); >+ g_return_val_if_fail (event != NULL, FALSE); >+ >+ entry = GTK_ICON_ENTRY (widget); >+ priv = GTK_ICON_ENTRY_GET_PRIVATE (entry); Again bogus _return_if_fail. >+ default: >+ g_warning ("Unsupported image type: %d", icon_info->storage_type); >+ break; >+ } That one should be localized. >+void >+gtk_icon_entry_set_icon_from_pixbuf (GtkIconEntry *entry, >+ GtkIconEntryPosition icon_pos, >+ GdkPixbuf *pixbuf) >+{ >+ GtkIconEntryPrivate *priv; >+ EntryIconInfo *icon_info; >+ >+ g_return_if_fail (GTK_IS_ICON_ENTRY (entry)); >+ g_return_if_fail (IS_VALID_ICON_ENTRY_POSITION (icon_pos)); Missing a g_return_if_fail (!pixbuf || GDK_IS_PIXBUF (pixbuf); >+void >+gtk_icon_entry_set_icon_from_gicon (GtkIconEntry *entry, >+ GtkIconEntryPosition icon_pos, >+ GIcon *icon) >+{ >+ GtkIconEntryPrivate *priv; >+ EntryIconInfo *icon_info; >+ GdkScreen *screen; >+ GtkIconTheme *icon_theme; >+ GtkSettings *settings; >+ gint width, height; >+ GtkIconInfo *info; >+ >+ g_return_if_fail (GTK_IS_ICON_ENTRY (entry)); >+ g_return_if_fail (IS_VALID_ICON_ENTRY_POSITION (icon_pos)); Missing a g_return_if_fail (!icon || G_IS_ICON (icon); >+ gtk_icon_entry_clear (entry, icon_pos); >+ >+ priv = GTK_ICON_ENTRY_GET_PRIVATE (entry); >+ icon_info = &priv->icons[icon_pos]; >+ >+ screen = gtk_widget_get_screen (GTK_WIDGET (entry)); >+ icon_theme = gtk_icon_theme_get_for_screen (screen); >+ settings = gtk_settings_get_for_screen (screen); This needs to be conditional in case screen is NULL. >+/** >+ * gtk_icon_entry_set_icon_highlight: >+ * @entry: A #GtkIconEntry >+ * @position: Icon position >+ * @highlight: %TRUE if the icon should highlight on mouse-over >+ * >+ * Determines whether the icon will highlight on mouse-over. >+ */ I wonder, should this be called or refer to 'prelight'? This is what the description is explaining at least. >+/** >+ * gtk_icon_entry_set_tooltip: >+ * @entry: A #GtkIconEntry >+ * @position: Icon position >+ * @text: The text to be used for the tooltip >+ * >+ * Sets the tooltip text used for the specified icon. >+ */ >+void >+gtk_icon_entry_set_tooltip (GtkIconEntry *entry, >+ GtkIconEntryPosition icon_pos, >+ const gchar *text) >+{ This should probably be _set_icon_tooltip, otherwise it is easy to confuse with gtk_widget_set_tooltip_text, particularly in bindings: "myEntry.SetTooltipText(text); myEntry.setTooltip(pos, text)". >+ EntryIconInfo *icon_info; >+ GtkIconEntryPrivate *priv; >+ gchar *new_tooltip; >+ >+ g_return_if_fail (GTK_IS_ICON_ENTRY (entry)); >+ g_return_if_fail (IS_VALID_ICON_ENTRY_POSITION (icon_pos)); >+ >+ priv = GTK_ICON_ENTRY_GET_PRIVATE (entry); >+ >+ icon_info = &priv->icons[icon_pos]; >+ >+ new_tooltip = g_strdup (text); >+ if (icon_info->tooltip_text != NULL) >+ g_free (icon_info->tooltip_text); >+ icon_info->tooltip_text = new_tooltip; No need for the if (!= NULL) here, g_free does that for free. >+++ tests/testiconentry.c 2008-11-05 01:56:20.000000000 -0500 >@@ -0,0 +1,159 @@ > [...] >+int >+main (int argc, char **argv) Nitpick, indent argv in its own line. >+ /* >+ * Search - Uses a helper function >+ */ >+ label = gtk_label_new ("Search:"); >+ gtk_table_attach (GTK_TABLE (table), label, 0, 1, 2, 3, >+ GTK_FILL, GTK_FILL, 0, 0); >+ gtk_misc_set_alignment (GTK_MISC (label), 0.0, 0.5); >+ >+ icon_entry = gtk_icon_entry_new (); >+ gtk_table_attach (GTK_TABLE (table), icon_entry, 1, 2, 2, 3, >+ GTK_FILL | GTK_EXPAND, GTK_FILL, 0, 0); >+ >+#if 0 >+ gtk_icon_entry_set_icon_from_stock (GTK_ICON_ENTRY (icon_entry), >+ GTK_ICON_ENTRY_PRIMARY, >+ GTK_STOCK_CLEAR); >+#endif >+ >+#if 1 >+ gtk_icon_entry_set_icon_from_stock (GTK_ICON_ENTRY (icon_entry), >+ GTK_ICON_ENTRY_SECONDARY, >+ GTK_STOCK_FIND); >+ gtk_icon_entry_set_icon_highlight (GTK_ICON_ENTRY (icon_entry), >+ GTK_ICON_ENTRY_SECONDARY, >+ TRUE); >+ g_signal_connect (icon_entry, "icon-pressed", >+ G_CALLBACK (find_pressed), NULL); >+#endif An unconditional separate entry would be nice.
> >+ g_warning ("Unsupported image type: %d", icon_info->storage_type); > >+ break; > >+ } > > That one should be localized. I'd rather just replace it with a g_assert_not_reached(). This is just an internal consistency check. > This should probably be _set_icon_tooltip, otherwise it is easy to confuse with > gtk_widget_set_tooltip_text particularly in bindings: > "myEntry.SetTooltipText(text); myEntry.setTooltip(pos, text)". As mentioned, the whole tooltip handling needs to be redone to be in line with the new tooltip api. > >+int > >+main (int argc, char **argv) > > Nitpick, indent argv in its own line. Strangely, I prefer this formatting for main (and only for main). The rest of your comments is right on.
Hey Matthias, thanks for posting a new patch. Sorry I haven't had time to work on this again recently. Just want to add one more thing to the "still open" list so we don't forget: - drag and drop
Created attachment 122080 [details] [review] another iteration
New patch should take care of Christians comments. I've redone the tooltip api to go along with the new tooltip api. I've left testiconentry out of this patch. Remaining issues: - cursors - subclass - dnd
Thinking about this some more, I think we should just not expose freely settable cursors. The only use case is dnd, and for that we should just set a proper cursor ourselves when the icon is turned into a drag source.
Created attachment 122170 [details] [review] another iteration Here is another iteration, with the following changes: - replace the hardcoded ICON_MARGIN by something derived from GtkEntry::inner-border - fix window positioning logic to not let the entry window show through (since that causes problems for cursor setting) - remove the set_cursor method, instead suitable cursors are now determined based on icon sensitivity and activatability - replace prelight by activatable. Only activatable icons get the ::icon-pressed signal emitted, and they get a suitable cursor. Whether they prelight on mouseover is determined by a style property Left to do: - subclass ? - dnd
(In reply to comment #52) > Created an attachment (id=122170) [edit] > another iteration Just a few minor hints this time, after a test run: get_pixbuf_from_icon is unused VOID:INT,BOXED needs to be added to gtkmarshalers.list unused "GdkCursor *cursor;" in gtk_icon_entry_set_icon_sensitive testiconentry somehow shows a Clear icon in the entry with the find icon when I click on it, and hides it after clicking again, and the next time it shows the icon and so on. I wonder, should the mouse pointer not be a default pointer instead of a text insertion pointer, when hovering over a non-activatable icon? Otherwise I intuitively expect to move the caret somewhere around the icon, which obviously won't work.
> testiconentry somehow shows a Clear icon in the entry with the find icon when I > click on it, and hides it after clicking again, and the next time it shows the > icon and so on. Yeah, it does. I used that as a test case for resizing. It is not meant to be meaningful. > I wonder, should the mouse pointer not be a default pointer instead of a text > insertion pointer, when hovering over a non-activatable icon? Otherwise I > intuitively expect to move the caret somewhere around the icon, which obviously > won't work. My argument was that the icon is visually part of the entry, so unless there is some special action that you trigger by clicking on it, it should have the same cursor as its surrounding. The cursor change from caret to arrow would indicate that there is something active that you can click on, which is why I do that for activatable icons.
Created attachment 122336 [details] [review] including dnd api
This patch includes a proposal for dnd support: /** * gtk_icon_entry_set_icon_drag_source: * @entry: a #GtkIconEntry * @icon_pos: icon position * @target_list: the targets (data formats) in which the data can be provided * @actions: a bitmask of the allowed drag actions * * Sets up the icon at the given position so that GTK+ will start a drag * operation when the user clicks and drags the icon. * * To handle the drag operation, you need to connect to the usual * #GtkWidget::drag-data-get (or possibly #GtkWidget::drag-data-delete) * signal, and use gtk_icon_entry_get_current_icon_drag_source() in * your signal handler to find out if the drag was started from * an icon. */ void gtk_icon_entry_set_icon_drag_source (GtkIconEntry *entry, GtkIconEntryPosition icon_pos, GtkTargetList *target_list, GdkDragAction actions) /** * gtk_icon_entry_get_current_icon_drag_source: * @entry: a #GtkIconEntry * * Returns the index of the icon which is the source of the current * DND operation, or -1. * * This function is meant to be used in a #GtkWidget::drag-data-get * callback. * * Returns: index of the icon which is the source of the current * DND operation, or -1. */ gint gtk_icon_entry_get_current_icon_drag_source (GtkIconEntry *entry) The implementation seems to work ok, and manages to not interfere with regular GtkEntry DND support. There are a few questions left (see FIXMEs in the code): - Do we need to allow triggering dnd with specific buttons (see the start_button_mask argument to gtk_drag_source_set) ? I think just supporting button1 drags is fine, personally. Thats also what we do for selection dnd from GtkEntry already - Setting a custom drag icon requires g_signal_connect_after to ::drag-begin signal, since it is a RUN_LAST signal. This is a bit ugly, but the default icon that we set up in the class handler should be fine 90% of the time. We just need to document this quirk.
Created attachment 122387 [details] [review] cleaned up patch
Some more notes on merging into entry: - the signals will have to be renamed, otherwise we break SexyIconEntry users - GtkSpinButton does not work quite right: * prelight on icons gets stuck * secondary icons overlap with the up/down arrows
> * prelight on icons gets stuck Fixed in trunk.
More things to test: - icons in file chooser entry - interaction with entry completion
The placement of secondary icons in spinbuttons can be fixed by replacing the get_text_area_size() function in gtkiconentry.c by static void get_text_area_size (GtkIconEntry *entry, GtkAllocation *alloc) { GTK_ENTRY_GET_CLASS (entry)->get_text_area_size (entry, &alloc->x, &alloc->y, &alloc->width, &alloc->height); } Interaction with entry completion looks fine to me. Two more issues: - gtkspinbutton takes care to make width-chars work accurately, even though the arrows take up extra space. gtkiconentry does not, currently, thus you don't actually get as many chars as requested with width-chars when icons are present - scroll offset is not maintained properly when icons are shown and hidden
> scroll offset is not maintained properly when icons are shown and hidden exporting gtk_entry_adjust_scroll for internal use, and calling it from place_windows fixes this.
Created attachment 123719 [details] [review] _gtk_entry_adjust_scroll Here is a patch to export gtk_entry_adjust_scroll for internal use. This is only temporary, merging the icon support into gtkentry.c will make that unnecessary.
Created attachment 123720 [details] [review] latest version
The latest version of the patch fixes scroll offsets and placement of icons in spinbuttons. The one issue that is still outstanding is width-chars handling, which I think will be easier to fix after merging the icon functionality into gtkentry.c.
Hey I'm finally starting to merge this into GtkEntry and I happened to notice the following in gtk_icon_entry_button_release(): + if (should_prelight (entry, i) && + event->x >= 0 && event->y >= 0 && + event->x <= width && event->y <= height) Shouldn't the width/height checks be using < instead of <= ? So I think it would look like: if (should_prelight (entry, i) && event->x >= 0 && event->y >= 0 && event->x < width && event->y < height) Also, the icon position enum.. that should go in gtkenums.h right? Right now I'm just doing: typedef enum { GTK_ENTRY_ICON_PRIMARY, GTK_ENTRY_ICON_SECONDARY } GtkEntryIconPosition; But I feel like it should probably have a more generic name (rather than being specific to entries). Do you agree? I thought I had another question, but I can't think of it now. I'll post any other questions/comments I have.. otherwise I'll just get a patch posted soon. Thanks!
> Shouldn't the width/height checks be using < instead of <= ? Looks like it. > But I feel like it should probably have a more generic name (rather than being > specific to entries). Do you agree? Well, you _could_ just reuse GtkPackType, if you are confident that we'll never grow a tertiary, etc position.
Hi Matthias, in place_windows() there is this: + _gtk_entry_adjust_scroll (GTK_ENTRY (entry)); Where does that come from? Did you maybe leave something out of your last patch?
see commment 63
If you're using git, I pushed my branch up onto my github area: http://github.com/bratsche/gtk/tree/iconentry
Created attachment 124377 [details] [review] Merge with GtkEntry Here's the current patch, integrated with GtkEntry
Some things went wrong with the merge here: - drag_data_get and drag_data_delete are set twice in class_init. Thats obviously just an oversight, the handlers need to be merged instead. - the code of gtk_icon_entry_motion_notify seems to have been lost + gtk_widget_set_has_tooltip (GTK_WIDGET (entry), icon_info->tooltip != NULL); Not your fault, but this is a bit broken. It needs to a) also look at the tooltip of the other icon b) take into account that the user may have set has-tooltip on the entry himself + if (icon_info->tooltip && !icon_info->insensitive) + { + gtk_tooltip_set_markup (tooltip, icon_info->tooltip); Not your fault either, but I think we should not forbid tooltips on insensitive. Allowing that for widgets was one of the bigger advantages of the new tooltips api... I'd like to see testentryicons.c expanded to cover dnd from icons, tooltips with markup, ::icon-pressed handlers (was the search example supposed to cover that ?), and icons in spinbuttons. Entries are common widgets, so I'm a bit concerned that this more than doubles the size of each entry in every gtk app. As indicated earlier, I'd prefer to see icon-related parts of the private struct allocated on demand. That also goes for the server-side resources. As this patch currently stands, it will make every entry allocate 2 extra windows. I'm also concerned that the addition of ::icon-pressed and ::icon-released leaves us without padding in the class. Maybe we should go without class handlers for these signals ? I can't really imagine a use for them. In any case, if we decide to keep them, they must go at the end of the class, not in the middle of the existing vfuncs.
I'll be pushing fixes into my git branch until I fix everything in your comment, and then I'll post a revised patch. I also noticed that get_borders() and get_pixbuf_from_icon() are not used anywhere, so I removed those.
Created attachment 124871 [details] [review] Update Okay, so I lied.. this (I believe) fixes everything except the deferred creation of the windows. I'll work on that now and post a new patch. I just wanted to post this patch because of the change to removing ::icon-pressed. I moved a couple things from the class handler function into gtk_entry_button_press() before it emits the signal. The diff for that commit: http://github.com/bratsche/gtk/commit/77c0f93ce539ebb63acb295008ba38d6232c11af
Created attachment 124875 [details] [review] Fix broken prelighting in button_press_event I broke the icon prelighting when I tried to merge code from the default icon_pressed handler into button_press_event. This patch fixes it.
Created attachment 124912 [details] [review] Create resources on demand This patch should defer creating the EntryIconInfo data until it is needed. It does this in construct_icon_info(), which is called from - gtk_entry_set_icon_from_pixbuf - gtk_entry_set_icon_from_stock - gtk_entry_set_icon_from_icon_name - gtk_entry_set_icon_from_gicon If the widget has already been realized, then construct_icon_info() will "realize" the icon windows, and if the window has already been mapped then it will show them. If the widget hasn't yet been realized/mapped then it won't, and the widget's realize/map methods will check if there is an EntryIconInfo (which indicates an icon is set) and then will realize/map. Possible issues here are that other icon public methods such as gtk_entry_set_icon_activatable() will not do anything if there is not yet an EntryIconInfo at the specified position; right now only the gtk_entry_set_icon_from_foo() functions will actually create the EntryIconInfo. What is the desired behavior here? Should the other methods go ahead and create the resources, or should they do a g_return_if_fail() check and force you to have already set an icon? Here is the diff just for this change: http://github.com/bratsche/gtk/commit/730c3c46cdbfe805ae657c2b1d4a0d40abad77af
I think this is the last of your comments on the GtkEntry patch itself. I'll start working on an improved test program next.
(In reply to comment #76) > Created an attachment (id=124912) [edit] > Create resources on demand > > Possible issues here are that other icon public methods such as > gtk_entry_set_icon_activatable() will not do anything if there is not yet an > EntryIconInfo at the specified position; right now only the > gtk_entry_set_icon_from_foo() functions will actually create the EntryIconInfo. > What is the desired behavior here? Should the other methods go ahead and > create the resources, or should they do a g_return_if_fail() check and force > you to have already set an icon? I would think from a semantical point of view, any icon related function/ property should just setup the icon structure. Other widgets don't normally try to warn you if a property isn't useful without some other one, ie. "use-markup" doesn't care whether you actually set markup or not. It might even be convenient for the developer if they can set 'activatable' before knowing what the icon looks like. A short sentence somewhere in the introduction mentioning that kind of memory handling might be good, ie. "if icon related functionality is used, the entry reserves memory dynamically".
Created attachment 124940 [details] [review] Construct icon info in all 'set' methods Thanks Christian! It now constructs the icon info whenever you call something like set_icon_activateable() or set_icon_sensitive(). http://github.com/bratsche/gtk/commit/a1dac1d798c198a859fa4bb53411bd4fe6e1b6f5
Looks like this still doesn't contain the merged drag_data_get and drag_data_delete handlers. Expanding the test cases to cover dnd will probably reveal that it doesn't work. The rest of the patch looks good to me now, but it seems we are failing to free the dynamically allocated EntryIconInfo in finalize. > A short sentence somewhere in the introduction mentioning that kind of memory > handling might be good, ie. "if icon related functionality is used, the entry > reserves memory dynamically". I don't think implementation details like this have any place in the docs.
Created attachment 124985 [details] [review] Fixes to finalize, drag_data_get, drag_data_delete Oops, sorry I forgot about the drag_data_get/drag_data_delete. And thanks for noticing the lack of finalize stuff. Here is a new patch that fixes both of these. Here are the two diffs: * drag_data_get/drag_data_delete changes: http://github.com/bratsche/gtk/commit/fcf319ac855ae325b822a7583ca9f84899278e1b * finalize cleanup http://github.com/bratsche/gtk/commit/3afc088a4e09a7dc6efcc237e9dbe375b3ee0f06 I'm not sure if the unreffing of the target_list is right.. you didn't have that in your patch, but it seemed like it should be done unless that happens elsewhere.
good point about the target_list. It made me notice that your patch is missing the implementations of gtk_entry_set_icon_drag_source and gtk_entry_get_current_icon_drag_source.
Created attachment 124986 [details] [review] Add set_icon_drag_source, get_current_icon_drag_source Thanks for catching that. I'll try to get an expanded test program done soon and post that. Here's another patch that adds set_icon_drag_source() and get_current_icon_drag_source(). diff is here: http://github.com/bratsche/gtk/commit/72895beced22b378ef29e8b6cdb39beb3926cca4
Looks good to me now. I think further fixes and test improvements can go in after the inital commit.
2008-12-19 Cody Russell <bratsche@gnome.org> Bug 85292 – add an icon to gtkentry * gtk/gtkmarshalers.list: Add VOID:INT,BOXED * tests/testentryicons.c: Initial icon entry test * tests/Makefile.am: Add testentryicons * gtk/gtkentry.[ch]: Add API for setting primary/secondary icons and other features related to them.