Show output of commands in Debug Console #1593
Conversation
There was a problem hiding this comment.
Looking fine, apart from inline comments. :-)
Would it maybe be helpful to not only write the output of the command, but also a line with the final command being executed? That may help in finding issues with used variables, for example.
Executing: echo "Hello World"
src/tiled/command.cpp
Outdated
| : QProcess(DocumentManager::instance()) | ||
| , mName(command.name) | ||
| , mFinalCommand(command.finalCommand()) | ||
| , mFinalWorkingDirectory(command.finalWorkingDirectory()) | ||
| , mLogger(CommandManager::instance()->logger()) |
There was a problem hiding this comment.
If the logger is available from the global CommandManager instance, I don't see a reason to also have it as member of CommandProcess.
src/tiled/command.cpp
Outdated
| @@ -209,12 +216,20 @@ CommandProcess::CommandProcess(const Command &command, bool inTerminal) | |||
|
|
|||
| connect(this, SIGNAL(finished(int)), SLOT(deleteLater())); | |||
|
|
|||
| if (showOutput) | |||
| connect(this, &QProcess::readyReadStandardOutput, this, &CommandProcess::consoleOutput); | |||
There was a problem hiding this comment.
You'll want to also connect to readyReadStandardError or use setProcessChannelMode to merge the standard error channel into the standard out channel.
Of course, it would be nice to report the error channel as appropriate to the LoggingInterface, so that the Debug Console will show errors in red.
src/tiled/command.cpp
Outdated
| if (!mFinalWorkingDirectory.trimmed().isEmpty()) | ||
| setWorkingDirectory(mFinalWorkingDirectory); | ||
|
|
||
| start(mFinalCommand); | ||
| } | ||
|
|
||
| void CommandProcess::consoleOutput() | ||
| { | ||
| mLogger->log(LoggingInterface::INFO, QLatin1String(readAllStandardOutput())); |
There was a problem hiding this comment.
You shouldn't wrap the returned QByteArray with a QLatin1String, cause that will break whenever the encoding of the output is not actually latin1. Since we can't really know the output encoding, we should probably assume local encoding, which means using QString::fromLocal8Bit.
| @@ -132,7 +143,7 @@ void CommandDialog::updateWidgets(const QModelIndex ¤t, const QModelIndex | |||
| mUi->exBrowseButton->setEnabled(enable); | |||
| mUi->keySequenceEdit->setEnabled(enable); | |||
| mUi->clearButton->setEnabled(enable); | |||
| mUi->saveBox->setEnabled(enable); | |||
There was a problem hiding this comment.
This line shouldn't be deleted, right?
There was a problem hiding this comment.
Actually I noticed that this line is already there in top 😂
There was a problem hiding this comment.
So I deleted this one.
src/tiled/commanddialog.ui
Outdated
| <item row="4" column="0" colspan="3"> | ||
| <widget class="QCheckBox" name="outputBox"> | ||
| <property name="text"> | ||
| <string>Show Output on Console</string> |
There was a problem hiding this comment.
Please use "Show output in Debug Console".
src/tiled/commanddialog.ui
Outdated
| @@ -154,7 +154,7 @@ | |||
| <item row="4" column="0" colspan="3"> | |||
| <widget class="QCheckBox" name="outputBox"> | |||
| <property name="text"> | |||
| <string>Show Output on Console</string> | |||
| <string>Show Output in Debug Console</string> | |||
There was a problem hiding this comment.
Small nit: please lowercase "output"
src/tiled/command.cpp
Outdated
| if (showOutput) | ||
| if (showOutput) { | ||
| CommandManager::instance()->logger()->log(LoggingInterface::INFO, | ||
| QString(QLatin1String("Executing: ")) |
There was a problem hiding this comment.
This should really be a translatable string, like tr("Executing: %1").arg(...)
|
It's working well, nicely done! |
This closes #1552. I've attached a screenshot of the same.