2009/3/27 Pablo Castellano <pablog.ubu...@gmail.com>
> I have now understood how do RestorePreferences/SavePreferences work and
> I have improved the patch.
Great job there!
Comments inlined in the diff!
Although there are quite a lot of comments, it's just because I'm very picky
on the code I review! but in general, I liked your code very much, much
cleaner and simpler than before! Thanks a lot for that! :)
I was just about to fix it all myself and commit, but there are some stuff
that I wasn't too sure about why you did it or how it should be done
correctly, so i just commented on everything and I hope your next patch will
get into svn :)
Thanks,
KaKaRoTo
>
>
>
> Youness Alaoui wrote:
> > Hi,
> > Thanks for the patch and for giving a detailed explanation
> > of your changes...
> > however, I see two problems :
> > 1 - code duplication.. it would be better to have the code
> > that checks for the system configs in config.tcl after we
> ind> loadProfile... this way, the code will be there only one...
> > Also, make sure that the option for 'use system settings'
> > would only be available when it is actually possible to
> > check user settings (on mac, windows, do not show the
> > option)..
> > also note that it might be confusing if someone has old
> > settings for gnome, and is using KDE, and amsn takes gnome
> > settings instead of his new KDE settings... maybe check for
> > the $::env(DESKTOP) variable too...
> >
> > Thanks again for the contribution!
> >
> > KaKaRoTo
> >
>
>
> --
> Regards, Pablo.
>
> Index: lang/langen
> ===================================================================
> --- lang/langen (revisión: 11112)
> +++ lang/langen (copia de trabajo)
> @@ -940,6 +940,7 @@
> subject Subject
> svnversion SVN Version
> syntaxerror Syntax Error
> +systemconnection Use system proxy settings
> tabbedglobal A tabbed window for all users in the contact list
> tabbedgroups A tabbed window for users from the same group
> tabbed Tabbed Windows
> Index: lang/langes
> ===================================================================
> --- lang/langes (revisión: 11112)
> +++ lang/langes (copia de trabajo)
> @@ -941,6 +941,7 @@
> subjectrequired Asunto requerido
> svnversion Versión SVN
> syntaxerror Error de sintaxis
> +systemconnection Usar los datos de proxy del sistema
> tabbedglobal Una ventana con pestañas para todos los usuarios
> tabbedgroups Una ventana con pestañas para cada grupo
> tabbed Ventanas con pestañas
> Index: login_screen.tcl
> ===================================================================
> --- login_screen.tcl (revisión: 11112)
> +++ login_screen.tcl (copia de trabajo)
> @@ -786,6 +786,10 @@
> CreateProfile $user
> }
>
> + if {[OnUnix] && [::config::getKey usesystemproxy] == 1} {
> + load_unix_system_proxy
> + }
> +
Why do this in login_screen.tcl ? I guess if you want it to be on startup,
you should do it in ::MSN::connect or in the proxy object constructor...
Anyways, find a better, place, login_screen is not the place to start doing
proxy stuff (keeping the code clean)
>
> # Login with them
> $self login $user $pass
> }
> Index: preferences.tcl
> ===================================================================
> --- preferences.tcl (revisión: 11112)
> +++ preferences.tcl (copia de trabajo)
> @@ -2221,6 +2221,11 @@
> grid $lfname.4.ssl -row 1 -column 2 -sticky w -pady 5 -padx 10
> grid $lfname.4.socks5 -row 1 -column 3 -sticky w -pady 5 -padx 10
>
> + #Only show this option if the OS supports a system-wide proxy
> configuration.
> + if { [OnUnix] } {
> + checkbutton $lfname.4.system -text "[trans
> systemconnection]" -onvalue 1 -offvalue 0 -variable [::config::getVar
> usesystemproxy] -command UpdatePreferences
> + grid $lfname.4.system -row 1 -column 4 -sticky w -pady 5
> -padx 10
> + }
>
> label $lfname.5.lserver -text "[trans server] :" -padx 5 -font
> sboldf
> entry $lfname.5.server -font splainf -width 20 -textvariable
> proxy_server
> @@ -3108,11 +3113,11 @@
> $lfname.lfname2.statelist.box insert end [lindex [StateList
> get $idx] 0]
> }
>
> - # Fill the user's lists
> - #set lfname [Rnotebook:frame $nb $Preftabs(privacy)]
> + # Fill the user's lists
> + #set lfname [Rnotebook:frame $nb $Preftabs(privacy)]
> set lfname [$nb.nn getframe privacy]
> set lfname [$lfname.sw.sf getframe]
> - Fill_users_list "$lfname.lfname" "$lfname.lfname2"
> + Fill_users_list "$lfname.lfname" "$lfname.lfname2"
I hate stuff like that because it is very confusing.. I waste a lot of time
comparing the two lines one char at a time trying to understand "what did he
change in here!" before realizing it's just a whitespace/indentation
issue... try to clean your patches of stuff like that when you want it to be
reviewed by someone! Thanks
>
>
> }
>
> @@ -3120,7 +3125,9 @@
> # This is where the preferences entries get enabled disabled
> proc UpdatePreferences {} {
> global Preftabs
> -
> + global proxy_server proxy_port proxy_user proxy_pass
> + global saved_settings was_system
> +
> set nb .cfg.notebook
>
> #fonts
> @@ -3164,8 +3171,14 @@
> $lfname.4.post configure -state normal
> $lfname.4.ssl configure -state disable
> $lfname.4.socks5 configure -state normal
> + $lfname.4.system configure -state normal
bad indentation!
>
> $lfname.5.server configure -state normal
> $lfname.5.port configure -state normal
> +
> + if { ![info exists was_system] } {
> + set was_system [::config::getKey usesystemproxy]
> + }
> +
> if { [::config::getKey proxytype] == "socks5" ||
> [::config::getKey proxytype] == "http"} {
> $lfname.5.user configure -state normal
> $lfname.5.pass configure -state normal
> @@ -3173,10 +3186,45 @@
> $lfname.5.user configure -state disabled
> $lfname.5.pass configure -state disabled
> }
> +
> + if { [::config::getKey usesystemproxy] == 1 } {
> +
> + if { $was_system == 0 } {
> + set saved_settings [list $proxy_server
> $proxy_port [::config::getKey proxyauthenticate] $proxy_user $proxy_pass]
> + }
> +
> + load_unix_system_proxy
> + $lfname.5.server configure -state disabled
> + $lfname.5.port configure -state disabled
> + $lfname.5.user configure -state disabled
> + $lfname.5.pass configure -state disabled
> + } else {
> +
> + if { [info exists saved_settings] } {
> + set proxy_server [lindex $saved_settings 0]
> + set proxy_port [lindex $saved_settings 1]
> + ::config::setKey proxyauthenticate [lindex
> $saved_settings 2]
> + set proxy_user [lindex $saved_settings 3]
> + set proxy_pass [lindex $saved_settings 4]
> + } elseif { [::config::getKey usesystemproxy] == 0
> && $was_system == 1 } {
> + set proxy_server ""
> + set proxy_port 8080
> + ::config::setKey proxyauthenticate 0
> + set proxy_user ""
> + set proxy_pass ""
> + }
> +
> + }
> +
> + if { $was_system == 0 || [::config::getKey usesystemproxy]
> == 0 } {
> + set was_system [::config::getKey usesystemproxy]
> + }
> +
I don't really understand this... I don't see why you have the 'was_system'
variable... Also, the saved_settings should be stored in the config as
'saved_user_proxy_settings' or something like that... because if you restart
aMSN and you disable the use system settings, then it should revert to what
the user had..
Also (see below) the saving/loading of the system settings should be done in
my opinion inside the load_unix_system_settings proc you have in
config.tcl.. and the UpdatePreferences should actually just do a :
if {usesystemsettings} {
load_unix_system_settings
} else {
load_user_saved_proxy_settings
}
so it can switch on/off what the user chose as soon as you enable/disable
the option.
>
> } else {
> $lfname.4.post configure -state disabled
> $lfname.4.ssl configure -state disabled
> $lfname.4.socks5 configure -state disabled
> + $lfname.4.system configure -state disabled
> $lfname.5.server configure -state disabled
> $lfname.5.port configure -state disabled
> $lfname.5.user configure -state disabled
> @@ -3249,6 +3297,7 @@
> proc SavePreferences {} {
> global auto_path HOME2 tlsinstalled
> global myconfig proxy_server proxy_port list_BLP temp_BLP Preftabs
> libtls proxy_user proxy_pass pager locale_codes
> + global saved_settings was_system
>
> set nb .cfg.notebook
>
> @@ -3279,6 +3328,9 @@
> ::config::setKey proxyauthenticate 0
> }
>
> + catch { unset saved_settings }
> + catch { unset was_system }
> +
> if {![string is digit [::config::getKey initialftport]] || [string
> length [::config::getKey initialftport]] == 0 } {
> ::config::setKey initialftport 6891
> }
> @@ -3450,7 +3502,11 @@
> if { $win != ".cfg" } { return }
>
> global myconfig proxy_server proxy_port
> -
> + global saved_settings was_system
> +
> + catch { unset saved_settings }
> + catch { unset was_system }
> +
> ::config::setAll [array get myconfig]
> array unset myconfig
>
> Index: config.tcl
> ===================================================================
> --- config.tcl (revisión: 11112)
> +++ config.tcl (copia de trabajo)
> @@ -33,6 +33,7 @@
> ::config::setKey proxyauthenticate 0 ;# SOCKS5
> use username/password
> ::config::setKey proxyuser "" ;# user and password
> for SOCKS5 proxy
> ::config::setKey proxypass "" ;#
> + ::config::setKey usesystemproxy 0 ;#System-wide proxy
> configuration
>
> ::config::setKey sound 1 ;#Sound
> enabled: 0|1
> ::config::setKey mailcommand "" ;#Command for
> checking mail. Blank for hotmail
> @@ -836,6 +837,205 @@
>
> }
>
> +
> +#Original by Isma from desktop_integration plugin
> +#Modified by PabloCastellano
> +#######################################################################
> +# It says which desktop are we using, and so, what program #
> +# Return values: gnome, kde, xfce, unknown #
> +#######################################################################
> +proc WhichDesktop {} {
> + variable plugin_name
> + global env
> +
> + # Find zenity and kdialog
> + catch {exec which zenity} zenity_path
> + catch {exec which kdialog} kdialog_path
> +
> + #See which one of the programs do we have
> + set has_zenity [file executable $zenity_path ]
> + set has_kdialog [file executable $kdialog_path ]
> +
> + #If we only have one of them, we choose it
> + if {$has_zenity && !$has_kdialog} {
> + status_log "Found Gnome" red
> + return "gnome"
> + } elseif {!$has_zenity && $has_kdialog} {
> + status_log "Found KDE" red
> + return "kde"
> + } elseif {!$has_zenity && !$has_kdialog} {
> + # If no program is installed, we return 'noone'
> + status_log "Found neither Gnome nor KDE" red
> + return "unknown"
> + }
> +
> + # If both of them are installed, we guess the desktop
> + status_log "Found both Gnome and KDE" red
> +
> + # First, see if environment var exists
> + if { [info exists env(DESKTOP_SESSION)] } {
> + set session_var $env(DESKTOP_SESSION)
> +
> + if {$session_var == "gnome" || $session_var == "kde" ||
> $session_var == "xfce"} {
> + return $session_var
> + }
> + }
> +
> + # If the variable doesn't help, let's see the number of processes
> + if { [catch {exec ps -A | grep gnome | wc -l} n_gnome] } {
> + set n_gnome 0
> + }
> +
> + if { [catch {exec ps -A | grep kde | wc -l} n_kde] } {
> + set n_kde 0
> + }
> +
> + if {$n_gnome >= $n_kde} {
> + return "gnome"
> + } else {
> + return "kde"
> + }
> +
> +}
> +
> +
> +#Returns a gnome gconf key
> +proc get_gconf_key { key {default ""} } {
> +
> + if { ! [catch { eval [concat [list "exec"] "gconftool -g $key"]}
> value ] } {
> + return $value
> + } else {
> + return $default
> + }
> +
> +}
> +
> +
> +#Returns a kde kconf key
> +proc get_kconf_key { rcfile group key {default ""} } {
> +
> + set command "kreadconfig --file $rcfile --group $group --key $key
> --default $default"
> + catch { eval [concat [list "exec"] $command]} value
> + return value
> +
> +}
if the catch fails here, then the return value could be "command not found",
so you should actually do like for gconf, an if/else and return $default if
it fails
>
> +
>
> +#///////////////////////////////////////////////////////////////////////////////
> +# System-wide proxy settings by PabloCastellano
> +# It first tries to get the configuration from your windows manager and if
> it
> +# fails then it tries to get envvar $http_proxy
> +# Also note that $::env(http_proxy) will only exist if amsn is launched
> from a terminal
> +# and that won't be updated until you open a new terminal.
> +proc load_unix_system_proxy {} {
> +
> + global proxy_server proxy_port proxy_user proxy_pass
> proxyauthenticate
> +
> + set mydesktop [WhichDesktop]
> + set g_proxymode [get_gconf_key "/system/proxy/mode" PROXYERROR]
> + set k_proxymode [get_kconf_key kioslaverc "Proxy Settings"
> ProxyType PROXYERROR]
> +
> + if { $mydesktop == "gnome" && $g_proxymode != "PROXYERROR"} {
> +
> + #Maybe use_same_proxy and autoconfig_url keys should be
> considered in future.
> + status_log "systemproxy: using gconf\n" red
> +
> + set usehttp [get_gconf_key
> "/system/http_proxy/use_http_proxy"]
> +
> + if {$usehttp == "true"} {
> + #Should I check if the returned data is valid?
> + if { [::config::getKey proxytype] == "http" } {
You should check here for $g_proxymode not [config::getKey proxytype]
>
> + set proxy_server [get_gconf_key
> "/system/http_proxy/host"]
> + set proxy_port [get_gconf_key
> "/system/http_proxy/port"]
> + set proxyauth [get_gconf_key
> "/system/http_proxy/use_authentication"]
> + } else {
> + #socks...
> + set proxy_server [get_gconf_key
> "/system/proxy/socks_host"]
> + set proxy_port [get_gconf_key "/system/proxy/socks_port"]
> + set proxyauth [get_gconf_key
> "/system/http_proxy/use_authentication"]
proxyauth for socks is stored in /system/http_proxy/use_authentication ?
>
> + }
> +
> + if {$proxyauth == "true"} {
> + set proxyauthenticate 1
> + set proxy_user [get_gconf_key
> "/system/http_proxy/authentication_user"]
> + set proxy_pass [get_gconf_key
> "/system/http_proxy/authentication_password"]
> + } else {
> + set proxyauthenticate 0
> + set proxy_user ""
> + set proxy_pass ""
> + }
> +
> + } else {
> + #Configure as direct. (use_http_proxy == "false" ||
> mode == "none")
> + set proxy_server ""
> + set proxy_port ""
> + set proxyauthenticate 0
> + set proxy_user ""
> + set proxy_pass ""
> + }
The else is only for use_http_proxy, not for mode... maybe you're missing
another else?
Also, why have a use_http_proxy AND a mode ? Maybe a comment explaining the
difference would help!
>
> +
> + #TODO: Improve KDE authentication support.
> + } elseif { $mydesktop == "kde" && $k_proxymode != "PROXYERROR" } {
> +
> + status_log "systemproxy: using kconf\n" red
> + set proxy [get_kconf_key kioslaverc "Proxy Settings"
> httpProxy]
> +
> + if { $k_proxymode == 1 && $proxy != "" } {
> + #regexp. http://%s:%...@%s:%d
> + regsub
> {^(?:(?:[^:/?#]+):)?(?://(?:(?:(?:([^:@]*):?([^:@]*))?@)?([^:/?#]*)(?::(\d*).*)?))?}
> $proxy {"\1" "\2" "\3" "\4"} result
> + set proxy_user [lindex $result 0]
> + set proxy_pass [lindex $result 1]
> + set proxy_server [lindex $result 2]
> + set proxy_port [lindex $result 3]
> +
> + set proxyauthenticate [get_kconf_key kioslaverc
> "Proxy Settings" AuthMode]
> +
> + } else {
> + #Configure as direct.
> + set proxy_server ""
> + set proxy_port ""
> + set proxyauthenticate 0
> + set proxy_user ""
> + set proxy_pass ""
> + }
No socks support ?
>
> +
> + } elseif { [info exists ::env(http_proxy)] } {
> + status_log "systemproxy: using
> \$http_proxy=$::env(http_proxy)\n" red
> + #regexp. http://%s:%...@%s:%d
> + regsub
> {^(?:(?:[^:/?#]+):)?(?://(?:(?:(?:([^:@]*):?([^:@]*))?@)?([^:/?#]*)(?::(\d*).*)?))?}
> $::env(http_proxy) {"\1" "\2" "\3" "\4"} result
Nice regexp! :)
>
> +
> + set proxy_user [lindex $result 0]
> + set proxy_pass [lindex $result 1]
> + set proxy_server [lindex $result 2]
> + set proxy_port [lindex $result 3]
> +
> + if { [llength $proxy_user] == 0 } {
> + set proxyauthenticate 0
> + } else {
> + set proxyauthenticate 1
> + }
> +
> + } else { #it's not gnome nor kde and ENV $http_proxy is not set. =>
> Don't use proxy
> + status_log "systemproxy: couldn't find any system proxy
> configuration\n" red
> +
> + #Configure as direct
> + set proxy_server ""
> + set proxy_port ""
> + set proxyauthenticate 0
> + set proxy_user ""
> + set proxy_pass ""
> + }
the else should use the settings previously set by the user...
>
> +
> + #This is needed for autoconnect
> + ::config::setKey proxy [list $proxy_server $proxy_port]
You shouldn't change the proxy settings without previously storing what the
user had in its saved settings... (which means load the saved user settings
in order not to overwrite them if this proc is called twice).
>
> +
> + #Print result
> + status_log "systemproxy: host is: $proxy_server\n" red
> + status_log "systemproxy: port is: $proxy_port\n" red
> + status_log "systemproxy: auth is: $proxyauthenticate\n" red
> + status_log "systemproxy: user is: $proxy_user\n" red
> + status_log "systemproxy: pass is: $proxy_pass\n" red
> +}
> +
This whole function had quite a lot of indentation issues! That's not nice,
please make sure your indentation is correct before sending the next patch!
>
> proc load_config {} {
> global HOME password osspecific_keys
>
> @@ -955,6 +1155,18 @@
> ::ChatWindow::ShowHideChatWindowMenus . 0
> }
>
> + #load system-wide proxy settings
> + if { [::config::getKey connectiontype] == "proxy" &&
> [::config::getKey usesystemproxy] == 1 } {
> +
> + if { [OnUnix] } {
> + load_unix_system_proxy
> + } else {
> + status_log "connectiontype reset to direct"
> + ::config::setKey connectiontype direct
> + }
> +
> + }
> +
shouldn't you just check for the usesystemproxy settings ? the user could
choose 'direct connection' but enable use system proxy settings and it
should work for him..
Also, the 'else' shouldn't modify anything so if someone is on windows and
enabled the option (using both windows and linux with a shared network drive
for the profile configuration), then you shouldn't reset his windows
settings... just do nothing is better.
>
> #load Snack
> if {![catch {require_snack} res]} {
> # TODO this is bad because it already gets called by
> require_snack once...
> Index: proxy.tcl
> ===================================================================
> --- proxy.tcl (revisión: 11112)
> +++ proxy.tcl (copia de trabajo)
> @@ -899,7 +899,6 @@
>
> variable proxy_session_id
> # variable proxy_gateway_ip
> - variable proxy_data
> variable options
>
> after cancel [list $self HTTPPoll $name]
>
>
> ------------------------------------------------------------------------------
>
> _______________________________________________
> Amsn-devel mailing list
> Amsn-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/amsn-devel
>
>
------------------------------------------------------------------------------
_______________________________________________
Amsn-devel mailing list
Amsn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/amsn-devel
