gEDA-dev: [PATCH] gschem: recent files support

[Ivan Stankovic]

On Fri, Apr 20, 2007 at 11:37:48PM +0100, Peter Clifton wrote:
> 1.
> 
> Silly nitty one.. Hunk 1 of the patch to gschem.c just adds whitespace -
> can be deleted.

Sure, will fix.

> 2.
> 
> I've been playing with the file-open code myself, (see patch in the
> SourceForge tracker which fixes a bug or two opening new windows etc..).
> 
> --- a/gschem/src/x_fileselect.c
> +++ b/gschem/src/x_fileselect.c
> @@ -189,6 +189,7 @@ x_fileselect_open(TOPLEVEL *toplevel)
>      /* open each file */ 
>      for (tmp = filenames; tmp != NULL;tmp = g_slist_next (tmp)) {
>        x_window_open_page (toplevel, (gchar*)tmp->data);
> +      recent_files_add((char *)tmp->data);
>      }
>     
>      /* free the list of filenames */
> 
> Code like this is fine, however it can build quickly if we need to add
> new actions upon opening files (like zooming to their extents etc..).
> 
> I've spent some time collecting many of these post-file open actions in
> x_window_open_page its self (see the patch on the SourceForge tracker),
> and it would seem like a better place to add the "recent_files_add"
> call.
> 
> The patch also makes files opened on the command line use the
> x_window_open_page call too, helping with point 5.

I agree, but I couldn't find the patch you are talking about.
All I could find is this entry in the tracker:

[ 1699970 ] New Untitled page problem, and with opening new windows

But it doesn't have any patches attached.

> 3.
> 
> +static void recent_file_clicked(gpointer data)
> +{
> +   TOPLEVEL *w = s_toplevel_new();
> +   x_window_setup(w);
> +   x_window_open_page(w, (char *)data);
> +}
> 
> This code opens a new window for each file - Is that the desired
> behaviour?

Yes. But that's just my preference. I could probably make it
configurable, to allow opening in a new page or in a new window, 
but I'm not sure if it would be worth the time/code. What do others
think?

> 4.
> 
> +  GList *p = g_list_first(recent_files);
> 
> (Similar appears in several places)
> 
> Doesn't recent_files point to the start of the GList anyway? Might as
> well do:
> 
> GList *p = recent_files;

Yes, you're right. Will make that change.

> 5.
> 
> It seems you don't add to the recent files list for files opened from
> the command line. This would be addressed nicely with the fix to 2.

Agreed. I will wait for your patch to be in CVS.

> 6.
> 
> I think you also want to add on "save as", or "save", to add newly named
> files to the list.

Definitely, thanks for catching that.

> 7.

[...]

> Code like this:
> 
> +  gtk_menu_shell_insert(GTK_MENU_SHELL(submenu), recent_menu_item, 11);
> 
> and
> 
> +     GtkWidget *p = g_list_nth_data(tmp, 11);
> 
> again violates the run-time building of the menu, and is probably not a
> great idea even with a static menu.
> 
> Make a scheme callable function to add the recent files menu where the
> user wants it, and then use the gtk_object_get_data(..) call to find it
> again when the menu needs modifying).

Hmm, I'll need to have a closer look at how menus are created from
scheme.... (The above was easiest for me to do since I'm not so
familiar with gschem code.)

Thanks for the comments!

-- 
Ivan Stankovic, ivan.stankovic at fer.hr

"Protect your digital freedom and privacy, eliminate DRM, 
learn more at http://www.defectivebydesign.org/what_is_drm"