<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/129231/">https://git.reviewboard.kde.org/r/129231/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On October 20th, 2016, 11:28 a.m. CEST, <b>Aleix Pol Gonzalez</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="https://git.reviewboard.kde.org/r/129231/diff/1/?file=482741#file482741line120" style="color: black; font-weight: bold; text-decoration: underline;">shell/documentcontroller.h</a>
<span style="font-weight: normal;">
(Diff revision 1)
</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">120</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="kt">bool</span> <span class="nf">updateRecentFilesMenu</span><span class="p">(</span><span class="kt">bool</span> <span class="n">update</span><span class="p">)</span> <span class="n">override</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I'm not very fond of adding a new state for the DocumentController. Maybe it would make sense to add an argument to openDocument that specifies if it should be added to the recent files list?</p></pre>
</blockquote>
<p>On October 20th, 2016, 12:53 p.m. CEST, <b>René J.V. Bertin</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I thought about that, but decided against it.
Off the top of my head there are 3 places in the private DocumentController class which are accessed through the openDocument methods and at least 1 other which isn't (activateDocument). That came a bit as a surprise even after looking through the code, when I wrapped just the patchfile openDocument with disabled recent file updating.
IOW, several functions would need an additional argument with a default value, breaking ABI compatibility. The argument might seem out of place in some of those functions (though it could have the merit of drawing attention to the function's side effect one might not expect). Adding an extra argument doesn't always make the code using the API easier to read and maintain. Being able to turn off the feature (and back on at some later point) means you don't have to modify any of the code in between, and anyone working on that code doesn't have to worry about the local policy in this matter.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Evidently this is all a question of personal preference - as long as the derivatives of <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">IDocumentController</code> don't have to be reentrant. I have presumed this is not the case, but if that's incorrect it'll be easier to add an argument to the methods that are concerned.</p></pre>
</blockquote>
<p>On October 20th, 2016, 1:50 p.m. CEST, <b>René J.V. Bertin</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I see I remembered wrong: there are only 2 private methods that do the actual menu updating, one of which is a slot. I haven't yet counted the exact number of openDocument methods that would need to take an additional argument but it's more than 3. At least one is a slot, too.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">So there's also the question whether we'd want to be able to control updating of the recent files menu for files opened through Qt's signal/slot mechanism, which doesn't seem possible except through a state variable.</p></pre>
</blockquote>
<p>On October 20th, 2016, 3:02 p.m. CEST, <b>Kevin Funk</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">What about just extending <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">IDocumentController::DocumentActivationParams</code>?</p></pre>
</blockquote>
<p>On October 20th, 2016, 3:34 p.m. CEST, <b>René J.V. Bertin</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">That would be a logical choice (which I completely overlooked) to alleviate the ABI change, except for <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">activateDocument</code>. Which surprisingly doesn't get this kind of argument.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I would still prefer the use of state because of the other arguments brought forward, but as I said that's my preference against the collective preference.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I could imagine a different implementation of that state variable though. Apparently there already is a notion of a default <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">IDocumentController::DocumentActivationParams</code> state; my proposed state getsetter could take an instance of that type rather than a <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">bool</code>. You'd get the best of both worlds.
But I have the impression that we couldn't make the distinction between a non-specified <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">activationParams</code> argument and a user-specified one that has all flags unset, correct? I'm not sure what assigning <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">nullptr</code> does to something that's not a pointer...</p></pre>
</blockquote>
<p>On October 20th, 2016, 3:40 p.m. CEST, <b>René J.V. Bertin</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Edit: seems that overriding a "global" non-default state could be done by another additional flag.</p></pre>
</blockquote>
<p>On October 20th, 2016, 6:06 p.m. CEST, <b>Kevin Funk</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I think you're overcomplicating things here. Please just extend <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">DocumentActivationParams</code> with a <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">DoNotAddToRecentFiles</code> flag; be done.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Adding state to the document controller for this doesn't make a lot of sense.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">@Aleix: You agree?</p></pre>
</blockquote>
<p>On October 20th, 2016, 6:08 p.m. CEST, <b>Kevin Funk</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">... and make just <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">openDocuments(...)</code> honor the flag. No need to touch <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">changeDocumentUrl(...)</code>. No need to touch <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">activateDocument</code> either -- if a user wants control over the activation params he/she should just use <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">openDocuments(...)</code>...</p></pre>
</blockquote>
<p>On October 20th, 2016, 6:46 p.m. CEST, <b>Aleix Pol Gonzalez</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Yes, makes a lot of sense, I suggested adding a parameter without looking/remembering the API.</p></pre>
</blockquote>
<p>On October 20th, 2016, 10:15 p.m. CEST, <b>René J.V. Bertin</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Touching activateDocument will only work if you either use state, or if you modify the implementation so that activating a document doesn't add it to the open recent menu.</p></pre>
</blockquote>
<p>On October 20th, 2016, 10:56 p.m. CEST, <b>René J.V. Bertin</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Also, what is the intended use case for <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">changeDocumentUrl()</code> which requires it to add the (new?) document URL to the recent files menu systematically?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">If this method is used for renaming files, wouldn't it have to do something like the following?
- check if the old name is in the menu
- if so, remove the old name, add the new name
- it not, don't do anything</p></pre>
</blockquote>
<p>On October 21st, 2016, 12:10 a.m. CEST, <b>Kevin Funk</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Makes sense. Though this is both a different issue than what this patch is about and not easy to implement:
<code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">KRecentFilesAction</code> doesn't allow you to replace entries (or remove entries + insert entries at random positions)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">PS: Yes, there are ways to do it (backup entries of <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">KRecentFilesAction</code>, clear, then adding them one after one in your desired order), but I'm not sure it's necessary to wrap your head around this issue. It won't happen often.</p></pre>
</blockquote>
<p>On October 21st, 2016, 1:48 a.m. CEST, <b>René J.V. Bertin</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">If we (or the KRecentFilesAction authors) accept the fact that a stale entry remains if you rename a file, couldn't we accept the idea that the new name isn't added? It depends on how you look at it, but one can argue that changing an open document's name doesn't mean you've opened it.</p></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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Either way, I suppose I can always get back to it and address this in a follow-up patch should it turn out to be an issue I can't live with :)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;"><code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">activateDocument</code> is a bit different; it is used by the patch review plugin and currently I have to implement it directly to avoid adding the patch file to the recent files menu.
I find that counter-intuitive (someone might not read the comment and wonder why there are 2 openDocument() calls for the same file). It's also semantically incorrect, in principle.
Consider a session that's been opening for weeks with a particular set of documents. Just opening the session doesn't add the files to the recent files menu, for good reason. Now the user does something that causes one of those long-term residents to be activated through <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">activateDocument</code>, knowing full well that the document is part of the regular working set. Why would s/he expect to find that document's name in the recent files menu?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">So if it were me I'd propose to use the DoNotAddToRecentOpen flag in <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">activateDocument</code>; devs can use <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">openDocument</code> directly if they want a different behaviour; using an open action when you want the action to do everything an open action does seems less counter-intuitive to me.
Since <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">activateDocument</code> is relevant for the patch at hand this change could be part of the current patch - in fact I almost included it in the current version (which I think takes care of all other change requests).</p></pre>
<br />
<p>- René J.V.</p>
<br />
<p>On October 20th, 2016, 1:52 p.m. CEST, René J.V. Bertin wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: 1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
<div>Review request for KDevelop.</div>
<div>By René J.V. Bertin.</div>
<p style="color: grey;"><i>Updated Oct. 20, 2016, 1:52 p.m.</i></p>
<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=371210">371210</a>
</div>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kdevplatform
</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I've recently started noticing that my recent files menu got populated with files opened automatically by the patchreview plugin. That can lead to surprises if one of the patches you review opens files changed sufficiently long ago. What's more, the review files themselves, the temporary patch files, also end up in the menu.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The former is a more or less minor aesthetic issue, the latter is something I consider a bug; cf. the linked bug report.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The patch proposed here provides a mechanism to control whether or not files are added to the Files/Open Recent menu. Rather than adding a flag to all *DocumentController methods that might lead to adding a file to that menu (= not only the openDocument methods) I've opted for an approach with a state variable and a setter function that returns the previous state. I've kept <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">IDocumentController</code> purely abstract, so the actual implementation including the state member variable is provided by the <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">DocumentController</code> class.
I think this approach should also maintain ABI compatibility.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The mechanism is put to use in the patch review plugin to disable updating of the recent files menu in the 2 places where files are opened automatically.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The patch also improves the (somewhat related) maximum number of documents to open feature which I think never worked as intended.</p></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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">With this patch in place the patch review no longer adds all documents it opens to the Files/Open Recent menu, but this concerns only the patchfile itself and the files opened automatically and initially. Any action to open a file by the user or even activate one of the already open files still leads to adding that file to the menu, as one would expect.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Currently tested on OS X only but there is no reason this would work differently elsewhere.</p></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>interfaces/idocumentcontroller.h <span style="color: grey">(b8a41f0)</span></li>
<li>plugins/patchreview/patchreview.cpp <span style="color: grey">(1ddec7f)</span></li>
<li>plugins/patchreview/patchreviewtoolview.cpp <span style="color: grey">(de52800)</span></li>
<li>shell/documentcontroller.cpp <span style="color: grey">(c1e9c4c)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/129231/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>