Review Request 124426: Move populating of context menu to background

[Milian Wolff]

> On July 24, 2015, 10:15 a.m., 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?
> 
> Maciej Poleski wrote:
>     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.

You can also use signals and slots, i.e. emit a signal from one thread and have it connected to a slot in another thread. That will safe you the manual invoke calls, and hopefully make things simpler to follow around.


> On July 24, 2015, 10:15 a.m., Milian Wolff wrote:
> > refactoring/contextmenumutator.cpp, line 55
> > <https://git.reviewboard.kde.org/r/124426/diff/2/?file=387053#file387053line55>
> >
> >     style: const before typename
> 
> Maciej Poleski wrote:
>     this is const pointer, not pointer to const

ups, sorry! add space please between `* const`


> On July 24, 2015, 10:15 a.m., 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?
> 
> Maciej Poleski wrote:
>     `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.

fair enough.


> On July 24, 2015, 10:15 a.m., 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
> 
> Maciej Poleski wrote:
>     I started reading documentation of ThreadWeaver. At this point I can't settle if this framework can be (reasonably easy) used here.

you mentioned above you need at most one thread running these actions, so yes - maybe threadweaver is not the right option. but do try to use signals/slots instead which hopefully allows to cleanup the code a bit.


- Milian


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


On July 23, 2015, 1:53 p.m., Maciej Poleski wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/124426/
> -----------------------------------------------------------
> 
> (Updated July 23, 2015, 1:53 p.m.)
> 
> 
> 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/6a7044da/attachment.html>