Review Request 124369: Encapsulate field refactoring

[Milian Wolff]

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


the dialog UI needs to be revamped, the functionality and code is nice though - good work!

a) the line edit for the field to encapsulate, why is it a line edit? or is it read only? what happens if the user changes the contents?
b) the dialog structure is really awkward, I have to say. please use a form layout with labels left and widgets right. Insteaqd of the radio boxes, use combo boxes, and don't use a sentence structure to layout the widgets, that will break horribly once the dialog is translated (think about RTL languages!). And maybe, it's best to let the user configure the signature in pure line edits? Adding combo boxes for noexcept etc. pp. will never scale. Or maybe even a text edit so the bodies can be edited directly as well? My current approach would be something like this: http://i.imgur.com/wWHOIHd.png the sources you can find at https://paste.kde.org/pdrz1iqo6 . I'm only unsure whether we can easily get the name of the setter argument to create the body. And should we also allow configuration about inlining the bodies? Probably a good idea for the future?


refactoring/encapsulatefielddialog.h (line 40)
<https://git.reviewboard.kde.org/r/124369/#comment57154>

    remove the get prefix



refactoring/encapsulatefielddialog.cpp (line 32)
<https://git.reviewboard.kde.org/r/124369/#comment57155>

    deintent to align the comma with the colon



refactoring/encapsulatefieldrefactoring.h (line 62)
<https://git.reviewboard.kde.org/r/124369/#comment57156>

    spaces around the operators, here around the =



refactoring/encapsulatefieldrefactoring.cpp (line 88)
<https://git.reviewboard.kde.org/r/124369/#comment57157>

    again, align : and ,



refactoring/encapsulatefieldrefactoring.cpp (line 196)
<https://git.reviewboard.kde.org/r/124369/#comment57158>

    same



refactoring/encapsulatefieldrefactoring_changepack.cpp (line 42)
<https://git.reviewboard.kde.org/r/124369/#comment57159>

    same



refactoring/refactoringmanager.cpp (line 141)
<https://git.reviewboard.kde.org/r/124369/#comment57160>

    same



refactoring/refactoringmanager.cpp (line 191)
<https://git.reviewboard.kde.org/r/124369/#comment57161>

    same


- Milian Wolff


On July 23, 2015, 2:07 p.m., Maciej Poleski wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/124369/
> -----------------------------------------------------------
> 
> (Updated July 23, 2015, 2:07 p.m.)
> 
> 
> Review request for KDevelop.
> 
> 
> Repository: kdev-clang
> 
> 
> Description
> -------
> 
> Encapsulate field refactoring
> 
> - Displays dialog
> - Transforms "use" site - replaces direct uses of encapsulated variable by calls to getter and setter method/function.
> - Generates getter and setter implementation
> 
> Attached image presents GUI with its default content (prefilled by heuristics on DeclaratorDecl AST node).
> 
> 
> Diffs
> -----
> 
>   refactoring/CMakeLists.txt PRE-CREATION 
>   refactoring/encapsulatefielddialog.h PRE-CREATION 
>   refactoring/encapsulatefielddialog.cpp PRE-CREATION 
>   refactoring/encapsulatefielddialog.ui PRE-CREATION 
>   refactoring/encapsulatefieldrefactoring.h PRE-CREATION 
>   refactoring/encapsulatefieldrefactoring.cpp PRE-CREATION 
>   refactoring/encapsulatefieldrefactoring_changepack.h PRE-CREATION 
>   refactoring/encapsulatefieldrefactoring_changepack.cpp PRE-CREATION 
>   refactoring/redeclarationchain.cpp PRE-CREATION 
>   refactoring/refactoringmanager.cpp PRE-CREATION 
>   refactoring/tudecldispatcher.h PRE-CREATION 
>   refactoring/tudecldispatcher.cpp PRE-CREATION 
>   refactoring/utils.h PRE-CREATION 
>   refactoring/utils.cpp PRE-CREATION 
>   tests/CMakeLists.txt 20d17efae9a2a77cd7ef76bf7484ccfcf12e4cd8 
>   tests/test_encapsulatefield.h PRE-CREATION 
>   tests/test_encapsulatefield.cpp PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/124369/diff/
> 
> 
> Testing
> -------
> 
> Manual testing, unit testing
> 
> 
> File Attachments
> ----------------
> 
> Encapsulate field dialog
>   https://git.reviewboard.kde.org/media/uploaded/files/2015/07/15/ea15c55b-cc4d-4dfa-ac34-8c568a93701d__snapshot3.png
> 
> 
> Thanks,
> 
> Maciej Poleski
> 
>

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