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
>
no, that's cool! Thanks a lot! :)
> 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
>
yep, thanks also for this! :)
Actually, the best thing would have been a diff with code indentation and one
without code indentation for us to review.. but no need
to do it next time, I'm just saying (also another way would be to put the
indented diff, we apply it locally, then do a 'svn diff -w'
for skipping whitespace changes (indentation) so we can review it without
indent, but still have a good indented code)
> 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]
>
damn.. And I kept reading the diff last time trying to figure out what else was
being done for this specific issue... now I see that
those lines are the onl thing related to it, lol.. much better this way! :D
anyways.. I'll review the code right now.. I'm answering now in case someone
starts reviewing he can see that I'll do it and we won't
do the work twice.
so.. if anyone was going to review this code.. forget it, I'm doing it right
now!
will email you later..
KKRT
> 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.
>
> 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
>
> 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)
>
> 5.diff
> We need to call that proc only if $state_change is 1
>
> 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.
>
> 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".
>
> 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.
>
> 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.
>
>
> -----
> 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
> 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
