Review Request 124426: Move populating of context menu to background

[Milian Wolff]

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


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?


refactoring/contextmenumutator.h (line 58)
<https://git.reviewboard.kde.org/r/124426/#comment57173>

    why not use a QMenu here, and store it in a QPointer, then check that for validity and then add to that menu?



refactoring/contextmenumutator.cpp (line 42)
<https://git.reviewboard.kde.org/r/124426/#comment57171>

    align with :



refactoring/contextmenumutator.cpp (line 55)
<https://git.reviewboard.kde.org/r/124426/#comment57172>

    style: const before typename



refactoring/encapsulatefieldrefactoring.cpp (line 465)
<https://git.reviewboard.kde.org/r/124426/#comment57174>

    unrelated (but good, just push it in a separate commit directly)



refactoring/kdevrefactorings.h (line 66)
<https://git.reviewboard.kde.org/r/124426/#comment57175>

    style: const before typename



refactoring/kdevrefactorings.cpp (line 37)
<https://git.reviewboard.kde.org/r/124426/#comment57176>

    align , with :



refactoring/qactionwatcher.h (line 32)
<https://git.reviewboard.kde.org/r/124426/#comment57177>

    do not name classes such that they start with a Q. It confuses and could potentially clash with Qt classes if they'd ever introduce the QActionWatcher



refactoring/qactionwatcher.h (line 35)
<https://git.reviewboard.kde.org/r/124426/#comment57178>

    afaik Q_OBJECT does this internally already, no?



refactoring/qactionwatcher.cpp (line 54)
<https://git.reviewboard.kde.org/r/124426/#comment57179>

    space after :



refactoring/refactoringcontext_worker.cpp (line 29)
<https://git.reviewboard.kde.org/r/124426/#comment57180>

    I'd urge you to use threadweaver instead of using raw QThread


- Milian Wolff


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/3901d87f/attachment.html>