Re: templates patch ...

[Michael Meeks <michael meeks novell com>]

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.

-- 
 michael meeks novell com  <><, Pseudo Engineer, itinerant idiot