KDE/kdevplatform

Aleix Pol aleixpol at kde.org
Thu Jul 9 14:54:36 UTC 2009


On Wed, Jul 8, 2009 at 9:32 PM, Andreas Pakulat <apaku at gmx.de> wrote:

> 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.


agree. but we still don't want it to crash until it's done.


>
> > ---
> 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).

The project is not needed, it's in the path.


>
>
> We need at least an "item name" to indicate the custom target.
>
We can have it in the path some way. Or when we need it, we add it. I don't
think it's a good idea to have it specified before having it.


>
> 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.


That will be fixed in my next commit. Paths are QStringLists from now on



>
>
> > +    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.


True, fixed on the next commit.


>
>
> Andreas
>
> --
> You have a will that can be influenced by all with whom you come in
> contact.
>
> _______________________________________________
> KDevelop-devel mailing list
> KDevelop-devel at kdevelop.org
> https://barney.cs.uni-potsdam.de/mailman/listinfo/kdevelop-devel
>

Thanks for the review :P and next time ping me before reverting :)
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20090709/29df1d58/attachment.html>


More information about the KDevelop-devel mailing list