KDE/kdevplatform

[Andreas Pakulat]

On 08.07.09 18:17:02, Aleix Pol Gonzalez wrote:
> SVN commit 993467 by apol:
> 
> A little bit of refactoring and reorganization on the buildsets.
> Simplifies the buildset creation a lot.
> 
> If it does not fix the bug, we will have to create some kind of "restructuring" state for the project or just add the new tree to the project when it's ready.

Ok, took the time now to do a complete review. I still don't like this
commit, it mixes too many things within the same commit.
 
> --- trunk/KDE/kdevplatform/plugins/projectmanagerview/builditembuilderjob.cpp #993466:993467
> @@ -29,7 +29,9 @@
>  {
>      foreach( const BuildItem &item, items )
>      {
> -        addItem( t, item.findItem() );
> +        KDevelop::ProjectBaseItem *it=item.findItem();
> +        if(it)
> +            addItem( t, it );
>      }

I don't think this is the right fix for the problem. If an item from the
buildset is currently being reloaded, we shouldn't build the buildset at
all, because we cannot know what changes are being done to the cmake files
(that are the cause of the reload). We should just disable building as long
as the reloading takes if a buildset is active.

> --- trunk/KDE/kdevplatform/plugins/projectmanagerview/projectbuildsetmodel.cpp #993466:993467
> @@ -1,6 +1,7 @@
>  /***************************************************************************
>   *   This file is part of KDevelop                                         *
> - *   Copyright 2007 Andreas Pakulat <apaku at gmx.de>                        *
> + *   Copyright 2007 Andreas Pakulat <apaku at gmx.de>                         *
> + *   Copyright 2009 Aleix Pol <aleixpol at kde.org>                           *
>   *                                                                         *
>   *   This program is free software; you can redistribute it and/or modify  *
>   *   it under the terms of the GNU Library General Public License as       *
> @@ -33,69 +34,12 @@
>  
>  #include <project/projectmodel.h>
>  
> -QString getRelativeFolder( KDevelop::ProjectBaseItem* item )
> -{
> -    if( !item )
> -        return "";
> -
> -    if( item->type() == KDevelop::ProjectBaseItem::Folder
> -          || item->type() == KDevelop::ProjectBaseItem::BuildFolder )
> -    {
> -
> -        return item->project()->relativeUrl( item->folder()->url() ).toLocalFile();
> -    }else
> -    {
> -        return getRelativeFolder( dynamic_cast<KDevelop::ProjectBaseItem*>( item->parent() ) );
> -    }
> -}
> -
> -KDevelop::ProjectBaseItem* findItem( const QString& item, const QString& path, KDevelop::ProjectBaseItem* top )
> -{
> -    if( top && top->text() == item && getRelativeFolder( top ) == path )
> -    {
> -        return top;
> -    }else if( top->hasChildren() )
> -    {
> -        for( int i = 0; i < top->rowCount(); i++ )
> -        {
> -            QStandardItem* sitem = top->child( i );
> -            KDevelop::ProjectBaseItem* prjitem = dynamic_cast<KDevelop::ProjectBaseItem*>(sitem);
> -            if( prjitem )
> -            {
> -                if( prjitem->file()
> -                    && prjitem->text() == item
> -                    && path == getRelativeFolder( prjitem->file() ) )
> -                {
> -                    return prjitem;
> -                }else if( prjitem->folder()
> -                          && prjitem->text() == item
> -                          && path == getRelativeFolder( prjitem->folder() ) )
> -                {
> -                    return prjitem;
> -                }else if( prjitem->target()
> -                          && prjitem->text() == item
> -                          && path == getRelativeFolder( prjitem->target() ) )
> -                {
> -                    return prjitem;
> -                }else
> -                {
> -                    KDevelop::ProjectBaseItem* tmp = findItem( item, path, prjitem );
> -                    if( tmp )
> -                        return tmp;
> -                }
> -            }
> -        }
> -    }
> -    return 0;
> -}
> -
> -
>  BuildItem::BuildItem()
>  {
>  }
>  
> -BuildItem::BuildItem( const QString& itemName, const QString& projectName, const QString& itemPath )
> -        : m_itemName( itemName ), m_projectName( projectName ), m_itemPath( itemPath )
> +BuildItem::BuildItem( const QString & itemPath )
> +        : m_itemPath( itemPath )
>  {
>  }
>  
> @@ -106,43 +50,41 @@
>  
>  BuildItem::BuildItem( const BuildItem& rhs )
>  {
> -    m_itemName = rhs.itemName();
> -    m_projectName = rhs.projectName();

This is not going to work in the long term. We have open reports from
people that would like to put in arbitrary items into the buildset that are
not in the project tree. Which is actually needed for proper
custom-makefile support (unless someone writes a makefile parser that
produces meaningful project tree with all targets).

We need at least an "item name" to indicate the custom target.

Hmm, how is the buildset stored? Wasn't that using the projectName to store
each item into the project it belongs to? This is a bit harder to do now...

Last but not least: This breaks when a file has a "/" in it as you're using
lastIndexOf() instead of properly parsing the path.

> +    QStringList paths;
>      foreach( const BuildItem &item, m_items)
>      {
> -        if( item.projectName() == project->name() )
> -        {
> -            KConfigGroup grp = base.group(QString("Builditem%1").arg(count));
> -            grp.writeEntry("Projectname", item.projectName());
> -            grp.writeEntry("Itemname", item.itemName());
> -            grp.writeEntry("Itempath", item.itemPath());
> -            count++;
> -        }
> +        paths.append(item.itemPath());
>      }
> -    base.writeEntry("Number of Builditems", count);
> +    KConfigGroup base = project->projectConfiguration()->group("Buildset");
> +    base.writeEntry("Builditems", paths);

Ah, there we go, this is just wrong. You're now storing the complete path
of all buildset items into each project. But a project should only have the
paths of items that are inside this particular project.

Andreas

-- 
You have a will that can be influenced by all with whom you come in contact.