-
Notifications
You must be signed in to change notification settings - Fork 63
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
Fixes #251: segfault (stopClient) and showing error (not active window) #252
base: master
Are you sure you want to change the base?
Conversation
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.
Thank you so much for looking into the issue and fixing it!
Just some small changes and I'm ready to merge it.
@@ -830,9 +833,8 @@ void MainWindow::resetLibraryPlaylist() const | |||
|
|||
void MainWindow::onSpotifyStatusChanged(const QString &status) | |||
{ | |||
if ((windowState() & Qt::WindowActive) > 0) |
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 think the main point of this was to not grab attention when the window is minimized. Maybe StatusMessage::error()
is better when the window isn't active? That way, you have to actively open the window to see if anything went wrong.
@@ -243,7 +242,29 @@ void SpotifyClient::Runner::onStarted() | |||
|
|||
void SpotifyClient::Runner::onErrorOccurred(QProcess::ProcessError error) | |||
{ | |||
emit statusChanged(QString("Error %1").arg(error)); | |||
std::string_view message; |
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.
Please note that this is still primarily a C++11 project, so std::string_view
isn't available. It's something I'm planning to change for the next major release, as some configurations require C++17, and mixing versions is quite messy. However, in this case, I think QStringLiteral
makes more sense, no point in working with C++ strings, when you only want to use it as a Qt string.
First, thanks for the application :)
This PR fixes these two errors, reported here #251
It also provides some improvements related to error reporting.
Debugged and found.
I don't know what is the use case to only show the error messages when the current window (main one) is the active one, as those messages are coming from the settings window. But I have found a problematic behavior with it, reported in the issue. I have spent a bunch of time trying to guess what was the error here (not considering that it was a configuration error, since first it segfaulted due to using a null ptr). If I saw the error "no username", I would have just checked configuration and changed it. So, definitely, there is value in showing errors independently of the active window.
Also, I don't get why we are not showing an error with Bad credentials if we are still not using the KEYCHAIN. So, I have modified it to be able to see this message:
Furthermore, as it can be seen, I propose changing the beginning of that message (from "Failed to start Spotify client:" to "Spotify client error:"), as it will make sense in all scenarios. For instance, when spotify client crashes (e.g. myself killing the librespot process):
(You can see how I propose changing also the error messages from "Error 1" to "Process crashed").
Furthermore, since we can stop the client on purpose, and to avoid modifying the settings panel logic, I have updated that message to be "Process stopped or crashed", as can be both cases. I am reusing the
mainContent
variable to detect if we are closing the app to avoid showing error messages (established inevent->accept()
on closeEvent)Then, clicking in "Stop client":
But not errors are shown in a dialog. The same when we close the application window.