Re: [evolution-patches] patch for bug 48759

[Jeffrey Stedfast <fejj ximian com>]

On Wed, 2003-09-24 at 11:37, Charles Zhang wrote:
> Hello, Jeffrey.
> 
> Maybe you have misunderstood me because of my poor expressing. :)
> I will raise my points with two parts of words below.
> 
> 
> Jeffrey Stedfast wrote:
> > please rewrite this as the following:
> > 
> > {
> > 	char *filename = gtk_file_selection_get_filename (selection);
> > 	struct stat st;
> > 	
> > 	if (stat (filename, &st) == -1 || S_ISDIR (st.st_mode))
> > 		return TRUE;
> > 	
> > 	return FALSE;
> > }
> > 
> > - there should always be 1 blank line after variable declarations.
> > - use stat() rather than g_file_text to be consistant with the rest of
> > the mailer code.
> >   
> En, thank you Jeffrey, I should pay more attation to the coding style.
> 
> This is the dialog for selecting the file to save.
> 
> If stat() return -1, it means that the file described with filename
> doesn't exist or something else.
> What we should do is , 
> to keep the dialog open when the file described with filename exists
> and it is a directory indeed .
> So I prefer this one:
>         char *pathname = g_path_get_dirname (filename);          //
> get it's pathdir
>         if (stat (pathname, &st) == -1)                               
> // if the pathdir doestn't exist, then ok doesn't work
>                 return TRUE;
>         else if (! S_ISDIR (st.st_mode))                             
> // if the pathdir isn't a dir, then ok doesn't work
>                 return TRUE;

those 2 if's are not neccessary. if we can't stat the filename, it
doesn't matter if the parent directory exists or not.

>         else if (stat (filename, &st) == -1)                          
> // if the file doesn't exist, then ok works
>                 return FALSE;
>         else if (! S_ISREG (st.st_mode))                             
> // if the file isn't a regular file, then ok doesn't work
>                 return TRUE;
>         else
>                 return FALSE;
>                 // else, ok works

what you want is this:

	if (stat (filename, &st) == 0 && !S_ISREG (st.st_mode))
		return TRUE;
	
	return FALSE;


btw, for future reference: NEVER use c++-style comments. EVER.

also, if you are going to put a comment, do it like this:

	} else if (stat (filename, &st) == -1) {
		/* if the file doesn't exist, then ok works */
		return FALSE;
	} else ...

ie. enclose the comment and the accompanying statements all within a
code block.

> Why I write so many creteria is to handle the case of "incorrect
> filename" and "file doesn't exist" separately.
> For example, I can leave the "~/does_not_exist" filename to the upper
> caller to process,
> but I cannot leave the "~/does_not_exist/a.msg" filename to the upper
> caller to process.

it is by far simpler to just do what my code does above, it is by far
simpler.

> 
> And I don't like to put the criteria into one instrument like
> "stat()==0 && S_ISDIR()",
> I think it isn't clear enough and the result relies on the compiler's
> feature.

it does not rely on compilers features. any compiler that doesn't accept
that is broken.

> Or you think it's not necessary to think about so much?
> > > +	/* FIXME: the next statement should be "run_selector(composer, title, FALSE, NULL)",
> > > +	   otherwise this dialog's title is wrong and more than one file can be selected. */
> > > +
> > >     
> > uh... the whole point is that the user can select multiple files.
> >   
> Again, this dialog is for selecting the file to save.
> 
> I think it's no need to save a mail's content to more than one files.
> And if it is at last, at least we should change the title to correct
> words.

yea, sorry - I missed the "save-as" part. I was thinking of the "attach
file" selector dialog. appologies.

> 
> Such we should change the origin instrument
> from
> "  run_selector(composer, _("Attach file(s)"), TRUE, NULL);  "
> to
> "  run_selector(composer, title, FALSE, NULL);  "

yes, this is correct I think.

just change the stat calls to what I suggested above.

Jeff

-- 
Jeffrey Stedfast
Evolution Hacker - Ximian, Inc.
fejj ximian com  - www.ximian.com