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

[Andreas Pakulat]

> 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;
> 
>  wrote:
>     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.

Hm, indeed. Wonder why I was under the impression that only operator[] will automatically insert the item.

I still think the find-code is clearer and you don't have to lookup the docs to see that it works as expected. Not to mention that it doesn't pollute the factories map and you don't have to check for null-pointer from anything coming from that map.


> 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?
> 
>  wrote:
>     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.

I'll see if I get around to that tonight or tomorrow. In fact I think I'll move the xtest stuff and eventually veritas to playground as its completely unmaintained at the moment.


> 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?
> 
>  wrote:
>     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

Hmm, thats true... But please change the url scheme to something which resembles "patch" and not vcs as this can also be used (potentially) to review patches from the disk. Something like kdevpatch:///.

Also I'm wondering how we would fit remote-diffs into this, they cannot always be represented by a url (the remote file I mean), so we might need to adjust the View-Id==url concept at some point anyway.


- Andreas


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


On 2009-08-12 23:34:36, Aleix Pol wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/1284/
> -----------------------------------------------------------
> 
> (Updated 2009-08-12 23:34:36)
> 
> 
> 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 1010540 
>   trunk/KDE/kdevplatform/interfaces/ipatchdocument.h PRE-CREATION 
>   trunk/KDE/kdevplatform/shell/CMakeLists.txt 1010540 
>   trunk/KDE/kdevplatform/shell/documentcontroller.h 1010540 
>   trunk/KDE/kdevplatform/shell/documentcontroller.cpp 1010540 
>   trunk/KDE/kdevplatform/shell/kross/wrappers/krosstopducontext.cpp 1010548 
>   trunk/KDE/kdevplatform/shell/partdocument.h 1010540 
>   trunk/KDE/kdevplatform/shell/partdocument.cpp 1010540 
>   trunk/KDE/kdevplatform/shell/patchdocument.h PRE-CREATION 
>   trunk/KDE/kdevplatform/shell/patchdocument.cpp PRE-CREATION 
>   trunk/KDE/kdevplatform/vcs/vcspluginhelper.cpp 1010540 
> 
> Diff: http://reviewboard.kde.org/r/1284/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Aleix
> 
>