[Patch] Qmake project manager Qt module include paths

[Sean Harmer]

Hi,

On Friday 10 April 2009 00:46:57 Andreas Pakulat wrote:
> On 09.04.09 17:26:18, Sean Harmer wrote:
> > Hi,
> >
> > On Thursday 09 April 2009 14:22:26 Sean Harmer wrote:
> > > Could somebody please review the patch please?
> >
> > I have improved the patch a little more so that it now also resolves
> > environment variables in the INCLUDEPATH qmake variable. Updated patch
> > attached for review.
>
> Sorry, but not ok. Envvars need to be read via project config and the
> envvar-stuff from kdevplatform. That way we get the standard env in
> which kdevelop was started, but overriden by any user-chosen
> env configuration via the project config.
That's fine I'm just not very familiar with the kdevplatform code base yet. 
Could you give me a pointer as to where to look for this environment support 
please?

> > +    // Let's cache the Qt include dir
> > +    QProcess qtInc;
> > +    qtInc.start( "qmake", QStringList() << "-query" <<
> > "QT_INSTALL_HEADERS" ); +    if ( !qtInc.waitForFinished() )
> > +    {
> > +        kDebug(9024) << "Failed to query Qt header path using qmake";
> > +    } else
> > +    {
> > +        QByteArray result = qtInc.readAll();
> > +        m_qtIncludeDir = result.simplified();
> > +    }
> > +
>
> This part is ok design-wise, but the conversion from QByteArray to
> QString is broken - I suggest to make sure that qmake runs in an UTF8
> locale and then use QString::fromUtf8().
Fair enough, do you have a preferred way of launching apps with QProcess in a 
UTF-8 locale?

> > @@ -152,11 +180,57 @@
> >      kDebug(9024) << variableValues("QMAKE_INCDIR_QT");
> >      if( variableValues("CONFIG").contains("qt") )
> >      {
> > -        //@TODO add QtCore,QtGui and so on depending on CONFIG values,
> > -        //      as QMAKE_INCDIR_QT only contains the include/ dir
> > -        foreach( const QString& val, variableValues("QMAKE_INCDIR_QT") )
> > +        KUrl url(m_qtIncludeDir);
> > +        if( !list.contains( url ) )
> > +            list << url;
> > +
> > +        kDebug(9024) << "QT =" << variableValues("QT");
> > +        foreach( const QString& module, variableValues("QT") )
> >          {
> > -            KUrl url(val);
> > +            KUrl url;
> > +            if ( module.compare( "core" ) == 0 )
> > +                url.setUrl(m_qtIncludeDir + "/QtCore");
<snip>
> I like if( module == "XXX" )  more than the compare() version (looks so
> old-style c-like ;), so I'd appreciate it if you could change that.
> Other than that, this part looks fine.
>
> Oh and please check for extra debug output, that shouldn't be comitted.
OK I'll tidy up this part and re-post for another review.

Cheers,

Sean