Alright now for the comments!
On Sun, Dec 16, 2007 at 06:59:38PM +0000, square87 wrote:
> Here i am
> So i split my diff file in 7 diffs... don't tell me now that they are too
> many! :D
>
> In some diffs i haven't put code indentation, so it should be more simple to
> read the "svn diff" result. If you commit a change please check the code
> indentation, i love it :P
>
> 1.diff
> We compare: $user_name != [::abook::getNick $user]
> to see if the name given from the server is different from the nick that we
> stored. But [::abook::getNick $user] return a parsed nick... it means:
> if you are using ColoredNicks, the original nick is [c=3]Square87[/c] but
> using getnick you'll get Square87. This cause the useless calculation of
> some code and in one case it causes that in CL we have a colored nick while
> in topCW we have a nick without style. The solution is to compare $user_name
> with the real nick stored:
>
> $user_name != [::abook::getContactData $user nick]
>
ok, good.. but you put the 'set nick_changed 0' in the if $user_name == "",
while it would have been better to keep it as it was before
(a separate if), because if we ever change the 'else', do something more than
just 'set nick_changed 0' then we would have to duplicate
the code (or worse, forget that part and have two different things being done).
> 2.diff
> That var is useless, it's not used anymore. The code did use that var is
> already commented so the var setting is useless.
>
yeah, thx, I completely removed the line.
a small comment, when you comment a line, and want to ad something, add a ;#
before your comment, so this :
+# set state_no [::MSN::stateToNumber $substate ] ;#this var, at the
moment, is not used anymore
instead of this :
+# set state_no [::MSN::stateToNumber $substate ] this var, at the moment,
is not used anymore
This way if we uncomment it, it won't bug...
> 3.diff
> I removed "switch cases" they are useless. It says:
> if NLN calls run_alarm values..... [trans online]
> if IDL calls run_alarm value.... [trans away] (this is also an error idle !=
> away)
> if BSY calls run_alarm value... [trans busy]
> etc... etc...
>
> Now values are always the same the only value that change is the status...
> so i put:
> set status "[trans [::MSN::stateToDescription $substate]]"
> and then just:
> run_alarm value.... $status
>
brilliant! commited patch as is.
> 4.diff
> That code does one thing it says: "Hey the $user has (changed (his/her)
> status/ log out/ log in)"
> So put at the top of that code If {$state_changed} { .... }
> In that code every block starts with $if {state_changed} i just removed it
> because i put it on top.
> Then i removed also "$nick_changed check" i don't understand the reason when
> an user change his/her nickname we should use code for status_changed... And
> it cause also a bug:
> 1) block an user (and maybe stay invisible)
> 2) set that you want a notify window when user change his status
> 3) The blocked user change his DP
> you'll get a notify window that the user has changed his status... without
> changing it (this is caused in particular when the blocked user has a
> colored nick and thanks to the bug with nick comparing, correct with 1.diff,
> the $nick_changed is equal to 1)
>
hehe, yeah it's funny all those if $state_changed.. I tried to understand the
nick_change, and I think it's because you usually want to
get a notification window when a user changes his nick (that's how it was used
before). This is going to be backward incompatible, but
I'll take the risk, in any case, the requirement is for a notify window when
the user changes his status...
But I still added a block before that code for the remote controler, if the
user changes his nick, it should notify him.
not indenting was a HUGE benefit here, hehe.. I committed the code indented.
> 5.diff
> We need to call that proc only if $state_change is 1
>
not necessarly, but I applied it.. in theory, if the state didn't change, then
that line just does nothing...
a 'set' versus a 'if'.. no optimisation there...
> 6.diff
> Actually we always do: ::abook::setContactData $user displaypicfile $newPic
> It means that we register the name of newPic but we need to do that ONLY IF
> oldPic != newPic AND if we can load the newPic.
>
why AND if we can load the newpic ? we don't... the user changed his DP, then
it has to be saved.. I'm ok for 'set it only if it
changed', but not 'only if it can be loaded'.. you block a user, he changes his
DP, you always see the old DP even if he changed it?
no, it's not real.. + if you unblock him, then what ? you won't know he has a
new DP so amsn won't download the new DP?
> Then when $oldPIC != $newPIC the first thing that we can do is to check:
> if we already have in our cache that dp -> if yes load it
> else check the old cases.
> Adding that new check we can load a dp when sometimes we wasn't able.
> So this diffs correct a bug and it's more "realistic".
>
true, very good!
> About DP there are two others cases that should be fixed but i don't want to
> talk about that, because i suppose a solution but i never tested them.
>
> Anyway just i said before i'd like to have this code in a separated proc,
> maybe not in protocol.tcl anymore. Because it can be useful also for other
> cases.
>
well, first, you added a $pic_changed, but never used.. so I removed it...
also, I added the setContatData for all cases and removed it from every little
if.
You also didn't think of something.. what if we are hidden, but we already have
an SB open for someone.. so the last 'else' should
actually search for an open SB just like what it does when the user is
blocked...
And finally, you missed something.. if you have two SBs with someone (a private
chat and a multi convo chat), the foreach will find the
two SBs and will try to load the DP from each SB which could result in file
corruption... so I added 'break' once it finds one open SB
for the user.
> 7.diff
> We need to call that proc only if a value (or more) is 1
> Think for example when you logout and after X minutes you login (maybe just
> to restart aMSN). I don't think that ALL users has changed in X minutes
> nick, dp or status.
>
oh, here's the use of $pic_changed, hehe..
anyways, it's not really needed...
But anyways, that line is only used to resort the contact list, so it should
only be done if the status/nick changes, not when the
DP changes
And by the way, we flush the whole CL if you logout, it's not cached and it
shouldn't be, so your example is wrong :)
>
> -----
> For me, the actual version of this proc is not so nice and it spends time on
> useless thing.
>
> That's all folks :P
> Sorry for my English
> and keep me updated :)
>
> Square87
Anyways, all of this is some very well done work, thanks a lot for it, thx for
taking the time to do it, fix some nice stuff there, and
good explanation and one of the first times you send a patch that I didn't need
to answer you with 'badly done' :p hehe
and thx for splitting it in several files, it sure as hell helped!!!
KaKaRoTo
> Index: protocol.tcl
> ===================================================================
> --- protocol.tcl (revisione 9113)
> +++ protocol.tcl (copia locale)
> @@ -4856,11 +4856,9 @@
> }
>
> if {$user_name == ""} {
> - set user_name [::abook::getNick $user]
> - }
> -
> -
> - if {$user_name != [::abook::getNick $user]} {
> + set user_name [::abook::getContactData $user nick]
> + set nick_changed 0
> + } elseif {$user_name != [::abook::getContactData $user nick]} {
> #Nick differs from the one on our list, so change it
> #in the server list too
> ::abook::setContactData $user nick $user_name
> Index: protocol.tcl
> ===================================================================
> --- protocol.tcl (revisione 9113)
> +++ protocol.tcl (copia locale)
> @@ -4889,7 +4889,7 @@
>
> set custom_user_name [::abook::getDisplayNick $user]
>
> - set state_no [::MSN::stateToNumber $substate ]
> +# set state_no [::MSN::stateToNumber $substate ] this var, at the moment,
> is not used anymore
>
>
> #alarm system (that must replace the one that was before) - KNO
> Index: protocol.tcl
> ===================================================================
> --- protocol.tcl (revisione 9113)
> +++ protocol.tcl (copia locale)
> @@ -4905,54 +4905,11 @@
> }
>
> } else {
> + set status "[trans [::MSN::stateToDescription
> $substate]]"
> if { ( [::alarms::isEnabled $user] == 1 )&& (
> [::alarms::getAlarmItem $user onstatus] == 1) } {
> - switch -exact [lindex $recv 1] {
> - "NLN" {
> - run_alarm $user $user
> $custom_user_name "[trans changestate $custom_user_name [trans online]]"
> - }
> - "IDL" {
> - run_alarm $user $user
> $custom_user_name "[trans changestate $custom_user_name [trans away]]"
> - }
> - "BSY" {
> - run_alarm $user $user
> $custom_user_name "[trans changestate $custom_user_name [trans busy]]"
> - }
> - "BRB" {
> - run_alarm $user $user
> $custom_user_name "[trans changestate $custom_user_name [trans rightback]]"
> - }
> - "AWY" {
> - run_alarm $user $user
> $custom_user_name "[trans changestate $custom_user_name [trans away]]"
> - }
> - "PHN" {
> - run_alarm $user $user
> $custom_user_name "[trans changestate $custom_user_name [trans onphone]]"
> - }
> - "LUN" {
> - run_alarm $user $user
> $custom_user_name "[trans changestate $custom_user_name [trans gonelunch]]"
> - }
> - }
> + run_alarm $user $user $custom_user_name "[trans
> changestate $custom_user_name $status]"
> } elseif { ( [::alarms::isEnabled all] == 1 )&& (
> [::alarms::getAlarmItem all onstatus] == 1)} {
> - switch -exact [lindex $recv 1] {
> - "NLN" {
> - run_alarm all $user
> $custom_user_name "[trans changestate $custom_user_name [trans online]]"
> - }
> - "IDL" {
> - run_alarm all $user
> $custom_user_name "[trans changestate $custom_user_name [trans away]]"
> - }
> - "BSY" {
> - run_alarm all $user
> $custom_user_name "[trans changestate $custom_user_name [trans busy]]"
> - }
> - "BRB" {
> - run_alarm all $user
> $custom_user_name "[trans changestate $custom_user_name [trans rightback]]"
> - }
> - "AWY" {
> - run_alarm all $user
> $custom_user_name "[trans changestate $custom_user_name [trans away]]"
> - }
> - "PHN" {
> - run_alarm all $user
> $custom_user_name "[trans changestate $custom_user_name [trans onphone]]"
> - }
> - "LUN" {
> - run_alarm all $user
> $custom_user_name "[trans changestate $custom_user_name [trans gonelunch]]"
> - }
> - }
> + run_alarm all $user $custom_user_name "[trans
> changestate $custom_user_name $status]"
> }
> }
> }
> Index: protocol.tcl
> ===================================================================
> --- protocol.tcl (revisione 9113)
> +++ protocol.tcl (copia locale)
> @@ -4959,29 +4959,27 @@
> #end of alarm system
>
>
> + if {$state_changed} {
> set maxw [expr {([::skin::getKey notifwidth]-53)*2} ]
> set short_name [trunc $custom_user_name . $maxw splainf]
>
> #User logsout
> if {$substate == "FLN"} {
>
> - if { $state_changed } {
> #Register last logout, last seen and notify it in the
> events
> ::abook::setAtomicContactData $user [list last_logout
> last_seen] \
> [list [clock format [clock seconds] -format "%D -
> %H:%M:%S"] [clock format [clock seconds] -format "%D - %H:%M:%S"]]
> ::log::event disconnect $custom_user_name
> - }
>
> # Added by Yoda-BZH
> - if { ($remote_auth == 1) && $state_changed } {
> + if { $remote_auth == 1 } {
> set nameToWriteRemote "$user_name ($user)"
> write_remote "** $nameToWriteRemote [trans logsout]"
> event
> }
>
> - if { ($state_changed || $nick_changed) &&
> - (([::config::getKey notifyoffline] == 1 &&
> + if { ([::config::getKey notifyoffline] == 1 &&
> [::abook::getContactData $user notifyoffline -1] != 0) ||
> - [::abook::getContactData $user notifyoffline -1] == 1) } {
> + [::abook::getContactData $user notifyoffline -1] == 1 } {
> #Show notify window if globally enabled, and not
> locally disabled, or if just locally enabled
> ::amsn::notifyAdd "$short_name\n[trans logsout]." ""
> offline offline $user
> }
> @@ -4990,28 +4988,24 @@
> # an initial state notification
> } elseif {[::abook::getVolatileData $user state FLN] != "FLN" &&
> [lindex $recv 0] != "ILN" } {
>
> - if { $state_changed } {
> #Notify in the events
> ::log::event state $custom_user_name
> [::MSN::stateToDescription $substate]
> - }
>
> # Added by Yoda-BZH
> - if { ($remote_auth == 1) && ($state_changed || $nick_changed)
> } {
> + if { $remote_auth == 1 } {
> set nameToWriteRemote "$user_name ($user)"
> write_remote "** [trans changestate $nameToWriteRemote
> [trans [::MSN::stateToDescription $substate]]]" event
> }
>
> - if { ($state_changed || $nick_changed) &&
> - (([::config::getKey notifystate] == 1 &&
> + if { ([::config::getKey notifystate] == 1 &&
> [::abook::getContactData $user notifystatus -1] != 0) ||
> - [::abook::getContactData $user notifystatus -1] == 1) } {
> + [::abook::getContactData $user notifystatus -1] == 1 } {
> ::amsn::notifyAdd "$short_name\n[trans
> statechange]\n[trans [::MSN::stateToDescription $substate]]." \
> "::amsn::chatUser $user" state state $user
> }
>
> } elseif {[lindex $recv 0] == "NLN"} { ;# User was offline, now online
>
> - if { $state_changed } {
> #Register last login and notify it in the events
> ::abook::setContactData $user last_login [clock format
> [clock seconds] -format "%D - %H:%M:%S"]
> ::log::event connect $custom_user_name
> @@ -5023,29 +5017,26 @@
> #later on with x-clientcaps
> ::abook::setContactData $user clientname ""
> ::plugins::PostEvent UserConnect evPar
> - }
>
> # Added by Yoda-BZH
> - if { ($remote_auth == 1) && $state_changed } {
> + if { $remote_auth == 1 } {
> set nameToWriteRemote "$user_name ($user)"
> write_remote "** $nameToWriteRemote [trans logsin]"
> event
> }
>
> - if { ($state_changed || $nick_changed) &&
> - (([::config::getKey notifyonline] == 1 &&
> + if { ([::config::getKey notifyonline] == 1 &&
> [::abook::getContactData $user notifyonline -1] != 0) ||
> - [::abook::getContactData $user notifyonline -1] == 1) } {
> + [::abook::getContactData $user notifyonline -1] == 1 } {
> ::amsn::notifyAdd "$short_name\n[trans logsin]."
> "::amsn::chatUser $user" online online $user
> }
>
> - if { $state_changed } {
> if { ( [::alarms::isEnabled $user] == 1 )&& (
> [::alarms::getAlarmItem $user onconnect] == 1)} {
> run_alarm $user $user $custom_user_name
> "$custom_user_name [trans logsin]"
> } elseif { ( [::alarms::isEnabled all] == 1 )&& (
> [::alarms::getAlarmItem all onstatus] == 1)} {
> run_alarm all $user $custom_user_name
> "$custom_user_name [trans logsin]"
> }
> - }
> }
> + }
>
> # Retreive the new display picture if it changed
> # set oldmsnobj [::abook::getVolatileData $user msobj]
> Index: protocol.tcl
> ===================================================================
> --- protocol.tcl (revisione 9113)
> +++ protocol.tcl (copia locale)
> @@ -5051,7 +5051,9 @@
> # set oldmsnobj [::abook::getVolatileData $user msobj]
> #set list_users [lreplace $list_users $idx $idx [list $user $user_name
> $state_no $msnobj]]
>
> + if {$state_changed} {
> ::abook::setVolatileData $user state $substate
> + }
> ::abook::setVolatileData $user msnobj $msnobj
> set oldPic [::abook::getContactData $user displaypicfile]
> set newPic [::MSNP2P::GetFilenameFromMSNOBJ $msnobj]
> Index: protocol.tcl
> ===================================================================
> --- protocol.tcl (revisione 9113)
> +++ protocol.tcl (copia locale)
> @@ -5055,32 +5055,40 @@
> ::abook::setVolatileData $user msnobj $msnobj
> set oldPic [::abook::getContactData $user displaypicfile]
> set newPic [::MSNP2P::GetFilenameFromMSNOBJ $msnobj]
> - ::abook::setContactData $user displaypicfile $newPic
> -
> - if { ($oldPic != $newPic) && ($newPic == "") } {
> - ::skin::getDisplayPicture $user 1
> - } elseif { $oldPic != $newPic} {
> - status_log "picture changed for user $user\n" white
> - if { [::config::getKey lazypicretrieval] ||
> [::MSN::userIsBlocked $user]} {
> - global sb_list
> - foreach sb $sb_list {
> - set users_in_chat [$sb cget -users]
> - if { [lsearch $users_in_chat $user] != -1 } {
> - status_log "User changed image while
> image in use!! Updating!!\n" white
> - ::MSNP2P::loadUserPic [::MSN::ChatFor
> $sb] $user
> - }
> - }
> +
> + if { $oldPic != $newPic } {
> + set pic_changed 1
> +
> + if { $newPic == "" } {
> + ::skin::getDisplayPicture $user 1
> } else {
> - if { [::MSN::myStatusIs] != "FLN" &&
> [::MSN::myStatusIs] != "HDN"} {
> - if { ![file readable "[file join $HOME
> displaypic cache ${newPic}].png"] } {
> - set chat_id [::MSN::chatTo $user]
> - ::MSN::ChatQueue $chat_id [list
> ::MSNP2P::loadUserPic $chat_id $user]
> - } else {
> - #We already have the image so don't
> open a convo to get it just load it
> - ::MSNP2P::loadUserPic "" $user
> + status_log "picture changed for user $user\n" white
> +
> + if { [file readable "[file join $HOME displaypic cache
> ${newPic}].png"] } {
> + ;#it's possible that the user set again a DP that we
> already have in our cache so just load it again, even if we are HDN, or the
> user is blocked.
> + ::MSNP2P::loadUserPic "" $user
> + ::abook::setContactData $user displaypicfile
> $newPic
> + } elseif { [::config::getKey lazypicretrieval] ||
> [::MSN::userIsBlocked $user]} {
> + global sb_list
> +
> + foreach sb $sb_list {
> + set users_in_chat [$sb cget -users]
> + if { [lsearch $users_in_chat $user] !=
> -1 } {
> + status_log "User changed image
> while image in use!! Updating!!\n" white
> + ::MSNP2P::loadUserPic
> [::MSN::ChatFor $sb] $user
> + ::abook::setContactData $user
> displaypicfile $newPic
> + }
> }
> + } elseif { [::MSN::myStatusIs] != "FLN" &&
> [::MSN::myStatusIs] != "HDN"} {
> + set chat_id [::MSN::chatTo $user]
> + ::MSN::ChatQueue $chat_id [list
> ::MSNP2P::loadUserPic $chat_id $user]
> + ::abook::setContactData $user displaypicfile
> $newPic
> + } else {
> + set pic_changed 0
> }
> }
> + } else {
> + set pic_changed 0
> }
>
>
>
>
>
>
> ----------------------
> The following is how the code appear without diffs.
> ----------------------
>
>
> if { $oldPic != $newPic } {
> set pic_changed 1
>
> if { $newPic == "" } {
> ::skin::getDisplayPicture $user 1
> } else {
> status_log "picture changed for user $user\n" white
>
> if { [file readable "[file join $HOME displaypic cache
> ${newPic}].png"] } {
> ;#it's possible that the user set again a DP that we
> already have in our cache so just load it again, even if we are HDN, or the
> user is blocked.
> ::MSNP2P::loadUserPic "" $user
> ::abook::setContactData $user displaypicfile
> $newPic
> } elseif { [::config::getKey lazypicretrieval] ||
> [::MSN::userIsBlocked $user]} {
> global sb_list
>
> foreach sb $sb_list {
> set users_in_chat [$sb cget -users]
> if { [lsearch $users_in_chat $user] !=
> -1 } {
> status_log "User changed image
> while image in use!! Updating!!\n" white
> ::MSNP2P::loadUserPic
> [::MSN::ChatFor $sb] $user
> ::abook::setContactData $user
> displaypicfile $newPic
> }
> }
> } elseif { [::MSN::myStatusIs] != "FLN" &&
> [::MSN::myStatusIs] != "HDN"} {
> set chat_id [::MSN::chatTo $user]
> ::MSN::ChatQueue $chat_id [list
> ::MSNP2P::loadUserPic $chat_id $user]
> ::abook::setContactData $user displaypicfile
> $newPic
> } else {
> set pic_changed 0
> }
> }
> } else {
> set pic_changed 0
> }
> Index: protocol.tcl
> ===================================================================
> --- protocol.tcl (revisione 9113)
> +++ protocol.tcl (copia locale)
> @@ -5083,8 +5083,10 @@
> }
> }
>
> + if { $state_changed || $nick_changed || $pic_changed} {
> + ::MSN::contactListChanged
> + }
>
> - ::MSN::contactListChanged
> if { $state_changed || $nick_changed } {
>
> foreach chat_id [::ChatWindow::getAllChatIds] {
> -------------------------------------------------------------------------
> SF.Net email is sponsored by:
> Check out the new SourceForge.net Marketplace.
> It's the best place to buy or sell services
> for just about anything Open Source.
> http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace
> _______________________________________________
> Amsn-devel mailing list
> Amsn-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/amsn-devel
-------------------------------------------------------------------------
SF.Net email is sponsored by:
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services
for just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace
_______________________________________________
Amsn-devel mailing list
Amsn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/amsn-devel
