From 5d34c52b66b013ecadfe6cf4a42623674a998f56 Mon Sep 17 00:00:00 2001 From: Taka Date: Tue, 7 May 2019 16:16:15 +0000 Subject: [PATCH] Tightened up validation and error handling of server configuration in the desktop app. --- vasl_templates/server_settings.py | 306 +++++++++++++-------------- vasl_templates/ui/server_settings.ui | 8 +- 2 files changed, 148 insertions(+), 166 deletions(-) diff --git a/vasl_templates/server_settings.py b/vasl_templates/server_settings.py index 9ce224a..8c85976 100644 --- a/vasl_templates/server_settings.py +++ b/vasl_templates/server_settings.py @@ -1,6 +1,7 @@ """Implement the "server settings" dialog.""" import os +import shutil import logging import traceback @@ -17,8 +18,24 @@ from vasl_templates.webapp.utils import MsgStore # --------------------------------------------------------------------- +_EXE_FSPEC = [ "Executable files (*.exe)" ] if os.name == "nt" else [] + +SERVER_SETTINGS = { + "vassal-dir": { "type": "dir", "name": "VASSAL directory" }, + "vasl-mod": { "type": "file", "name": "VASL module", "fspec": ["VASL module files (*.vmod)"] }, + "vasl-extns-dir": { "type": "dir", "name": "VASL extensions directory" }, + "boards-dir": { "type": "dir", "name": "VASL boards directory" }, + "java-path": { "type": "file", "name": "Java executable", "allow_on_path": True, "fspec": _EXE_FSPEC }, + "webdriver-path": { "type": "file", "name": "webdriver", "allow_on_path": True, "fspec": _EXE_FSPEC }, + "chapter-h-notes-dir": { "type": "dir", "name": "Chapter H notes directory" }, + "chapter-h-image-scaling": { "type": "int", "name": "Chapter H image scaling" }, + "user-files-dir": { "type": "dir", "name": "user files directory" }, +} + +# --------------------------------------------------------------------- + class ServerSettingsDialog( QDialog ): - """Let the user manage the server settings.""" + """Let the user configure the server settings.""" def __init__( self, parent ) : @@ -29,30 +46,40 @@ class ServerSettingsDialog( QDialog ): base_dir = os.path.split( __file__ )[0] dname = os.path.join( base_dir, "ui/server_settings.ui" ) uic.loadUi( dname, self ) - for btn in ["vassal_dir", "vasl_mod", "vasl_extns_dir", "boards_dir", - "java", "webdriver", - "chapter_h_notes_dir", "user_files_dir" - ]: - getattr( self, "select_{}_button".format(btn) ).setIcon( - QIcon( os.path.join( base_dir, "resources/file_browser.png" ) ) - ) self.setFixedSize( self.size() ) + # initialize the UI + for key in SERVER_SETTINGS: + btn = getattr( self, "select_{}_button".format( key.replace("-","_") ), None ) + if btn: + btn.setIcon( QIcon( os.path.join( base_dir, "resources/file_browser.png" ) ) ) + self.vassal_dir.setToolTip( "Supported versions: {}".format( SUPPORTED_VASSAL_VERSIONS_DISPLAY ) ) + self.vasl_mod.setToolTip( "Supported versions: {}".format( SUPPORTED_VASL_MOD_VERSIONS_DISPLAY ) ) + self.webdriver_path.setToolTip( "Configure either geckodriver or chromedriver here." ) + # initialize the UI for attr in dir(self): attr = getattr( self, attr ) if isinstance( attr, QGroupBox ): - attr.setStyleSheet("QGroupBox { font-weight: bold; } ") - - # initialize handlers - self.select_vassal_dir_button.clicked.connect( self.on_select_vassal_dir ) - self.select_vasl_mod_button.clicked.connect( self.on_select_vasl_mod ) - self.select_vasl_extns_dir_button.clicked.connect( self.on_select_vasl_extns_dir ) - self.select_boards_dir_button.clicked.connect( self.on_select_boards_dir ) - self.select_java_button.clicked.connect( self.on_select_java ) - self.select_webdriver_button.clicked.connect( self.on_select_webdriver ) - self.select_chapter_h_notes_dir_button.clicked.connect( self.on_select_chapter_h_notes_dir ) - self.select_user_files_dir_button.clicked.connect( self.on_select_user_files_dir ) + attr.setStyleSheet( "QGroupBox { font-weight: bold; } " ) + + # initialize click handlers + def make_click_handler( func, *args ): #pylint: disable=missing-docstring + # FUDGE! Python looks up variables passed in to a lambda when it is *invoked*, so we need + # this intermediate function to create lambda's with their arguments at *creation time*. + return lambda: func( *args ) + for key,vals in SERVER_SETTINGS.items(): + key2 = key.replace( "-", "_" ) + btn = getattr( self, "select_{}_button".format( key2 ), None ) + if btn: + ctrl = self._get_control( key ) + if vals["type"] == "dir": + func = make_click_handler( self._on_select_dir, ctrl, vals["name"] ) + elif vals["type"] == "file": + func = make_click_handler( self._on_select_file, ctrl, vals["name"], vals["fspec"] ) + else: + assert False + btn.clicked.connect( func ) self.ok_button.clicked.connect( self.on_ok ) self.cancel_button.clicked.connect( self.on_cancel ) @@ -60,160 +87,88 @@ class ServerSettingsDialog( QDialog ): self.chapter_h_notes_dir.textChanged.connect( self.on_chapter_h_notes_dir_changed ) # load the current server settings - self.vassal_dir.setText( app_settings.value( "ServerSettings/vassal-dir" ) ) - self.vassal_dir.setToolTip( - "Supported versions: {}".format( SUPPORTED_VASSAL_VERSIONS_DISPLAY ) - ) - self.vasl_mod.setText( app_settings.value( "ServerSettings/vasl-mod" ) ) - self.vasl_mod.setToolTip( - "Supported versions: {}".format( SUPPORTED_VASL_MOD_VERSIONS_DISPLAY ) - ) - self.vasl_extns_dir.setText( app_settings.value( "ServerSettings/vasl-extns-dir" ) ) - self.boards_dir.setText( app_settings.value( "ServerSettings/boards-dir" ) ) - self.java_path.setText( app_settings.value( "ServerSettings/java-path" ) ) - self.webdriver_path.setText( app_settings.value( "ServerSettings/webdriver-path" ) ) - self.webdriver_path.setToolTip( "Configure either geckodriver or chromedriver here." ) - self.chapter_h_notes_dir.setText( app_settings.value( "ServerSettings/chapter-h-notes-dir" ) ) - scaling = app_settings.value( "ServerSettings/chapter-h-image-scaling" ) - if scaling: - self.chapter_h_image_scaling.setText( str( scaling ) ) - self.user_files_dir.setText( app_settings.value( "ServerSettings/user-files-dir" ) ) - - def on_select_vassal_dir( self ): - """Let the user locate the VASSAL installation directory.""" - dname = QFileDialog.getExistingDirectory( - self, "Select VASSAL installation directory", - self.vassal_dir.text(), - QFileDialog.ShowDirsOnly - ) - if dname: - self.vassal_dir.setText( dname ) - - def on_select_vasl_mod( self ): - """Let the user select a VASL module.""" - fname = QFileDialog.getOpenFileName( - self, "Select VASL module", - self.vasl_mod.text(), - "VASL module files (*.vmod);;All files (*.*)" - )[0] - if fname: - self.vasl_mod.setText( fname ) - - def on_select_vasl_extns_dir( self ): - """Let the user locate the VASL extensions directory.""" - dname = QFileDialog.getExistingDirectory( - self, "Select VASL extensions directory", - self.vasl_extns_dir.text(), - QFileDialog.ShowDirsOnly - ) - if dname: - self.vasl_extns_dir.setText( dname ) + for key in SERVER_SETTINGS: + val = app_settings.value( "ServerSettings/"+key ) or "" + ctrl = self._get_control( key ) + ctrl.setText( str(val).strip() ) - def on_select_boards_dir( self ): - """Let the user locate the VASL boards directory.""" + def _on_select_dir( self, ctrl, name ): + """Ask the user to select a directory.""" dname = QFileDialog.getExistingDirectory( - self, "Select VASL boards directory", - self.boards_dir.text(), + self, "Select {}".format( name ), + ctrl.text(), QFileDialog.ShowDirsOnly ) if dname: - self.boards_dir.setText( dname ) - - def on_select_java( self ): - """Let the user locate the Java executable.""" - fname = QFileDialog.getOpenFileName( - self, "Select Java executable", - self.java_path.text(), - _make_exe_filter_string() - )[0] - if fname: - self.java_path.setText( fname ) + ctrl.setText( dname ) - def on_select_webdriver( self ): - """Let the user locate the webdriver executable.""" + def _on_select_file( self, ctrl, name, fspec ): + """Ask the user to select a file.""" + assert isinstance( fspec, list ) + fspec = fspec[:] + fspec.append( "All files ({})".format( "*.*" if os.name == "nt" else "*" ) ) fname = QFileDialog.getOpenFileName( - self, "Select webdriver executable", - self.webdriver_path.text(), - _make_exe_filter_string() + self, "Select {}".format( name ), + ctrl.text(), + ";;".join( fspec ) )[0] if fname: - self.webdriver_path.setText( fname ) - - def on_select_chapter_h_notes_dir( self ): - """Let the user locate their Chapter H notes directory.""" - dname = QFileDialog.getExistingDirectory( - self, "Select Chapter H notes directory", - self.chapter_h_notes_dir.text(), - QFileDialog.ShowDirsOnly - ) - if dname: - self.chapter_h_notes_dir.setText( dname ) - - def on_select_user_files_dir( self ): - """Let the user locate their user files directory.""" - dname = QFileDialog.getExistingDirectory( - self, "Select user files directory", - self.user_files_dir.text(), - QFileDialog.ShowDirsOnly - ) - if dname: - self.user_files_dir.setText( dname ) + ctrl.setText( fname ) def on_ok( self ): """Accept the new server settings.""" - # unload the dialog - try: - chapter_h_image_scaling = self.chapter_h_image_scaling.text().strip() - if chapter_h_image_scaling: - chapter_h_image_scaling = int( self.chapter_h_image_scaling.text() ) - except ValueError: - MainWindow.showErrorMsg( "Image scaling must be a numeric percentage value." ) - self.chapter_h_image_scaling.setFocus() - return - - # save the current values for key settings - KEY_SETTINGS = { - "vassal-dir": "VASSAL directory", - "vasl-mod": "VASL module", - "vasl-extns-dir": "VASL extensions directory", - "chapter-h-notes-dir": "Chapter H directory", - } - prev_vals = { - k: app_settings.value( "ServerSettings/"+k, "" ) - for k in KEY_SETTINGS + # save a copy of the current settings + prev_settings = { + key: app_settings.value( "ServerSettings/"+key, "" ) + for key in SERVER_SETTINGS } - # save the new settings - app_settings.setValue( "ServerSettings/vassal-dir", self.vassal_dir.text().strip() ) - app_settings.setValue( "ServerSettings/vasl-mod", self.vasl_mod.text().strip() ) - app_settings.setValue( "ServerSettings/vasl-extns-dir", self.vasl_extns_dir.text().strip() ) - app_settings.setValue( "ServerSettings/boards-dir", self.boards_dir.text().strip() ) - app_settings.setValue( "ServerSettings/java-path", self.java_path.text().strip() ) - app_settings.setValue( "ServerSettings/webdriver-path", self.webdriver_path.text().strip() ) - app_settings.setValue( "ServerSettings/chapter-h-notes-dir", self.chapter_h_notes_dir.text().strip() ) - app_settings.setValue( "ServerSettings/chapter-h-image-scaling", chapter_h_image_scaling ) - app_settings.setValue( "ServerSettings/user-files-dir", self.user_files_dir.text().strip() ) + # unload the dialog + # NOTE: Typing an unknown path into QFileDialog.getExistingDirectory() causes that directory + # to be created!?!? It doesn't really matter, since the user could have also manually typed + # an unknown path into an edit box, so we need to validate everything anyway. + new_settings = {} + for key, vals in SERVER_SETTINGS.items(): + ctrl = self._get_control( key ) + func = getattr( self, "_unload_"+vals["type"] ) + args, kwargs = [ vals["name"] ], {} + if "allow_on_path" in vals: + kwargs[ "allow_on_path" ] = vals["allow_on_path"] + val = func( ctrl, *args, **kwargs ) + if val is None: + # nb: something failed validation, an error message has already been shown + return + new_settings[ key ] = val # install the new settings - # NOTE: We should really do this before saving the new settings, but that's more trouble - # than it's worth at this stage... :-/ + for key in SERVER_SETTINGS: + app_settings.setValue( "ServerSettings/"+key, new_settings[key] ) try: install_server_settings( False ) except Exception as ex: #pylint: disable=broad-except logging.error( traceback.format_exc() ) MainWindow.showErrorMsg( "Couldn't install the server settings:\n\n{}".format( ex ) ) + # rollback the changes + for key,val in prev_settings.items(): + app_settings.setValue( "ServerSettings/"+key, val ) + try: + install_server_settings( False ) + except Exception as ex: #pylint: disable=broad-except + logging.error( traceback.format_exc() ) + MainWindow.showErrorMsg( "Couldn't rollback the server settings:\n\n{}".format( ex ) ) return self.close() # check if any key settings were changed + KEY_SETTINGS = [ "vassal-dir", "vasl-mod", "vasl-extns-dir", "chapter-h-notes-dir" ] changed = [ - k for k in KEY_SETTINGS - if app_settings.value( "ServerSettings/"+k, "" ) != prev_vals[k] + key for key in KEY_SETTINGS + if app_settings.value( "ServerSettings/"+key, "" ) != prev_settings[key] ] if len(changed) == 1: MainWindow.showInfoMsg( "The {} was changed - you should restart the program.".format( - KEY_SETTINGS[changed[0]] + SERVER_SETTINGS[ changed[0] ][ "name" ] ) ) elif len(changed) > 1: MainWindow.showInfoMsg( "Some key settings were changed - you should restart the program." ) @@ -222,7 +177,7 @@ class ServerSettingsDialog( QDialog ): """Cancel the dialog.""" self.close() - def update_ui( self ): + def _update_ui( self ): """Update the UI.""" rc = self.chapter_h_notes_dir.text().strip() != "" self.chapter_h_image_scaling_label.setEnabled( rc ) @@ -231,15 +186,48 @@ class ServerSettingsDialog( QDialog ): def on_chapter_h_notes_dir_changed( self, val ): #pylint: disable=unused-argument """Called when the Chapter H notes directory is changed.""" - self.update_ui() - -def _make_exe_filter_string(): - """Make a file filter string for executables.""" - buf = [] - if os.name == "nt": - buf.append( "Executable files (*.exe)" ) - buf.append( "All files (*.*)" ) - return ";;".join( buf ) + self._update_ui() + + @staticmethod + def _unload_dir( ctrl, name ): + """Unload and validate a directory path.""" + dname = ctrl.text().strip() + if dname and not os.path.isdir( dname ): + MainWindow.showErrorMsg( "Can't find the {}:\n {}".format( name, dname ) ) + ctrl.setFocus() + return None + return dname + + @staticmethod + def _unload_file( ctrl, name, allow_on_path=False ): + """Unload and validate a file path.""" + fname = ctrl.text().strip() + def is_valid( fname ): #pylint: disable=missing-docstring + if not os.path.isabs(fname) and allow_on_path: + return shutil.which( fname ) is not None + return os.path.isfile( fname ) + if fname and not is_valid(fname): + if not os.path.isabs(fname) and allow_on_path: + MainWindow.showErrorMsg( "Can't find the {} on the PATH:\n {}".format( name, fname ) ) + else: + MainWindow.showErrorMsg( "Can't find the {}:\n {}".format( name, fname ) ) + ctrl.setFocus() + return None + return fname + + @staticmethod + def _unload_int( ctrl, name ): + """Unload and validate an integer value.""" + val = ctrl.text().strip() + if val and not val.isdigit(): + MainWindow.showErrorMsg( "{} must be a numeric value.".format( name ) ) + ctrl.setFocus() + return None + return val + + def _get_control( self, key ): + """Return the UI control for the specified server setting.""" + return getattr( self, key.replace("-","_") ) # --------------------------------------------------------------------- @@ -248,15 +236,9 @@ def install_server_settings( is_startup ): # install the server settings from vasl_templates.webapp import app as app - app.config[ "VASSAL_DIR" ] = app_settings.value( "ServerSettings/vassal-dir" ) - app.config[ "VASL_MOD" ] = app_settings.value( "ServerSettings/vasl-mod" ) - app.config[ "VASL_EXTNS_DIR" ] = app_settings.value( "ServerSettings/vasl-extns-dir" ) - app.config[ "BOARDS_DIR" ] = app_settings.value( "ServerSettings/boards-dir" ) - app.config[ "JAVA_PATH" ] = app_settings.value( "ServerSettings/java-path" ) - app.config[ "WEBDRIVER_PATH" ] = app_settings.value( "ServerSettings/webdriver-path" ) - app.config[ "CHAPTER_H_NOTES_DIR" ] = app_settings.value( "ServerSettings/chapter-h-notes-dir" ) - app.config[ "CHAPTER_H_IMAGE_SCALING" ] = app_settings.value( "ServerSettings/chapter-h-image-scaling" ) - app.config[ "USER_FILES_DIR" ] = app_settings.value( "ServerSettings/user-files-dir" ) + for key in SERVER_SETTINGS: + key2 = key.replace( "-", "_" ).upper() + app.config[ key2 ] = app_settings.value( "ServerSettings/"+key ) # initialize if is_startup: diff --git a/vasl_templates/ui/server_settings.ui b/vasl_templates/ui/server_settings.ui index 8dbea45..70b45bf 100644 --- a/vasl_templates/ui/server_settings.ui +++ b/vasl_templates/ui/server_settings.ui @@ -66,7 +66,7 @@ - + 0 @@ -111,7 +111,7 @@ - + 22 @@ -582,9 +582,9 @@ user_files_dir select_user_files_dir_button java_path - select_java_button + select_java_path_button webdriver_path - select_webdriver_button + select_webdriver_path_button ok_button cancel_button