[apaku at gmx.de: Re: KDE/kdevelop/lib/plugins/outputviews]

[dukju ahn]

2007/5/13, Andreas Pakulat <apaku at gmx.de>:
> Hi,
>
> don't know wether you read kde-commits, if you don't have a look at
> http://commitfilter.kde.org/
> it allows to get commit mails for just a specific directory in svn that
> you're interested on (and the replies to such commits).

I didn't subscribe kde-commit. Thank you for replying this to me.
My comments are belows.

>
> From: Andreas Pakulat <apaku at gmx.de>
> To: kde-commits at kde.org
> Date: Sat, 12 May 2007 17:28:52 +0200
> Subject: Re: KDE/kdevelop/lib/plugins/outputviews
> On 12.05.07 14:47:00, Dukju Ahn wrote:
> > Rearrangements and fixes in Outputview.
> >
> > 3. When excuting command, Model is now created inside addNewTab() along with QListView.
>
> Why did you do that?

The reason is related with momory free(delete).
In previous version, OutputViewCommand didn't get deleted when it finishes
its excution. So its memory leak. I added deleteLater() call inside
the finished() slot.
But the problem was that if the command owns model, the model is deleted inside
OutputViewCommand's destructor, causing the outputview widget empty
( i.e. , If the model is deleted, the QListView becomes empty)

Also in model-view approach, I tought model and view should be created
and delted
in same location, because model is associated with view.

> This breaks having multiple outputviews, now each
> of the outputviews has its own models for the command-stuff. The models
> need to stay at the plugin.

I didn't think about multiple outputviews. But there is solution.
OutputViewcommand can have QList<Model*>. That way, every view can be
updated simultaneously

> > --- trunk/KDE/kdevelop/lib/plugins/outputviews/outputviewcommand.h #663872:663873
> > @@ -35,9 +35,14 @@
> >  Q_OBJECT
> >  public:
> >      OutputViewCommand( const KUrl& workdir, const QStringList& command,
> > -                       const QMap<QString, QString>& env, QStandardItemModel* model );
> > +                       const QMap<QString, QString>& env, QStandardItemModel* model = NULL );
>
> We use 0 not NULL.

I didn't know. I'll fix it.

> >  void OutputWidget::addNewTab(const QString& title, QStandardItemModel* model )
> >  {
> > -    if( !model || title.isEmpty() || m_listviews.contains( title ) )
> > +    if( !model || title.isEmpty() )
> >          return;
> > -    QListView* listview = new QListView(this);
> > -    listview->setModel( model );
> > -    m_listviews[title] = listview;
> > -    addTab( listview, title );
> > +
> > +    if( !m_listviews.contains( title ) )
> > +    {
> > +        QListView* listview = new QListView(this);
> > +        listview->setModel( model );
> > +        m_listviews[title] = listview;
> > +        addTab( listview, title );
> > +    }
> > +    else
> > +    {
> > +        QListView* listview = m_listviews[title];
> > +        listview->setModel( model );
> > +    }
> >  }
>
> Whats the reasoning for this? I don't see why we should replace the
> model if somebody requests to add a tab with the same name as already
> exists.

Because in this way, if sombody requests adding a tab with the previous name,
it can start with a new model, discarding old model.

> These two can't stay, at least not unless you don't use them. Please
> have a look at the qmake manager to see how a  manager is supposed to
> work like. It is not allowed to store the top-level item, because 1
> plugin instance handles all projects for that type of buildsystem. So
> this needs to go away.
>
> > +    KDevelop::ProjectItem *myPrjItem = baseitem->project()->projectItem();
> > +    if( myPrjItem != d->m_rootItem )
>
> The above means you can't do this, rather you could ask the project
> about its managerPlugin and then compare that to this. Or you could get
> the project config and read the Manager entry and check if its
> CustomMakeManager (or whatever you use)

OK. I think I should fix it.

> On 13.05.07 07:31:06, Dukju Ahn wrote:
> > Porting ProcessLineMaker into KDev4. This wraps signal line-by-line, rather than emitting text whenever there is incomplete stdout/stderror.
>
> What does that class do? Can it process output of various types of
> processes or just make? Does it make sense to produce an interface for
> this so a buildmanager can hook up its own thing here.

It processes every kind of K3Process outputs.
Regarding the filter thing, the concept of LineMaker is not filter. It
just emit signal
whenever a "completed" line was read.
If K3Process reads some output but that's an incomplete line,
normal K3Process would emit signal. But lineMaker
waits until a completed one-line is captured. Only after one complete
line is formed
it emits signal.

> Also for internationalization matters I think parts of the regexp this
> uses (it does use a regexp right?) should be editable, so people don't
> have to run Kdevelop in english just to get proper click-on-errors...

No there is no QRegExp in ProcessLineMaker. The only string is '\n'

> Uhm, and now that we're reading line-by-line the outputview will be even
> slower. Before we had the opportunity to use our own model to insert
> multiple lines at once.

If we don't use LineMaker the following hypotetic situation may happen.

exec make
gcc -c        // KProcess emits "gcc -c"
somefile.c  // KProcess emits "somefile.c"

Then there would be 2 line in outputview.

> >  install(FILES kdevexport.h DESTINATION ${INCLUDE_INSTALL_DIR}/kdevelop/)
> > --- trunk/KDE/kdevelop/lib/kdevexport.h #664120:664121
> > @@ -99,6 +99,7 @@
> >  #define KDEVPLATFORMEDITOR_EXPORT KDE_EXPORT
> >  #define KDEVPLATFORMLANGUAGE_EXPORT KDE_EXPORT
> >  #define KDEVPLATFORMPROJECT_EXPORT KDE_EXPORT
> > +#define KDEVUTIL_EXPORT KDE_EXPORT
>
> You just broke the windows build. Please add a proper line in the
> win-part of kdevexport.h

OK I'll try