Re: patch for #83964

[Alexander Larsson <alexl redhat com>]

On 2 Oct 2002, jacob berkman wrote:

> On Wed, 2002-10-02 at 03:12, Jeff Waugh wrote:
> > <quote who="jacob berkman">
> > 
> > > > It also doesn't work with Link type .desktop files (perhaps Command -> URL
> > > > for these?)
> > > 
> > > fixed.
> > > 
> > > alex, dave?  thoughts?
> > 
> > The only remaining oddness that I can see is that you can't switch the type
> > to Application.
> 
> hmm somehow that got left off.  also i had forgotten to add the code to
> actually change the type.

Some general comments:

Switching types seems a bit busted. It's frequently very slow, and 
sometimes it doesn't work att all after a while (the menu keeps 
disappearing each time you pop it up.)

I don't think we really want to expose all the crack desktop file types in 
a general desktop file editor. Only Application and Link, and possibly 
FSDevice (but we should not call it that).

It doesn't seem done yet ui-wise. Only Application and Link have 
reasonable UI for editing, and even Link has some issues (It still has the 
run in terminal checkbox).

I did a quick review of the code:

Some variables are assigned where they are declared, this does not follow 
the Nautilus coding standard.

Missing space in:
+	if (ditem == NULL){
+		gtk_widget_hide (window->details->launcher_vbox);
+		return;
+	}


This didn't compile:
+static GtkWidget *
+append_ditem_pair (FMPropertiesWindow *window,
+		   GtkTable           *table,
+		   const char         *title,
+		   gboolean            is_localestring,
+		   char               *attr,
+		   GtkWidget         **label)
+{
+	GtkWidget *entry;
+	GtkLabel *my_label;
+	guint last_row;
+
+	if (label == NULL) {
+		label = &my_label;
+	}
Since label and my_label has different types. Change label to GtkLabel ** 
and change the  type of command_label in FMPropertiesWindowDetails to 
GtkLabel *.

+	/* We always create it, but it is hidden
+	 * when the file is not a ditem
+	 */
+	create_launcher_page (window);
+	

Why do you do this? It would seem better to only create the page when it 
is needed.

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander Larsson                                            Red Hat, Inc 
                   alexl redhat com    alla lysator liu se 
He's a fiendish pirate ex-con haunted by memories of 'Nam. She's a wealthy 
insomniac bodyguard looking for love in all the wrong places. They fight 
crime!