Review Request 114105: kcm proxy: fix for noProxyFor setting when 'system proxy' type

[Dawit Alemayehu]

> On Nov. 28, 2013, 5:17 a.m., Dawit Alemayehu wrote:
> > konqueror/settings/kio/kproxydlg.cpp, line 444
> > <http://git.reviewboard.kde.org/r/114105/diff/1/?file=219633#file219633line444>
> >
> >     This does not make sense. I explicitly check show me the value and you uncheck it as a result of me clicking on the "Auto Detect" button? No.
> 
> Andrea Iacovitti wrote:
>     This is my (user) point of view:
>     if i'm configuring "system proxy" and i click "Auto Detect" button i'm interested to know/see what env vars i'm actually going to use,
>     not really their values. If i'm a curious user i could ask the gui to show me their *actual* values
>     (those values can also not be the same later on, for example because i re-assigned the env vars being configured).
>     
>     This change is also a fast and simple fix for the following use case:
>     suppose i have set the following env vars on my system
>     $export my_proxy=http://localhost:1111
>     $export http_proxy=http://localhost:2222
>     and i have configured "system proxy" to use my_proxy (in kioslaverc httpProxy=my_proxy).
>     I open proxy config module and check the "Show the value of the environment variables" checkbox,
>     "http://localhost:1111" is showed in "HTTP Proxy:",
>     then i push "Auto Detect",
>     "HTTP Proxy:" changes to "http://localhost:2222",
>     i save and exit: nothing is really saved because of the logic adopted in KProxyDialog::save().

Well when a user explicitly checks an option, which by default is not checked, it should be not reverted as a result of another action. I as a user do no want to see that. After all I purposefully checked that option to begin with, which can only mean one thing. I actually wanted to see the values and not the *actual* proxy variables. Otherwise, I would not have checked it to begin with. So I do not agree with this particular change and would like to see it removed from this patch.

Having said that the logic for the auto detection button needs to be fixed to ensure that it shows the proper text in those line edits. Currently it does not and that is what breaks KProxyDialog::save(), but that is easy enough to fix and something that needs to be absent this patch. I will deal with that after the rest of this patch is committed.


- Dawit


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/114105/#review44646
-----------------------------------------------------------


On Nov. 27, 2013, 9:22 p.m., Andrea Iacovitti wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/114105/
> -----------------------------------------------------------
> 
> (Updated Nov. 27, 2013, 9:22 p.m.)
> 
> 
> Review request for KDE Base Apps and Dawit Alemayehu.
> 
> 
> Repository: kde-baseapps
> 
> 
> Description
> -------
> 
> In case of 'system proxy' proxyType, NoProxyFor config key holds the name of the env variable (e.g. no_proxy) and not its value.
> Because of this KProtocolManager::noProxyFor() can not be used to get NoProxyFor config setting in KProxyDialog::load(), as it returns the resolved value of the environment variable and not its name: i added helper method KSaveIOConfig::noProxyFor() to read that value directly from config file.
> Also make sure to uncheck showEnvValueCheckBox before filling proxy edit fields with environment variable names in KProxyDialog::on_autoDetectButton_clicked().
> 
> 
> Diffs
> -----
> 
>   konqueror/settings/kio/kproxydlg.cpp e80afeb 
>   konqueror/settings/kio/ksaveioconfig.h 2318198 
>   konqueror/settings/kio/ksaveioconfig.cpp c822f7b 
> 
> Diff: http://git.reviewboard.kde.org/r/114105/diff/
> 
> 
> Testing
> -------
> 
> To reproduce the issue:
> $ export no_proxy=kde.org
> $ kcmshell4 proxy
> choose "Use system proxy configuration", push "Auto Detect" button, close the gui interface and reopen it:
> $ kcmshell4 proxy
> see how Exceptions fields contains "kde.org" and not "no_proxy"
> 
> 
> Thanks,
> 
> Andrea Iacovitti
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20131128/6512de80/attachment.htm>