<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/114105/">http://git.reviewboard.kde.org/r/114105/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On November 28th, 2013, 5:17 a.m. UTC, <b>Dawit Alemayehu</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="http://git.reviewboard.kde.org/r/114105/diff/1/?file=219633#file219633line444" style="color: black; font-weight: bold; text-decoration: underline;">konqueror/settings/kio/kproxydlg.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">void KProxyDialog::defaults()</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">444</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">mUi</span><span class="p">.</span><span class="n">showEnvValueCheckBox</span><span class="o">-></span><span class="n">setChecked</span><span class="p">(</span><span class="nb">false</span><span class="p">);</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</blockquote>
<p>On November 28th, 2013, 10:10 a.m. UTC, <b>Andrea Iacovitti</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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().</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
<br />
<p>- Dawit</p>
<br />
<p>On November 27th, 2013, 9:22 p.m. UTC, Andrea Iacovitti wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for KDE Base Apps and Dawit Alemayehu.</div>
<div>By Andrea Iacovitti.</div>
<p style="color: grey;"><i>Updated Nov. 27, 2013, 9:22 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kde-baseapps
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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().</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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"
</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>konqueror/settings/kio/kproxydlg.cpp <span style="color: grey">(e80afeb)</span></li>
<li>konqueror/settings/kio/ksaveioconfig.h <span style="color: grey">(2318198)</span></li>
<li>konqueror/settings/kio/ksaveioconfig.cpp <span style="color: grey">(c822f7b)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/114105/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>