Review Request 129231: [WIP] allow control over adding to "files/open recent" menu and use it in the patchreview plugin to prevent pollution

[Aleix Pol Gonzalez]

> On Oct. 20, 2016, 11:28 a.m., Aleix Pol Gonzalez wrote:
> > shell/documentcontroller.h, line 120
> > <https://git.reviewboard.kde.org/r/129231/diff/1/?file=482741#file482741line120>
> >
> >     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?
> 
> René J.V. Bertin wrote:
>     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.
>     
>     Evidently this is all a question of personal preference - as long as the derivatives of `IDocumentController` 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.
> 
> René J.V. Bertin wrote:
>     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.
>     
>     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.
> 
> Kevin Funk wrote:
>     What about just extending `IDocumentController::DocumentActivationParams`?
> 
> René J.V. Bertin wrote:
>     That would be a logical choice (which I completely overlooked) to alleviate the ABI change, except for `activateDocument`. Which surprisingly doesn't get this kind of argument.
>     
>     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.
>     
>     I could imagine a different implementation of that state variable though. Apparently there already is a notion of a default `IDocumentController::DocumentActivationParams` state; my proposed state getsetter could take an instance of that type rather than a `bool`. You'd get the best of both worlds.
>     But I have the impression that we couldn't make the distinction between a non-specified `activationParams` argument and a user-specified one that has all flags unset, correct? I'm not sure what assigning `nullptr` does to something that's not a pointer...
> 
> René J.V. Bertin wrote:
>     Edit: seems that overriding a "global" non-default state could be done by another additional flag.
> 
> Kevin Funk wrote:
>     I think you're overcomplicating things here. Please just extend `DocumentActivationParams` with a `DoNotAddToRecentFiles` flag; be done.
>     
>     Adding state to the document controller for this doesn't make a lot of sense.
>     
>     @Aleix: You agree?
> 
> Kevin Funk wrote:
>     ... and make just `openDocuments(...)` honor the flag. No need to touch `changeDocumentUrl(...)`. No need to touch `activateDocument` either -- if a user wants control over the activation params he/she should just use `openDocuments(...)`...

Yes, makes a lot of sense, I suggested adding a parameter without looking/remembering the API.


- Aleix


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129231/#review100162
-----------------------------------------------------------


On Oct. 20, 2016, 1:52 p.m., René J.V. Bertin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129231/
> -----------------------------------------------------------
> 
> (Updated Oct. 20, 2016, 1:52 p.m.)
> 
> 
> Review request for KDevelop.
> 
> 
> Bugs: 371210
>     http://bugs.kde.org/show_bug.cgi?id=371210
> 
> 
> Repository: kdevplatform
> 
> 
> Description
> -------
> 
> 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.
> 
> The former is a more or less minor aesthetic issue, the latter is something I consider a bug; cf. the linked bug report.
> 
> 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 `IDocumentController` purely abstract, so the actual implementation including the state member variable is provided by the `DocumentController` class.
> I think this approach should also maintain ABI compatibility.
> 
> 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.
> 
> The patch also improves the (somewhat related) maximum number of documents to open feature which I think never worked as intended.
> 
> 
> Diffs
> -----
> 
>   interfaces/idocumentcontroller.h b8a41f0 
>   plugins/patchreview/patchreview.cpp 1ddec7f 
>   plugins/patchreview/patchreviewtoolview.cpp de52800 
>   shell/documentcontroller.h 35c2057 
>   shell/documentcontroller.cpp c1e9c4c 
> 
> Diff: https://git.reviewboard.kde.org/r/129231/diff/
> 
> 
> Testing
> -------
> 
> 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.
> 
> Currently tested on OS X only but there is no reason this would work differently elsewhere.
> 
> 
> Thanks,
> 
> René J.V. Bertin
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20161020/19e5be13/attachment-0001.html>