<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 />
<p>
Ship it!
</p>
<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;">LGTM. </p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">PS: Not sure whether we want to waste our time with backporting this to 5.0. It's only a minor issue. /me is not volunteering.</p></pre>
<br />
<p>- Kevin Funk</p>
<br />
<p>On October 21st, 2016, 1:28 p.m. UTC, 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. 21, 2016, 1:28 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.h <span style="color: grey">(35c2057)</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>