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