Review Request 113584: Ask git if it is safe to reload a document

[Milian Wolff]

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



plugins/git/gitplugin.h
<http://git.reviewboard.kde.org/r/113584/#comment31076>

    please follow our coding style:
    
    { on new line and rest of the body de-indented (except toe Q_OBJECT, that is correct)



plugins/git/gitplugin.h
<http://git.reviewboard.kde.org/r/113584/#comment31080>

    m_ prefix missing



plugins/git/gitplugin.cpp
<http://git.reviewboard.kde.org/r/113584/#comment31077>

    #include <QTextCodec>



plugins/git/gitplugin.cpp
<http://git.reviewboard.kde.org/r/113584/#comment31078>

    Hmm while nice, I somehow doubt this is required. We only support UTF8 in the language plugins after all... Anyhow, if you want to keep it in, then fine with me. But please "constify" variables that don't need to be changed later on.
    
    also, this still uses document->text instead of the QStringList



plugins/git/gitplugin.cpp
<http://git.reviewboard.kde.org/r/113584/#comment31079>

    these processes will then "leak" until kdevelop is closed, that's not going to work. delete them when they finished (but ensure they are also deleted when they failed)



plugins/git/gitplugin.cpp
<http://git.reviewboard.kde.org/r/113584/#comment31081>

    error handling? at least some debug output that something failed is necessary imo 



plugins/git/gitplugin.cpp
<http://git.reviewboard.kde.org/r/113584/#comment31082>

    this looks really bad, you want setStandardOutputProcess no?



plugins/git/gitplugin.cpp
<http://git.reviewboard.kde.org/r/113584/#comment31083>

    read line-bye-line and check for the contains



shell/textdocument.h
<http://git.reviewboard.kde.org/r/113584/#comment31087>

    no need for an extra function, just call it close with discard directly.



shell/textdocument.cpp
<http://git.reviewboard.kde.org/r/113584/#comment31086>

    only if dirty == true, no?



shell/textdocument.cpp
<http://git.reviewboard.kde.org/r/113584/#comment31084>

    here and below: remove whitespace after !



shell/textdocument.cpp
<http://git.reviewboard.kde.org/r/113584/#comment31085>

    should be checked before creating the job, and asserted here.



shell/textdocument.cpp
<http://git.reviewboard.kde.org/r/113584/#comment31088>

    no newline after }



vcs/interfaces/icontentawareversioncontrol.h
<http://git.reviewboard.kde.org/r/113584/#comment31089>

    no newline before { for namespaces



vcs/interfaces/icontentawareversioncontrol.h
<http://git.reviewboard.kde.org/r/113584/#comment31090>

    newline befor { of classes, structs



vcs/interfaces/icontentawareversioncontrol.h
<http://git.reviewboard.kde.org/r/113584/#comment31091>

    reuse the killable capability of KJob here instead. killing qprocesses should be fine.



vcs/interfaces/icontentawareversioncontrol.h
<http://git.reviewboard.kde.org/r/113584/#comment31092>

    pimpl this, it's an exported class


- Milian Wolff


On Nov. 6, 2013, 1:05 a.m., Sven Brauch wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/113584/
> -----------------------------------------------------------
> 
> (Updated Nov. 6, 2013, 1:05 a.m.)
> 
> 
> Review request for KDevelop, Aleix Pol Gonzalez and Milian Wolff.
> 
> 
> Repository: kdevplatform
> 
> 
> Description
> -------
> 
> When a file is changed on disk, we can look in the git object repository
> and see whether the contents currently in the editor are stored somewhere
> there. In that case, there is no risk of data loss when we just silently
> reload the document; the user can always retrieve the old version from
> git.
> 
> For this purpose I added a new VCS interface which version control plugins
> can choose to implement if they are able and willing to provide this
> functionality. If a plugin does not implement the interface, the old
> behaviour is unaffected.
> 
> I implemented the interface for the git plugin and it seems to be working fine.
> There's two issues we might want to talk about:
>  - We can not retrieve the line ending mode from kate. Thus, currently it will
>    only work for files with \n line endings (old behaviour is used instead for
>    different line endings).
>  - It's not blazingly fast. It's not exactly slow, but if you have like 35
>    documents open, the freeze on switching to a completely different branch
>    is noticeable. Test it yourself and give your opinion on whether this
>    is acceptable or not.
> 
> 
> Diffs
> -----
> 
>   shell/textdocument.cpp 187a071d78c3e2d00092bb2adde533486be11eee 
>   shell/textdocument.h 8bb29fea8395d4cac956778fae85a4a9f57c2cce 
>   plugins/git/gitplugin.h 2f60c24b9d223a815eda6627d1328ce2404e11af 
>   plugins/git/gitplugin.cpp 27f4eaeea46afdfccb88d64caf161cc84a1e5bad 
>   vcs/CMakeLists.txt c6854ed895e4284e4b3d355b0b549ef74a8f84de 
>   vcs/interfaces/icontentawareversioncontrol.h PRE-CREATION 
>   vcs/interfaces/icontentawareversioncontrol.cpp PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/113584/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Sven Brauch
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20131106/5a67cf95/attachment-0001.html>