[Differential] [Requested Changes To] D2650: AddMissingIncludePath dialog for easier fixing of missing includes

[mwolff (Milian Wolff)]

mwolff requested changes to this revision.
mwolff added a reviewer: mwolff.
mwolff added a comment.
This revision now requires changes to proceed.


  I like the idea (I think, could you add some screenshots please)?
  
  Some code changes are required, but overall this is pretty good already - thanks!

INLINE COMMENTS

> missingincludepathproblem.cpp:52
>      {
> -        openConfigurationPage(m_path.str());
> +        KDevelop::IDefinesAndIncludesManager* dim =
> +            KDevelop::IDefinesAndIncludesManager::manager();

auto

> addmissingincludepath.cpp:37
> +{
> +    QList<Path> m_paths;
> +public:

use Path::List (i.e. QVector!)

style: move down into an explicitly private section

> addmissingincludepath.cpp:51
> +
> +    Path path(int row)
> +    {

const

> addmissingincludepath.cpp:53
> +    {
> +        if (row < 0 || row >= m_paths.size())
> +            return {};

afaik you can use m_paths.value(row, {})

> addmissingincludepath.cpp:62
> +    {
> +        if (!index.isValid() || role != Qt::DisplayRole)
> +            return {};

`|| !hasIndex(index.row(), index.size(), index.parent())`

> addmissingincludepath.cpp:65
> +
> +        if (index.row() >= 0 && index.row() < m_paths.size()) {
> +            IProjectController* projectController =

the above check makes this obsolete

> addmissingincludepath.cpp:66
> +        if (index.row() >= 0 && index.row() < m_paths.size()) {
> +            IProjectController* projectController =
> +                ICore::self()->projectController();

auto

> addmissingincludepath.h:72
> +     */
> +    KDevelop::Path includePath();
> +

const

> addmissingincludepath.h:78
> +     */
> +    KDevelop::Path folder();
> +

const

> definesandincludesmanager.cpp:281
> +{
> +    AddMissingIncludePath dlg;
> +

better not put it on the stack (could be deleted via dbus or funky stuff like that), but rather create on the heap and then set `Qt::WA_DeleteOnClose` on the dialog

> definesandincludesmanager.cpp:297
> +    // Find candidate include paths among all opened projects.
> +    const Path includePath(QUrl::fromLocalFile(includeName));
> +    for (IProject *project : projectController->projects()) {

the QUrl::fromLocalFile should not be required, Path(QString) does what you want, no?

> definesandincludesmanager.cpp:298
> +    const Path includePath(QUrl::fromLocalFile(includeName));
> +    for (IProject *project : projectController->projects()) {
> +        for (ProjectFileItem *item : KDevelop::allFiles(project->projectItem())) {

this algorithm should be put somewhere out of this dialog, and get properly tested

also, the implementation looks pretty bad, performance wise. I think you want to put the includeName into an `IndexedString` and then use `ProjectModel::item{,s}ForPath`.

> definesandincludesmanager.cpp:322
> +    // Now show the dialog and process results
> +    if (dlg.exec() == QDialog::Accepted) {
> +        Path chosenIncludePath = dlg.includePath();

better use show and connect lambdas to signals that handle when the dialog got accepted

> definesandincludesmanager.cpp:334
> +{
> +    KDevelop::IProjectController* projectController =
> +        KDevelop::ICore::self()->projectController();

auto

> definesandincludesmanager.cpp:335
> +    KDevelop::IProjectController* projectController =
> +        KDevelop::ICore::self()->projectController();
> +    KDevelop::IProject* project = projectController->findProjectForUrl(folder.toUrl());

auto

REPOSITORY
  rKDEVELOP KDevelop

REVISION DETAIL
  https://phabricator.kde.org/D2650

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: nicolaih, #kdevelop, mwolff
Cc: mwolff, kdevelop-devel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20160905/263576d0/attachment-0001.html>