Tightened up validation and error handling of server configuration in the desktop app.

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:

vasl_templates/ui/server_settings.ui

@@ -66,7 +66,7 @@
         <widget class="QLineEdit" name="java_path"/>
        </item>
        <item>
-        <widget class="QPushButton" name="select_java_button">
+        <widget class="QPushButton" name="select_java_path_button">
          <property name="sizePolicy">
           <sizepolicy hsizetype="Fixed" vsizetype="Fixed">
            <horstretch>0</horstretch>
@@ -111,7 +111,7 @@
         <widget class="QLineEdit" name="webdriver_path"/>
        </item>
        <item>
-        <widget class="QPushButton" name="select_webdriver_button">
+        <widget class="QPushButton" name="select_webdriver_path_button">
          <property name="minimumSize">
           <size>
            <width>22</width>
@@ -582,9 +582,9 @@
   <tabstop>user_files_dir</tabstop>
   <tabstop>select_user_files_dir_button</tabstop>
   <tabstop>java_path</tabstop>
-  <tabstop>select_java_button</tabstop>
+  <tabstop>select_java_path_button</tabstop>
   <tabstop>webdriver_path</tabstop>
-  <tabstop>select_webdriver_button</tabstop>
+  <tabstop>select_webdriver_path_button</tabstop>
   <tabstop>ok_button</tabstop>
   <tabstop>cancel_button</tabstop>
  </tabstops>