Review Request 129231: [WIP] allow control over adding to "files/open recent" menu and use it in the patchreview plugin to prevent pollution
René J.V. Bertin
rjvbertin at gmail.com
Fri Oct 21 07:40:31 UTC 2016
> 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(...)`...
>
> Aleix Pol Gonzalez wrote:
> Yes, makes a lot of sense, I suggested adding a parameter without looking/remembering the API.
>
> René J.V. Bertin wrote:
> 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.
>
> René J.V. Bertin wrote:
> Also, what is the intended use case for `changeDocumentUrl()` which requires it to add the (new?) document URL to the recent files menu systematically?
>
> 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
>
> Kevin Funk wrote:
> Makes sense. Though this is both a different issue than what this patch is about and not easy to implement:
> `KRecentFilesAction` doesn't allow you to replace entries (or remove entries + insert entries at random positions)
>
> PS: Yes, there are ways to do it (backup entries of `KRecentFilesAction`, 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.
>
> René J.V. Bertin wrote:
> 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.
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 :)
`activateDocument` 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 `activateDocument`, 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?
So if it were me I'd propose to use the DoNotAddToRecentOpen flag in `activateDocument`; devs can use `openDocument` 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 `activateDocument` 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).
- René J.V.
-----------------------------------------------------------
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.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/20161021/2a0e9088/attachment-0001.html>
More information about the KDevelop-devel
mailing list