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

Wed Aug 12 23:06:13 UTC 2009

> On 2009-08-11 23:11:21, Andreas Pakulat wrote:
> > /trunk/KDE/kdevplatform/shell/documentcontroller.cpp, line 782
> > <http://reviewboard.kde.org/r/1284/diff/1/?file=9939#file9939line782>
> >
> >     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;

>From the Qt documentation:

If the map contains no item with key key, the function returns a default-constructed value. If there are multiple items for key in the map, the value of the most recently inserted one is returned.

Here the default construction is 0. So I think it's correct... if it's not then the doc is buggy.

> On 2009-08-11 23:11:21, Andreas Pakulat wrote:
> > /trunk/KDE/kdevplatform/shell/partdocument.h, line 76
> > <http://reviewboard.kde.org/r/1284/diff/1/?file=9942#file9942line76>
> >
> >     Hmm, why not really override createViewWidget?

Because i'd have to create all that logic all over again. I prefer to reuse it.

> On 2009-08-11 23:11:21, Andreas Pakulat wrote:
> > /trunk/KDE/kdevplatform/shell/patchdocument.cpp, line 62
> > <http://reviewboard.kde.org/r/1284/diff/1/?file=9945#file9945line62>
> >
> >     This can be reduced by using IPartController::findPartFactory(), that one already takes a servicetypes-argument.

Agree, just used that because i copied the code from Kompare.

> On 2009-08-11 23:11:21, Andreas Pakulat wrote:
> > /trunk/KDE/kdevplatform/vcs/vcspluginhelper.cpp, line 235
> > <http://reviewboard.kde.org/r/1284/diff/1/?file=9946#file9946line235>
> >
> >     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?

You can do that yourself. I can do it too. I don't care either way.
Having just a test area is meaningless anyway.
The patchreview plugin should be moved there as well.

> On 2009-08-11 23:11:21, Andreas Pakulat wrote:
> > /trunk/KDE/kdevplatform/vcs/vcspluginhelper.cpp, line 252
> > <http://reviewboard.kde.org/r/1284/diff/1/?file=9946#file9946line252>
> >
> >     to is only set in the else-part, the if-part doesn't set it, leaving the url unset. Is that intended?

No, here we should use the KTemp's

> On 2009-08-11 23:11:21, Andreas Pakulat wrote:
> > /trunk/KDE/kdevplatform/vcs/vcspluginhelper.cpp, line 256
> > <http://reviewboard.kde.org/r/1284/diff/1/?file=9946#file9946line256>
> >
> >     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?

I don't like it either. The thing is: imagine you want to have the same file and patch open at the same time. You can't, because the view ID is the url. I actually think this should be extended in case you want to have also different versions of a file

- Aleix

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

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
> 
>