<br><br><div class="gmail_quote">On Wed, Jul 8, 2009 at 9:32 PM, Andreas Pakulat <span dir="ltr"><<a href="mailto:apaku@gmx.de">apaku@gmx.de</a>></span> wrote:<br><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<div class="im">On 08.07.09 18:17:02, Aleix Pol Gonzalez wrote:<br>
</div><div class="im">> SVN commit 993467 by apol:<br>
><br>
> A little bit of refactoring and reorganization on the buildsets.<br>
> Simplifies the buildset creation a lot.<br>
><br>
> 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.<br>
<br>
</div>Ok, took the time now to do a complete review. I still don't like this<br>
commit, it mixes too many things within the same commit.<br>
<div class="im"><br>
> --- trunk/KDE/kdevplatform/plugins/projectmanagerview/builditembuilderjob.cpp #993466:993467<br>
> @@ -29,7 +29,9 @@<br>
> {<br>
> foreach( const BuildItem &item, items )<br>
> {<br>
> - addItem( t, item.findItem() );<br>
> + KDevelop::ProjectBaseItem *it=item.findItem();<br>
> + if(it)<br>
> + addItem( t, it );<br>
> }<br>
<br>
</div>I don't think this is the right fix for the problem. If an item from the<br>
buildset is currently being reloaded, we shouldn't build the buildset at<br>
all, because we cannot know what changes are being done to the cmake files<br>
(that are the cause of the reload). We should just disable building as long<br>
as the reloading takes if a buildset is active.</blockquote><div><br>agree. but we still don't want it to crash until it's done.<br><br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<br>
<div><div></div><div class="h5"><br>
> --- trunk/KDE/kdevplatform/plugins/projectmanagerview/projectbuildsetmodel.cpp #993466:993467<br>
> @@ -1,6 +1,7 @@<br>
> /***************************************************************************<br>
> * This file is part of KDevelop *<br>
> - * Copyright 2007 Andreas Pakulat <<a href="mailto:apaku@gmx.de">apaku@gmx.de</a>> *<br>
> + * Copyright 2007 Andreas Pakulat <<a href="mailto:apaku@gmx.de">apaku@gmx.de</a>> *<br>
> + * Copyright 2009 Aleix Pol <<a href="mailto:aleixpol@kde.org">aleixpol@kde.org</a>> *<br>
> * *<br>
> * This program is free software; you can redistribute it and/or modify *<br>
> * it under the terms of the GNU Library General Public License as *<br>
> @@ -33,69 +34,12 @@<br>
><br>
> #include <project/projectmodel.h><br>
><br>
> -QString getRelativeFolder( KDevelop::ProjectBaseItem* item )<br>
> -{<br>
> - if( !item )<br>
> - return "";<br>
> -<br>
> - if( item->type() == KDevelop::ProjectBaseItem::Folder<br>
> - || item->type() == KDevelop::ProjectBaseItem::BuildFolder )<br>
> - {<br>
> -<br>
> - return item->project()->relativeUrl( item->folder()->url() ).toLocalFile();<br>
> - }else<br>
> - {<br>
> - return getRelativeFolder( dynamic_cast<KDevelop::ProjectBaseItem*>( item->parent() ) );<br>
> - }<br>
> -}<br>
> -<br>
> -KDevelop::ProjectBaseItem* findItem( const QString& item, const QString& path, KDevelop::ProjectBaseItem* top )<br>
> -{<br>
> - if( top && top->text() == item && getRelativeFolder( top ) == path )<br>
> - {<br>
> - return top;<br>
> - }else if( top->hasChildren() )<br>
> - {<br>
> - for( int i = 0; i < top->rowCount(); i++ )<br>
> - {<br>
> - QStandardItem* sitem = top->child( i );<br>
> - KDevelop::ProjectBaseItem* prjitem = dynamic_cast<KDevelop::ProjectBaseItem*>(sitem);<br>
> - if( prjitem )<br>
> - {<br>
> - if( prjitem->file()<br>
> - && prjitem->text() == item<br>
> - && path == getRelativeFolder( prjitem->file() ) )<br>
> - {<br>
> - return prjitem;<br>
> - }else if( prjitem->folder()<br>
> - && prjitem->text() == item<br>
> - && path == getRelativeFolder( prjitem->folder() ) )<br>
> - {<br>
> - return prjitem;<br>
> - }else if( prjitem->target()<br>
> - && prjitem->text() == item<br>
> - && path == getRelativeFolder( prjitem->target() ) )<br>
> - {<br>
> - return prjitem;<br>
> - }else<br>
> - {<br>
> - KDevelop::ProjectBaseItem* tmp = findItem( item, path, prjitem );<br>
> - if( tmp )<br>
> - return tmp;<br>
> - }<br>
> - }<br>
> - }<br>
> - }<br>
> - return 0;<br>
> -}<br>
> -<br>
> -<br>
> BuildItem::BuildItem()<br>
> {<br>
> }<br>
><br>
> -BuildItem::BuildItem( const QString& itemName, const QString& projectName, const QString& itemPath )<br>
> - : m_itemName( itemName ), m_projectName( projectName ), m_itemPath( itemPath )<br>
> +BuildItem::BuildItem( const QString & itemPath )<br>
> + : m_itemPath( itemPath )<br>
> {<br>
> }<br>
><br>
> @@ -106,43 +50,41 @@<br>
><br>
> BuildItem::BuildItem( const BuildItem& rhs )<br>
> {<br>
> - m_itemName = rhs.itemName();<br>
> - m_projectName = rhs.projectName();<br>
<br>
</div></div>This is not going to work in the long term. We have open reports from<br>
people that would like to put in arbitrary items into the buildset that are<br>
not in the project tree. Which is actually needed for proper<br>
custom-makefile support (unless someone writes a makefile parser that<br>
produces meaningful project tree with all targets).</blockquote><div>The project is not needed, it's in the path.<br> <br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<br>
<br>
We need at least an "item name" to indicate the custom target.<br>
</blockquote><div>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.<br> <br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<br>
Hmm, how is the buildset stored? Wasn't that using the projectName to store<br>
each item into the project it belongs to? This is a bit harder to do now...<br>
<br>
Last but not least: This breaks when a file has a "/" in it as you're using<br>
lastIndexOf() instead of properly parsing the path.</blockquote><div><br>That will be fixed in my next commit. Paths are QStringLists from now on<br><br> <br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<br>
<div class="im"><br>
> + QStringList paths;<br>
> foreach( const BuildItem &item, m_items)<br>
> {<br>
> - if( item.projectName() == project->name() )<br>
> - {<br>
> - KConfigGroup grp = base.group(QString("Builditem%1").arg(count));<br>
> - grp.writeEntry("Projectname", item.projectName());<br>
> - grp.writeEntry("Itemname", item.itemName());<br>
> - grp.writeEntry("Itempath", item.itemPath());<br>
> - count++;<br>
> - }<br>
> + paths.append(item.itemPath());<br>
> }<br>
> - base.writeEntry("Number of Builditems", count);<br>
> + KConfigGroup base = project->projectConfiguration()->group("Buildset");<br>
> + base.writeEntry("Builditems", paths);<br>
<br>
</div>Ah, there we go, this is just wrong. You're now storing the complete path<br>
of all buildset items into each project. But a project should only have the<br>
paths of items that are inside this particular project.</blockquote><div><br>True, fixed on the next commit.<br> <br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<br>
<br>
Andreas<br>
<font color="#888888"><br>
--<br>
You have a will that can be influenced by all with whom you come in contact.<br>
</font><div><div></div><div class="h5"><br>
_______________________________________________<br>
KDevelop-devel mailing list<br>
<a href="mailto:KDevelop-devel@kdevelop.org">KDevelop-devel@kdevelop.org</a><br>
<a href="https://barney.cs.uni-potsdam.de/mailman/listinfo/kdevelop-devel" target="_blank">https://barney.cs.uni-potsdam.de/mailman/listinfo/kdevelop-devel</a><br>
</div></div></blockquote></div><br>Thanks for the review :P and next time ping me before reverting :)<br><br>