On Sat, Oct 20, 2007 at 06:55:18PM +0200, Peter Kümmel wrote:
> >As far as I can tell using QtCore in support/*.cpp is acceptable
> >nowadays, so just use QProcess and be done.
>
> also mocing?
Ask the others. I don't have a problem with it.
> >We can think about a split in support as soon as we have a non-Qt
> >frontend _and_ do not want to link against Qt anymore. No need to
> >be pro-active here.
> >
> >[We currently do somethign weird for alerts, though...]
> >
> >>+QTextEdit* GuiViewBase::progressWidget()
> >
> >const?
>
> no, later on non-const functions will be called on this object.
> And it woild be also better to introduce a new interface for the progress
> widget which supports more than QTextEdit, eg. setting the page number.
I mean
QTextEdit* GuiViewBase::progressWidget() const
> >#include "..."
>
> What's wrong with <...>?
It's conceptionally for system libraries. Also, we use "..." for our
stuff consistently and I see no reason to deviate from that practice.
> >>+int CoreSystemProcess::start(const std::string& cmd, bool
> >>waitForFinished)
> >>+{
> >>+ if (view) {
> >>+ view->clear();
> >>+ view->append("starting LaTex with command");
> >>+ view->append(cmd.c_str());
> >>+ }
> >>+
> >>+ process.setReadChannel(QProcess::StandardOutput);
> >>+ process.start(cmd.c_str(), QStringList(), QIODevice::ReadOnly);
> >>+ if (waitForFinished) {
> >>+ // no timeout
> >>+ process.waitForFinished(-1);
> >>+ return process.exitCode();
> >>+ } else {
> >
> >No need for 'else' after 'return'
>
>
> Isn't the code more readable with the else?
Not in my opinion. Less line noise, a level less of indentation.
> >>+void CoreSystemProcess::newProcessOutput()
> >>+{
> >>+ const QString output =
> >>QString::fromLocal8Bit(process.readAllStandardOutput());
> >>+ // parse output for page number, etc., could be used for a progress
> >>bar
> >>+ view->append(output);
> >>+ QApplication::processEvents();
> >>+}
> >
> >Have you checked QApplication::processEvents() is needed?
>
> No, maybe you are right and it isn't needed. But the event handling
> isn't perfect. When I process the UserGuide, the textedit widget is updated
> only after killing the latex process; I have to find a other solution.
>
> >
> >>+void CoreSystemProcess::processStarted()
> >>+{
> >>+ view->append("LaTex started\n");
> >>+ QApplication::processEvents();
> >>+}
> >
> >There's also something wrong with spacing in the last four functions.
>
> I know, all my new code uses space instead of tabs, I fix it before
> checking in.
[Why not create new code with tabs from the beginning?]
> >>+boost::function<SystemProcess*()> Systemcall::processCreator;
> >>+
> >
> >Why do why need boost::function?
> >
>
> To store the creator function, but thinking in include file count
> a typedef would be better ;)
Yes.
> >>Index: src/support/SystemProcess.h
> >>===================================================================
> >>--- src/support/SystemProcess.h (revision 0)
> >>+++ src/support/SystemProcess.h (revision 0)
> >>@@ -0,0 +1,43 @@
> >>+// -*- C++ -*-
> >>+/**
> >>+ * \file GuiView.h
> >>+ * This file is part of LyX, the document processor.
> >>+ * Licence details can be found in the file COPYING.
> >>+ *
> >>+ * \author Peter K?mmel
> >>+ *
> >>+ * Full author contact details are available in file CREDITS.
> >>+ */
> >>+
> >>+#ifndef LYX_SUPPORT_SYSTEMPROCESS_H
> >>+#define LYX_SUPPORT_SYSTEMPROCESS_H
> >>+
> >>+
> >>+namespace lyx {
> >>+namespace support {
> >>+
> >>+
> >>+class SystemProcess
> >>+{
> >>+public:
> >>+ virtual ~SystemProcess(){}
> >>+
> >>+ // creator template
> >>+ template<class T>
> >>+ static SystemProcess* creator() {
> >>+ return new T;
> >>+ }
> >
> >Hm?
> >
>
> somewhere I have to define a create function when I don't wanna
> link support against frontend. This is not needed when I move
> CoreSystemProcess to support.
So just do that (unless some of the others shout louader than I).
Andre'
