Review Request 113815: Patch for Bug 221792 - integrate folder browser in "Setup Custom Include Paths"

[Kevin Funk]

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



languages/cpp/codegen/customincpath.h
<https://git.reviewboard.kde.org/r/113815/#comment34358>

    Please group the includes:
    
    See http://qt-project.org/wiki/Coding-Conventions#32bc73e08b315a2d85bfd10b2e8c867a (hence, "ui_custom_include_paths.h" will come first)



languages/cpp/codegen/customincpath.h
<https://git.reviewboard.kde.org/r/113815/#comment34353>

    Please rename the class to CustomIncludePaths, and the file to customincludepaths.h. That way it is consistent to the UI file's class name.
    
    Don't forget to fix include guards as well.
    
    More: I'd prefer to have setters and getters for the attributes of Ui::CustomIncludePaths instead of making that public.
    
    E.g. add 'void setStorageDirectory(const KUrl&)', 'KUrl storageDirectory() const' etc. pp. You should end up with 4 setters and 4 getters.
    
    Additional note: Class members are written in the following order: First: public members, then protected members, then private members.



languages/cpp/codegen/customincpath.h
<https://git.reviewboard.kde.org/r/113815/#comment34355>

    Doesn't need to be virtual + rename to 'openAddIncludeDirectoryDialog'. Always prefix mutators with a verb so it's clear what they're doing.



languages/cpp/codegen/customincpath.h
<https://git.reviewboard.kde.org/r/113815/#comment34354>

    The convention is to name this variable 'ui', please change.
    
    + Make private



languages/cpp/codegen/customincpath.cpp
<https://git.reviewboard.kde.org/r/113815/#comment34351>

    "/usr" is not cross-platform, rather leave that empty for now => KUrl().



languages/cpp/codegen/unresolvedincludeassistant.cpp
<https://git.reviewboard.kde.org/r/113815/#comment34352>

    Move that up to line 19, so includes from the same directory are grouped together



languages/cpp/codegen/unresolvedincludeassistant.cpp
<https://git.reviewboard.kde.org/r/113815/#comment34357>

    'inc_obj': Unclear varable naming + bad coding style. Call that 'customIncludePathDialog'.



languages/cpp/codegen/unresolvedincludeassistant.cpp
<https://git.reviewboard.kde.org/r/113815/#comment34356>

    Move the next 3 lines in CustomIncludePath().


- Kevin Funk


On Jan. 17, 2014, 7:43 a.m., Meenakshi Khorana wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/113815/
> -----------------------------------------------------------
> 
> (Updated Jan. 17, 2014, 7:43 a.m.)
> 
> 
> Review request for KDevelop.
> 
> 
> Repository: kdevelop
> 
> 
> Description
> -------
> 
> Patch for Bug 221792 - integrate folder browser in "Setup Custom Include Paths" 
> 
> Integrated a folder browser to add custom include paths. User can add custom include paths on a button click and can view the added paths in plain text edit box.
> 
> 
> Diffs
> -----
> 
>   languages/cpp/CMakeLists.txt 66030be 
>   languages/cpp/codegen/customincpath.h PRE-CREATION 
>   languages/cpp/codegen/customincpath.cpp PRE-CREATION 
>   languages/cpp/codegen/ui/custom_include_paths.ui 1a7c1ed 
>   languages/cpp/codegen/unresolvedincludeassistant.cpp 93a72d3 
>   languages/cpp/tests/CMakeLists.txt 9ce3c7b 
> 
> Diff: https://git.reviewboard.kde.org/r/113815/diff/
> 
> 
> Testing
> -------
> 
> Compiled and Tested successfully on local machine.
> 
> 
> File Attachments
> ----------------
> 
> Added open file dialog to custom include path
>   https://git.reviewboard.kde.org/media/uploaded/files/2013/11/12/34ab7f7c-7aca-4c16-ab3f-add513c22f56__folder_browser_1.jpg
> Folder browser for selecting include paths
>   https://git.reviewboard.kde.org/media/uploaded/files/2013/11/12/29a48901-93ee-40e7-9455-94acb648025e__folder_browser_2.jpg
> Added include paths will be visible to user in text edit
>   https://git.reviewboard.kde.org/media/uploaded/files/2013/11/12/bf9fb89b-8e6e-49f8-b2b5-d865ded209b7__folder_browser_3.jpg
> 
> 
> Thanks,
> 
> Meenakshi Khorana
> 
>

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