xqemu / xqemu-manager

Simple graphical user interface to manage XQEMU
http://xqemu.com
GNU General Public License v2.0
34 stars 10 forks source link

Use QProcess instead of subprocess, add log area #48

Closed thrimbor closed 5 years ago

thrimbor commented 5 years ago

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_20191002_015005

Screenshot after xqemu exiting with a non-zero return value (note that the Start/Stop button was updated correctly when xqemu quit): Screenshot_20191002_015316

JayFoxRox commented 5 years ago

Looks great! I have tried this now and got some feedback:

(All points in this comment have been addressed since then)

thrimbor commented 5 years ago
* Is there an unused status-bar at the bottom of the Window? The distance of the log to the bottom of the window would suggest it, which is a bit confusing.

There is. I'm not sure whether it's required in order to have that little triangle thing that helps you resize the window.

* The log window is empty and unlabeled at XQEMU-Manager startup; this is potentially confusing for new users. It should be labeled as "XQEMU Messages" or something.

I added a label above it now.

* When restarting XQEMU, the log window is not cleared. This makes it harder to see new messages.

Fixed.

* The equivalent log in XQEMU-Manager looks weird (contains empty lines):

Ah, I didn't notice that appendPlaintext always adds a newline - I replaced it with insertPlainText and moveCursor, which fixes this.

/edit: Updated screenshot: Screenshot_20191003_023429

mborgerson commented 5 years ago

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.

JayFoxRox commented 5 years ago

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.

Agreed.

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.

I think we should have something like MSVS error list (but much simpler), in a separate window:

MSVS Error list

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'll let @JayFoxRox merge it once he feels his nits are addressed.

I don't care about the nits (as they also add risk of another review pass); I'll merge and move nits into issues.