QProcess vs K3Process

[Oswald Buddenhagen]

On Wed, Jun 06, 2007 at 07:24:48PM +0200, Thiago Macieira wrote:
> Oswald Buddenhagen wrote:
> >    void setEnv(const QString &name, const QString &value);
> 
> No need to shorten it: call this setEnvironment.
> 
eh, no. this is not supposed to shadow/overload the QProcess function -
it does something slightly different (and you don't want to call it
setEnvironmentVariable, right?).
also, setenv is ... uh, well ... POSIX. :)

> >    void unsetEnv(const QString &name) { setEnv( name, QString() ); }
> 
> and please uninline this.
> 
i'm not sure i like that. or rather, i'm sure i don't. this is just a
convenience wrapper for (supposedly) documented behavior of the "proper"
function. this is not going to change, ever. why would one want to
uninline it?

> >    void setProgram(const QStringList &argv);
> 
> I'm sorry, what is this? Shouldn't it be setArguments instead?
> 
to quote myself:
> > i added an overload to set program + args with one list, as that's
> > the expected output from KShell::splitArgs() and possibly other
> > sources.

> >    KProcess &operator<<(const QString& arg);
> >    KProcess &operator<<(const QStringList& args);
> 
> I don't like these,
> 
to quote myself again:
> > the operator<<()s look a bit weird, but i guess we want to keep that
> > convenience.
i'm not religious about it ...
btw, they are sort of consistent with the single-QStringList()-syntax,
too. :)

> but for completeness, also add operator+=.
>
i don't see why operator+= should be added - we never had such a thing
and it would lead to even more inconsistent code.

> >    void setShellCommand(const QString &cmd, const char *shell = 0);
> 
> Why is the shell a const char*? I think a QString would be best, like 
> everything else.
> 
well, this is going to come from ::getenv() or a hard-coded string in
most cases, so it is not too far off. but well ...

> Note that the result of getenv could change, so it should not be stored.
> 
... it's converted to a QString anyway ...
... so why not do Just Do It (TM) the conventional way. :}

> >    void start();
> 
> Please add the OpenMode argument with a default value of 
> QIODevice::ReadWrite.
> 
that omission is in fact intentional. the argument doesn't fulfill any
useful function as far as i can see - the mode is determined by the
channel modes + redirections.
fwiw, i also suggested this to tt for the new overloads.

> >    int execute();
> 
> You could also add this blurb to the method's apidox:
>   This method is roughly equivalent to:
>   <code>
>     proc.start();
>     proc.waitForFinished(-1);
>   </code>
> 
well, i could, but what's the point?

> >    // too bad one cannot "unshadow" overloads
> 
> Sure you can:
>     using QProcess::execute;
> 
you live and learn. :)
however, some googling indicates that this might not be universally
suppported yet?

> >private:
> >    // hide those
> >    void setReadChannelMode();
> >    void readChannelMode();
> >    void setProcessChannelMode();
> >    void processChannelMode();
> 
> Same:
>     using QProcess:setReadChannelMode;
>     using QProcess:readChannelMode;
>     using QProcess::setProcessChannelMode;
>     using QProcess::processChannelMode;
> 
this works for hiding, too?

hmm, maybe one should instead inherit from QProcess privately and
"import" everything explicitly? that's a lot of usings, though.

> >#ifdef Q_OS_WIN
> ># include <windows.h>
> 
> Uh... no. Move this to kprocess_win.cpp.
> 
this feels sooooo silly for a few lines ... oh, well.

> >        // workaround for QProcess dropping the read buffer on channel switch 
> >        int ba = q->bytesAvailable(); 
> >        QByteArray buf = q->read(ba);
> >
> >        q->setReadChannel(good);
> >        writeAll(q->readAll(), fd);
> >        q->setReadChannel(bad);
> >
> >        // how "efficient" ...
> >        for (int i = ba; --i >= 0;)
> >            q->ungetChar(buf[i]);
> 
> Has this been reported to TT?
> 
yes.

> >    }
> >    QString fname(name);
> >    fname.append('=');
> >    for (QStringList::Iterator it = env.begin(); it != env.end(); ++it)
> >        if ((*it).startsWith(fname)) {
> >            if (value.isNull()) {
> >                env.erase(it);
> >                goto hit;
> 
> Ewww... please use break;
> 
this is the equivalent construct to python's for:-break-else:. i find a
short-distance jump way clearer than having to realize that the below
condition is mutually exclusive to it. i generally think that
eliminating gotos for the sake of it is a sickness.

> >            } else {
> >                *it = fname.append(value);
> >                setEnvironment(env);
> >                return;
> >            }
> >        }
> 
> >    if (!value.isNull())
> >        env.append(fname.append(value));
> >  hit:
> >    if (env.isEmpty())
> >        env.append(DUMMYENV);
> 
> Suggestion:
>     if (env.isEmpty() && value.isNull())
>         env.append(QLatin1String(DUMMYENV)));
>     else if (!value.isNull())
>         env.append(fname.append(value));
> 

> >void KProcess::setShellCommand(const QString &cmd, const char *shell)
> >{
> >    d->prog = QString::fromLatin1(
> >        shell ?
> >            shell :
> >#ifdef Q_OS_UNIX
> >// #ifdef NON_FREE // ... as they ship non-POSIX /bin/sh
> ># if !defined(__linux__) && !defined(__FreeBSD__) &&
> > !defined(__NetBSD__) && !defined(__OpenBSD__) &&
> > !defined(__DragonFly__) && !defined(__GNU__)
> 
> Q_OS_LINUX, Q_OS_BSD4 (all BSDs, including MacOSX)
> 
Q_OS_BSD4 would most probably cover solaris 1.x as well.
however, one could invert the conditions - the list of non-free oses
sort of doesn't grow each day.

> > !access("/usr/xpg4/bin/sh", X_OK) ? // Solaris POSIX ...
> > "/usr/xpg4/bin/sh" :
> >        !access("/bin/ksh", X_OK) ? // ... which links here anyway
> >            "/bin/ksh" :
> >        !access("/usr/ucb/sh", X_OK) ? // dunno, maybe superfluous?
> >            "/usr/ucb/sh" :
> ># endif
> >            "/bin/sh"
> >#else
> >//         !access("c:\\windows\\cmd.exe", X_OK) ?
> >//             "c:\\windows\\cmd.exe" :
> >//             "c:\\command.com";
> >// need to determine system dir and system drive properly.
> >// consider COMSPEC - for WinDOS it is always command.com-compatible.
> > dunno // about WinNT vs. cmd.
> ># error i am broken. fix me
> >#endif
> >    );
> 
> Well, I trust you copied that from existing code,
>
... which i wrote ... :=)

> but can't you make it a bit more... palatable? ifdefs inside a ternary
> operator inside a call to QString::fromLatin1... ugh
> 
see, i *hate* duplication. ;)

> >#ifdef Q_OS_UNIX
> >    d->args = QStringList() << "-c" << cmd;
> >#else
> >    // XXX this has a good chance of being broken, definitely for
> > command.com. // split on whitespace and make a list of that, maybe?
> > don't forget // about consecutive spaces in this case.
> >    d->args = QStringList() << "/c" << cmd;
> 
> I think this is correct. Windows merges all arguments into a single 
> string, so it doesn't matter if you passed one or several.
> 
the problem is that QProcess will quote the argument so it is
interpreted as one by "conformant programs". but command.com isn't
conformant. dunno about cmd.exe, but Win9x doesn't have it anyway.

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