[MPlayer-dev-eng] [PATCH] Mac OS X Finder support

[Chris Roccati]

On 6 Nov 2004, at 22:42, Reimar Döffinger wrote:

> That is okay. Only problem with any kind of printf is that it will
> hinder working support of piping to stdout (will this ever happen?).

No, because that part of the code is only called when MPlayer is  
started from the finder. In these cases, stdin is already /dev/null and  
both stdout and stderr are redirected to  
/Library/Logs/Consoles/<username>/console.log

> Still a few things I have been thinking about.
> Hardly important, but you're not very consistent when it comes to  
> placing spaces (e.g. between function arguments) ;-).

I should reformat the code after copy/paste: yet, much of the carbon  
code comes from apple examples, which I've imported long time ago...

>> +               Boolean valid;
> MPlayer usually uses int (and 0/1 instead of FALSE/TRUE) because  
> Boolean is not standard C. But that's up to you to decide how you want  
> it.

That's the carbon Boolean. One could decide to use MPlayer's boolean if  
there's one.

>> +                       argv[0]=NULL;
> If you have a simple way to pass it something useful, do it.

The "real" argv[0] is going to be something like  
/Applications/MPlayer.app/Contents/MacOS/mplayer which is not what the  
user expects. The right thing to do is something along:

{
ProcessSerialNumber mypsn;
CFStringRef name;
char *procname;
int len;

	GetCurrentProcess(&mypsn);
	CopyProcessName(&mypsn,&name);
	len=CFStringGetLength(name)+1;
	argv[0]=malloc(len);
	CFStringGetCString(name,argv[0],len,kCFStringEncodingISOLatin1);
	argv[0][len-1]=0;
}

I'm not sure if this can be called "simple".

>> + parm[actualSize]=NULL;
> This is confusing. parm[actualSize] is a char, not a pointer. You  
> should
> use 0 or '\0' instead of NULL (I personally prefer the former).
>> + argv[argc]=malloc(strlen(url->file));
> strlen + 1 I think...
>> + url_unescape_string(argv[argc++],url->file);
> Although it is less to write, using constructs with argc++ in the  
> middle
> is risky, easily leads to bugs during later changes. Please put it
> separately.

Ok.

>> +  
>> snprintf(myPsnStr,5+10+1+10+1,"- 
>> psn_%u_%u",myPsn.highLongOfPSN,myPsn.lowLongOfPSN);
> call me paranoid, but I'd actually prefer something like
> snprintf(myPsnStr, 5+10+1+10, ...);
> myPsnStr[5+10+1+10] = 0;

There should be actually no chance for an unsigned long to be more than  
10 chars: '4294967296'...
Should I send a fourth version of the patch?