Refactor/settings cleanup

mycroft/skills/settings.py

@@ -113,28 +113,6 @@ def get_display_name(skill_name: str):
     return camel_case_split(skill_name)
 
 
-def _extract_settings_from_meta(settings_meta: dict) -> dict:
-    """Extract the skill setting name/value pairs from settingsmeta.json
-
-    Args:
-        settings_meta: contents of the settingsmeta.json
-
-    Returns:
-        Dictionary of settings keyed by name
-    """
-    fields = {}
-    try:
-        sections = settings_meta['skillMetadata']['sections']
-    except KeyError:
-        pass
-    else:
-        for section in sections:
-            for field in section.get('fields', []):
-                fields[field['name']] = field['value']
-
-    return fields
-
-
 class SettingsMetaUploader:
     """Synchronize the contents of the settingsmeta.json file with the backend.
 
@@ -171,10 +149,9 @@ def msm(self):
         return self._msm
 
     def get_local_skills(self):
-        return {
-            skill.path: skill for skill in
-            self.msm.local_skills.values()
-        }
+        """Generate a mapping of skill path to skill name for all local skills.
+        """
+        return {skill.path: skill for skill in self.msm.local_skills.values()}
 
     @property
     def skill_gid(self):
@@ -260,7 +237,7 @@ def upload(self):
             self.upload_timer.start()
 
     def stop(self):
-        """ Stop upload attempts if Timer is running."""
+        """Stop upload attempts if Timer is running."""
         if self.upload_timer:
             self.upload_timer.cancel()
         # Set stopped flag if upload is running when stop is called.
@@ -321,20 +298,17 @@ def _issue_api_call(self):
 
 
 class SkillSettingsDownloader:
-    """Manages the contents of the settings.json file.
+    """Manages download of skill settings.
 
-    The settings.json file contains a set of name/value pairs representing
-    the values of the settings defined in settingsmeta.json
+    Performs settings download on a repeating Timer. If a change is seen
+    the data is sent to the relevant skill.
     """
 
     def __init__(self, bus):
         self.bus = bus
         self.continue_downloading = True
-        self.changed_callback = None
-        self.settings_meta_fields = None
         self.last_download_result = {}
         self.remote_settings = None
-        self.settings_changed = False
         self.api = DeviceApi()
         self.download_timer = None
 
@@ -348,15 +322,13 @@ def stop_downloading(self):
     def download(self):
         """Download the settings stored on the backend and check for changes"""
         if is_paired():
-            download_success = self._get_remote_settings()
-            if download_success:
-                self.settings_changed = (
-                    self.last_download_result != self.remote_settings
-                )
-                if self.settings_changed:
+            remote_settings = self._get_remote_settings()
+            if remote_settings:
+                settings_changed = self.last_download_result != remote_settings
+                if settings_changed:
                     LOG.debug('Skill settings changed since last download')
-                    self._emit_settings_change_events()
-                    self.last_download_result = self.remote_settings
+                    self._emit_settings_change_events(remote_settings)
+                    self.last_download_result = remote_settings
                 else:
                     LOG.debug('No skill settings changes since last download')
         else:
@@ -376,37 +348,32 @@ def _get_remote_settings(self):
         """Get the settings for this skill from the server
 
         Returns:
-            skill_settings (dict or None): returns a dict if matches
+            skill_settings (dict or None): returns a dict on success, else None
         """
         try:
             remote_settings = self.api.get_skill_settings()
         except Exception:
             LOG.exception('Failed to download remote settings from server.')
-            success = False
-        else:
-            self.remote_settings = remote_settings
-            success = True
+            remote_settings = None
 
-        return success
+        return remote_settings
 
-    def _emit_settings_change_events(self):
-        for skill_gid, remote_settings in self.remote_settings.items():
+    def _emit_settings_change_events(self, remote_settings):
+        """Emit changed settings events for each affected skill."""
+        for skill_gid, skill_settings in remote_settings.items():
             settings_changed = False
             try:
-                previous_settings = self.last_download_result[skill_gid]
-            except KeyError:
-                if remote_settings:
-                    settings_changed = True
+                previous_settings = self.last_download_result.get(skill_gid)
             except Exception:
                 LOG.exception('error occurred handling setting change events')
             else:
-                if previous_settings != remote_settings:
+                if previous_settings != skill_settings:
                     settings_changed = True
             if settings_changed:
                 log_msg = 'Emitting skill.settings.change event for skill {} '
                 LOG.info(log_msg.format(skill_gid))
                 message = Message(
                     'mycroft.skills.settings.changed',
-                    data={skill_gid: remote_settings}
+                    data={skill_gid: skill_settings}
                 )
                 self.bus.emit(message)

test/unittests/skills/test_settings.py

@@ -235,6 +235,16 @@ def test_download_failed(self):
             self.downloader.last_download_result
         )
 
+    def test_stop_downloading(self):
+        """Ensure that the timer is cancelled and the continue flag is lowered.
+        """
+        self.is_paired_mock.return_value = False  # Skip all the download logic
+        self.downloader.download()  # Start downloading creates the timer
+        self.downloader.stop_downloading()
+        self.assertFalse(self.downloader.continue_downloading)
+        self.assertTrue(
+            self.downloader.download_timer.cancel.called_once_with())
+
     def _check_api_called(self):
         self.assertListEqual(
             [call.get_skill_settings()],