Review Request: Make the VCS diff widget a self-contained dialog like the commit one.

[Andreas Pakulat]

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


I'm not sure what Aleix is hinting towards, but the general idea of this patch sounds wrong. The widget is a widget on purpose (IIRC) since the intention was to have a re-usable widget that can work with any vcs-plugin and can be shown in different places. So turning it into a dialog just does not make sense from the design point-of-view. Back when I worked on this I actually planned to some day have kdiff3 or so being used as widget and allowing to use this as editor to resolve conflicts partially graphically - i.e. show a side-by-side diff and allow to take certain lines into the merged code but also allow editing the code manually if just taking either side doesn't work. But thats a rather huge task and these days with the review area I think the balance between amount of work and additional benefit is too little. 

If it turns out that a diff-widget is not needed anymore because there's always the review-area - which I think is the case, its hardcoded into the shell isn't it - then it should just be dropped IMHO.

In general of course there should be as much code-sharing as possible, preferably by having shared UI (where possible, some vcs are too different from one another) with the plugin API providing access to the data that the UI displays/changes. The git/dvcs-stuff is not that nice from a design point of view, since it was added on top ignoring some parts of the vcs-api and working around it instead of improving it, but since it mostly works its also hard to justify changing that since that means regressions (due to little test coverage) and boring work.

- Andreas Pakulat


On Oct. 29, 2012, 2:26 p.m., Ivan Shapovalov wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/107112/
> -----------------------------------------------------------
> 
> (Updated Oct. 29, 2012, 2:26 p.m.)
> 
> 
> Review request for KDevelop.
> 
> 
> Description
> -------
> 
> As stated in subject (VcsDiffWidget -> VcsDiffDialog and inherit from KDialog).
> Also stop the dialog from appearing in the vcs history dialog when there is a patchreview plugin available.
> 
> 
> Diffs
> -----
> 
>   plugins/subversion/kdevsvnplugin.cpp 560982e 
>   vcs/CMakeLists.txt 60fc7e9 
>   vcs/widgets/vcsdiffdialog.h PRE-CREATION 
>   vcs/widgets/vcsdiffdialog.cpp PRE-CREATION 
>   vcs/widgets/vcsdiffdialog.ui PRE-CREATION 
>   vcs/widgets/vcsdiffwidget.h aabc783 
>   vcs/widgets/vcsdiffwidget.cpp 54787b9 
>   vcs/widgets/vcsdiffwidget.ui ccde3a0 
>   vcs/widgets/vcseventwidget.cpp 5abad09 
> 
> Diff: http://git.reviewboard.kde.org/r/107112/diff/
> 
> 
> Testing
> -------
> 
> Manual.
> 
> 
> Thanks,
> 
> Ivan Shapovalov
> 
>

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