Review Request 124426: Move populating of context menu to background

Sat Jul 25 16:47: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?
> 
> 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.
> 
> Milian Wolff wrote:
>     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.
> 
> Maciej Poleski wrote:
>     This would remove manual invoke calls in worker (and old implementation followed this style), but it had side efect - signals were emited in a bit awkward style (`schedue` method only emited signal `sheduled` pretending that task was scheduled, worker was connected to these signals - it was misleading - `sheduled` didn't inform that task was scheduled, real sched was a result of this signal).
>     Signals carry information (data). I can't define signals dynamicaly. To completly remove manual invoke calls worker would have to be extended with "finished" signals for any particular result type. What is more we would need seperate "schedule" method for any possible result type (to emit appropriate "finished" signal). That look like very poor programing style. (coupling all uses of worker with implemetation of worker)
>     
>     I hope to clean it up using some callback mechanism (wrap manual invoke calls in one place - implementation of callbacks mechanism, maybe some clever use of `QVariant` to move data between threads + pack/unpack operations instead of many sched/finished signals), but I don't have convincing ideas for that.
> 
> Milian Wolff wrote:
>     That doesn't sound good to me. Why do you want to programatically define signals? Just put the data into the arguments, no? And don't use QVariant for callbacks, `std::function` is the right data type to use there.
> 
> Maciej Poleski wrote:
>     Because tasks in general yield results (data). And this result must be handed back to GUI thread. Direct call of `std::function` would work on wrong thread.
> 
> Milian Wolff wrote:
>     Hm whenever I have such a problem I simply solved it by using signals/slots.
>     
>         emit dataReady(theData);
>     
>     and connect that to whatever "callback" slot you want. Why does this not work?

I don't say this does not work, but
```
worker would have to be extended with "finished" signals for any particular result type. What is more we would need seperate "schedule" method for any possible result type (to emit appropriate "finished" signal). That look like very poor programing style. (coupling all uses of worker with implemetation of worker)
```
note that
```
 It is going to be used also to background refactoring.
```
and the reason of this discussion is (i guess)
```
 Could you try to clean it up?
```
that's what i'm trying to achieve

- 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/20150725/725f8eb1/attachment-0001.html>