GNOME Bugzilla – Bug 140153
[PATCH] "Recent Documents" menu not updating
Last modified: 2005-08-22 20:22:32 UTC
The Actions -> "Recent Documents" menu is not updated dynamically if there is no FAM support. In case FAM is unavailable, the .recently-used file should be manually polled so that dynamic updation of menu is possible.
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).