Created attachment 89595 [details] [review]
proposed patch

for evolution;

Ill commit this for stable and head. (It shouldn't break any UI freeze)

ok, I tested it and I think the fix is not exactly good. I tried to make snapshots  to make the point but it seems I can't get it with the mouse hover effect so let's try with word.

So first, the size of the icon is indeed wrong, we talked with Milan about it on irc and it looks like it's ~16x24 but there is another problem. To me, it looks like the button in itself is not like as large as a regular button.

Compared to the widget used in gedit (open) and epiphany (back & forward), the new* widget in evolution is thiner. I beleive that this is something to solve alongside with the inner size of the icon.

Gilles,

I think the patch is right, but not the results. Size of the toolbar and the widget packing sort of compresses it. You are right. The widget should be packed right with some padding to solve this.

Hmm, I think this patch should wait, till the packing gets right. It will look bad till then. 

(In reply to comment #5)
> Hmm, I think this patch should wait, till the packing gets right. It will look
> bad till then. 
> 

There is other kind of problem with this control, it didn't behave as other toolbar items, you could not take focus on it with keyboard (notice that, when you run Evolution, there is a focus on "Send/Receive" and you could move with left/right arrow over toolbar items, but only not on the "New" item. Also, with theme "Crux", the item is not drawn as other items.)

I found out that there is a gtk widget GtkMenuToolButton, since 2.6, but unfortunately, I could not force it to show label on itself. (I don't know why, either I setup a label text or label widget, it did't want to show label there.) This widget looks similar to that evo-special widget, so it may be fine to use this from gtk.

I did investigate about that GtkMenuToolButton a bit, and a reason why it didn't want to show that label is because of no parent set for new_button, specially it needs a toolbar as parent, if there is no such parent, then the style is always "ICON only". But I don't know how to manage this work with bonobo.

I think the "New" button is still our own custom widget.

See widgets/misc/e-combo-button.c.  The copyright is dated 2001.

Probably written before GtkMenuToolButton existed.  We should drop our widget.

The narrowness of the button is caused by the toolbar being in non-homogenous mode.  Homogenous mode would make all the buttons as wide as "Send / Receive".  A little extra padding should do the trick, like Srini suggested.

Since we're using a custom widget for this button anyway, and the widget is only used for the "New" button, we could just hardcode some extra padding into the widget itself as a lame, short-term solution.

Bumping version to a stable release.

Awhile back I tried killing this widget but Bonobo got in the way.
I'm in favor of just padding EComboButton for now.

Created attachment 114116 [details] [review]
evolution-2.12.3-fix-e-icon-factory.patch

while working on solving general theming issues in evolution, I found out that evolution didn't respect size settings for icons. This patch removes the hard coded values and uses the theme to get that information.

I post this here because when I apply milan's patch over this one and change SMALL_TOOLBAR to LARGE_TOOLBAR, I get a toolbar with a consistent size. This is with one more patch from us that changes the current "new" button into a simple GTK_BUTTON so I still have to check if this is good for regular evolution but at least the current patch still has value on its own.

It has been written against 2.12.3 but will probably apply to 2.22 and 2.23 without a hitch.

Haven't tested patch yet but I'm strongly in favor of the idea.  Note, you'll be wanting to remove those g_message() calls before committing, or at least convert them to disabled debug messages.

Gilles the patch looks fine for me. But I didn't test it, just see if it applies cool to trunk, if so just test and commit it.

Please test also a theme where only images 128x128 are available. I recall seeing such bug about having problem with Evolution with such theme, but cannot find it now.

Gilles, ping :-) 

Gilles, double ping ;-) So as far as I can see, there is a request from Srini to test this patch in comment #13. Would be nice if you could do this, if not, please drop us a line.

BTW: if that patch obsoletes patch #89595 we should mark it as obsolete.

Ah ha!  I was looking for that patch a few days ago while chopping limbs off e-icon-factory.  I vote for just killing the E_ICON_SIZE enumeration altogether; it's entirely redundant with GtkIconSize.

w00t, I finally had time to test and commit this.

Révision 36000 propagée.

I'll try to write a E_ICON_SIZE killing patch if I have time this weekend.

Created attachment 116718 [details] [review]
evolution-2.23.90-new-mail-icon.patch

this patch is an update of the original proposal of Milan. It wasn't applying to head properly. It still has the same flow as it doesn't change the EComboBox.

Note, when killing E_ICON_SIZE there's still a few places in addressbook that are passing icon size "48" to e_icon_factory_get_filename().  Those should be changed to GTK_ICON_SIZE_DIALOG.

Also, if you're so inclined, the "art" directory has nothing to offer to e-icon-factory these days, so that clunky directory-name-parsing section in load_icon() can be ripped out.  Nor is the icon name ever an absolute path (I checked).

Commit this now to trunk.

Patch committed to SVN trunk as r36007
http://svn.gnome.org/viewvc/evolution?view=revision&revision=36007

the bug has not been closed is there any other change required?

It doesn't seem to be to me, even some other question/work has been arisen, it doesn't belong to this bug at all. Thus closing. (Feel free to reopen in case you think I'm wrong.)