[PATCH] KProcess port of khelpcenter

[Oswald Buddenhagen]

On Thu, Dec 13, 2007 at 06:43:25PM +0100, Ralf Habacker wrote:
> appended is a patch which ports KHelpcenter to KProcess.

> Are there any objecitivies against submitting ?
>
yes, i have *objections*. i demand you to un-post this patch. :-D

now seriously ...

> Index: CMakeLists.txt
> ===================================================================
> --- CMakeLists.txt	(revision 747562)
> +++ CMakeLists.txt	(working copy)
> @@ -6,7 +6,6 @@
>  include(MacroOptionalAddSubdirectory)
> -
>  macro_optional_find_package(USB)
>
please try to keep patch submissions free of such non-changes.
not sure why removed that empty line anyway. well, who cares.

> Index: runtime/khelpcenter/glossary.cpp
> ===================================================================
> --- runtime/khelpcenter/glossary.cpp	(revision 747562)
> +++ runtime/khelpcenter/glossary.cpp	(working copy)
> @@ -147,25 +147,34 @@
>  void Glossary::rebuildGlossaryCache()
>  {
>  	KXmlGuiWindow *mainWindow = dynamic_cast<KXmlGuiWindow *>( kapp->activeWindow() );
> +#ifndef Q_WS_WIN
>  	Q_ASSERT( mainWindow );
> +#else
> +    // there is a case when mainwindow is zero 
> +	if (mainWindow)
> +#endif
>
how about explaining "a case"?

> +#ifdef Q_WS_WIN
> +    // KStandardDirs::locate return "" on win32 
> +	*m_meinProc << "meinproc4"; 
> +#else 
> +	*m_meinProc << KStandardDirs::locate( "exe", QLatin1String( "meinproc4" ) );
> +#endif	
>
findExe(). if that doesn't work, fix the library.

> Index: runtime/khelpcenter/kcmhelpcenter.cpp
> ===================================================================
> --- runtime/khelpcenter/kcmhelpcenter.cpp	(revision 747562)
> +++ runtime/khelpcenter/kcmhelpcenter.cpp	(working copy)
> -  if ( !mProcess->start( K3Process::NotifyOnExit, K3Process::AllOutput ) ) {
> -    kError() << "KCMHelpcenter::startIndexProcess(): Failed to start process."
> -      << endl;
> -  }
> +  mProcess->start();
>
use waitForStarted().
fwiw, i suggest this for meinproc, too, even if the original code did no
error checking.

> +void KCMHelpCenter::slotReceivedStdout()
>  {
> -  QString text = QString::fromLocal8Bit( buffer, buflen );
> -  int pos = text.lastIndexOf( QLatin1Char('\n') );
> +  QByteArray text= mProcess->readAllStandardOutput();
>
good. but what then?

> +  int pos = text.lastIndexOf( '\n' );
>    if ( pos < 0 ) {
>      mStdOut.append( text );
>    } else {
> @@ -624,9 +611,9 @@
>    }
>  }
>  
> +void KCMHelpCenter::slotReceivedStderr( )
>  {
> -  QString text = QString::fromLocal8Bit( buffer, buflen );
> +  QString text = mProcess->readAllStandardError();
>
bad.

http://techbase.kde.org/Development/Tutorials/Common_Programming_Mistakes#Reading_QString_from_a_KProcess

> Index: runtime/khelpcenter/khc_indexbuilder.cpp
> ===================================================================
> --- runtime/khelpcenter/khc_indexbuilder.cpp	(revision 747562)
> +++ runtime/khelpcenter/khc_indexbuilder.cpp	(working copy)

>  
> -  if ( !proc->start( K3Process::NotifyOnExit, K3Process::AllOutput ) ) {
> +  mProcess->start();

> +  while (mProcess->state() == KProcess::Starting)
> +    ;
>
busy-looping? hello?

> +   if (mProcess->state() == KProcess::NotRunning)  {
>    }
>
indentation

> Index: runtime/khelpcenter/scrollkeepertreebuilder.cpp
> ===================================================================
> --- runtime/khelpcenter/scrollkeepertreebuilder.cpp	(revision 747562)
> +++ runtime/khelpcenter/scrollkeepertreebuilder.cpp	(working copy)
> @@ -57,6 +59,10 @@
>    QString lang = KGlobal::locale()->language();
>  
>    kDebug(1400) << "ScrollKeeper language: " << lang;
> +#ifdef Q_WS_WIN
> +  kDebug(1400) << "running \"scrollkeeper-get-content-list\" not implemented yet";
>
oh, c'mon, porting k3procio => kprocess really isn't hard.
if the code is inapplicable as such, then disable it completely.

> Index: runtime/khelpcenter/searchengine.cpp
> ===================================================================
> --- runtime/khelpcenter/searchengine.cpp	(revision 747562)
> +++ runtime/khelpcenter/searchengine.cpp	(working copy)
> +void SearchEngine::searchStdout()
>  {
> +  mSearchResult += mProc->readAllStandardOutput();
>  }
>  
> +void SearchEngine::searchStderr()
>  {
> +  mStderr.append(mProc->readAllStandardError());
>  }
>  
leave that up to kprocess. see the techbase entry.

> Index: runtime/khelpcenter/searchhandler.cpp
> ===================================================================
> --- runtime/khelpcenter/searchhandler.cpp	(revision 747562)
> +++ runtime/khelpcenter/searchhandler.cpp	(working copy)
> +bool SearchJob::startLocal(const QString &cmdString)
> +{
> +    QStringList cmd = cmdString.split( " ");
> +    QStringList::ConstIterator it;
> +    for( it = cmd.begin(); it != cmd.end(); ++it ) {
> +      QString arg = *it;
> +      if ( arg.left( 1 ) == "\"" && arg.right( 1 ) =="\"" ) {
> +        arg = arg.mid( 1, arg.length() - 2 );
> +      }
> +      *mProcess << arg.toUtf8();
> +    }
>
KShell::splitArgs().

> +    mProcess->start();
> +    while (mProcess->state() == KProcess::Starting)
> +        ;
>
lalala ...

> +void SearchJob::searchStdout()
> +{
> +    mResult += mProcess->readAllStandardOutput();
> +}
> +
> +void SearchJob::searchStderr()
> +{
> +    mError += mProcess->readAllStandardOutput();
> +}
> +
s.a.

> Index: runtime/khelpcenter/toc.cpp
> ===================================================================
> --- runtime/khelpcenter/toc.cpp	(revision 747562)
> +++ runtime/khelpcenter/toc.cpp	(working copy)
> @@ -23,7 +23,7 @@
> +#ifdef Q_WS_WIN
> +    // KStandardDirs::locate return "" on win32, hardcoded for now
> +	*m_meinproc << "meinproc4"; 
> +#else 
> +    *m_meinproc << *m_meinproc << KStandardDirs::locate( "exe", "meinproc4" );
> +#endif
>
s.a.

there is also some refactoring in the patch that makes the review
harder. i feel too bad now to be able to keep track of the structure, so
i won't comment on this.

the rest looks reasonable. looking forward to a new patch.

-- 
Hi! I'm a .signature virus! Copy me into your ~/.signature, please!
--
Confusion, chaos, panic - my work here is done.