diff --git a/picard/oauth.py b/picard/oauth.py index 048736f13..6e4335341 100644 --- a/picard/oauth.py +++ b/picard/oauth.py @@ -26,6 +26,7 @@ from functools import partial +from json.decoder import JSONDecodeError import time from PyQt5.QtCore import ( @@ -223,17 +224,20 @@ class OAuthManager(object): def on_exchange_authorization_code_finished(self, scopes, callback, data, http, error): successful = False + error_msg = None try: if error: log.error("OAuth: authorization_code exchange failed: %s", data) + error_msg = self._extract_error_description(http, data) else: self.set_refresh_token(data["refresh_token"], scopes) self.set_access_token(data["access_token"], data["expires_in"]) successful = True except Exception as e: log.error('OAuth: Unexpected error handling authorization code response: %r', e) + error_msg = _('Unexpected authentication error') finally: - callback(successful=successful) + callback(successful=successful, error_msg=error_msg) def fetch_username(self, callback): log.debug("OAuth: fetching username") @@ -244,14 +248,24 @@ class OAuthManager(object): def on_fetch_username_finished(self, callback, data, http, error): successful = False + error_msg = None try: if error: log.error("OAuth: username fetching failed: %s", data) + error_msg = self._extract_error_description(http, data) else: self.username = data["sub"] log.debug("OAuth: got username %s", self.username) successful = True except Exception as e: log.error('OAuth: Unexpected error handling username fetch response: %r', e) + error_msg = _('Unexpected authentication error') finally: - callback(successful) + callback(successful=successful, error_msg=error_msg) + + def _extract_error_description(self, http, data): + try: + response = load_json(data) + return response['error_description'] + except (JSONDecodeError, KeyError, TypeError): + return _('Unexpected request error (HTTP code %s)') % self.webservice.http_response_code(http) diff --git a/picard/tagger.py b/picard/tagger.py index 4beba698c..f63ca2832 100644 --- a/picard/tagger.py +++ b/picard/tagger.py @@ -325,20 +325,20 @@ class Tagger(QtWidgets.QApplication): authorization_code, scopes, partial(self.on_mb_authorization_finished, callback)) else: - callback(False) + callback(False, None) - def on_mb_authorization_finished(self, callback, successful=False): + def on_mb_authorization_finished(self, callback, successful=False, error_msg=None): if successful: self.webservice.oauth_manager.fetch_username( partial(self.on_mb_login_finished, callback)) else: - callback(False) + callback(False, error_msg) @classmethod - def on_mb_login_finished(self, callback, successful): + def on_mb_login_finished(self, callback, successful, error_msg): if successful: load_user_collections() - callback(successful) + callback(successful, error_msg) def mb_logout(self): self.webservice.oauth_manager.revoke_tokens() diff --git a/picard/ui/mainwindow.py b/picard/ui/mainwindow.py index 4d5670077..19a82034b 100644 --- a/picard/ui/mainwindow.py +++ b/picard/ui/mainwindow.py @@ -1501,9 +1501,14 @@ class MainWindow(QtWidgets.QMainWindow, PreserveGeometry): dialog = PasswordDialog(authenticator, reply, parent=self) dialog.exec_() - @classmethod - def on_mb_login_finished(self, successful): - log.debug('MusicBrainz authentication finished: %s', successful) + def on_mb_login_finished(self, successful, error_msg): + if successful: + log.debug('MusicBrainz authentication finished successfully') + else: + log.info('MusicBrainz authentication failed: %s', error_msg) + QtWidgets.QMessageBox.critical(self, + _("Authentication failed"), + _('Login failed: %s') % error_msg) def show_proxy_dialog(self, proxy, authenticator): dialog = ProxyDialog(authenticator, proxy, parent=self) diff --git a/picard/ui/options/__init__.py b/picard/ui/options/__init__.py index f89abb971..ae429d76d 100644 --- a/picard/ui/options/__init__.py +++ b/picard/ui/options/__init__.py @@ -44,7 +44,7 @@ class OptionsPage(QtWidgets.QWidget): SORT_ORDER = 1000 ACTIVE = True HELP_URL = None - STYLESHEET_ERROR = "QWidget { background-color: #f55; color: white; font-weight:bold }" + STYLESHEET_ERROR = "QWidget { background-color: #f55; color: white; font-weight:bold; padding: 2px; }" STYLESHEET = "QLabel { qproperty-wordWrap: true; }" def __init__(self, *args, **kwargs): diff --git a/picard/ui/options/general.py b/picard/ui/options/general.py index cff14a0cc..7e6074d5c 100644 --- a/picard/ui/options/general.py +++ b/picard/ui/options/general.py @@ -86,6 +86,8 @@ class GeneralOptionsPage(OptionsPage): self.ui.logout.clicked.connect(self.logout) self.ui.analyze_new_files.toggled.connect(self._update_cluster_new_files) self.ui.cluster_new_files.toggled.connect(self._update_analyze_new_files) + self.ui.login_error.setStyleSheet(self.STYLESHEET_ERROR) + self.ui.login_error.hide() self.update_login_logout() def load(self): @@ -129,20 +131,25 @@ class GeneralOptionsPage(OptionsPage): else: self.ui.server_host_primary_warning.show() - def update_login_logout(self): + def update_login_logout(self, error_msg=None): if self.tagger.webservice.oauth_manager.is_logged_in(): config = get_config() self.ui.logged_in.setText(_("Logged in as %s.") % config.persist["oauth_username"]) self.ui.logged_in.show() + self.ui.login_error.hide() self.ui.login.hide() self.ui.logout.show() - else: + elif error_msg: self.ui.logged_in.hide() + self.ui.login_error.setText(_('Login failed: %s') % error_msg) + self.ui.login_error.show() + self.ui.login.show() + self.ui.logout.hide() + else: + self.ui.logged_in.hide() + self.ui.login_error.hide() self.ui.login.show() self.ui.logout.hide() - # Workaround for Qt not repainting the view on macOS after the changes. - # See https://tickets.metabrainz.org/browse/PICARD-1654 - self.ui.vboxlayout.parentWidget().repaint() def login(self): self.tagger.mb_login(self.on_login_finished, self) @@ -151,8 +158,8 @@ class GeneralOptionsPage(OptionsPage): super().restore_defaults() self.logout() - def on_login_finished(self, successful): - self.update_login_logout() + def on_login_finished(self, successful, error_msg=None): + self.update_login_logout(error_msg) def logout(self): self.tagger.mb_logout() diff --git a/picard/ui/ui_options_general.py b/picard/ui/ui_options_general.py index ad10dfd79..6d7819c50 100644 --- a/picard/ui/ui_options_general.py +++ b/picard/ui/ui_options_general.py @@ -59,21 +59,29 @@ class Ui_GeneralOptionsPage(object): self.vboxlayout.addWidget(self.groupBox) self.rename_files_2 = QtWidgets.QGroupBox(GeneralOptionsPage) self.rename_files_2.setObjectName("rename_files_2") - self.gridlayout1 = QtWidgets.QGridLayout(self.rename_files_2) - self.gridlayout1.setSpacing(2) - self.gridlayout1.setObjectName("gridlayout1") - self.login = QtWidgets.QPushButton(self.rename_files_2) - self.login.setObjectName("login") - self.gridlayout1.addWidget(self.login, 1, 0, 1, 1) - spacerItem1 = QtWidgets.QSpacerItem(40, 20, QtWidgets.QSizePolicy.Expanding, QtWidgets.QSizePolicy.Minimum) - self.gridlayout1.addItem(spacerItem1, 1, 2, 1, 1) - self.logout = QtWidgets.QPushButton(self.rename_files_2) - self.logout.setObjectName("logout") - self.gridlayout1.addWidget(self.logout, 1, 1, 1, 1) + self.verticalLayout_3 = QtWidgets.QVBoxLayout(self.rename_files_2) + self.verticalLayout_3.setSpacing(2) + self.verticalLayout_3.setObjectName("verticalLayout_3") + self.login_error = QtWidgets.QLabel(self.rename_files_2) + self.login_error.setText("") + self.login_error.setObjectName("login_error") + self.verticalLayout_3.addWidget(self.login_error) self.logged_in = QtWidgets.QLabel(self.rename_files_2) self.logged_in.setText("") self.logged_in.setObjectName("logged_in") - self.gridlayout1.addWidget(self.logged_in, 0, 0, 1, 3) + self.verticalLayout_3.addWidget(self.logged_in) + self.horizontalLayout = QtWidgets.QHBoxLayout() + self.horizontalLayout.setSpacing(6) + self.horizontalLayout.setObjectName("horizontalLayout") + self.login = QtWidgets.QPushButton(self.rename_files_2) + self.login.setObjectName("login") + self.horizontalLayout.addWidget(self.login) + self.logout = QtWidgets.QPushButton(self.rename_files_2) + self.logout.setObjectName("logout") + self.horizontalLayout.addWidget(self.logout) + spacerItem1 = QtWidgets.QSpacerItem(40, 20, QtWidgets.QSizePolicy.Expanding, QtWidgets.QSizePolicy.Minimum) + self.horizontalLayout.addItem(spacerItem1) + self.verticalLayout_3.addLayout(self.horizontalLayout) self.vboxlayout.addWidget(self.rename_files_2) self.groupBox_2 = QtWidgets.QGroupBox(GeneralOptionsPage) self.groupBox_2.setObjectName("groupBox_2") @@ -148,9 +156,7 @@ class Ui_GeneralOptionsPage(object): QtCore.QMetaObject.connectSlotsByName(GeneralOptionsPage) GeneralOptionsPage.setTabOrder(self.server_host, self.server_port) GeneralOptionsPage.setTabOrder(self.server_port, self.use_server_for_submission) - GeneralOptionsPage.setTabOrder(self.use_server_for_submission, self.login) - GeneralOptionsPage.setTabOrder(self.login, self.logout) - GeneralOptionsPage.setTabOrder(self.logout, self.analyze_new_files) + GeneralOptionsPage.setTabOrder(self.use_server_for_submission, self.analyze_new_files) GeneralOptionsPage.setTabOrder(self.analyze_new_files, self.ignore_file_mbids) GeneralOptionsPage.setTabOrder(self.ignore_file_mbids, self.check_for_updates) GeneralOptionsPage.setTabOrder(self.check_for_updates, self.update_check_days) diff --git a/ui/options_general.ui b/ui/options_general.ui index b910edb89..665f28c55 100644 --- a/ui/options_general.ui +++ b/ui/options_general.ui @@ -114,44 +114,58 @@ QCheckBox { color: black } MusicBrainz Account - + 2 - - + + - Log in + - - - - Qt::Horizontal - - - - 40 - 20 - - - - - - - - Log out - - - - + + + + + 6 + + + + + Log in + + + + + + + Log out + + + + + + + Qt::Horizontal + + + + 40 + 20 + + + + + + @@ -292,8 +306,6 @@ QCheckBox { color: black } server_host server_port use_server_for_submission - login - logout analyze_new_files ignore_file_mbids check_for_updates