<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/111335/">http://git.reviewboard.kde.org/r/111335/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On July 5th, 2013, 2:23 p.m. UTC, <b>David Faure</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/111335/diff/2/?file=167766#file167766line766" style="color: black; font-weight: bold; text-decoration: underline;">kio/kio/scheduler.cpp</a>
<span style="font-weight: normal;">
(Diff revision 2)
</span>
</th>
</tr>
</thead>
<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">766</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">KMessageBox</span><span class="o">::</span><span class="n">information</span><span class="p">(</span><span class="n">window</span><span class="p">,</span> <span class="n">text</span><span class="p">,</span> <span class="n">caption</span><span class="p">,</span> <span class="n">dontAskAgainName</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;">Please don't put this code in scheduler.cpp
I'm trying to properly split core and gui aspects of KIO in frameworks, and scheduler is definitely core, while kmessagebox is definitely not.
Please find a way to separate the two.</pre>
</blockquote>
<p>On July 5th, 2013, 9:25 p.m. UTC, <b>Dawit Alemayehu</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;">So how were you planning to separate the core and gui aspects in frameworks? Without this patch KIO::Scheduler will still be linking against gui libraries because of its use of KIO::Slave. Perhaps if I know how you were planning to perform the split, I could follow the same approach to resolve this issue and it would be one less thing you have to deal with.
</pre>
</blockquote>
<p>On July 6th, 2013, 10:10 a.m. UTC, <b>David Faure</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;">I'm separating core/gui stuff for jobs using delegates and delegate extensions and factories.
(the trick is that the kiowidgets library can register stuff using code that runs automatically, just by linking to the library, see Q_CONSTRUCTOR_FUNCTION in kio/jobuidelegate.cpp)
But nothing is done yet for the messagebox stuff in Slave. So this is good timing, let's come up with a solution (which we could probably apply to both branches).
Context: SlaveInterface::messageBox() is called by kioslaves, in the application process.
Let's brainstorm. I can think of 3 solutions on top of my head:
1) Defining an interface in kiocore and implementing it in both libs. The core implementation would have to return "Cancel" every time, for lack of a possibility to interact with the user. Apps could still reimplement that interface to give predefined answers.
2) Propagating the call up to the job, which can then use the delegate mechanism for showing the messagebox (with again a canned reply for core-only code)
3) Delegating the messagebox to a separate process, e.g. kuiserver. This is the KDE3 solution, actually. The commit that removed that (8d6f7d340e0) says that modal messagebox were blocking kuiserver. But we could use non-modal messageboxes instead. Either with a blocking dbus call (using dbus transactions in kuiserver), or with real async everywhere. Problem: what if kuiserver isn't available. Or what if you wanted a core-only command-line tool which would not interact with the user.
Any other ideas? Any input on the above possibilities?
I kind of like number 2, to reduce the number of interfaces being used to call gui stuff from kiocore.</pre>
</blockquote>
<p>On July 6th, 2013, 8:57 p.m. UTC, <b>Dawit Alemayehu</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;">I do not have any additional ideas than the one you presented here. I like #2 as well. I am not fond of the separate process solution at all. One cannot really associate the dialog with the client (read: window). We have hacks that attempt to simulate that, but they are still hacks ; so I prefer solution #2 as well.</pre>
</blockquote>
<p>On July 6th, 2013, 10:40 p.m. UTC, <b>David Faure</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;">OK then, let's go for that. Do you plan on implementing it, or should I put it on my own TODO list?</pre>
</blockquote>
<p>On July 7th, 2013, 12:53 a.m. UTC, <b>Dawit Alemayehu</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;">I can do it since I want to resolve this bug and have already started the process so to speak. But I need your help to get started. What should I look at to understand how the separation of concerns (core/gui) work in frameworks? I see how jobuidelegate works in KIO under KDE 4 and seems to be a straight forward step for me to move the current code there, but I suspect that class has changed some in frameworks to do the split, correct?</pre>
</blockquote>
<p>On July 8th, 2013, 9:31 a.m. UTC, <b>David Faure</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;">In frameworks, KIO's JobUiDelegate still exists, but implements new interfaces (now it derives from both KDialogJobUiDelegate and JobUiDelegateExtension) so that its kio-specific API (askFileRename/askSkip/...) can be called from within core-only KIO jobs.
So if you need to add new API to KIO::JobUiDelegate in kde4 that's fine, I'll just add it to JobUiDelegateExtension in frameworks so that it can still be called.</pre>
</blockquote>
<p>On July 10th, 2013, 5 a.m. UTC, <b>Dawit Alemayehu</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;">Hmm... I can only move the GUI related portion of the code I wrote out of the scheduler. Everything else has to remain there in order for this to work since the message box is much like the password server. In fact, if this separation works, we should also switch the password server to this model in frameworks. Anyhow, this is my current plan:
1.) Move the messagebox portion of the current code, effectively the else statement in UserNotificationHandler::processRequest, to jobuidelegate.
2.) Move the code from step one to KIO::JobUiDelete::askUser(...)???
2.) Change the else statement in UserNotificationHandler::processRequest to call either r->slave->job()->requestMessageBox or r->slave->job()->ui()->askUser. Probably the former one which itself will end up calling the latter one.
Does this seem reasonable to you? Any objections/concerns?</pre>
</blockquote>
<p>On July 10th, 2013, 6:45 a.m. UTC, <b>David Faure</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;">Yes, the caching can stay in kiocore.
Note that this is rather different from the password server, which is handled directly in slavebase.cpp -- i.e. in the kioslave process, rather than in the app process.
And because the passwd server is across DBus, there's no gui code in slavebase, which is another difference. Let's not mix the two, I think.
Back to messageboxes: I would prefer the UserNotificationHandler code not to be related to the scheduler. After all, this has nothing to do with scheduling.
Could the code be in slaveinterface -- or in a separate header called by slaveinterface? Seems like you need it to be a singleton (which is why you put it in kio::scheduler, to reuse an existing singleton), but then just use Q_GLOBAL_STATIC for UserNotificationHandler.
I'm not sure what difference you make between step 1 and step 2 (the first step 2 :-) ). It's both about moving that gui code to KIO::JobUiDelegate.
About the last step: slave knows job, job knows uidelegate, don't call the uidelegate directly from the slave, it creates more dependencies. So I agree with "the former one".
Thanks!</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;">The first #2 was not supposed to be there. :) As far as the password dialog is concerned, I do understand how it works now. I was simply suggesting that should deal with it the same way we are dealing with this message box in the future. That way all the issues we currently have with tying the password dialog with the specific application window goes away. But you are right, this is not something we should concern ourselves with at the moment. Anyhow, I will move the UserNotificationHandler out of the Scheduler and use Q_GLOBAL_STATIC as you suggested.</pre>
<br />
<p>- Dawit</p>
<br />
<p>On July 3rd, 2013, 12:19 p.m. UTC, Dawit Alemayehu 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 kdelibs.</div>
<div>By Dawit Alemayehu.</div>
<p style="color: grey;"><i>Updated July 3, 2013, 12:19 p.m.</i></p>
<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;">The attached patch addresses one of the oldest bugs in KIO. Due to the muti-process nature of KIO, if any of the ioslaves encounter something that requires user input, the user might end up getting prompted multiple times. The best example of this is SSL error warnings sent to the client by kio_http. The patch completely resolves this problem using the same approach as kpasswdserver, but without the need for an additional kded process.</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;">Visit a site that throws up SSL warnings and causes KIO to show more than one error dialog.
</pre>
</td>
</tr>
</table>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="http://bugs.kde.org/show_bug.cgi?id=154100">154100</a>,
<a href="http://bugs.kde.org/show_bug.cgi?id=265228">265228</a>
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>kio/kio/scheduler.h <span style="color: grey">(04edb40)</span></li>
<li>kio/kio/scheduler.cpp <span style="color: grey">(802f8b8)</span></li>
<li>kio/kio/scheduler_p.h <span style="color: grey">(d68f645)</span></li>
<li>kio/kio/slaveinterface.h <span style="color: grey">(4bfcec8)</span></li>
<li>kio/kio/slaveinterface.cpp <span style="color: grey">(aa0fc44)</span></li>
<li>kio/kio/slaveinterface_p.h <span style="color: grey">(e31ec5e)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/111335/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>