Hi,
first, thanks Gabriel for yet another interesting patch!
I have (as always) a few comments to make :
first, I'll start with the 'philosophical' par of the questiont.. I don't like
that much the use of a
combobox and have the user set
it to get the behavior.. I see two options :
1 - always show all the users, as it's done now with your patch when the
checkbox is enabled
2 - at the end of the list of users, add a separator (a text with "-----------"
or something) and add
underneath it all the users who have a log and aren't on your list...
this is just my point of view, and I think it would be better, give a better
user experience. Although,
the way you did it looks already pretty good to me.
Let's see what others think!
now, let's do the 'technical' part.. here's the code review I'd give you :
set bMatch [ string match "*.log" $sLogFile ]
if ($bMatch) {
set sLogFile [ string range $sLogFile 0 [ expr { [string
length $sLogFile] - 5 }] ]
the string match is not necessary because you alreary a glob on *.log, but this
is minor
then the string range, I would rather do a [filenoext $sLogFile] to remove the
extension..
+ set bListAllUsers 0
you set it outside a proc, is it a global var? a namespace var? if it's a
namespace var, it should be
instanciated at the start of the file (just after the namespace eval line) and
using :
variable bListAllUsers 0
+ if { $::log::bListAllUsers } {
use :
variable bListAllUsers
then you can use the variable directly, it will be taken from your namespace
var list... it's better
than having the full variable path, in case we decide to change the namespace
(not that we'll change
it, but it's just better programming practice)
+ proc GetContactList { } {
the proc either returns all the users with log or the list of all users (even
without log), I think the
second behavior is the one expected, so maybe in the first if, check if a user
is in the contact list
and doesn't have a log file, then add it to the list of users...
+ proc LoadUsersCombo { wname email } {
+
+ $wname.top.contact.list list delete 0 end
it would be safer to add
if {[winfo exists $wname.top.contact.list ] } {
...
}
+ $wname.top.contact.bAllUsers configure -command
"::log::LoadUsersCombo ${wname} ${email}"
here, even if the wname or email can't have a space, it's better if all
callbacks are done using [list]
to avoid such problems when an argument has a space :
+ $wname.top.contact.bAllUsers configure -command [list
::log::LoadUsersCombo ${wname} ${email}
+ $wname.top.contact.bAllUsers configure -variable
"::log::bListAllUsers"
no need for quotes... this is minor..
I final note, you use a variable bListAllUsers, but it should actually be
[::config::getKey ...] and
[::config::setKey .. ...] and for the -variable of the combobox a
[::config::getVar ...] so the setting
stays saved in the user's config (+ adding a setKey in the config.tcl file)
now, that was the 'technical' part... overall, the code looks well done, for a
first touch to tcl/tk,
it's very well done! :)
KaKaRoTo
On Thu, Mar 15, 2007 at 12:20:41AM -0300, Gabriel Gambetta wrote:
> Here's a patch which does the following. In the View History window
> there's a checkbox next to the user combobox which says "All users with
> logs" and it's initially unchecked.
>
> When this checkbox is unchecked, you get the current behavior of AMSN -
> the combobox is populated with addresses from the current profile's
> contact list.
>
> When the checkbox is checked, however, the combobox is populated with
> the addresses of all the users for whom there are logs in the log
> folder. This includes people no longer in the user's contact list.
>
> I did this because I made a script that converted my old GAIM log files
> to AMSN log files since I've migrated to AMSN a few months ago.
>
> The patch touches loging.tcl (I also added the English and Spanish
> versions of the "All users with logs" text).
>
> GetUsersWithLogs() scans the log directory and finds all the users for
> whom there are log files. Returns an unordered list of such names.
>
> GetContactList() returns either the original contact list (ie the old
> behavior) or the list of users with logs - in both cases it appends the
> eventlog file, as it was done before. The desired behavior is chosen by
> the bListAllUsers variable, which the checkbox bounds to.
>
> LoadUsersCombo() refreshes the combo box with usernames and optionally
> selects an email. This was done in OpenLogWin() but it is now called
> when the state of the checkbox changes.
>
> The changes in OpenLogWin() just use the new functions. No new behavior
> is introduced here except for adding the checkbox to the dialog.
>
> This is my first Tcl/Tk code ever so it probably violates every piece of
> conventional wisdom and good programming practices you may imagine :)
>
> Hope this is useful,
> --Gabriel
>
> Index: lang/langen
> ===================================================================
> --- lang/langen (revisión: 8228)
> +++ lang/langen (copia de trabajo)
> @@ -37,6 +37,7 @@
> allowseen Allow this person to see when I'm online and contact me
> allow_sms Allow users to contact me on my MSN mobile device
> allprofilesinuse All existing profiles are currently in use
> +alluserswithlogs All users with logs
> animatedemoticon This emoticon is animated
> animatedsmileys Enable animated emoticons
> animatenotify Animated Notify Window
> Index: lang/langes
> ===================================================================
> --- lang/langes (revisión: 8228)
> +++ lang/langes (copia de trabajo)
> @@ -37,6 +37,7 @@
> allowseen Permitir a esta persona ver cuándo estás conectado y contactar
> allow_sms Permitir que los usuarios contacten conmigo en mi dispositivo
> móvil MSN
> allprofilesinuse Todos los perfiles que existen están siendo usados
> actualmente
> +alluserswithlogs Todos los registros
> animatedemoticon Emoticono con animación
> animatedsmileys Activar emoticonos animados
> animatenotify Ventana de notificaciones con animación
> Index: loging.tcl
> ===================================================================
> --- loging.tcl (revisión: 8228)
> +++ loging.tcl (copia de trabajo)
> @@ -398,9 +398,94 @@
> }
> }
> }
> +
>
> +
> #///////////////////////////////////////////////////////////////////////////////
> + # GetUsersWithLogs ()
> + # Returns a list of user emails for which there are logs
> +
> + proc GetUsersWithLogs {} {
> + global log_dir
>
> + set lDirs [concat ${log_dir} [glob -nocomplain -types d
> "${log_dir}/*"]]
> +
> + foreach sDir $lDirs {
> + foreach sLogFile [glob -tails -nocomplain -types f
> -directory ${sDir} "*.log"] {
> +
> + set bMatch [ string match "*.log" $sLogFile ]
> + if ($bMatch) {
> + set sLogFile [ string range $sLogFile 0
> [ expr { [string length $sLogFile] - 5 } ] ]
> + set hNames($sLogFile) 1
> + }
> + }
> + }
> +
> + return [ array names hNames ]
> + }
> +
> +
>
> #///////////////////////////////////////////////////////////////////////////////
> + #
> + # GetContactList ()
> + #
> + # Returns the list of contacts, either listing all the users with logs
> or the users
> + # in the contact list
> + #
> + set bListAllUsers 0
> +
> + proc GetContactList { } {
> +
> + if { $::log::bListAllUsers } {
> +
> + set contact_list [ GetUsersWithLogs ]
> +
> + } else {
> +
> + foreach contact [::abook::getAllContacts] {
> + #Selects the contacts who are in our list and
> adds them to the contact_list
> + if {[string last "FL" [::abook::getContactData
> $contact lists]] != -1} {
> + lappend contact_list $contact
> + }
> + }
> + }
> +
> + #Sorts contacts
> + set sortedcontact_list [lsort -dictionary $contact_list]
> +
> + #Add the eventlog
> + lappend sortedcontact_list eventlog
> +
> + return $sortedcontact_list
> + }
> +
> +
> +
> +
> #///////////////////////////////////////////////////////////////////////////////
> + #
> + # LoadUsersCombo(wname)
> + #
> + # Reload the usernames combo
> +
> + proc LoadUsersCombo { wname email } {
> +
> + $wname.top.contact.list list delete 0 end
> +
> + set contact_list [ GetContactList ]
> +
> + foreach sContact $contact_list {
> + $wname.top.contact.list list insert end $sContact
> + }
> +
> + #Get all the list
> + set list [$wname.top.contact.list list get 0 end]
> + #Do a search in that list to find where is exactly the email we
> need
> + set exactMatch [lsearch -exact $list $email]
> + #Select the email in the list when we open the window with the
> result of the search
> + $wname.top.contact.list select $exactMatch
> + }
> +
> +
> +
> #///////////////////////////////////////////////////////////////////////////////
> # OpenLogWin (email)
> # Opens log window for user given by email, Called when History is
> chosen
> # Thinking of adding a button to chat window and History to right click
> in list
> @@ -412,21 +497,9 @@
> proc OpenLogWin { {email ""} } {
>
> global log_dir langenc logvar
> +
> + set sortedcontact_list [ GetContactList ]
>
> - #Get all the contacts
> - foreach contact [::abook::getAllContacts] {
> - #Selects the contacts who are in our list and adds them
> to the contact_list
> - if {[string last "FL" [::abook::getContactData $contact
> lists]] != -1} {
> - lappend contact_list $contact
> - }
> - }
> -
> - #Sorts contacts
> - set sortedcontact_list [lsort -dictionary $contact_list]
> -
> - #Add the eventlog
> - lappend sortedcontact_list eventlog
> -
> #If there is no email defined, we remplace it by the first
> email in the dictionary order
> if {$email == ""} {
> set email [lindex $sortedcontact_list 0]
> @@ -491,21 +564,18 @@
>
> frame $wname.top.contact -class Amsn -borderwidth 0
> combobox::combobox $wname.top.contact.list -editable true
> -highlightthickness 0 -width 22 -bg #FFFFFF -font splainf
> - $wname.top.contact.list list delete 0 end
> - foreach contact $sortedcontact_list {
> - $wname.top.contact.list list insert end $contact
> - }
> -
> - #Get all the list
> - set list [$wname.top.contact.list list get 0 end]
> - #Do a search in that list to find where is exactly the email we
> need
> - set exactMatch [lsearch -exact $list $email]
> - #Select the email in the list when we open the window with the
> result of the search
> - $wname.top.contact.list select $exactMatch
> +
> + LoadUsersCombo $wname $email
> $wname.top.contact.list configure -command "::log::ChangeLogWin
> $wname $email"
> $wname.top.contact.list configure -editable false
>
> pack $wname.top.contact.list -side left
> +
> + checkbutton $wname.top.contact.bAllUsers -text "[trans
> alluserswithlogs]"
> + $wname.top.contact.bAllUsers configure -command
> "::log::LoadUsersCombo ${wname} ${email}"
> + $wname.top.contact.bAllUsers configure -variable
> "::log::bListAllUsers"
> + pack $wname.top.contact.bAllUsers -side left
> +
> pack $wname.top.contact -side left
>
> ::log::LogsByDate $wname $email "1"
> -------------------------------------------------------------------------
> 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
