Ok.. here are my comments.. first lines of the diff and I already see something
wrong. Vivia said she didn't
notice it because she didn't think of 'clicking there'. I also just tried it
and I got a bug.
so, first, in gui.tcl, line 1768+ you add items to the menu for each user
allowing it to change the custom DP. That's wrong, because of we
follow the HIG (we don't but still, we try to), those shouldn't be there, at
least, in my logic. The place for doing this is totally in the
properties window, not in the menu. We just recently rewrote all of our menus
in order to make them more user-friendly, and now this makes
them non-user friendly again, so I suggest we remove this.
the bug I got was after clicking there, I got the dpbrowser, and I just
canceled it :
[10:15:58] -----------------------------------------
[10:15:58] >>> GOT TCL/TK ERROR : {{can't read "[EMAIL PROTECTED]": no such
variable}}
>>> Stack:
can't read "[EMAIL PROTECTED]": no such variable
while executing
"set customdp_$email"
(procedure "autoChangeCustomDp" line 7)
invoked from within
"autoChangeCustomDp [EMAIL PROTECTED]"
(menu invoke)
I also just noticed that the DP browser has two small visual issues, first, the
label of our DPs is "my cahced display pictures", it should
be "my display pictures", not "cached" since the cached ones are below (this
might be vivia's mistake), also, i noticed that you didn't put
the filenames of the images anymore. This of course shouldn't apply to the
cached DPs, but for your own, it should, "amsn', "ball",
"shell", "rubberduck", or the filename (without extension) of the files the
user added. The date is not necessary afaik. (only talking
about the top frame)
I also noticed that when we open the DP browser, it doesn't automatically
select the current DP. I think it would be nicer that way.
more code comments :
+ if { [::abook::getContactData $target_user customdp] != "" } {
+ set image_name [::abook::getContactData $target_user
customdp ""]
maybe the first line should also have a "" for default..
+ set selected_path [file join $HOME displaypic cache
[filenoext $image_name].png]
+ set selected_image "[filenoext $selected_path].png"
wtf is the purpose of the second line ? selected_path is already the path with
a .png extension, so why selected_image is being built this
way ? there's no need for it.
$combo list insert end "Select a contact:"
This isn't Cristofaro's code, it was there before, but it's not translated, it
should be...
+ set selection [expr [$combo list index end] - 1]
as stated in my mail below, putting that in accolades would be better
probably...
label $w.dppreviewtxt -text "Preview:"
no translation again.. there are probably a lot like these.. needs to be
checked.
+ if { $image_name == "" } {
+ label $w.dppreview -image displaypicture_std_none
+ } else {
+ label $w.dppreview -image [image create photo
[TmpImgName] -file $selected_path -format cximage]
+ }
I don't understand the reason of using the [TmpImgName] thing.. shouldn't it by
displaypicture_std_$targetuser instead ? there must be a
way. + this seems like a temp image is being created and never deleted, a
memory leak it seems.. can you check this out please ?
ohh, there you go... :
couldn't open "C:/Documents.png": no such file or directory
while executing
"image create photo [TmpImgName] -file $selected_path -format cximage"
(procedure "dpBrowser" line 94)
invoked from within
"dpBrowser $email"
that's probably because of the selected_path used before... for some reason
it's wrong.
humm.. ok, it's not how it's retreive, it's how it's set :
::abook::getContactData [EMAIL PROTECTED] customdp
C:/Documents
- button $w.lowerpane.ok -text "[trans ok]" -command "set_displaypic
\${selected_image};destroy $w"
+ button $w.lowerpane.ok -text "[trans ok]" -command "set_displaypic
\${selected_image} $target_user;destroy $w"
This.. just.. seems wrong! if ${selected_image} is being escaped, then it means
that it's not interpreted at this moment, but will only be
interpreted when the ok button is pressed, but then it's interpreted in the
global namespace and that variable is not set in that
namespace, so it should bug.. weird that it doesn't...
This probably should be done some other way, with [list] it would be better I
suppose.. but then, the $selected_image.. it isn't the real
selected image, it's just the value of $selected_image of the local var of when
you just entered the proc...
humm.. ok, I just saw that you made selected_image into a global variable,
that's even worse, we should totally avoid the use of global
variables. Also, I noticed that you set that var if target_user != self, but if
it's self, then what happens ? the var will never be
initialized, so we'll get a 'variable not found' error. I didn't test but it
seems logical that if you open the change DP browser for self
once and you click ok without selecting any DP, then selected_image will never
be set.
So either set it before, or.. try to find a way to make that work without the
user of a global variable (if possible and easy).
+ set image_name [image create photo [TmpImgName] -file $selected_path
-format cximage]
+ $w.dppreview configure -image $image_name
again a memleak whenever someone selects a DP (in updateDPBrowserSelection) :
+proc autoChangeCustomDp { email } {
+proc autoRemoveCustomDp { email } {
Since we'll remove the menu items from the CW, then there's no need for these
functions, which I still don't understand what there purpose
is...
Ahh, before I forget, I also noticed that, with this new dpbrowser, we cant'
access DPs from users who aren't in our list anymore, so if
you have a cahced image and you deleted the user, you need to add him again to
be able to access his cached dp... maybe adding a 'others'
in the menu would be good.. but added only if there *are* cached DPs from users
not in our list anymore.
oh well.. this is it for gui.tcl.. I also took a quick look at skins.tcl and it
seemed ok..
now dpbrowser.tcl remains and abook.tcl and automsg.tcl but I have to get back
to work... I'll conitnue the review of the code soon,
I hope you'll take my comments into consideration and provide us soon with an
additional patch or actually.. you could do it yourself.. I
suggest you read the svnbook to learn a bit more about svn, and then do the
fixes yourself and commit them. yes, indeed, welcome to the
team, you are now an amsnian :)
So, basic rule (for now) is to make sure you update before commiting, and make
sure to save your files before updating, and to never save
after an update (unless you revert to the on disc version, otherwise you might
overwrite a previous commit).
when you commit, you'll need to enter your sourceforge username and password
and you're done!.
I just want to say that I'm pretty impressed by your work. I know that I gave a
lot of comments, and I always do that, but this doesn't
mean that your code is bad, on the contrary, it's good, which is why you joined
the team (I would never have added someone after only one
patch, but your was enough to convince me).
I think the only thing that your work lacked, which caused this big review is
our lack of contribution. You made decisions, you did some
stuff without consulting us and we didn't follow up with you very much, so the
fault is shared on both sides,, I hope next time we'll be
able to share more and don't hesitate to ask questions and ask for our
experience.
Also, a code review is usually necessary because noone is able to view his own
mistakes.
Thanks again and welcome to the team!
KaKaRoTo
On Mon, Jan 08, 2007 at 10:13:37AM -0500, Youness Alaoui wrote:
> Hi Vivia, Cristofaro, everyone else...
> First, Cristofaro, sorry for not committing the patch earlier, I started
> looking at it and didn't have time to
> finish reviewing the code. I just starting looking at it again when I saw
> vivia committed it already. So again,
> sorry for the slow response from my side.
> Vivia, you screwed up big time! As you should know, we must never commit any
> patch to SVN without prior checking
> and reviewing the code. You just told me that you didn't have time to review
> the code, I understand, and you
> didn't want the thread to go unnoticed, I understand, but you know me, I
> rarely forget about some thread and I
> was surely going to do this (especially since Cristofaro is in my MSN list,
> so everytime I look at my CL I
> remember the patch). Also, you should have just answered to say "hey, don't
> forget about this patch.." or
> something.
> I will know review all that code, I don't want any more commits without
> knowing what the code contains. It's not
> because I don't trust the code, it's just that whenever there is new code,
> it's better to review it to make sure
> it's good code, no hacky code, nothing that gets forgotten.. as you should
> know, there were many commits that
> were done with some small typos, lower/upper case, or a == 0 instead of != 0,
> etc.. and even if the code works,
> the bug may not arise because you can't test all cases, and the best way to
> catch the bugs, is as early as
> possible, which means, before the commit (or right after the commit when
> looking at the amsn-commits ML). In
> this case, I'd like to review the code to make sure there's nothing that was
> forgotten.
>
> Cristofaro is also new to amsn and to Tcl, so there are many things he
> doesn't know because of experience, for
> example, a bind $w <1> "set_display_pic $filename" is something that might
> work and he may not see the problem
> with it, but actually, it's a problem in case there's a space, because Tcl
> will evaluate this and make spaces in
> $filename as multiple arguments. the best way to fix it is doing bind $w <1>
> [list set_display_pic $filename].
> Those are tricks we got to know by experience and it took a lot of time to
> figure those out.
> Same for if we see [expr $x * 2] that's wrong, it should be [expr {$x *2}]..
> it's the same thing, but the second
> form will avoid the Tcl core to go through some processing that will make the
> expr evaluate with an increased
> performance.. etc..
>
> I'll answer back again once I review the code.. and I may hopefully be able
> to commit fixes to it too.
>
> KKRT
>
>
> On Mon, Jan 08, 2007 at 04:41:06PM +0200, Vivia Nikolaidou wrote:
> >
> > Thanx, committed :)
> >
> > On Mon, 8 Jan 2007, Cristofaro Del Prete wrote:
> >
> > > Vivia Nikolaidou ha scritto:
> > > > It's OK, but since I committed your previous patch already, can you
> > > > diff
> > > > against current SVN and resend it? :)
> > >
> > > I checked out amsn-devel before of amsn-commits... sorry
> > > Patch attached to message.
> > >
> >
> > -------------------------------------------------------------------------
> > Take Surveys. Earn Cash. Influence the Future of IT
> > Join SourceForge.net's Techsay panel and you'll get the chance to share your
> > opinions on IT & business topics through brief surveys - and earn cash
> > http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
> > _______________________________________________
> > Amsn-devel mailing list
> > Amsn-devel@lists.sourceforge.net
> > https://lists.sourceforge.net/lists/listinfo/amsn-devel
>
> -------------------------------------------------------------------------
> Take Surveys. Earn Cash. Influence the Future of IT
> Join SourceForge.net's Techsay panel and you'll get the chance to share your
> opinions on IT & business topics through brief surveys - and earn cash
> http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
> _______________________________________________
> Amsn-devel mailing list
> Amsn-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/amsn-devel
-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
Amsn-devel mailing list
Amsn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/amsn-devel
