Move assistant infrastructure to kdevplatform
Review Request #118542 - Created June 4, 2014 and submitted
Information | |
---|---|
Kevin Funk | |
kdevplatform | |
Reviewers | |
kdevelop | |
Move assistant infrastructure to kdevplatform Noteworthy changes: * StaticCodeAssistant (from CPP plugin) is now StaticAssistantsManager, now lives in kdevplatform, so all languages can benefit from it * RenameAssistant was moved to kdevplatform, as it it useful for all languages * BasicRefactoring now has new virtual methods that control the behavior for renaming actions LanguageSupport got a new property: refactoring (so languages can expose their custom BaseRefactoring implementation) New API: * New base class for "static" assistants: StaticAssistant Static assistants exist during the whole session They are notified when documents change and create solutions based on that. Current implementations: AdaptDefinitionSignatureAssistant (CPP language) and RenameAssistant (kdevplatform) * New manager class: StaticAssistantsManager: Tracks the StaticAssistant instances, takes care of notifying them about changes in the editors. Single entry point for registering StaticAssistants. Entry-point: LanguageController::staticAssistantsManager
Note: This is by far not a 1:1 copy of the stuff from KDevelop, it needed quite some adjustments. The RenameAssistant in kdevplatform is now language-agnostnic, and its behavior is influenced by BasicRefactoring (which in turn belongs to a language support plugin). TODO: Pimpl the new classes. I'd like to have some feedback about the initial design of the new API in kdevplatform.
It's great to see this work done. I was suspicious about reusing StaticCodeAssistant, but this looks like a good cleanup that Solves Real Problems. It's much simpler to follow the logic now. Maybe add braces to the one-line if statements before the style-nazi gets to it.
-
language/assistant/renameaction.h (Diff revision 1) -
Does this comment refer to the struct above it?
-
language/assistant/renameassistant.cpp (Diff revision 1) -
This code was written with the assumption that textChanged will always come from the same view, is it possible here to begin editing the same range in another view and bypass this reset?
-
language/assistant/staticassistantsmanager.cpp (Diff revision 1) -
Is this comment still relevant? The meaning isn't clear to me. These aren't queued connections.
-
language/assistant/staticassistantsmanager.cpp (Diff revision 1) -
Delete me
-
language/assistant/staticassistantsmanager.cpp (Diff revision 1) -
Needs more indentation
looks cool to me, some style issues and of course the missing pimpl stuff. did you try it out with non-c++ language plugins like PHP with its $foo-like variables?
-
language/assistant/renameaction.h (Diff revision 1) -
nitpick: * and & go next to type in kdev land
-
language/assistant/renameaction.cpp (Diff revision 1) -
auto, also below
-
language/assistant/renameaction.cpp (Diff revision 1) -
spaces
-
language/assistant/renameaction.cpp (Diff revision 1) -
indent level is off
-
language/assistant/renameaction.cpp (Diff revision 1) -
braces
-
language/assistant/renameassistant.h (Diff revision 1) -
what for?
-
language/assistant/renameassistant.h (Diff revision 1) -
indent
-
language/assistant/renameassistant.cpp (Diff revision 1) -
spaces
-
language/assistant/renameassistant.cpp (Diff revision 1) -
return !firstRange.instersect(secondRange + Range(0, -1, 0, +1)).isEmpty()); achieves the same, no?
-
language/assistant/renameassistant.cpp (Diff revision 1) -
auto cursor = changed.start();
-
language/assistant/renameassistant.cpp (Diff revision 1) -
here and below: braces
-
language/assistant/staticassistantsmanager.cpp (Diff revision 1) -
yay in principle. but: should this be a list or something? could an assistant not support multiple languages or should we keep it as-is and let multiple assistants be registered for every language? not opening an issue, just want to know your input on that
-
language/assistant/staticassistantsmanager.cpp (Diff revision 1) -
here and below: braces
-
language/assistant/staticassistantsmanager.cpp (Diff revision 1) -
here and below: space after if
-
language/assistant/staticassistantsmanager.cpp (Diff revision 1) -
braces
This review has been submitted with commit a0b1219be69ef129498c5c0461e1106a78773603 by Kevin Funk to branch master.