Move assistant infrastructure to kdevplatform

Reviews
Diff

- Download Diff

Information
Submitter:	Kevin Funk
Repository:	kdevplatform
Branch:
Bugs:
Depends On:
Reviewers
Groups:	kdevelop
People:

Description

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

Testing Done

Issues

0
18
1
All Issues: 19

Description	From	Last Updated
Does this comment refer to the struct above it?	Olivier de Gaalon	June 7, 2014, 12:30 a.m.
This code was written with the assumption that textChanged will always come from the same view, is it possible here ...	Olivier de Gaalon	June 7, 2014, 12:30 a.m.
Is this comment still relevant? The meaning isn't clear to me. These aren't queued connections.	Olivier de Gaalon	June 7, 2014, 12:30 a.m.
Delete me	Olivier de Gaalon	June 7, 2014, 12:31 a.m.
Needs more indentation	Olivier de Gaalon	June 7, 2014, 12:32 a.m.
nitpick: * and & go next to type in kdev land	Milian Wolff	June 7, 2014, 12:34 a.m.
auto, also below	Milian Wolff	June 7, 2014, 12:34 a.m.
spaces	Milian Wolff	June 7, 2014, 12:34 a.m.
indent level is off	Milian Wolff	June 7, 2014, 12:35 a.m.
braces	Milian Wolff	June 7, 2014, 12:35 a.m.
what for?	Milian Wolff	June 7, 2014, 12:36 a.m.
indent	Milian Wolff	June 7, 2014, 12:36 a.m.
spaces	Milian Wolff	June 7, 2014, 12:37 a.m.
return !firstRange.instersect(secondRange + Range(0, -1, 0, +1)).isEmpty()); achieves the same, no?	Milian Wolff	June 9, 2014, 8:01 a.m.
auto cursor = changed.start();	Milian Wolff	June 7, 2014, 12:38 a.m.
here and below: braces	Milian Wolff	June 7, 2014, 12:38 a.m.
here and below: braces	Milian Wolff	June 7, 2014, 12:47 a.m.
here and below: space after if	Milian Wolff	June 7, 2014, 12:43 a.m.
braces	Milian Wolff	June 7, 2014, 12:43 a.m.

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.

Ship it!

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)

Show all issues

Does this comment refer to the struct above it?

language/assistant/renameassistant.cpp (Diff revision 1)

Show all issues

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)

Show all issues

Is this comment still relevant? The meaning isn't clear to me. These aren't queued connections.

language/assistant/staticassistantsmanager.cpp (Diff revision 1)

Show all issues

Delete me

language/assistant/staticassistantsmanager.cpp (Diff revision 1)

Show all issues

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?

Kevin Funk June 9, 2014, 9:28 a.m.

Note: Doesn't work with the PHP plugin (didn't look deeper into it yet). However, the rename assistant didn't show up before my patch either. Can you confirm?

language/assistant/renameaction.h (Diff revision 1)

Show all issues

nitpick: * and & go next to type in kdev land

language/assistant/renameaction.cpp (Diff revision 1)

Show all issues

auto, also below

language/assistant/renameaction.cpp (Diff revision 1)

Show all issues

spaces

language/assistant/renameaction.cpp (Diff revision 1)

Show all issues

indent level is off

language/assistant/renameaction.cpp (Diff revision 1)

Show all issues

braces

language/assistant/renameassistant.h (Diff revision 1)

Show all issues

what for?

language/assistant/renameassistant.h (Diff revision 1)

Show all issues

indent

language/assistant/renameassistant.cpp (Diff revision 1)

Show all issues

spaces

language/assistant/renameassistant.cpp (Diff revision 1)

Show all issues

return !firstRange.instersect(secondRange + Range(0, -1, 0, +1)).isEmpty());

achieves the same, no?

language/assistant/renameassistant.cpp (Diff revision 1)

Show all issues

auto cursor = changed.start();

Kevin Funk June 9, 2014, 9:21 a.m.
```
Doesn't really qualify for auto, imo.
```

language/assistant/renameassistant.cpp (Diff revision 1)

Show all issues

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

Kevin Funk June 9, 2014, 9:21 a.m.

Yes, I'd keep it as-is. Having just one language per assistant makes it much easier to manage, imo. Each language can then decide whether it wants to instantiate a RenameAssistant.

language/assistant/staticassistantsmanager.cpp (Diff revision 1)

Show all issues

here and below: braces

language/assistant/staticassistantsmanager.cpp (Diff revision 1)

Show all issues

here and below: space after if

language/assistant/staticassistantsmanager.cpp (Diff revision 1)

Show all issues

braces

This review has been submitted with commit a0b1219be69ef129498c5c0461e1106a78773603 by Kevin Funk to branch master.

You have a pending review.

Review Board 2.5.6.1

Screenshots

Files

Issues

Status: Closed (submitted)