Review Request: Kompare integration in KDevelop to review patches and vcs differences

[Andreas Pakulat]

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


Regarding the compilation with komparepart, can you elaborate? I can't see any real problem with that.


/trunk/KDE/kdevplatform/shell/documentcontroller.cpp
<http://reviewboard.kde.org/r/1284/#comment1329>

    This will assert if a mime type is passed in thats not contained in the factories.
    
    Better do:
    QMap<QString,IDocumentFactory*>::const_iterator it = d->factories.find( mime );
    if( it != d->factories.end() )
      return *it;
    else
      return 0;



/trunk/KDE/kdevplatform/shell/partcontroller.h
<http://reviewboard.kde.org/r/1284/#comment1331>

    done already in trunk :)



/trunk/KDE/kdevplatform/shell/partdocument.h
<http://reviewboard.kde.org/r/1284/#comment1332>

    Hmm, why not really override createViewWidget?



/trunk/KDE/kdevplatform/shell/patchdocument.cpp
<http://reviewboard.kde.org/r/1284/#comment1333>

    This can be reduced by using IPartController::findPartFactory(), that one already takes a servicetypes-argument.



/trunk/KDE/kdevplatform/shell/patchdocument.cpp
<http://reviewboard.kde.org/r/1284/#comment1335>

    Hmm, on a second thought, could we instead reduce this whole stuff to just fetching a KService::Ptr and then let that one create the KPart? After all the API being used here and in findPartFactory is deprecated, so I'd like to avoid their use. Look at the konsole plugin to see how you get a proper factory with "new style api".



/trunk/KDE/kdevplatform/shell/patchdocument.cpp
<http://reviewboard.kde.org/r/1284/#comment1336>

    aah, so thats why you need addPartForView, so you can update the internal map... Another option would be making the d-pointer protected to be able to access it directly from subclasses. Thats whats usually done in Qt, though in this current case there's no real benefit in either way.



/trunk/KDE/kdevplatform/vcs/vcspluginhelper.cpp
<http://reviewboard.kde.org/r/1284/#comment1337>

    Hmm, we should first (drop the "test" area and ?) create a "review" area. Do you want to do that as part of this patch or should I do that tomorrow evening directly in trunk?



/trunk/KDE/kdevplatform/vcs/vcspluginhelper.cpp
<http://reviewboard.kde.org/r/1284/#comment1339>

    to is only set in the else-part, the if-part doesn't set it, leaving the url unset. Is that intended?



/trunk/KDE/kdevplatform/vcs/vcspluginhelper.cpp
<http://reviewboard.kde.org/r/1284/#comment1338>

    Hmm, I don't particularly like this fake url thing. Why do we need that, isn't the create function under "our control", i.e. can't we just treat any url passed into the create function as one thats used for a patch-view?


- Andreas


On 2009-08-11 16:19:12, Aleix Pol wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/1284/
> -----------------------------------------------------------
> 
> (Updated 2009-08-11 16:19:12)
> 
> 
> Review request for KDevelop.
> 
> 
> Summary
> -------
> 
> This adds a new PatchDocument and a IPatchDocument interface that lets us work with patches integrating them using the Kompare part.
> 
> It adapts that to the Vcs support so that we can review local changes. (not complete support of all the VcsDiff cases)
> 
> There is more work to do still, but I wanted you to see what's been worked one and to criticize it if needed. Now when you compare to base you're moved to the Test area (which should be renamed to Review area, imho) and opens there all the different files. One document per changed file.
> 
> The compilation when the KomparePart is not installed is not really sorted out yet, I'll add that on a next version of the patch.
> 
> Some work will be needed on the KomparePart as well. Now when we compare a file with a string, Kompare assumes the new one is the string and in our case it's not (it's the string that comes from the VCS support).
> 
> We should make it possible to specify the base directory when working with 1 patch.
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdevplatform/interfaces/idocumentcontroller.h 1009258 
>   /trunk/KDE/kdevplatform/shell/CMakeLists.txt 1009258 
>   /trunk/KDE/kdevplatform/shell/documentcontroller.h 1009258 
>   /trunk/KDE/kdevplatform/shell/documentcontroller.cpp 1009258 
>   /trunk/KDE/kdevplatform/shell/partcontroller.h 1009258 
>   /trunk/KDE/kdevplatform/shell/partcontroller.cpp 1009258 
>   /trunk/KDE/kdevplatform/shell/partdocument.h 1009258 
>   /trunk/KDE/kdevplatform/shell/partdocument.cpp 1009258 
>   /trunk/KDE/kdevplatform/shell/patchdocument.h PRE-CREATION 
>   /trunk/KDE/kdevplatform/shell/patchdocument.cpp PRE-CREATION 
>   /trunk/KDE/kdevplatform/vcs/vcspluginhelper.cpp 1009258 
> 
> Diff: http://reviewboard.kde.org/r/1284/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Aleix
> 
>