Hi Christian, On Fri, 2008-06-27 at 00:05 +0200, Christian Neumair wrote: > Thanks for your efforts. I am mostly fine with the patch, but I have a > few minor comments:
Much appreciated. > How does KDE sort the menu items? IMO it would make sense to merge all > the template sources (including subdirectories), and then sort the items > by display name. Oh; no idea. I can look I guess - I tried to leave whatever sorting / ordering was there originally (none?) un-touched. > I wonder why you migrated the template parameters to textual URIs. Using > GFiles and NautilusFiles rather than URI strings is a Nautilus > convention we should stick to. Weelll ... I didn't like the look of the 'File' APIs - looked to scary to me; as if simply creating a GFile was going to do a chunk of I/O work. Looking at the API methods on them it looks like simply creating one of these objects is going to do lots of I/O - statting the file, digging around it to bed it in & so on ;-) Of course, that's just ignorance mostly I guess - but the name 'file' lead me to believe that: it seems the 'file' API mixes abstract URI handling, and file-system operations - which is what confused me. > g_str_has_suffix() would also look more readable for detecting a > ".desktop" suffix in add_template_to_templates_menus(). Sure, but not cope with ".DESKTOP" files ;-) at least, I was trying to code defensively. Is there a g_strcase_has_suffix() ? > Regarding the get_template_source_if_valid(), you should use > g_ascii_strncasecmp() for the desktop file type check, and ensure that > it is not NULL before strcmp'ing it. Good catch on the != NULL; the strncasecmp doesn't match the KDE approach they check for only "Link" - do we really want to match all "LinkExtended" type values ? > You also seem to return the .desktop file's URI if it is not a link, > instead of returning NULL. This looks wrong. I'll add a comment: it's right - if the .desktop file is not a link, presumably someone intended to add it to their Templates as a desktop file init's own right "create .desktop file from template" - KDE use this for various (urk?!) "device" .desktop files. > > [ Incidentally - if it is more widely used - should it not be one slot > > higher up than "Add Launcher" in the right-click context menu ? ] > > You mean it should be at the top, above "New Folder"? I often created > new folders, but almost never new documents. Nah - I mean on the desktop context menu I propose: - "Create Folder, Create Launcher, Create Document" + "Create Folder, Create Document, Create Launcher" simple tweak to nautilus-directory-view-ui.xml's "background" ordering (if that's ok ?). Anyhow - I'll clean up the above; then may I commit ? [ one final point - we seem to do a lot of this work at startup time, before we've even rendered the desktop background - which seems a little sub-optimal - I might look at deferring some of that work ] Thanks, Michael. -- [EMAIL PROTECTED] <><, Pseudo Engineer, itinerant idiot -- nautilus-list mailing list nautilus-list@gnome.org http://mail.gnome.org/mailman/listinfo/nautilus-list