Review request: control flow graph plugin

[Esben Mose Hansen]

On Monday 09 November 2009 16:15:41 Andreas Pakulat wrote:
> On 09.11.09 14:55:26, Esben Mose Hansen wrote:
> > On Saturday 17 October 2009 20:02:06 Andreas Pakulat wrote:
> > > - line 54/55 in controlflowgraphview.cpp look like they should be a
> > >   normal function call, not s signal/slot connection? Also if you want
> > > a read-write part you should use the create<KParts::ReadWritePart>
> >
> > This also took some digging. Apparently, the function call is not
> > possible because the function is defined in the specific KPart, which we
> > I suppose we can't just include and cast to. Instead, I have called the
> > method via. QMetaObject::invokeMethod(...)
> 
> We must not rely on internal functions of a KPart, so kgraphviewer needs to
> expose this function in a public interface to which we can cast the KPart
> instance.

Ok, I will have to get in touch with the developers of that KPart... as far as 
I can see, no public interface is installed.  I have written to kleag about 
it, so hopefully I can clear this up. Until then, I will keep the ugly hack... 
at least, *noone* who sees this is in doubt it is a hack!

> 
> > > - deregistering the toolview should be done in the unload method, as
> > >   plugin instances are never deleted (until end of the program)
> >
> > Ok, I have arranged that the plugin emits unloaded(), which is connected
> > to ControlFlowViewGraph::slotUnloaded(), which in turn call
> > unregisterToolView(). I hope this is correct --- do we have a tutorial or
> > something about how this is supposed to work?
> 
> Usually the plugin instance directly calls unregisterToolView() as it knows
> the factory. That'll destroy the view and all connected qobject's. Just
> look at other toolviews, like the classbrowser.

TODO :)

> 
> > > - the toolbutton signal/slot connection in controlflowgraphview would
> > >   benefit from a QSignalMapper to hide the (IMHO) ugly use of
> > >   sender()+object names
> >
> > I don't see how this is possible given that the signals we would map have
> > a parameter (bool checked). (Insert rant about Qt primitive signal-slot
> > connections compared to the competition).
> 
> Maybe I didn't look close enough, I thought the parameter is not used (or
> can be fetched in another way)

It was used in one of them, and not in the other. But this way is actually 
less or about the same amount of code, and except for 2 lines who got 
duplicated, better in every way. So I think we're cool.

> Right, you can't fix that only Sandro can.

On the other hand, we just need a license and an OK from him, no? 

> 
> > The code is unstable, though, with some memory corruption. Stil working
> > on this. From valgrind, I suspect that this is the problematic code:
> >
> > QString DUChainControlFlow::globalNamespaceOrFolderNames(Declaration
> > *declaration)
> > {
> >     IBuildSystemManager *buildSystemManager;
> >     if (m_useFolderName && m_currentProject && (buildSystemManager =
> > m_currentProject->buildSystemManager()))
> >     {
> >         if (KDevelop::ProjectBaseItem *project_item = m_currentProject-
> >
> > >projectItem())
> >
> >         {
> >             KUrl::List list = buildSystemManager->includeDirectories(
> > project_item );
> >             int minLength = std::numeric_limits<int>::max();
> 
> Where is m_currentProject coming from? I guess this is somehow calculated
> based on the currently open file? If m_currentProject is always a proper
> project reference (one needs to monitor the signals from the project
> controller!) then the code shouldn't crash.

See, this is interesting. Whenever the cursor is moved, the current project is 
determined  (l.188)

    m_currentProject = ICore::self()->projectController()-
>findProjectForUrl(view->document()->url());

and then this and several other member variables are used in another thread, 
run via. ThreadWeaver.. including several (3)  DUchain objects. Now, the lock 
on DUChain expires with the function, so those DUChain objects are surely 
potentially invalid when the threadweaver executes the class's run method?! 

Too tired to work this out right now, but I think the cursorPositionChanged 
should basically just record the position and let the run method do all the 
real work, including locking the DUContext. 

Input as to whether I might be on the right track is very welcome :D

-- 
Kind regards, Esben