Created attachment 123999 [details] [review]
planner-0.14.3-configure.patch

here is a patch to migrate to GtkRecentManager API. Maybe I forgot to update gtk+ dependency but it should work well. FYI, adding recent files directly in the File menu is possible, see gedit code, but I was too lazy so I did it the submenu way.

 Makefile.am               |    1 
 configure.in              |    2 
 data/ui/main-window.ui    |    3 -
 po/POTFILES.in            |    2 
 po/POTFILES.skip          |    2 
 src/Makefile.am           |    1 
 src/planner-application.c |   20 ++-----
 src/planner-application.h |    9 +--
 src/planner-main.c        |    3 +
 src/planner-window.c      |  125 ++++++++++++++++++++++++++++------------------
 tests/Makefile.am         |    1 
 11 files changed, 95 insertions(+), 74 deletions(-)

Thanks for considering.

Created attachment 124000 [details] [review]
planner-HEAD-kill-libegg.patch

here is a patch to migrate to GtkRecentManager API. Maybe I forgot to update gtk+ dependency but it should work well. FYI, adding recent files directly in the File menu is possible, see gedit code, but I was too lazy so I did it the submenu way.

 Makefile.am               |    1 
 configure.in              |    2 
 data/ui/main-window.ui    |    3 -
 po/POTFILES.in            |    2 
 po/POTFILES.skip          |    2 
 src/Makefile.am           |    1 
 src/planner-application.c |   20 ++-----
 src/planner-application.h |    9 +--
 src/planner-main.c        |    3 +
 src/planner-window.c      |  125 ++++++++++++++++++++++++++++------------------
 tests/Makefile.am         |    1 
 11 files changed, 95 insertions(+), 74 deletions(-)

Thanks for considering.

oops, sorry for double posting, I picked the wrong patch.

Patch WFM with 0.14.4; GTK_REQUIRED needs to be bumped to 2.10.0 to match.

*** Bug 581989 has been marked as a duplicate of this bug. ***

ping - Maurice, can this cleanup patch please get a review?

I'm currently reviewing it and rebasing it on current HEAD.

planner-application.c:
- don't unref the recent manager in application_finalize (see docs on gtk_recent_manager_get_default())
- should there really be a "..." at the end of the menu entry? Isn't that only for things that open up dialogs?

planner-window.c
- when I first looked at this patch I decided to replace the GTK_STOCK_OPEN with NULL, but now I can't remember why I did that.
- don't leave commented out lines, the patch has them around line 610
- I think the string (if not NULL) returned by g_filename_to_uri should be freed. Also don't call gtk_recent_manager_add_full if the string is NULL.

I'd prefer to also have the previous behaviour where the name of the file you hover over is put in the status bar, but maybe it's non-trivial.

Gilles or Alexandre, do you have time to update the patch based on the last comment?
If I get it right this will also kill Planner's Bonobo dependency.

Created attachment 139960 [details] [review]
reworked patch

Here's a new version of the patch, awaiting review.

(In reply to comment #8)
> planner-application.c:
> - don't unref the recent manager in application_finalize (see docs on
> gtk_recent_manager_get_default())

Fixed.

> - should there really be a "..." at the end of the menu entry? Isn't that only
> for things that open up dialogs?

There shouldn't be. Fixed.

> planner-window.c
> - when I first looked at this patch I decided to replace the GTK_STOCK_OPEN
> with NULL, but now I can't remember why I did that.

Indeed, the GTK_STOCK_OPEN icon looks weird on this submenu.

> - don't leave commented out lines, the patch has them around line 610

Fixed.

> - I think the string (if not NULL) returned by g_filename_to_uri should be
> freed. Also don't call gtk_recent_manager_add_full if the string is NULL.

Fixed, I reworked this part and it's cleaner now.

> I'd prefer to also have the previous behaviour where the name of the file you
> hover over is put in the status bar, but maybe it's non-trivial.

That looks indeed non-trivial and I think we can add it later on if we really want to.

>> I'd prefer to also have the previous behaviour where the name of the file you
>> hover over is put in the status bar, but maybe it's non-trivial.

> That looks indeed non-trivial and I think we can add it later on if we really
> want to.

I thought I said something about it but it seems not so, gedit does it, but if you look at it, you'll see it's a great amount of code for a small gain (imho).

Agreed, it's not worth the extra code.

Some comments on the new patch:

- The entire libegg subdir should be removed now that we don't use it anymore.

- libegg/recent-files/egg-recent-vfs-utils.c should not be added to POTFILES.skip

- There is a misalignment in the New FileOpenRecent entry in the list of GtkActionEntrys (planner-window.c:233)

- There is some trailing white space (planner-window.c lines 433, 435, 438, 628, 633 and 637). I think git would have complained

- There's a double + in your patch, causing a compilation error and a whitespace problem.

Other than that the patch looks fine.

Alexandre: Willing to fix the issues listed by Maurice and come up with a last, final version? :)

Thanks everybody here for working on this. I appreciate it.

I'm actually working on it :)

*** Bug 596171 has been marked as a duplicate of this bug. ***

Created attachment 147331 [details] [review]
hopefully final version of the patch

Well, at last I had a look and fixed the remaining problems. I also removed a line in Makefile.win32 so the Windows build needs to be verified. Maurice, if you think that this one is the good one, let me know and I'll push it.

Review of attachment 147331 [details] [review]:

Looks fine except for one small thing.

::: src/planner-window.c
@@ +441,3 @@
+                planner_window_open_in_existing_or_new (window, uri, FALSE);
+                g_free (uri);
+        }

The above lines use spaces for indenting instead of tabs

I forgot to add that it works on Windows as well.

Spaces have been fixed.

This has been pushed to master and will be available in next version.