Review Request 124369: Encapsulate field refactoring

[Milian Wolff]

> On July 24, 2015, 9:15 a.m., Milian Wolff wrote:
> > 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?
> 
> Maciej Poleski wrote:
>     ad a) line edit is read only. it is information for user which field is going to be encapsulated. user cannot change contents.
>     ad b) i'm very bad GUI designer, that's fact... radio boxes were used because need one click instead of two in combo boxes. scaling is an issue, i like idea to provide text edits with bodies (prefilled with default generated body). Currently implementation is placed in (all) RecordDecl (class or structure). I'm still thinking how to find appropriate place in appropriate implementation file. I can get "path" of record decls up to TU record decl (root), but then how to find the same scope (place) in another TU (and prove that this TU will be available available after compilation to all uses)?
> 
> Maciej Poleski wrote:
>     There are two more reviews waiting for attention:
>     - https://git.reviewboard.kde.org/r/124300/
>     - https://git.reviewboard.kde.org/r/124285/
>     
>     just sayin... :)

Would be cool if you could take some ideas from my GUI approach and mix and match it with your approach to get something useable.

Regarding your problem to find the implementation file, there are helper functions to figure out whether you are in a header or not. If you are not, then just add it to the current file. Otherwise, use the buddy feature to find the corresponding implementation file, so just reuse that code please.

Then, I don't know what you mean with the "record decls" stuff at the end - do you mean external users of the field you just wrapped in a getter/setter? Like:

    // file 1:
    struct foo {
        int bar;
    };
    // file 2:
    #include "file1.h"
    {
        foo f;
        f.bar = 1; // translate that to f.setBar(1);
    }

? This is going to be a hard problem. I'd be _very_ find with tackling the "simple" issue first of creating some code. Then, maybe, we should concentrate first on more local changes to get some useable stuff, like I mentioned before. What do you think? Tackling these hard problems we can still do later, no?

Sorry for lagging behind, my day job and now Akademy often keeps me distracted. Please continue to ping me about things I'm missing or not responding to quickly enough.


- Milian


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


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/20150725/eb2e3a40/attachment.html>