Created attachment 90577 [details] [review]
Patch which adds icon when dragging from toolbar to editor dialog

Have you tested this in epiphany too?

(In reply to comment #2)
> Have you tested this in epiphany too?
> 
No just in EOG. 

Is there a reason why it would not work in epiphany if it works in EOG???

Epiphany has some custom actions which aren't toolbuttons or separators, so I just wondered if it worked with them too.

I guess they won't. If you look at the patch you see that I check if it's a seperator or a toolbutton. So unless you inherited it from one of these two you won't get a real icon.

I could change the patch so I don't check if it is a toolbutton. I'll just try to get the icon_name and stock_id from the object. If your custom actions set either of them the code will show an icon.

Created attachment 90947 [details] [review]
Updated patch which uses icons of the same size as the toolbar for dragging

Furthermore this patch also works with some of the custom widgets in epiphany. It now works well with the "location entry" and the "related" widgets. It does not work with "Quick Bookmark" and "Quick Topic" because these seem to get tied to an action which does not have a stock_id or icon_name

Created attachment 90948 [details]
update-toolbareditor-from-libegg script

I noticed that you always ask to apply the patch on epiphany and eog as well. I thought it would be handier if applications which use the egg toolbar would put the following script in their repository. They then just need to run the script to get the latest egg-toolbar. After that they just need to do a commit and put in the changelog "update to latest eggtoolbar"

The patch seems to mix spaces and tabs....

+              char *name;

|const|.

+              action = name ? find_action (etoolbar, name) : NULL;
+
+	      g_object_get (action,
+                            "icon-name", &icon_name,
+                            "stock-id", &stock_id,
+		            NULL);

You need to protect that g_object_get with |if (action)| since action can be NULL from the 1st line.

+                  if (G_UNLIKELY (!pixbuf))
+                    return;

That leaks |icon_name| and |stock_id|.

With those fixed, ok to commit. Thanks!

Thanks for the review. Should I just use spaces?

BTW do you also want me to sync epiphany with the latest changes.
And should I put the script I attached also in epiphany SVN?

(In reply to comment #9)
> Thanks for the review. Should I just use spaces?

Spaces is fine.

(In reply to comment #10)
> BTW do you also want me to sync epiphany with the latest changes.
> And should I put the script I attached also in epiphany SVN?

Yes, and yes.

Created attachment 90966 [details] [review]
Refactored patch

Christian, can you review this refactored patch. It removes some code duplication and also adds the changes you requested.

BTW maybe it's also handy if I add the update script to toolbar editor in libegg. This way developers of other modules can automatically pick up the script. What's your opinion about that?

Looks fine, thanks.

(In reply to comment #13)
> BTW maybe it's also handy if I add the update script to toolbar editor in
> libegg. This way developers of other modules can automatically pick up the
> script. What's your opinion about that?

Sure, that's ok.

Commited to both libegg and epiphany. BTW I discovered a bug shortly afterwards *pixbuf has to be initialized to NULL. Otherwise it crashes in epiphany (I tested the updated patch on eog) because some of the custom widgets which don't have an icon_name or stock_id. 



Changelog epiphany

2007-07-01  Jaap Haitsma  <jaap@haitsma.org>

        * lib/egg/update-toolbareditor-from-libegg: script that syncs local
	toolbar editor copy with the one in libegg SVN
	* lib/egg/egg*: sync with toolbar editor in libegg. Sync obtained by 
	running update-toolbareditor-from-libegg
	* lib/egg/eggintl.h: removed not needed anymore by egg-toolbareditor
	* lib/egg/Makefile.am: remove eggintl.h

BTW should I remove update-from-egg.sh from epiphany svn. It's not needed anymore because now we have update-toolbareditor-from-libegg