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:
http://git.reviewboard.kde.org/r/113815/#review43678
-----------------------------------------------------------



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

    That is the wrong location for this code snippet. It should be in the yet-to-be-created CustomIncludePathsWidget class.



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

    You won't need the signal mapper in case you move the code to the correct location


Okay, there are some issues with this patch. In theory you shouldn't touch the unresolvedincludeassistant.* files at all. You want to modify the behavior of the custom include paths widget, hence you need also need to touch just that.

Proposed change: Make the include path resolver a proper widget subclass, e.g. by providing also a .cpp and .h to the .ui. That's the pattern used when widgets have more complex logic inside. When you have the .cpp and .h file, move the logic you added to unresolvedincludeassistant.* to those newly created files. Signal/slot handling will get much easier when you do it this way.

The .cpp/.h/.ui pattern is a commonly used in Qt, you can see several examples in the kdevelop code base.

Does that make sense to you?

- Kevin Funk


On Nov. 12, 2013, 11:50 a.m., Meenakshi Khorana wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/113815/
> -----------------------------------------------------------
> 
> (Updated Nov. 12, 2013, 11:50 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/codegen/unresolvedincludeassistant.cpp 93a72d3 
>   languages/cpp/codegen/ui/custom_include_paths.ui 1a7c1ed 
>   languages/cpp/codegen/unresolvedincludeassistant.h b072265 
> 
> Diff: http://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
>   http://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
>   http://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
>   http://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/20131114/39a6138a/attachment.html>