-
Notifications
You must be signed in to change notification settings - Fork 10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use QProcess instead of subprocess, add log area #48
Conversation
Looks great! I have tried this now and got some feedback: (All points in this comment have been addressed since then)
|
There is. I'm not sure whether it's required in order to have that little triangle thing that helps you resize the window.
I added a label above it now.
Fixed.
Ah, I didn't notice that |
|
||
def onReadyReadStandardError(self): | ||
text = self.inst._p.readAllStandardError().data().decode() | ||
self.log.insertPlainText(text) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to docs, this adds text at current cursor position. Would a user selecting text mean that text is added in wrong location?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're correct (just confirmed it by testing). The recommendation where I took that from actually set the cursor before and after, so I changed the code to do that as well. It has the downside of resetting a selection the user made whenever log output is added, but it seems like this is the only way how the additional whitespaces can be avoided.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sure there's a better way. We should create an issue after merge, so we can improve this later.
text = self.inst._p.readAllStandardError().data().decode() | ||
self.log.moveCursor(QtGui.QTextCursor.End) | ||
self.log.insertPlainText(text) | ||
self.log.moveCursor(QtGui.QTextCursor.End) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Somehow this also doesn't scroll all the way to the bottom. There appear to be 2 blank lines at the end.
Thanks! In terms of the UI, I think having a general log window on the main screen may be unnecessary. Generally, I think "no news is good news" and the user should not be presented with these details until an error occurs. What I had in mind was moving this log window to the error dialog, and providing a button there for the user to "copy to clipboard," or something to that effect. That said, this is an improvement over what we have now and we can refine the UX going forward. I think it's ready to merge, but I'll let @JayFoxRox merge it once he feels his nits are addressed. |
Agreed.
I think we should have something like MSVS error list (but much simpler), in a separate window: So, a list with each log entry, icons would show wether it's a note (stdout), a warning (stderr) or error (crash/exit error message). Anyhow we can iteratively improve this.
I don't care about the nits (as they also add risk of another review pass); I'll merge and move nits into issues. |
This has been lying around for a while. It adds a log area where xqemu output is displayed, a message that is displayed if xqemu quits with a non-zero return value, and generally improves keeping track of the subprocess.
/edit:
Screenshot after starting & stopping xqemu:
Screenshot after xqemu exiting with a non-zero return value (note that the Start/Stop button was updated correctly when xqemu quit):