Review Request 124426: Move populating of context menu to background

Fri Jul 24 13:48:13 UTC 2015

> On Lip 24, 2015, 12:15 po południu, Milian Wolff wrote:
> > I find the control flow really hard to follow. In the context menu extension, add a QMenu and then start some task via e.g. ThreadWeaver and push actions into the action when they are still valid or stop the action (if possible) if the menu gets destroyed. Could you try to clean it up?

It is probebly unclear that this feature is (going to be) composition of two mechanisms:
1) Background worker thread. ClangTool should run in another thread not to block GUI. It is forbidden to run more than one ClangTool concurrently (refactoring during refactoring) thus we need exactly one thread bounded to RefactoringContext (RefactoringContext is basically a source of informations for Clang tools)
2) Logic to populate context menu (in background)

First bits of this code was making no separation between these (even current name of thread is "RefactoringManager - Worker", but should be "RefactoringContext - Worker"). Worker was part of RefactoringManager used to move CPU intensive task to background. But basically the same problem is with concreate reafctoring actions, requirements are also the same. Thus I decided to move this worker to more general object - RefactoringContext - because more or less all actions are performed on behalf of this object. It is going to be used also to background refactoring.

Control flow is a bit complicated because in Qt we can modify GUI only from main (GUI) thread (i find this limitation a reasonable one). That's why there is a few `QMetaObject::invoke` calls.

> On Lip 24, 2015, 12:15 po południu, Milian Wolff wrote:
> > refactoring/contextmenumutator.h, line 58
> > <https://git.reviewboard.kde.org/r/124426/diff/2/?file=387052#file387052line58>
> >
> >     why not use a QMenu here, and store it in a QPointer, then check that for validity and then add to that menu?

`QWidget::addAction` family of functions _does not_ change parent of action. Any action provided in `ContextMenuExtension` have some other parent. If I add QAction/QMenu "here" (like placeholder) it is not going to be destroyed when menu is destroyed.
What is more KDevPlatform handles creation of submenu automatically if there is more than one action in group (and there is a "refactoring" group already). If I add QMenu here, we may (and in current implementation of the whole kdev-clang always will) get additional indirection in this menu (Refactoring->Refactoring->_my_refactoring_action_). Even more. I detected that sometimes placeholder action is added to more than one menu (it is exactly the case when both classic kdev-clang and refactorings part both give empty refactorings list, the second menu has a name and it is something with "k", "popup" and "menu" (i don't remember exactly)). Design of `ContextMenuExtension` and fact that `QWidget::addAction` doesn't reparent actions allows this behavior.

> On Lip 24, 2015, 12:15 po południu, Milian Wolff wrote:
> > refactoring/contextmenumutator.cpp, line 55
> > <https://git.reviewboard.kde.org/r/124426/diff/2/?file=387053#file387053line55>
> >
> >     style: const before typename

this is const pointer, not pointer to const

> On Lip 24, 2015, 12:15 po południu, Milian Wolff wrote:
> > refactoring/kdevrefactorings.h, line 69
> > <https://git.reviewboard.kde.org/r/124426/diff/2/?file=387057#file387057line69>
> >
> >     style: const before typename

this is const pointer, not pointer to const

> On Lip 24, 2015, 12:15 po południu, Milian Wolff wrote:
> > refactoring/qactionwatcher.h, line 35
> > <https://git.reviewboard.kde.org/r/124426/diff/2/?file=387059#file387059line35>
> >
> >     afaik Q_OBJECT does this internally already, no?

In Qt4 it were not. It was separated because generating things like `QActionWatcher(const QActionWatcher&) = delete;` requires name `QActionWatcher` which `Q_OBJECT` macro can hardly guess.

But now I noticed that I'm inheriting from `QObject` which should already by uncopyable...

> On Lip 24, 2015, 12:15 po południu, Milian Wolff wrote:
> > refactoring/refactoringcontext_worker.cpp, line 29
> > <https://git.reviewboard.kde.org/r/124426/diff/2/?file=387064#file387064line29>
> >
> >     I'd urge you to use threadweaver instead of using raw QThread

I started reading documentation of ThreadWeaver. At this point I can't settle if this framework can be (reasonably easy) used here.

- Maciej

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

On Lip 23, 2015, 3:53 po południu, Maciej Poleski wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/124426/
> -----------------------------------------------------------
> 
> (Updated Lip 23, 2015, 3:53 po południu)
> 
> 
> Review request for KDevelop.
> 
> 
> Repository: kdev-clang
> 
> 
> Description
> -------
> 
> Introduced worker thread for time-consuming Clang actions.
> 
> Moved populating of context menu (list of applicable refactorings) to background.
> 
> 
> Diffs
> -----
> 
>   refactoring/CMakeLists.txt PRE-CREATION 
>   refactoring/contextmenumutator.h PRE-CREATION 
>   refactoring/contextmenumutator.cpp PRE-CREATION 
>   refactoring/encapsulatefieldrefactoring.cpp PRE-CREATION 
>   refactoring/interface.h PRE-CREATION 
>   refactoring/interface.cpp PRE-CREATION 
>   refactoring/kdevrefactorings.h PRE-CREATION 
>   refactoring/kdevrefactorings.cpp PRE-CREATION 
>   refactoring/qactionwatcher.h PRE-CREATION 
>   refactoring/qactionwatcher.cpp PRE-CREATION 
>   refactoring/refactoringcontext.h PRE-CREATION 
>   refactoring/refactoringcontext.cpp PRE-CREATION 
>   refactoring/refactoringcontext_worker.h PRE-CREATION 
>   refactoring/refactoringcontext_worker.cpp PRE-CREATION 
>   refactoring/refactoringmanager.h PRE-CREATION 
>   refactoring/refactoringmanager.cpp PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/124426/diff/
> 
> 
> Testing
> -------
> 
> Manual testing
> 
> 
> Thanks,
> 
> Maciej Poleski
> 
>

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