Review Request 129398: Restore KTextEditor Document Dbus bindings

[Dominik Haumann]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129398/#review100918
-----------------------------------------------------------



In general ok, but could you please post an updated version of the patch?


src/document/katedocument.cpp (line 133)
<https://git.reviewboard.kde.org/r/129398/#comment67692>

    Please use a meaningful name, e.g. dbusDocumentNumber or similar.



src/document/katedocument.cpp (line 232)
<https://git.reviewboard.kde.org/r/129398/#comment67693>

    const QString pathName = QString::fromLatin1("...").arg(++dummy);
    
    Then pathName can be const.



src/utils/document.cpp (line 29)
<https://git.reviewboard.kde.org/r/129398/#comment67694>

    Could you please reformat the entire file?
    
    - 4 spaces indentation (and not 2)
    - put openng '{' on next line for functions
    - add a space after a ','



src/utils/document.cpp (line 30)
<https://git.reviewboard.kde.org/r/129398/#comment67696>

    Please use this notation:
    
        DocumentAdaptor::DocumentAdaptor(Document *document)
            : QDBusAbstractAdaptor(document)
            , m_document(document)
        {
        }



src/utils/document.cpp (line 97)
<https://git.reviewboard.kde.org/r/129398/#comment67695>

    Is this correct? y and x are switched, do you know any reason for this? Did you test?



src/utils/documentadaptor_p.h (line 26)
<https://git.reviewboard.kde.org/r/129398/#comment67697>

    Given this is private API, I would prefer to not have this in namespace KTextEditor, and instead name this class either "KateDocumentAdaptor", or put into namespace "Kate" with current name.
    
    And please reindent / format code similar to other comment :-)


- Dominik Haumann


On Nov. 14, 2016, 11:49 a.m., Luke Dashjr wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129398/
> -----------------------------------------------------------
> 
> (Updated Nov. 14, 2016, 11:49 a.m.)
> 
> 
> Review request for Kate, KDE Frameworks and Christoph Cullmann.
> 
> 
> Bugs: 369623
>     https://bugs.kde.org/show_bug.cgi?id=369623
> 
> 
> Repository: ktexteditor
> 
> 
> Description
> -------
> 
> Enable access to the document itself via dbus; this is mostly a revert of c8ce78a02794193f20caacdfb5ef68ac0d26064d and 6ff7cedb57abe7deb37e33bfa82d2f5530eb295c in the kate git repo (which removed the dbus bindings with no explanation other than "cleanups")
> 
> Intends to fix https://bugs.kde.org/show_bug.cgi?id=369623
> 
> 
> Diffs
> -----
> 
>   src/CMakeLists.txt 54cc157 
>   src/document/katedocument.cpp eb30049 
>   src/utils/document.cpp 44dc7cf 
>   src/utils/documentadaptor_p.h PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/129398/diff/
> 
> 
> Testing
> -------
> 
> qdbus org.kde.kate-6503 /Kate/Document/2 text
> qdbus org.kde.kate-6503 /Kate/Document/2 file
> 
> 
> Thanks,
> 
> Luke Dashjr
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20161117/95fcd195/attachment-0001.html>