Review Request: Make sig assistant rename

[Olivier Jean de Gaalon]

> On Feb. 21, 2012, 10:33 p.m., Milian Wolff wrote:
> > could you please elaborate on the "assistant stacking"?
> > 
> > and this code really could need some love, style-wise. Please, I'd really appreciate if you could clean the code up while you are at it. Also some more comments would certainly help! I don't understand e.g. what the three new functions in IAssistant are supposed to do... And please please please, don't inline all code, separate it cleanly between header (with apidox please?) and implementation...
> 
> Milian Wolff wrote:
>     oh and please - could you add unit tests? this should be possible I think...

"Assistant Stacking":
Currently if a sig assist shows, and then a rename assist is shown, the sig assist is destroyed (you'll never see it). A simple solution would be to hide the sig assist and show it when the rename assist is acknowledged. In any case, I removed it, because it'll take some work.
The only thing this patch changes is
1. "If changing a function declaration, the signature assistant renames the uses in the definition"

"Love, style-wise":
On one hand I do feel like I deserve a beating for throwing up a diff with as many ugly green lines as this one, but on the other, I really just left the code exactly as it is. You can see the ugly stuff supposedly added in parseJobFinished is the same as the removed stuff (perhaps it was the commented out code I removed).

"Three new functions":
the getRenameActions function gets the rename actions.
The other two functions are just refactoring stuff out of the rather large parseJobFinished function so it's not a PITA to work with. Let me know what you want on that.

"don't inline all code":
The RenameAction class used to be in the RenameAssistant's .cpp, exactly as it is, in like manner to the SignatureAssistant's AdaptSignatureAction before it. Only now it's included in two .cpp files, again for minimal changes. I'll change that if there's no reason for it to be inlined like that.

"tests":
I'm pretty sure there aren't any code assistant tests...?

In summary:
This is a small feature, I think I only improved the surrounding code when adding it, but there's certainly plenty to do in the SigAssist. If you don't want this area touched until the surrounding area is cleaned up, I'll have to discard this until I have time to do so.


> On Feb. 21, 2012, 10:33 p.m., Milian Wolff wrote:
> > languages/cpp/codegen/renameaction.h, line 50
> > <http://git.reviewboard.kde.org/r/103817/diff/3/?file=49576#file49576line50>
> >
> >     new code shouldn't pass anything to the lock's ctor. i.e.:
> >     
> >     DUChainReadLocker lock;

This isn't, strictly speaking, new code. It's been moved from a .cpp file as-is. An easy change though.


> On Feb. 21, 2012, 10:33 p.m., Milian Wolff wrote:
> > languages/cpp/codegen/signatureassistant.cpp, line 301
> > <http://git.reviewboard.kde.org/r/103817/diff/3/?file=49579#file49579line301>
> >
> >     could it happen that this is never called? yes, or - if the assistant is not applied? then we'd leak the rename actions.
> >     
> >     so add a qDeleteAll to the ctor of IAssistantAction

*facepalm*


- Olivier Jean de


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/103817/#review10815
-----------------------------------------------------------


On Feb. 17, 2012, 10:50 a.m., Olivier Jean de Gaalon wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/103817/
> -----------------------------------------------------------
> 
> (Updated Feb. 17, 2012, 10:50 a.m.)
> 
> 
> Review request for KDevelop.
> 
> 
> Description
> -------
> 
> After editing function declaration, when updating the definition to match, rename uses of renamed declarations.
> 
> Note:
> Removed the assistant stacking since the sig assist depends on being destroyed (and it was hacking where change is needed). Shouldn't be much work to fix that, but time I don't have now...
> 
> 
> This addresses bug 255706.
>     http://bugs.kde.org/show_bug.cgi?id=255706
> 
> 
> Diffs
> -----
> 
>   languages/cpp/codegen/renameaction.h PRE-CREATION 
>   languages/cpp/codegen/renameassistant.cpp 9bbdce7 
>   languages/cpp/codegen/signatureassistant.h 2c6853d 
>   languages/cpp/codegen/signatureassistant.cpp 0231e90 
> 
> Diff: http://git.reviewboard.kde.org/r/103817/diff/
> 
> 
> Testing
> -------
> 
> Using the signature assistant.
> 
> 
> Thanks,
> 
> Olivier Jean de Gaalon
> 
>

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