On Thu, Oct 18, 2007 at 06:46:22AM +0000, square87 wrote:
> 2007/10/13, Youness Alaoui <[EMAIL PROTECTED]>:
> >
> > Hi,
> > Thanks Square.
> > Now time for my usual comments ;)
> >
> > 1 - Try to always send a clean patch, don't mix up two things in one
> > patch, it just becomes difficult to know what to look at. [cut...]
>
>
> Ok. I knew that i should always send an only patch... event if it's a big
> patch with various things... Anyway ok, there's no problem.
>
ah sorry sorry, yeah, totally my fault for not being consisten and confusing
you. Yes I did say to send all at once, and now I say send them
separate.. what I really meant is :
1 - don't send *multiple emails* it's better one mail to keep track of your
changes.. better to answer one mail than answer 10.. but put all
the patches in separate files and as separate attachments of the same email to
avoid confusing us...
> 2 - You put :
> > > + if {[winfo exists ".messagebox_$unique"]} { return
> > }
> >
> > This means that the customMessageBox will return "" what happens in that
> > case? did you do a grep on all customMessageBox calls to see what
> > would happen if that function returns nothing ? think of something like
> > that :
> > set ret [customMessageBox ... ]
> > if { $ret == "yes" } { set close 1 } elseif {$ret == "no" } { set close 0
> > }
> > if { $close } { ... }
> >
> > With your patch, we'll get an error with "close : undefined variable"
> > So you have two choices (or 3) :
> > a) Make customMessageBox return "duplicate" (or "") and do a grep and
> > wherever you find a call to customMessageBox, you make sure to check for
> > the "duplicate" case (and make sure that if you just return that it won't
> > affect the function that called the function that called
> > customMessageBox, etc...)
> > b) return "cancel" and make sure that it won't affect anything...
>
>
> Ok. But for the moment only ContainerClose can open more windows... so i
> think that the "duplicate" case should go only in that proc. Anyway i prefer
> "duplicate" than "cancel".
> What i can do now is to check if every procs that call customMessageBox have
> the correct check about the answer, I mean:
> (the "wrong" case): If {$answer == "yes"} {...} else {...}
> Instead of: if {$answer == "yes"} {...} elseif {$answer == "no"} {...}
> etc...
>
true, you're right, the else check should be verified only for procs that call
customMessageBox AND use the 'unique' argument.
> 3 - Why did you name the variable "block" you could have named it "unique"
> > instead, it would have been more logical and easier to
> > understand/maintain
>
>
> You are right... It was a "temporary name" to test the proc and I forgot to
> change it :P
>
yeah, I guessed it was a temporary name.. please clean your code before sending
a patch next time, in any case, it's ok, just rename it to
'unique' instead...
> Thanks,
> > KaKaRoTo
> >
>
> Thanks to you for explanation.
>
> About the patches i'll send them as soon as possible. Atm i am working also
> on others things.
> Bye.
>
take your time, I don't have time to apply anyways :p
> Square87
> -------------------------------------------------------------------------
> This SF.net email is sponsored by: Splunk Inc.
> Still grepping through log files to find problems? Stop.
> Now Search log events and configuration files using AJAX and a browser.
> Download your FREE copy of Splunk now >> http://get.splunk.com/
> _______________________________________________
> Amsn-devel mailing list
> Amsn-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/amsn-devel
-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems? Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
Amsn-devel mailing list
Amsn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/amsn-devel
