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

[Peter Clifton <pcjc2-at-NOSPAMcam_dot_ac_dot_uk>]

On Fri, 2007-04-20 at 23:31 +0200, Ivan Stankovic wrote:
> Hi everyone,
> 
> 
> Attached is a patch that adds recent files support to 
> gschem. The code basically reads $(HOME)/.gschem-recent-files
> (determined by a compile-time constant) in order to construct 
> a recently used files submenu. The patch applies cleanly
> with the latest CVS and works fine for me. The only thing
> that's missing is the "Clear" item in the submenu, but I
> plan on adding that shortly.
> 
> Please test and send your comments and suggestions. Special
> thanks to Ales for guiding me on this!


Dear Ivan, this looks really cool. I have some comments.


1.

Silly nitty one.. Hunk 1 of the patch to gschem.c just adds whitespace -
can be deleted.


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.


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?


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;

(I realise that was a nitty point, but if this isn't the case, and any
of the g_list functions don't always return the start of the list,
please let me know, as I assume in much of my code that they do).


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.


6.

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


7.

in x_menu_add_recent_files_item()

It appears you implicitly rely on "File" menu coming first in the menu
list, and worse, where the recent files item lives in this menu.

+     GList *menubar_list = gtk_container_get_children(GTK_CONTAINER(
+              w->menubar));
+     GtkWidget *file_menu =
GTK_WIDGET(g_list_first(menubar_list)->data);

Since the menus are user-defined in scheme, this assumption might not be
true.

More in keeping with the existing code would be to use the (deprecated)
call:
  
item = (GtkWidget *) gtk_object_get_data(GTK_OBJECT(w_current->menubar),
"File/Recent files");

to find your menu item to work on, as used in x_menus_sensitivity(). At
some point, we ought to switch these all to g_object_{get|set}_data().


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).


This is a cool feature to have in gschem, and I hope my comments are
useful. I hope to merge the fixes I had (my patch in the sourceforge
tracker) during the code-sprint (assuming it is well received), so
hopefully that will bring us closer to sorting out 2 and 5.

Kind Regards,

Peter C.




_______________________________________________
geda-dev mailing list
geda-dev@moria.seul.org
http://www.seul.org/cgi-bin/mailman/listinfo/geda-dev