Created attachment 105477 [details] [review]
attach-recent-docs

Patch uses a GtkRecentChooserMenu widget and a utility function to populate a bonoboUI. (The GtkWidget can be directly used, as is, once bonoboUI is obsoleted). 
Patch also contains related changes in message-composer and comp-editor. 

(NOTE: a new string is introduced in e-attachment-bar.c, file needs to added to POTFILES.in)

_("Recent Docu_ments") - Gimp uses "Open _Recent" for this menu item. having consistent terms in GNOME and consistent mnemonics but be great (i know that the R is used by Send/_Receive and i think we should not change that).

and there is a " " in your patch that does not belong there.

Suman 2.23.x material. Good job with this. I'm seeing the hack week proving worthy for Evolution :)

CC'ing myself on this for the composer rewrite.

Bumping version to a stable release.

Created attachment 107278 [details] [review]
attach-recent-docs

Minor bug fix (the sort type wasn't set previously) and some minor improvements w.r.t. the items shown in the menu.

Matt, will you review this? From my point of view it seems fine, even kinda strange to use g_list_first, is it necessary? But I didn't test the patch, so just wondering. Another thing, what if the file display name contains underscore? Is it fine? I guess the display name is derived from file name itself, so if the file name contains more than on underscore, will it change the right one?

Patch will need to be reworked since the composer doesn't use Bonobo anymore.  I'll look at adapting Suman's patch for the new composer.

Marking the patch as obsolete.

(In reply to comment #7)
> Matt, will you review this? From my point of view it seems fine, even kinda
> strange to use g_list_first, is it necessary? ...

g_list_first makes sure that the LRU (last recently used) file appears first :)
anyway.. all that was for bonoboUI which is no longer used.. hurray !

> Another thing, what if the file display name contains
> underscore? Is it fine? I guess the display name is derived from file name
> itself, so if the file name contains more than on underscore, will it change
> the right one?

yeah.. it can handle these scenarios (the code under "/* escape _'s in the display name so that it doesn't become an underline in a GtkLabel */" does that)

(In reply to comment #8)
> Patch will need to be reworked since the composer doesn't use Bonobo anymore. 
> I'll look at adapting Suman's patch for the new composer.

thanks Matt :)

Created attachment 108907 [details] [review]
attach-recent-docs-new-composer.diff

okay.. couldn't resist to hack on the new composer ;-)

Attached patch emulates all and same functionality as before. 
Although, I'm not fully convinced about: 

a) if EMsgComposer is the right place to have the new GtkActionGroup. Can we push this to EAttachmentBar ? [I did try out a few variations but I could not get it to work. To be more specific: having a new UI menu component in evolution-composer.ui and then coupling it with the RecentChooserMenu widget in EAttachmentBar. Maybe I missed/misused something :(]

b) GtkRecentAction - can we use this ? and, can we use this directly without depending on EAttachmentBar ?

Matt, could you review this please ? :)

srag: yeah.. the context-menu is not up yet.. should do that :)

oops.. forgot to mention.. first two hunks in e-composer-actions.c is a bug-fix which saves some CPU cycles :)

Created attachment 108915 [details] [review]
Patch for new composer

Hey Suman, looks like we raced on this one.  :)

Here's what I came up with.  The patch is just for the composer.  I had intended to have you combine it with the rest of your patch.

I took a different approach by using a GtkRecentAction directly since it pretty much does all the work for you.  Good suggestion in comment #11; I added that to my patch as well.

Suman, the rest of your patch looks good, though I do have some plans of my own for the EAttachmentBar (bug #516933).  I understand the extra complexity is due to having to deal with BonoboUI.  Hopefully we can eliminate that soon.

So I noticed the standard GTK+ file chooser dialog has a Recently Used section which lists exactly the same files, making this feature kinda redundant.

Having realized this, I think I'm -0 on the whole idea now.

first, about the patch: 
Matt, your patch is just the thing I mentioned in point (b) comment #10 :). Its clean, independent of other widgets and does the job just as well. +1 

There's still the issue that, assuming the comp-editor is also moved to GtkUIManager, we'd have duplicate code there too. At some point, we _should_ move this logic to EAttachmentBar (btw.. bug #516933 is a fantastic idea!!)
For now though, all is well :) I second your idea that we should merge our changes. Meanwhile, I'll see what I can do to get the context-menu up as well.

second, about the feature:
na.. it is about the ease of attaching these documents. Yes, there is a 'Recently Used' section in the FileChooserDialog. And, it takes:

Insert -> Attachment -> Recently Used -> Select -> Attach ~= 5 clicks (plus an extra-large window to distract you ;-))

The menu, OTOH, takes:

Insert -> Recent Documents -> Select ~= 3 clicks

IMHO, this feature is as little redundant as having (at-least) 3 different ways to send a message from the composer. So, /me still +1 about that :) 

(In reply to comment #15)
> IMHO, this feature is as little redundant as having (at-least) 3 different ways
> to send a message from the composer. So, /me still +1 about that :) 

My wife (a Mac user) convinced me to bump my rating to a +0.

Didn't mean to sound so negative before.  I usually chew on these UI issues for awhile before forming an opinion, and this time I was a bit premature.  Sorry about that.  Too early in the morning to trust my own judgement.  :)

Suman, go ahead and commit when you've merged the patches together.

Created attachment 108976 [details] [review]
merged-updated-patch

(In reply to comment #15)
> At some point, we _should_ move this logic to EAttachmentBar 

I was harping on this issue and these came to light: 
In 'composer_actions_add_recent' - the second parameter, i.e. EMsgComposer, is used as 'user_data' to the callback. In the callback, we use the EMsgComposer only to get a reference to the EAttachmentBar. The callback does not do anything specific to EMsgComposer. Hence, it could as well be in EAttachmentBar, provided we have a reference to an instance of EAttachmentBar. With this approach, we can have a static local callback in e-attachment-bar.c (which fully resolves ".. move this logic to EAttachmentBar ..")

This is possible by exposing an API in EAttachmentBar ('e_attachment_bar_add_recent_docs_action' in the patch). 

Also, by using GtkRecentInfo on the fly, there is no need for the extra-widget I had added earlier. (So, this should be removed once comp-editor is bonoboUI-free.)

Overall, with this patch, I felt it was the best way to extend the UI :)

Thoughts ?

I think I'd leave the action group out of it and just return a new, fully configured GtkRecentAction (cast to a GtkAction).

Something like:

  GtkAction *
  e_attachment_bar_recent_action_new (EAttachmentBar *bar,
                                      const gchar *action_name,
                                      const gchar *action_label)

Created attachment 109038 [details] [review]
attach-recent-docs-new-composer

(In reply to comment #18)
> I think I'd leave the action group out of it and just return a new, fully
> configured GtkRecentAction (cast to a GtkAction).

Perfect :-)

The (action != NULL) check in composer_setup_recent_menu() seems unnecessary, but otherwise looks great.  Approving.

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

The context-menu stuff can be tracked on bug #516933 comment #5
(I did try to use the existing EPopup menus and found it to be inflexible in this case. It should be a simple task if the EAttachmentBar is re-written to use GtkUIManager.)