Fix for makebuilder not opening properly files referenced by errors/warnings

[dizzy]

Hello Andreas

On Saturday 16 August 2008 23:26:09 Andreas Pakulat wrote:
> Hmm, makes me wonder wether cmake prints absolute paths for errors or
> why did I never have a problem with that - anybody?

AFAIK cmake never changes build directory. It generates a single Makefile 
(well one that includes others probably) but it never makes "make" change the 
build directory (all those add_subdirectory() are parsed by cmake to generate 
the build files but different from automake do not result in "make" changing 
any directory). So for cmake the assumption that build dir is project top dir 
always should be fine (then the relative paths gcc gives on errors are fine 
too).

> Before I now tear apart the patch itself, let me first thank you for
> working on the code. It really needs some love.

Thank you, I hope I can make it happy a little. Actually I expected much more 
critics being the first kdev/kde patch I ever made.

> >  private:
> > +    MakeOutputModel const& m_model;
>
> That should be a pointer not a const&.

I have noticed that KDE code makes heavy usage of pointers and almost no usage 
of references. I assume this has to do with the coding style or something else 
I do not know of. Can you point me to an explanation (so I can be more careful 
in the future, IMO references are a big plus of C++ over C since it provides 
builtin syntax to express that a "pointer" cannot be NULL which is the main 
usage of pointers in general, to be send as non NULL to achieve pass by 
reference semantics).

> >      QString errorText;
> > +    QString currDir; // store current directory if relative path is
> > stored in "file"
>
> Why not store the absolute path in file instead of a relative one, isn't
> it used just for opening the file?

I've thought about this issue too. Basically the main reasoning was that the 
user is expected to activate those items much less than they are generated (so 
I only convert to absolute path on activation). I realise now this is somewhat 
a stupid decision on my part, a user is to be expected to activate all 
warnings/errors (or almost all) and it's not like that conversion saves a lot 
of CPU (not like optimizing ActiveFilters::processAndCreate() regexp matching 
which is on my todo list :) ). I'll change it to compute the absolute path at 
item generation if that's fine.

> > +            url = KUrl::fromPath( warn->currDir );
> > +            url.cd( warn->file );
>
> Interesting that this actually works. For readability reasons I'd
> suggest to change it into addPath() nonetheless - unless that breaks
> something.

I think this was a residue from an earlier version of my code where I didn't 
checked for path being absolute in item generation before setting currDir and 
I wanted as "cd()" documentation says that if the given argument to cd() is 
abslute the resulting path to be absolute. This makes no sense in the current 
code so I will use addPath().

>
> > +        // FIXME: is this portable?
> > +        if (!file.isEmpty() && file[0] != '/')
>
> No its not portable, the right thing to do is use
> QFileInfo::isRelative()

Heh, QFileInfo and the other 100000 Qt/KDE API classes I do not know :) Will 
do.

-- 
Dizzy