Review Request: (code deduplication) Add class OutputExecuteJob as a common base for all output jobs which start processes

[Aleix Pol Gonzalez]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/106463/#review19055
-----------------------------------------------------------



outputview/outputexecutejob.h
<http://git.reviewboard.kde.org/r/106463/#comment15135>

    Does it really need to be both virtual and have a setter?



outputview/outputexecutejob.h
<http://git.reviewboard.kde.org/r/106463/#comment15136>

    Does it really need to be both virtual and have a setter?



outputview/outputexecutejob.h
<http://git.reviewboard.kde.org/r/106463/#comment15129>

    I don't really like the solution of passing it as a virtual method (and that's how MakeJob is using it) but additionally passing it with a setter.
    
    Also we probably just can expect the command to be specified with the "kdesudo" in the beginning, it's not something we need to control here.



outputview/outputexecutejob.h
<http://git.reviewboard.kde.org/r/106463/#comment15137>

    Does it really need to be both virtual and have a setter?



outputview/outputexecutejob.h
<http://git.reviewboard.kde.org/r/106463/#comment15138>

    It's public API, so a d-pointer here would go great.



outputview/outputexecutejob.cpp
<http://git.reviewboard.kde.org/r/106463/#comment15132>

    This class is probably going to be used for builders.
    
    Maybe we can change working directory to build directory here (and change the class name too, of course).



outputview/outputexecutejob.cpp
<http://git.reviewboard.kde.org/r/106463/#comment15124>

    If Kill is called, we probably want to kill it right away instead of waiting for 3s...



outputview/outputexecutejob.cpp
<http://git.reviewboard.kde.org/r/106463/#comment15125>

    Maybe we want a timeout here?



outputview/outputexecutejob.cpp
<http://git.reviewboard.kde.org/r/106463/#comment15126>

    Put it in the else?
    
    Otherwise we'll get both an Abort and a Warning



outputview/outputexecutejob.cpp
<http://git.reviewboard.kde.org/r/106463/#comment15127>

    had failed sounds very wrong.
    Maybe "Process has failed to start"
    
    Also it might be interesting to put more information, like having:
    "Make has failed to start".
    



outputview/outputexecutejob.cpp
<http://git.reviewboard.kde.org/r/106463/#comment15133>

    use KShell::joinArgs for this.
    
    http://api.kde.org/4.9-api/kdelibs-apidocs/kdecore/html/namespaceKShell.html#ad92ed56d00a678554dd04b0b1429760e



outputview/outputexecutejob.cpp
<http://git.reviewboard.kde.org/r/106463/#comment15134>

    This should be called when the arguments change as well, then.


Well, as I've slightly discussed in my comments, we probably would need to restrict the scope of this class to builders, but maybe not. I haven't seen this but the code that will be shared is not this much and I'm a bit afraid we could lose some semantics here and there, in terms of feedback to the user.

OTOH, I'm always in for some code removal :). Let's work it out a bit further.

- Aleix Pol Gonzalez


On Sept. 17, 2012, 9:21 a.m., Ivan Shapovalov wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106463/
> -----------------------------------------------------------
> 
> (Updated Sept. 17, 2012, 9:21 a.m.)
> 
> 
> Review request for KDevelop.
> 
> 
> Description
> -------
> 
> This class is going to be used instead of OutputJob as a common base for all such jobs which start processes (esp., builder jobs).
> 
> There are separate patches for make builder, cmake job and ninja builder.
> 
> 
> Diffs
> -----
> 
>   outputview/CMakeLists.txt 3dced4c 
>   outputview/outputexecutejob.h PRE-CREATION 
>   outputview/outputexecutejob.cpp PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/106463/diff/
> 
> 
> Testing
> -------
> 
> Existing unit-tests and manual testing (couple of days).
> 
> 
> Thanks,
> 
> Ivan Shapovalov
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20120917/75a99462/attachment.html>