Created attachment 26682 [details] [review]
patch which polls the .recently-used file in absence of FAM

http://bugzilla.gnome.org/show_bug.cgi?id=140153

  + Never patch egg-* in a module directly - the fix should always go into
    libegg itself

  + What happens to the timeout when the model is destroyed? Crash?

  + You're leaking a GnomeVFSFileInfo every second - at the very least you
    should free it. But I'd allocate it only once and re-use it in every
    timeout.

  + With the gnome-vfs monitor, egg_recent_model_changed() is invoked from
    a timeout - presumably there's a reason for that and you want to do the
    same when polling.

  + Having the mtime stored in a static variable in the timeout handle is
    broken. Its global data - why do you think everything in 
    EggRecentModelPrivate isn't stored in global variables?

  + I'd remove

+	g_return_val_if_fail (user_data != NULL, FALSE);
+	g_return_val_if_fail (EGG_IS_RECENT_MODEL (user_data), FALSE);

    its useless code which will be run every second.

Created attachment 26954 [details] [review]
patch with suggested changes

> + What happens to the timeout when the model is destroyed? Crash?

This was not crashing because the model was never getting destroyed. 

In panel-recent.c : panel_recent_append_documents_menu (), a g_object_unref ()
for the view is called when the recent menu is finalized. But the view in turn
has a reference to the recent menu (egg_recent_view_gtk_set_menu ()). Hence the
recent menu will never be finalized, which means, the view and in turn the model
are never finalized.

I have taken care of this in the patch by connecting a callback to the "destroy"
of the menu, in which I unref the view.

> + You're leaking a GnomeVFSFileInfo every second - at the very least you
should free it. But I'd allocate it only once and re-use it in every timeout.

I have added this to the model's private sturcture and allocating it in the
model's init func and freeing in the finalize func.

Also I have added the mtime and the poll timeout function in the model private
structure and removing the timeout when the model is finalized.

Kindly comment.

Comment on attachment 26954 [details] [review]
patch with suggested changes

sorry attached the wrong patch

Created attachment 26955 [details] [review]
patch which fixes the bug

Does this still apply? Can we get it reviewed please?

/me pokes Mark

Leena, you are still leaking the file info; you should free it before checking
the file mtime.

also, are we sure that using gnome.vfs is the right thing to do, performance
wise? the .recently-used file should always be local.  I think that a simple
stat() will do the trick, here.

Emmanuel: Just came across the libegg/recentchooser commits done by you. As I
understand, libegg/recentchooser will eventually replace the libegg/recent-files
module. Had some queries regarding the same. 

1. Do you plan to remove libegg/recent-files anytime soon ? 
2. Is the patch using the approach mentioned by you in Comment #9 to
   libegg/recent-files required ? 

Kindly comment.

replying to comment #10:

1. no, I don't plan removing it for the time being; first, I plan to fix all the
outstanding bugs for the recent-files component, while the recentchooser code
stabilise itself. then, the recent-files code will be marked as DEPRECATED, and
will need a flag in order to compile (this should break everything that uses
automated scripts, so I'll send an heads up message before actually doing it).
in the meantime, I will not fix the bugs which I consider (or are marked as)
enhancements - even though I'll leave open the bugs as a reference.

2. FAM should be installed by default on every box new enough to support an
updated recent-files code; anyway, if a patch using stat()/g_stat() instead of
GnomeVFSFileInfo is supplied, I'll apply it.

Created attachment 51097 [details] [review]
Patch with the suggested changes which fixes the bug.

2005-08-22  Emmanuele Bassi  <ebassi@cvs.gnome.org>

	* libegg/recent-files/egg-recent-model.c:
	(egg_recent_model_init), (egg_recent_model_poll_timeout),
	(egg_recent_model_finalize), (egg_recent_model_monitor): If FAM
	support is unavailable, manually poll the .recently-used file (patch
	by Dinoop Thomas, fixes bug #140153).

	* libegg/recent-files/egg-recent-model.c: fixes compilation on Darwin
	where lockf() isn't exported (patch by Jerry Talkington, fixes
	bug #124238).

	* libegg/recent-files/egg-recent-model.c:
	(egg_recent_model_clear): if FAM is not available, force update of the
	list when removing an item (patch by Mark McLoughlin, fixes
	bug #308779).

	* libegg/recent-files/egg-recent.h: added header guards; this is a
	convenience header, so you should not use it anyway (fixes
	bug #108101).