From c7347a013f6ce67b66f612381fd48b14835298a7 Mon Sep 17 00:00:00 2001
From: Daniel McKnight <daniel@neongecko.com>
Date: Wed, 5 Jul 2023 13:56:27 -0700
Subject: [PATCH 01/13] Refactors `GUIInterface` to send `page_names`
 containing GUI resource IDs Adds unit tests for `GUIInterface` methods and
 updates docstrings

---
 ovos_utils/gui.py          | 122 +++++++++++++++----------
 test/unittests/test_gui.py | 178 ++++++++++++++++++++++++++++++++++---
 2 files changed, 239 insertions(+), 61 deletions(-)

diff --git a/ovos_utils/gui.py b/ovos_utils/gui.py
index 857a9cb1..d9b8fbe2 100644
--- a/ovos_utils/gui.py
+++ b/ovos_utils/gui.py
@@ -1,10 +1,10 @@
 from os import walk
-from typing import List, Union, Optional, Callable, Any
+from typing import List, Union, Optional, Callable
 
 import time
 from collections import namedtuple
 from enum import IntEnum
-from os.path import join
+from os.path import join, splitext, isfile
 
 from ovos_utils import resolve_ovos_resource_file, resolve_resource_file
 from ovos_utils.log import LOG, log_deprecation
@@ -488,7 +488,8 @@ def __setitem__(self, key, value):
 
 
 class GUIInterface:
-    """Interface to the Graphical User Interface, allows interaction with
+    """
+    Interface to the Graphical User Interface, allows interaction with
     the mycroft-gui from anywhere
 
     Values set in this class are synced to the GUI, accessible within QML
@@ -500,7 +501,7 @@ class GUIInterface:
         text: sessionData.time
     """
 
-    def __init__(self, skill_id: str, bus = None,
+    def __init__(self, skill_id: str, bus=None,
                  remote_server: str = None, config: dict = None,
                  ui_directories: dict = None):
         """
@@ -511,6 +512,8 @@ def __init__(self, skill_id: str, bus = None,
         @param remote_server: Optional URL of a remote GUI server
         @param config: dict gui Configuration
         @param ui_directories: dict framework to directory containing resources
+            `all` key should reference a `gui` directory containing all
+            specific resource subdirectories
         """
         if not config:
             log_deprecation(f"Expected a dict config and got None.", "0.1.0")
@@ -525,7 +528,7 @@ def __init__(self, skill_id: str, bus = None,
             self.config["remote-server"] = remote_server
         self._bus = bus
         self.__session_data = {}  # synced to GUI for use by this skill's pages
-        self.pages = []
+        self._pages = []
         self.current_page_idx = -1
         self._skill_id = skill_id
         self.on_gui_changed_callback = None
@@ -574,7 +577,7 @@ def page(self) -> Optional[str]:
         """
         Return the active GUI page (file path) to show
         """
-        return self.pages[self.current_page_idx] if len(self.pages) else None
+        return self._pages[self.current_page_idx] if len(self._pages) else None
 
     @property
     def connected(self) -> bool:
@@ -586,6 +589,13 @@ def connected(self) -> bool:
             return False
         return can_use_gui(self.bus)
 
+    @property
+    def pages(self) -> List[str]:
+        """
+        Get a list of the active page ID's managed by this interface 
+        """
+        return self._pages
+
     def build_message_type(self, event: str) -> str:
         """
         Ensure the specified event prepends this interface's `skill_id`
@@ -717,7 +727,7 @@ def clear(self):
         the `release` method.
         """
         self.__session_data = {}
-        self.pages = []
+        self._pages = []
         self.current_page_idx = -1
         if not self.bus:
             raise RuntimeError("bus not set, did you call self.bind() ?")
@@ -743,7 +753,13 @@ def send_event(self, event_name: str,
                                "params": params}))
 
     def _pages2uri(self, page_names: List[str]) -> List[str]:
-        # Convert pages to full reference
+        """
+        Get a list of resolved URIs from a list of string page names.
+        @param page_names: List of GUI resource names (file basenames) to locate
+        @return: List of resolved paths to the requested pages
+        """
+        # TODO: This method resolves absolute file paths. These will no longer
+        #       be used with the implementation of `ovos-gui`
         page_urls = []
         extra_dirs = list(self.ui_directories.values()) or list()
         for name in page_names:
@@ -765,21 +781,35 @@ def _pages2uri(self, page_names: List[str]) -> List[str]:
         LOG.debug(f"Resolved pages: {page_urls}")
         return page_urls
 
+    @staticmethod
+    def _normalize_page_name(page_name: str) -> str:
+        """
+        Normalize a requested GUI resource
+        @param page_name:
+        @return:
+        """
+        if isfile(page_name):
+            LOG.warning("GUI resources should specify a resource name and not "
+                        "a file path. This may cause unexpected behavior.")
+            return page_name
+        file, ext = splitext(page_name)
+        if ext == ".qml":
+            log_deprecation("GUI resources should exclude gui-specific file "
+                            "extensions. In the future, this may cause "
+                            "unexpected behavior.", "0.1.0")
+            return file
+
+        return page_name
+
     # base gui interactions
     def show_page(self, name: str, override_idle: Union[bool, int] = None,
                   override_animations: bool = False):
         """
-        Begin showing the page in the GUI
-
-        Arguments:
-            name (str): Name of page (e.g "mypage.qml") to display
-            override_idle (boolean, int):
-                True: Takes over the resting page indefinitely
-                (int): Delays resting page for the specified number of
-                       seconds.
-            override_animations (boolean):
-                True: Disables showing all platform skill animations.
-                False: 'Default' always show animations.
+        Request to show a page in the GUI.
+        @param name: page resource requested
+        @param override_idle: number of seconds to override display for;
+            if True, override display indefinitely
+        @param override_animations: if True, disables all GUI animations
         """
         self.show_pages([name], 0, override_idle, override_animations)
 
@@ -787,20 +817,12 @@ def show_pages(self, page_names: List[str], index: int = 0,
                    override_idle: Union[bool, int] = None,
                    override_animations: bool = False):
         """
-        Begin showing the list of pages in the GUI.
-
-        Arguments:
-            page_names (list): List of page names (str) to display, such as
-                               ["Weather.qml", "Forecast.qml", "Details.qml"]
-            index (int): Page number (0-based) to show initially.  For the
-                         above list a value of 1 would start on "Forecast.qml"
-            override_idle (boolean, int):
-                True: Takes over the resting page indefinitely
-                (int): Delays resting page for the specified number of
-                       seconds.
-            override_animations (boolean):
-                True: Disables showing all platform skill animations.
-                False: 'Default' always show animations.
+        Request to show a list of pages in the GUI.
+        @param page_names: list of page resources requested
+        @param index: position to insert pages at (default 0)
+        @param override_idle: number of seconds to override display for;
+            if True, override display indefinitely
+        @param override_animations: if True, disables all GUI animations
         """
         if not self.bus:
             raise RuntimeError("bus not set, did you call self.bind() ?")
@@ -813,7 +835,11 @@ def show_pages(self, page_names: List[str], index: int = 0,
             LOG.error('Default index is larger than page list length')
             index = len(page_names) - 1
 
-        self.pages = page_names
+        # TODO: deprecate sending page_urls after ovos_gui implementation
+        page_urls = self._pages2uri(page_names)
+        page_names = [self._normalize_page_name(n) for n in page_names]
+
+        self._pages = page_names
         self.current_page_idx = index
 
         # First sync any data...
@@ -822,39 +848,39 @@ def show_pages(self, page_names: List[str], index: int = 0,
         LOG.debug(f"Updating gui data: {data}")
         self.bus.emit(Message("gui.value.set", data))
 
-        page_urls = self._pages2uri(page_names)
-
         # finally tell gui what to show
         self.bus.emit(Message("gui.page.show",
                               {"page": page_urls,
+                               "page_names": page_names,
                                "index": index,
                                "__from": self.skill_id,
                                "__idle": override_idle,
                                "__animations": override_animations}))
 
     def remove_page(self, page: str):
-        """Remove a single page from the GUI.
-
-        Arguments:
-            page (str): Page to remove from the GUI
+        """
+        Remove a single page from the GUI.
+        @param page: Name of page to remove
         """
         self.remove_pages([page])
 
     def remove_pages(self, page_names: List[str]):
         """
-        Remove a list of pages in the GUI.
-
-        Arguments:
-            page_names (list): List of page names (str) to display, such as
-                               ["Weather.qml", "Forecast.qml", "Other.qml"]
+        Request to remove a list of pages from the GUI.
+        @param page_names: list of page resources requested
         """
         if not self.bus:
             raise RuntimeError("bus not set, did you call self.bind() ?")
-        if not isinstance(page_names, list):
+        if isinstance(page_names, str):
             page_names = [page_names]
+        if not isinstance(page_names, list):
+            raise ValueError('page_names must be a list')
+        # TODO: deprecate sending page_urls after ovos_gui implementation
         page_urls = self._pages2uri(page_names)
+        page_names = [self._normalize_page_name(n) for n in page_names]
         self.bus.emit(Message("gui.page.delete",
                               {"page": page_urls,
+                               "page_names": page_names,
                                "__from": self.skill_id}))
 
     # Utils / Templates
@@ -1091,8 +1117,8 @@ def remove_input_box(self):
         """
         Remove an input box shown by `show_input_box`
         """
-        LOG.info(f"GUI pages length {len(self.pages)}")
-        if len(self.pages) > 1:
+        LOG.info(f"GUI pages length {len(self._pages)}")
+        if len(self._pages) > 1:
             self.remove_page("SYSTEM_InputBox.qml")
         else:
             self.release()
diff --git a/test/unittests/test_gui.py b/test/unittests/test_gui.py
index 10ffbd3b..3eb11328 100644
--- a/test/unittests/test_gui.py
+++ b/test/unittests/test_gui.py
@@ -1,7 +1,7 @@
 import unittest
-from os.path import join, dirname
+from os.path import join, dirname, isfile
 from threading import Event
-from unittest.mock import patch, call
+from unittest.mock import patch, call, Mock
 
 from ovos_bus_client.message import Message
 
@@ -171,25 +171,177 @@ def test_send_event(self):
         # TODO
         pass
 
-    def test_pages2uri(self):
-        # TODO
-        pass
+    @patch('ovos_utils.gui.resolve_resource_file')
+    @patch('ovos_utils.gui.resolve_ovos_resource_file')
+    def test_pages2uri(self, ovos_res, res):
+        def _resolve(name, config):
+            self.assertEqual(config, self.interface.config)
+            if name.startswith("ui/core"):
+                return f"res/{name}"
+
+        def _ovos_resolve(name, extra_dirs):
+            self.assertEqual(extra_dirs,
+                             list(self.interface.ui_directories.values()))
+            if name.startswith("ui/ovos"):
+                return f"ovos/{name}"
+
+        # Mock actual resource resolution methods
+        ovos_res.side_effect = _ovos_resolve
+        res.side_effect = _resolve
+
+        # remote_url is None
+        # OVOS Res
+        self.assertEqual(self.interface._pages2uri(["ui/ovos/test"]),
+                         ["file://ovos/ui/ovos/test"])
+        ovos_res.assert_called_once()
+        self.assertEqual(self.interface._pages2uri(["ovos/test"]),
+                         ["file://ovos/ui/ovos/test"])
+        res.assert_not_called()
+        # Core Res
+        self.assertEqual(self.interface._pages2uri(["ui/core/test"]),
+                         ["file://res/ui/core/test"])
+        res.assert_called_once()
+        self.assertEqual(self.interface._pages2uri(["core/test"]),
+                         ["file://res/ui/core/test"])
+
+    def test_normalize_page_name(self):
+        legacy_name = "test.qml"
+        name_with_path = "subdir/test"
+        name_with_dot = "subdir/test.file"
+        self.assertEqual(self.interface._normalize_page_name(legacy_name),
+                         "test")
+        self.assertEqual(self.interface._normalize_page_name(name_with_path),
+                         "subdir/test")
+        self.assertEqual(self.interface._normalize_page_name(name_with_dot),
+                         "subdir/test.file")
 
     def test_show_page(self):
-        # TODO
-        pass
+        real_show_pages = self.interface.show_pages
+        self.interface.show_pages = Mock()
+
+        # Default args
+        self.interface.show_page("test")
+        self.interface.show_pages.assert_called_once_with(["test"], 0,
+                                                          None, False)
+
+        self.interface.show_page("test2", True, True)
+        self.interface.show_pages.assert_called_with(["test2"], 0,
+                                                     True, True)
+
+        self.interface.show_page("test3", 30, True)
+        self.interface.show_pages.assert_called_with(["test3"], 0,
+                                                     30, True)
+        self.interface.show_pages = real_show_pages
 
     def test_show_pages(self):
-        # TODO
-        pass
+        msg: Message = Message("")
+        handled = Event()
+
+        def _gui_value_set(message):
+            self.assertEqual(message.data['__from'], self.interface.skill_id)
+
+        def _gui_page_show(message):
+            nonlocal msg
+            msg = message
+            handled.set()
+
+        self.bus.on('gui.value.set', _gui_value_set)
+        self.bus.on('gui.page.show', _gui_page_show)
+
+        # Test resource absolute paths
+        file_base_dir = join(dirname(__file__), "test_ui", "ui")
+        files = [join(file_base_dir, "test.qml"),
+                 join(file_base_dir, "subdir", "test.qml")]
+        self.interface.show_pages(files)
+        self.assertTrue(handled.wait(2))
+        self.assertEqual(msg.msg_type, "gui.page.show")
+        for page in msg.data['page']:
+            self.assertTrue(page.startswith("file://"))
+            path = page.replace("file://", "")
+            self.assertTrue(isfile(path), page)
+        self.assertEqual(len(msg.data['page']), len(msg.data['page_names']))
+        self.assertIsInstance(msg.data["index"], int)
+        self.assertEqual(msg.data['__from'], self.interface.skill_id)
+        self.assertIsNone(msg.data["__idle"])
+        self.assertIsInstance(msg.data["__animations"], bool)
+
+        # Test resources resolved locally
+        handled.clear()
+        files = ["file.qml", "subdir/file.qml"]
+        index = 1
+        override_idle = 30
+        override_animations = True
+        self.interface.show_pages(files, index, override_idle,
+                                  override_animations)
+        self.assertTrue(handled.wait(2))
+        self.assertEqual(msg.msg_type, "gui.page.show")
+        for page in msg.data['page']:
+            self.assertTrue(page.startswith("file://"))
+            path = page.replace("file://", "")
+            self.assertTrue(isfile(path), page)
+        self.assertEqual(msg.data["page_names"], ["file", "subdir/file"])
+        self.assertEqual(msg.data["index"], index)
+        self.assertEqual(msg.data["__from"], self.interface.skill_id)
+        self.assertEqual(msg.data["__idle"], override_idle)
+        self.assertEqual(msg.data["__animations"], override_animations)
+
+        # Test resources not resolved locally
+        handled.clear()
+        files = ["file.qml", "other.page"]
+        index = 1
+        override_idle = 30
+        override_animations = True
+        self.interface.show_pages(files, index, override_idle,
+                                  override_animations)
+        self.assertTrue(handled.wait(2))
+        self.assertEqual(msg.msg_type, "gui.page.show")
+        self.assertEqual(msg.data["page"], list())
+        self.assertEqual(msg.data["page_names"], ["file", "other.page"])
+        self.assertEqual(msg.data["index"], index)
+        self.assertEqual(msg.data["__from"], self.interface.skill_id)
+        self.assertEqual(msg.data["__idle"], override_idle)
+        self.assertEqual(msg.data["__animations"], override_animations)
 
     def test_remove_page(self):
-        # TODO
-        pass
+        real_remove_pages = self.interface.remove_pages
+        self.interface.remove_pages = Mock()
+        self.interface.remove_page("test_page")
+        self.interface.remove_pages.assert_called_once_with(["test_page"])
+        self.interface.remove_pages = real_remove_pages
 
     def test_remove_pages(self):
-        # TODO
-        pass
+        msg = Message("")
+        handled = Event()
+
+        def _gui_page_delete(message):
+            nonlocal msg
+            msg = message
+            handled.set()
+
+        self.bus.on("gui.page.delete", _gui_page_delete)
+
+        # Test resolved page
+        pages = ["test.qml"]
+        self.interface.remove_pages(pages)
+        self.assertTrue(handled.wait(2))
+        self.assertEqual(msg.msg_type, "gui.page.delete")
+        self.assertEqual(len(msg.data['page']), len(pages))
+        for page in msg.data['page']:
+            self.assertTrue(page.startswith("file://"))
+            path = page.replace("file://", "")
+            self.assertTrue(isfile(path), page)
+        self.assertEqual(msg.data['page_names'], ["test"])
+        self.assertEqual(msg.data['__from'], self.interface.skill_id)
+
+        # Test unresolved pages
+        handled.clear()
+        pages = ['file.qml', 'dir/other.file']
+        self.interface.remove_pages(pages)
+        self.assertTrue(handled.wait(2))
+        self.assertEqual(msg.msg_type, "gui.page.delete")
+        self.assertEqual(msg.data['page'], [])
+        self.assertEqual(msg.data['page_names'], ["file", "dir/other.file"])
+        self.assertEqual(msg.data['__from'], self.interface.skill_id)
 
     def test_show_notification(self):
         # TODO

From f7f7ba31b9a540ee62d7f4ff2cb064d7c5b12b69 Mon Sep 17 00:00:00 2001
From: Daniel McKnight <daniel@neongecko.com>
Date: Wed, 5 Jul 2023 14:51:14 -0700
Subject: [PATCH 02/13] Update logged deprecation warning

---
 ovos_utils/gui.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/ovos_utils/gui.py b/ovos_utils/gui.py
index d9b8fbe2..82cd8664 100644
--- a/ovos_utils/gui.py
+++ b/ovos_utils/gui.py
@@ -795,8 +795,8 @@ def _normalize_page_name(page_name: str) -> str:
         file, ext = splitext(page_name)
         if ext == ".qml":
             log_deprecation("GUI resources should exclude gui-specific file "
-                            "extensions. In the future, this may cause "
-                            "unexpected behavior.", "0.1.0")
+                            f"extensions. This call should probably pass "
+                            f"`{file}`, instead of `{page_name}`", "0.1.0")
             return file
 
         return page_name

From c2c60ce4875cd236f5d8cd618d80843c89b61d73 Mon Sep 17 00:00:00 2001
From: Daniel McKnight <daniel@neongecko.com>
Date: Wed, 5 Jul 2023 15:16:54 -0700
Subject: [PATCH 03/13] Upload GUI files as encoded bytes to support arbitrary
 file formats Catch and handle errors in GUI file upload

---
 ovos_utils/gui.py | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/ovos_utils/gui.py b/ovos_utils/gui.py
index 82cd8664..e20690cc 100644
--- a/ovos_utils/gui.py
+++ b/ovos_utils/gui.py
@@ -632,12 +632,15 @@ def upload_gui_pages(self, message: Message):
         res_dir = self.ui_directories[request_res_type]
         for path, _, files in walk(res_dir):
             for file in files:
-                full_path: str = join(path, file)
-                rel_path = full_path.replace(f"{res_dir}/", "", 1)
-                fname = join(self.skill_id, rel_path)
-                with open(full_path, 'r') as f:
-                    pages[fname] = f.read()
-
+                try:
+                    full_path: str = join(path, file)
+                    rel_path = full_path.replace(f"{res_dir}/", "", 1)
+                    fname = join(self.skill_id, rel_path)
+                    with open(full_path, 'rb') as f:
+                        file_bytes = f.read()
+                    pages[fname] = file_bytes.hex()
+                except Exception as e:
+                    LOG.exception(f"{file} not uploaded: {e}")
         self.bus.emit(message.forward("gui.page.upload",
                                       {"__from": self.skill_id,
                                        "framework": request_res_type,

From 7d509213249adf1391efd75fc509474c9f58e7a8 Mon Sep 17 00:00:00 2001
From: Daniel McKnight <daniel@neongecko.com>
Date: Wed, 5 Jul 2023 15:24:44 -0700
Subject: [PATCH 04/13] Update unit tests to handle GUI files as bytes instead
 of text

---
 test/unittests/test_gui.py | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/test/unittests/test_gui.py b/test/unittests/test_gui.py
index 3eb11328..b05b3fbc 100644
--- a/test/unittests/test_gui.py
+++ b/test/unittests/test_gui.py
@@ -113,7 +113,7 @@ def test_setup_default_handlers(self):
         pass
 
     def test_upload_gui_pages(self):
-        msg = None
+        msg = Message("")
         handled = Event()
 
         def on_pages(message):
@@ -137,10 +137,12 @@ def on_pages(message):
             self.assertIsInstance(val, str)
 
         test_file_key = join(self.iface_name, "test.qml")
-        self.assertEqual(pages.get(test_file_key), "Mock File Contents", pages)
+        self.assertEqual(bytes.fromhex(pages.get(test_file_key)),
+                         b"Mock File Contents", pages)
 
         test_file_key = join(self.iface_name, "subdir", "test.qml")
-        self.assertEqual(pages.get(test_file_key), "Nested Mock", pages)
+        self.assertEqual(bytes.fromhex(pages.get(test_file_key)),
+                         b"Nested Mock", pages)
         # TODO: Test other frameworks
 
     def test_register_handler(self):

From 7c83c1a73f857ee4baee4b86d2920b6508d8c8b8 Mon Sep 17 00:00:00 2001
From: Daniel McKnight <daniel@neongecko.com>
Date: Wed, 5 Jul 2023 16:49:25 -0700
Subject: [PATCH 05/13] Update logging and comments

---
 ovos_utils/gui.py | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/ovos_utils/gui.py b/ovos_utils/gui.py
index e20690cc..577b6016 100644
--- a/ovos_utils/gui.py
+++ b/ovos_utils/gui.py
@@ -624,10 +624,12 @@ def upload_gui_pages(self, message: Message):
             LOG.debug("No UI resources to upload")
             return
         request_res_type = message.data.get("framework", "qt5")
+        # Note that ui_directory "all" is a special case that will upload all
+        # gui files, including all framework subdirectories
         if request_res_type not in self.ui_directories:
             LOG.warning(f"Requested UI files not available: {request_res_type}")
             return
-
+        LOG.debug(f"Requested upload resources for: {request_res_type}")
         pages = dict()
         res_dir = self.ui_directories[request_res_type]
         for path, _, files in walk(res_dir):

From b41318a2f517e6991dad7353df4e9686047d7dda Mon Sep 17 00:00:00 2001
From: Daniel McKnight <daniel@neongecko.com>
Date: Wed, 5 Jul 2023 17:02:41 -0700
Subject: [PATCH 06/13] Update GUI to send resource directories for GUI module
 resource resolution

---
 ovos_utils/gui.py          | 1 +
 test/unittests/test_gui.py | 3 +++
 2 files changed, 4 insertions(+)

diff --git a/ovos_utils/gui.py b/ovos_utils/gui.py
index 577b6016..9a163502 100644
--- a/ovos_utils/gui.py
+++ b/ovos_utils/gui.py
@@ -857,6 +857,7 @@ def show_pages(self, page_names: List[str], index: int = 0,
         self.bus.emit(Message("gui.page.show",
                               {"page": page_urls,
                                "page_names": page_names,
+                               "skill_paths": self.ui_directories,
                                "index": index,
                                "__from": self.skill_id,
                                "__idle": override_idle,
diff --git a/test/unittests/test_gui.py b/test/unittests/test_gui.py
index b05b3fbc..dc73ba63 100644
--- a/test/unittests/test_gui.py
+++ b/test/unittests/test_gui.py
@@ -266,6 +266,7 @@ def _gui_page_show(message):
         self.assertEqual(msg.data['__from'], self.interface.skill_id)
         self.assertIsNone(msg.data["__idle"])
         self.assertIsInstance(msg.data["__animations"], bool)
+        self.assertEqual(msg.data["skill_paths"], self.interface.ui_directories)
 
         # Test resources resolved locally
         handled.clear()
@@ -286,6 +287,7 @@ def _gui_page_show(message):
         self.assertEqual(msg.data["__from"], self.interface.skill_id)
         self.assertEqual(msg.data["__idle"], override_idle)
         self.assertEqual(msg.data["__animations"], override_animations)
+        self.assertEqual(msg.data["skill_paths"], self.interface.ui_directories)
 
         # Test resources not resolved locally
         handled.clear()
@@ -303,6 +305,7 @@ def _gui_page_show(message):
         self.assertEqual(msg.data["__from"], self.interface.skill_id)
         self.assertEqual(msg.data["__idle"], override_idle)
         self.assertEqual(msg.data["__animations"], override_animations)
+        self.assertEqual(msg.data["skill_paths"], self.interface.ui_directories)
 
     def test_remove_page(self):
         real_remove_pages = self.interface.remove_pages

From a03441548a311eaeb9a507cf28d33caf61acfe09 Mon Sep 17 00:00:00 2001
From: Daniel McKnight <daniel@neongecko.com>
Date: Wed, 5 Jul 2023 17:19:38 -0700
Subject: [PATCH 07/13] Refactor `skill_paths` to `ui_directories` since a GUI
 interface isn't necessarily a skill

---
 ovos_utils/gui.py          | 2 +-
 test/unittests/test_gui.py | 9 ++++++---
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/ovos_utils/gui.py b/ovos_utils/gui.py
index 9a163502..f2f4aec6 100644
--- a/ovos_utils/gui.py
+++ b/ovos_utils/gui.py
@@ -857,7 +857,7 @@ def show_pages(self, page_names: List[str], index: int = 0,
         self.bus.emit(Message("gui.page.show",
                               {"page": page_urls,
                                "page_names": page_names,
-                               "skill_paths": self.ui_directories,
+                               "ui_directories": self.ui_directories,
                                "index": index,
                                "__from": self.skill_id,
                                "__idle": override_idle,
diff --git a/test/unittests/test_gui.py b/test/unittests/test_gui.py
index dc73ba63..97c4299f 100644
--- a/test/unittests/test_gui.py
+++ b/test/unittests/test_gui.py
@@ -266,7 +266,8 @@ def _gui_page_show(message):
         self.assertEqual(msg.data['__from'], self.interface.skill_id)
         self.assertIsNone(msg.data["__idle"])
         self.assertIsInstance(msg.data["__animations"], bool)
-        self.assertEqual(msg.data["skill_paths"], self.interface.ui_directories)
+        self.assertEqual(msg.data["ui_directories"],
+                         self.interface.ui_directories)
 
         # Test resources resolved locally
         handled.clear()
@@ -287,7 +288,8 @@ def _gui_page_show(message):
         self.assertEqual(msg.data["__from"], self.interface.skill_id)
         self.assertEqual(msg.data["__idle"], override_idle)
         self.assertEqual(msg.data["__animations"], override_animations)
-        self.assertEqual(msg.data["skill_paths"], self.interface.ui_directories)
+        self.assertEqual(msg.data["ui_directories"],
+                         self.interface.ui_directories)
 
         # Test resources not resolved locally
         handled.clear()
@@ -305,7 +307,8 @@ def _gui_page_show(message):
         self.assertEqual(msg.data["__from"], self.interface.skill_id)
         self.assertEqual(msg.data["__idle"], override_idle)
         self.assertEqual(msg.data["__animations"], override_animations)
-        self.assertEqual(msg.data["skill_paths"], self.interface.ui_directories)
+        self.assertEqual(msg.data["ui_directories"],
+                         self.interface.ui_directories)
 
     def test_remove_page(self):
         real_remove_pages = self.interface.remove_pages

From 63f072c117a94da472e9291c8d66936b548848b9 Mon Sep 17 00:00:00 2001
From: Daniel McKnight <daniel@neongecko.com>
Date: Wed, 5 Jul 2023 19:12:32 -0700
Subject: [PATCH 08/13] Refactor page upload to remove extra `skill_id` in path

---
 ovos_utils/gui.py | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/ovos_utils/gui.py b/ovos_utils/gui.py
index f2f4aec6..d05061f1 100644
--- a/ovos_utils/gui.py
+++ b/ovos_utils/gui.py
@@ -636,11 +636,10 @@ def upload_gui_pages(self, message: Message):
             for file in files:
                 try:
                     full_path: str = join(path, file)
-                    rel_path = full_path.replace(f"{res_dir}/", "", 1)
-                    fname = join(self.skill_id, rel_path)
+                    page_name = full_path.replace(f"{res_dir}/", "", 1)
                     with open(full_path, 'rb') as f:
                         file_bytes = f.read()
-                    pages[fname] = file_bytes.hex()
+                    pages[page_name] = file_bytes.hex()
                 except Exception as e:
                     LOG.exception(f"{file} not uploaded: {e}")
         self.bus.emit(message.forward("gui.page.upload",

From fc4ee91d16f38f49761bd527950da273cbcbc7f5 Mon Sep 17 00:00:00 2001
From: Daniel McKnight <daniel@neongecko.com>
Date: Wed, 5 Jul 2023 19:19:31 -0700
Subject: [PATCH 09/13] Add comment about file naming

---
 ovos_utils/gui.py | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/ovos_utils/gui.py b/ovos_utils/gui.py
index d05061f1..d11c4592 100644
--- a/ovos_utils/gui.py
+++ b/ovos_utils/gui.py
@@ -631,6 +631,8 @@ def upload_gui_pages(self, message: Message):
             return
         LOG.debug(f"Requested upload resources for: {request_res_type}")
         pages = dict()
+        # `pages` keys are unique identifiers in the scope of this interface;
+        # if ui_directory is "all", then pages are prefixed with `<framework>/`
         res_dir = self.ui_directories[request_res_type]
         for path, _, files in walk(res_dir):
             for file in files:

From 85e2988dd798a9fb44c7d935ca938c9389c7bf2f Mon Sep 17 00:00:00 2001
From: Daniel McKnight <daniel@neongecko.com>
Date: Thu, 6 Jul 2023 08:43:53 -0700
Subject: [PATCH 10/13] Update unit tests to include case for uploading all GUI
 resource files

---
 ovos_utils/gui.py                       | 13 +++++-----
 test/unittests/test_gui.py              | 32 +++++++++++++++++++++----
 test/unittests/test_ui/gui/qt5/test.qml |  1 +
 test/unittests/test_ui/gui/qt6/test.qml |  1 +
 4 files changed, 37 insertions(+), 10 deletions(-)
 create mode 100644 test/unittests/test_ui/gui/qt5/test.qml
 create mode 100644 test/unittests/test_ui/gui/qt6/test.qml

diff --git a/ovos_utils/gui.py b/ovos_utils/gui.py
index d11c4592..9e3bf003 100644
--- a/ovos_utils/gui.py
+++ b/ovos_utils/gui.py
@@ -616,8 +616,8 @@ def setup_default_handlers(self):
 
     def upload_gui_pages(self, message: Message):
         """
-        Emit a response Message with all known GUI resources managed by
-        this interface.
+        Emit a response Message with all known GUI files managed by
+        this interface for the requested infrastructure
         @param message: `gui.request_page_upload` Message requesting pages
         """
         if not self.ui_directories:
@@ -644,6 +644,7 @@ def upload_gui_pages(self, message: Message):
                     pages[page_name] = file_bytes.hex()
                 except Exception as e:
                     LOG.exception(f"{file} not uploaded: {e}")
+        # Note that `pages` in this context include file extensions
         self.bus.emit(message.forward("gui.page.upload",
                                       {"__from": self.skill_id,
                                        "framework": request_res_type,
@@ -791,12 +792,12 @@ def _pages2uri(self, page_names: List[str]) -> List[str]:
     def _normalize_page_name(page_name: str) -> str:
         """
         Normalize a requested GUI resource
-        @param page_name:
-        @return:
+        @param page_name: string name of a GUI resource
+        @return: normalized string name (`.qml` removed for other GUI support)
         """
         if isfile(page_name):
-            LOG.warning("GUI resources should specify a resource name and not "
-                        "a file path. This may cause unexpected behavior.")
+            log_deprecation("GUI resources should specify a resource name and "
+                            "not a file path.", "0.1.0")
             return page_name
         file, ext = splitext(page_name)
         if ext == ".qml":
diff --git a/test/unittests/test_gui.py b/test/unittests/test_gui.py
index 97c4299f..36d88290 100644
--- a/test/unittests/test_gui.py
+++ b/test/unittests/test_gui.py
@@ -121,7 +121,9 @@ def on_pages(message):
             msg = message
             handled.set()
 
-        self.bus.once('gui.page.upload', on_pages)
+        self.bus.on('gui.page.upload', on_pages)
+
+        # Upload default/legacy behavior (qt5 `ui` dir)
         message = Message('test', {}, {'context': "Test"})
         self.interface.upload_gui_pages(message)
         self.assertTrue(handled.wait(10))
@@ -136,14 +138,36 @@ def on_pages(message):
             self.assertIsInstance(key, str)
             self.assertIsInstance(val, str)
 
-        test_file_key = join(self.iface_name, "test.qml")
+        test_file_key = "test.qml"
         self.assertEqual(bytes.fromhex(pages.get(test_file_key)),
                          b"Mock File Contents", pages)
 
-        test_file_key = join(self.iface_name, "subdir", "test.qml")
+        test_file_key = join("subdir", "test.qml")
         self.assertEqual(bytes.fromhex(pages.get(test_file_key)),
                          b"Nested Mock", pages)
-        # TODO: Test other frameworks
+
+        # Upload all resources
+        handled.clear()
+        self.interface.ui_directories['all'] = join(dirname(__file__),
+                                                    'test_ui', 'gui')
+        message = Message('test', {"framework": "all"}, {'context': "All"})
+        self.interface.upload_gui_pages(message)
+        self.assertTrue(handled.wait(10))
+
+        self.assertEqual(msg.context['context'], message.context['context'])
+        self.assertEqual(msg.msg_type, "gui.page.upload")
+        self.assertEqual(msg.data['__from'], self.iface_name)
+
+        pages = msg.data['pages']
+        self.assertIsInstance(pages, dict)
+        for key, val in pages.items():
+            self.assertIsInstance(key, str)
+            self.assertIsInstance(val, str)
+
+        self.assertEqual(bytes.fromhex(pages.get("qt5/test.qml")),
+                         b"qt5", pages)
+        self.assertEqual(bytes.fromhex(pages.get("qt6/test.qml")),
+                         b"qt6", pages)
 
     def test_register_handler(self):
         # TODO
diff --git a/test/unittests/test_ui/gui/qt5/test.qml b/test/unittests/test_ui/gui/qt5/test.qml
new file mode 100644
index 00000000..75793eb6
--- /dev/null
+++ b/test/unittests/test_ui/gui/qt5/test.qml
@@ -0,0 +1 @@
+qt5
\ No newline at end of file
diff --git a/test/unittests/test_ui/gui/qt6/test.qml b/test/unittests/test_ui/gui/qt6/test.qml
new file mode 100644
index 00000000..3a5476f2
--- /dev/null
+++ b/test/unittests/test_ui/gui/qt6/test.qml
@@ -0,0 +1 @@
+qt6
\ No newline at end of file

From fce76e7535bc5d996e680a74e210f3ffcebe7ca6 Mon Sep 17 00:00:00 2001
From: Daniel McKnight <daniel@neongecko.com>
Date: Thu, 6 Jul 2023 09:06:36 -0700
Subject: [PATCH 11/13] Update `page` property docstring

---
 ovos_utils/gui.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ovos_utils/gui.py b/ovos_utils/gui.py
index 9e3bf003..7e6af181 100644
--- a/ovos_utils/gui.py
+++ b/ovos_utils/gui.py
@@ -575,7 +575,7 @@ def skill_id(self, val: str):
     @property
     def page(self) -> Optional[str]:
         """
-        Return the active GUI page (file path) to show
+        Return the active GUI page name to show
         """
         return self._pages[self.current_page_idx] if len(self._pages) else None
 

From 62e4cca996a5d69713086b122746af87aebdc68d Mon Sep 17 00:00:00 2001
From: Daniel McKnight <daniel@neongecko.com>
Date: Thu, 6 Jul 2023 09:37:31 -0700
Subject: [PATCH 12/13] Move `get_ui_directories` method from workshop to `gui`
 module Add unit tests for `get_ui_directories` Update default GUI path to
 support available `all` directory

---
 ovos_utils/gui.py                          | 28 ++++++++++++++++++++--
 test/unittests/test_gui.py                 | 13 ++++++++++
 test/unittests/test_ui/legacy/ui/test.qml  |  1 +
 test/unittests/test_ui/legacy/ui6/test.qml |  1 +
 4 files changed, 41 insertions(+), 2 deletions(-)
 create mode 100644 test/unittests/test_ui/legacy/ui/test.qml
 create mode 100644 test/unittests/test_ui/legacy/ui6/test.qml

diff --git a/ovos_utils/gui.py b/ovos_utils/gui.py
index 7e6af181..c8480e49 100644
--- a/ovos_utils/gui.py
+++ b/ovos_utils/gui.py
@@ -4,7 +4,7 @@
 import time
 from collections import namedtuple
 from enum import IntEnum
-from os.path import join, splitext, isfile
+from os.path import join, splitext, isfile, isdir
 
 from ovos_utils import resolve_ovos_resource_file, resolve_resource_file
 from ovos_utils.log import LOG, log_deprecation
@@ -79,6 +79,29 @@ def can_use_gui(bus=None,
     return can_use_local_gui() or is_gui_connected(bus)
 
 
+def get_ui_directories(root_dir: str) -> dict:
+    """
+    Get a dict of available UI directories by GUI framework.
+    @param root_dir: base directory to inspect for available UI directories
+    @return: Dict of framework name to UI resource directory
+    """
+    ui_directories = dict()
+    base_directory = root_dir
+    if isdir(join(base_directory, "gui")):
+        LOG.debug("Skill implements resources in `gui` directory")
+        ui_directories["all"] = join(base_directory, "gui")
+        return ui_directories
+    LOG.info("Checking for legacy UI directories")
+    if isdir(join(base_directory, "ui5")):
+        ui_directories["qt5"] = join(base_directory, "ui5")
+    if isdir(join(base_directory, "ui6")):
+        ui_directories["qt6"] = join(base_directory, "ui6")
+    if isdir(join(base_directory, "ui")) and "qt5" not in ui_directories:
+        LOG.debug("Handling `ui` directory as `qt5`")
+        ui_directories["qt5"] = join(base_directory, "ui")
+    return ui_directories
+
+
 def extend_about_data(about_data: Union[list, dict],
                       bus=None):
     """
@@ -623,7 +646,8 @@ def upload_gui_pages(self, message: Message):
         if not self.ui_directories:
             LOG.debug("No UI resources to upload")
             return
-        request_res_type = message.data.get("framework", "qt5")
+        request_res_type = message.data.get("framework") or "all" if "all" in \
+            self.ui_directories else "qt5"
         # Note that ui_directory "all" is a special case that will upload all
         # gui files, including all framework subdirectories
         if request_res_type not in self.ui_directories:
diff --git a/test/unittests/test_gui.py b/test/unittests/test_gui.py
index 36d88290..b1972def 100644
--- a/test/unittests/test_gui.py
+++ b/test/unittests/test_gui.py
@@ -79,6 +79,19 @@ def test_gui_dict(self):
         from ovos_utils.gui import _GUIDict
         # TODO
 
+    def test_get_ui_directories(self):
+        from ovos_utils.gui import get_ui_directories
+        test_dir = join(dirname(__file__), "test_ui")
+
+        # gui dir (best practice)
+        dirs = get_ui_directories(test_dir)
+        self.assertEqual(dirs, {"all": join(test_dir, "gui")})
+
+        # ui and uid dirs (legacy)
+        dirs = get_ui_directories(join(test_dir, "legacy"))
+        self.assertEqual(dirs, {"qt5": join(test_dir, "legacy", "ui"),
+                                "qt6": join(test_dir, "legacy", "ui6")})
+
 
 class TestGuiInterface(unittest.TestCase):
     from ovos_utils.messagebus import FakeBus
diff --git a/test/unittests/test_ui/legacy/ui/test.qml b/test/unittests/test_ui/legacy/ui/test.qml
new file mode 100644
index 00000000..75793eb6
--- /dev/null
+++ b/test/unittests/test_ui/legacy/ui/test.qml
@@ -0,0 +1 @@
+qt5
\ No newline at end of file
diff --git a/test/unittests/test_ui/legacy/ui6/test.qml b/test/unittests/test_ui/legacy/ui6/test.qml
new file mode 100644
index 00000000..3a5476f2
--- /dev/null
+++ b/test/unittests/test_ui/legacy/ui6/test.qml
@@ -0,0 +1 @@
+qt6
\ No newline at end of file

From 78de0c0541290ac48e8dec627319db2cce63c4a3 Mon Sep 17 00:00:00 2001
From: Daniel McKnight <daniel@neongecko.com>
Date: Thu, 6 Jul 2023 09:59:38 -0700
Subject: [PATCH 13/13] Drop `ui5`/`ui6` directory checks for paths that were
 never supported

---
 ovos_utils/gui.py                          | 6 +-----
 test/unittests/test_gui.py                 | 3 +--
 test/unittests/test_ui/legacy/ui6/test.qml | 1 -
 3 files changed, 2 insertions(+), 8 deletions(-)
 delete mode 100644 test/unittests/test_ui/legacy/ui6/test.qml

diff --git a/ovos_utils/gui.py b/ovos_utils/gui.py
index c8480e49..9e484753 100644
--- a/ovos_utils/gui.py
+++ b/ovos_utils/gui.py
@@ -92,11 +92,7 @@ def get_ui_directories(root_dir: str) -> dict:
         ui_directories["all"] = join(base_directory, "gui")
         return ui_directories
     LOG.info("Checking for legacy UI directories")
-    if isdir(join(base_directory, "ui5")):
-        ui_directories["qt5"] = join(base_directory, "ui5")
-    if isdir(join(base_directory, "ui6")):
-        ui_directories["qt6"] = join(base_directory, "ui6")
-    if isdir(join(base_directory, "ui")) and "qt5" not in ui_directories:
+    if isdir(join(base_directory, "ui")):
         LOG.debug("Handling `ui` directory as `qt5`")
         ui_directories["qt5"] = join(base_directory, "ui")
     return ui_directories
diff --git a/test/unittests/test_gui.py b/test/unittests/test_gui.py
index b1972def..fc384d02 100644
--- a/test/unittests/test_gui.py
+++ b/test/unittests/test_gui.py
@@ -89,8 +89,7 @@ def test_get_ui_directories(self):
 
         # ui and uid dirs (legacy)
         dirs = get_ui_directories(join(test_dir, "legacy"))
-        self.assertEqual(dirs, {"qt5": join(test_dir, "legacy", "ui"),
-                                "qt6": join(test_dir, "legacy", "ui6")})
+        self.assertEqual(dirs, {"qt5": join(test_dir, "legacy", "ui")})
 
 
 class TestGuiInterface(unittest.TestCase):
diff --git a/test/unittests/test_ui/legacy/ui6/test.qml b/test/unittests/test_ui/legacy/ui6/test.qml
deleted file mode 100644
index 3a5476f2..00000000
--- a/test/unittests/test_ui/legacy/ui6/test.qml
+++ /dev/null
@@ -1 +0,0 @@
-qt6
\ No newline at end of file