From 25c2d811274937c39a358a5dad250c7aa06a6546 Mon Sep 17 00:00:00 2001 From: Darsh Date: Mon, 14 Apr 2025 13:00:30 -0400 Subject: [PATCH 1/4] Updates to enable config Service --- .../widgets/helpwindow/helpwindowbridge.py | 36 +-- .../widgets/helpwindow/helpwindowmodel.py | 229 +++++++++--------- .../widgets/helpwindow/helpwindowpresenter.py | 105 ++++---- .../widgets/helpwindow/helpwindowview.py | 9 +- .../helpwindow/test/test_helpwindowmodel.py | 175 +++++++------ 5 files changed, 294 insertions(+), 260 deletions(-) diff --git a/qt/python/mantidqt/mantidqt/widgets/helpwindow/helpwindowbridge.py b/qt/python/mantidqt/mantidqt/widgets/helpwindow/helpwindowbridge.py index 547959c9f106..7b5b02ee708b 100644 --- a/qt/python/mantidqt/mantidqt/widgets/helpwindow/helpwindowbridge.py +++ b/qt/python/mantidqt/mantidqt/widgets/helpwindow/helpwindowbridge.py @@ -1,12 +1,8 @@ -# Mantid Repository : https://github.com/mantidproject/mantid -# # Copyright © 2017 ISIS Rutherford Appleton Laboratory UKRI, # NScD Oak Ridge National Laboratory, European Spallation Source, # Institut Laue - Langevin & CSNS, Institute of High Energy Physics, CAS # SPDX - License - Identifier: GPL - 3.0 + -import os import sys -import argparse from qtpy.QtWidgets import QApplication from mantidqt.widgets.helpwindow.helpwindowpresenter import HelpWindowPresenter @@ -14,43 +10,53 @@ _presenter = None -def show_help_page(relativeUrl, localDocs=None, onlineBaseUrl="https://docs.mantidproject.org/"): +def show_help_page(relativeUrl, onlineBaseUrl="https://docs.mantidproject.org/"): """ Show the help window at the given relative URL path. + Local docs path is now determined internally via ConfigService. """ global _presenter if _presenter is None: - # Create a Presenter once. Re-use it on subsequent calls. - _presenter = HelpWindowPresenter(localDocs=localDocs, onlineBaseUrl=onlineBaseUrl) + _presenter = HelpWindowPresenter(onlineBaseUrl=onlineBaseUrl) - # Ask the Presenter to load the requested page _presenter.show_help_page(relativeUrl) def main(cmdargs=sys.argv): """ Run this script standalone to test the Python-based Help Window. + Local docs path is determined from Mantid's ConfigService. """ + import argparse + parser = argparse.ArgumentParser(description="Standalone test of the Python-based Mantid Help Window.") parser.add_argument( "relativeUrl", nargs="?", default="", help="Relative doc path (e.g. 'algorithms/Load-v1.html'), defaults to 'index.html' if empty." ) - parser.add_argument("--local-docs", default=None, help="Path to local Mantid HTML docs. Overrides environment if set.") + parser.add_argument( "--online-base-url", default="https://docs.mantidproject.org/", - help="Base URL for online docs if local docs are not set or invalid.", + help="Base URL for online docs if local docs path from config is invalid or not found.", ) args = parser.parse_args(cmdargs or sys.argv[1:]) - # If user gave no --local-docs, fall back to environment - if args.local_docs is None: - args.local_docs = os.environ.get("MANTID_LOCAL_DOCS_BASE", None) + try: + import mantid.kernel + + log = mantid.kernel.Logger("HelpWindowBridge") + log.information("Mantid kernel imported successfully.") + except ImportError as e: + print(f"ERROR: Failed to import Mantid Kernel: {e}", file=sys.stderr) + print( + "Ensure Mantid is built and PYTHONPATH is set correctly (e.g., export PYTHONPATH=/path/to/mantid/build/bin:$PYTHONPATH)", + file=sys.stderr, + ) + sys.exit(1) app = QApplication(sys.argv) - # Show the requested help page - show_help_page(relativeUrl=args.relativeUrl, localDocs=args.local_docs, onlineBaseUrl=args.online_base_url) + show_help_page(relativeUrl=args.relativeUrl, onlineBaseUrl=args.online_base_url) sys.exit(app.exec_()) diff --git a/qt/python/mantidqt/mantidqt/widgets/helpwindow/helpwindowmodel.py b/qt/python/mantidqt/mantidqt/widgets/helpwindow/helpwindowmodel.py index b6ec805a7c84..7035ac1561cb 100644 --- a/qt/python/mantidqt/mantidqt/widgets/helpwindow/helpwindowmodel.py +++ b/qt/python/mantidqt/mantidqt/widgets/helpwindow/helpwindowmodel.py @@ -1,170 +1,171 @@ -# Mantid Repository : https://github.com/mantidproject/mantid -# # Copyright © 2017 ISIS Rutherford Appleton Laboratory UKRI, # NScD Oak Ridge National Laboratory, European Spallation Source, # Institut Laue - Langevin & CSNS, Institute of High Energy Physics, CAS # SPDX - License - Identifier: GPL - 3.0 + import os -import logging -from qtpy.QtCore import QUrl -from qtpy.QtWebEngineCore import QWebEngineUrlRequestInterceptor, QWebEngineUrlRequestInfo -# Module-level logger for functions outside of classes -_logger = logging.getLogger(__name__) +try: + from mantid.kernel import Logger + from mantid.kernel import ConfigService +except ImportError: + print("Warning: Mantid Kernel (Logger/ConfigService) not found, using basic print/dummy.") -def _get_version_string_for_url(): - """ - Returns the Mantid version string formatted for use in documentation URLs. - For example, "v6.12.0" from version "6.12.0.1" + class Logger: + def __init__(self, name): + self._name = name - Returns: - str: Formatted version string in the form "vX.Y.Z" or None if version cannot be determined - """ - versionStr = None + def warning(self, msg): + print(f"WARNING [{self._name}]: {msg}") + + def debug(self, msg): + print(f"DEBUG [{self._name}]: {msg}") + + def information(self, msg): + print(f"INFO [{self._name}]: {msg}") + + def error(self, msg): + print(f"ERROR [{self._name}]: {msg}") + + class ConfigService: + @staticmethod + def Instance(): + class DummyInstance: + def getString(self, key, pathAbsolute=True): + return None # Default to None + + return DummyInstance() + + +log = Logger("HelpWindowModel") + +from qtpy.QtCore import QUrl # noqa: E402 +from qtpy.QtWebEngineCore import QWebEngineUrlRequestInterceptor, QWebEngineUrlRequestInfo # noqa: E402 + + +def getMantidVersionString(): + """Placeholder function to get Mantid version (e.g., 'v6.13.0').""" try: import mantid - # Use the mantid version object (proper way) - versionObj = mantid.version() - # Retrieve the patch - patch = versionObj.patch.split(".")[0] - versionStr = f"v{versionObj.major}.{versionObj.minor}.{patch}" + if hasattr(mantid, "__version__"): + versionParts = str(mantid.__version__).split(".") + if len(versionParts) >= 2: + return f"v{versionParts[0]}.{versionParts[1]}.0" except ImportError: - _logger.warning("Could not determine Mantid version for documentation URL.") - except Exception as e: - _logger.warning(f"Error determining Mantid version for documentation URL: {e}") - - return versionStr + pass + log.warning("Could not determine Mantid version for documentation URL.") + return None class NoOpRequestInterceptor(QWebEngineUrlRequestInterceptor): - """ - A no-op interceptor that does nothing. Used if we're not loading local docs. - """ + """A no-op interceptor used when loading online docs.""" def interceptRequest(self, info: QWebEngineUrlRequestInfo): pass class LocalRequestInterceptor(QWebEngineUrlRequestInterceptor): - """ - Intercepts requests so we can relax the CORS policy for loading MathJax fonts - from cdn.jsdelivr.net when using local docs. - """ + """Intercepts requests for local docs (e.g., handle CORS).""" def interceptRequest(self, info: QWebEngineUrlRequestInfo): url = info.requestUrl() - if url.host() == "cdn.jsdelivr.net": + if url.host() == "cdn.jsdelivr.net": # Allow MathJax CDN info.setHttpHeader(b"Access-Control-Allow-Origin", b"*") class HelpWindowModel: - _logger = logging.getLogger(__name__) - - MODE_LOCAL = "Local Docs" + MODE_OFFLINE = "Offline Docs" MODE_ONLINE = "Online Docs" - def __init__(self, localDocsBase=None, onlineBase="https://docs.mantidproject.org/"): - self._rawLocalDocsBase = localDocsBase - self._rawOnlineBase = onlineBase.rstrip("/") + def __init__(self, online_base="https://docs.mantidproject.org/"): + local_docs_path_from_config = None + try: + config_service = ConfigService.Instance() + raw_path = config_service.getString("docs.html.root", True) + if raw_path: + local_docs_path_from_config = raw_path + log.debug(f"Retrieved 'docs.html.root' from ConfigService: '{local_docs_path_from_config}'") + else: + log.debug("'docs.html.root' property is empty or not found in ConfigService.") - self._isLocal = False - self._modeString = self.MODE_ONLINE - self._baseUrl = self._rawOnlineBase - self._versionString = None + except Exception as e: + log.error(f"Error retrieving 'docs.html.root' from ConfigService: {e}") - self._determine_mode_and_base_url() + self._raw_online_base = online_base.rstrip("/") + self._is_local = False + self._mode_string = self.MODE_ONLINE + self._base_url = self._raw_online_base + self._version_string = None + self._determine_mode_and_base_url(local_docs_path_from_config) - def _determine_mode_and_base_url(self): + def _determine_mode_and_base_url(self, local_docs_path): """ - Sets the internal state (_isLocal, _modeString, _baseWrl, _versionString) - based on the validity of localDocsBase and attempts to find a versioned URL. + Sets the internal state (_is_local, _mode_string, _base_url, _version_string) + based on the validity of the provided local_docs_path. """ - if self._rawLocalDocsBase and os.path.isdir(self._rawLocalDocsBase): - self._isLocal = True - self._modeString = self.MODE_LOCAL - absLocalPath = os.path.abspath(self._rawLocalDocsBase) - self._baseUrl = QUrl.fromLocalFile(absLocalPath).toString() - self._versionString = None - self._logger.debug(f"Using {self._modeString} from {self._baseUrl}") + log.debug(f"Determining mode with local_docs_path='{local_docs_path}'") + if local_docs_path and os.path.isdir(local_docs_path): + self._is_local = True + self._mode_string = self.MODE_OFFLINE + abs_local_path = os.path.abspath(local_docs_path) + self._base_url = QUrl.fromLocalFile(abs_local_path).toString() + self._version_string = None + log.debug(f"Using {self._mode_string} from {self._base_url}") else: - if self._rawLocalDocsBase: - self._logger.warning(f"Local docs path '{self._rawLocalDocsBase}' is invalid or not found. Falling back to online docs.") - - self._isLocal = False - self._modeString = self.MODE_ONLINE - - isLikelyRelease = self._rawLocalDocsBase is None - if isLikelyRelease: - self._versionString = _get_version_string_for_url() - - if self._versionString: - baseOnline = self._rawOnlineBase - if baseOnline.endswith("/stable"): - baseOnline = baseOnline[: -len("/stable")] - - if self._versionString not in baseOnline: - self._baseUrl = f"{baseOnline.rstrip('/')}/{self._versionString}" - self._logger.debug(f"Using {self._modeString} (Version: {self._versionString}) from {self._baseUrl}") + if local_docs_path: + log.warning( + f"Local docs path '{local_docs_path}' from ConfigService ('docs.html.root') is invalid or not found. Falling back to online docs." # noqa: E501 + ) + else: + log.debug("No valid local docs path found from ConfigService. Using online docs.") + + self._is_local = False + self._mode_string = self.MODE_ONLINE + self._version_string = getMantidVersionString() + + if self._version_string: + base_online = self._raw_online_base + if base_online.endswith("/stable"): + base_online = base_online[: -len("/stable")] + if self._version_string not in base_online: + self._base_url = f"{base_online.rstrip('/')}/{self._version_string}" + log.debug(f"Using {self._mode_string} (Version: {self._version_string}) from {self._base_url}") else: - self._baseUrl = self._rawOnlineBase - self._logger.debug(f"Using {self._modeString} (Using provided base URL, possibly stable/latest): {self._baseUrl}") + self._base_url = self._raw_online_base + log.debug(f"Using {self._mode_string} (Using provided base URL, possibly stable/latest): {self._base_url}") else: - self._baseUrl = self._rawOnlineBase - self._logger.debug(f"Using {self._modeString} (Version: Unknown/Stable) from {self._baseUrl}") + self._base_url = self._raw_online_base + log.debug(f"Using {self._mode_string} (Version: Unknown/Stable) from {self._base_url}") def is_local_docs_mode(self): - """ - :return: True if using local docs, False otherwise. Based on initial check. - """ - return self._isLocal + return self._is_local def get_mode_string(self): - """ - :return: User-friendly string indicating the mode ("Local Docs" or "Online Docs"). - """ - return self._modeString + return self._mode_string def get_base_url(self): - """` - :return: The determined base URL (either file:///path or https://docs...[/version]) - """ - return self._baseUrl.rstrip("/") + "/" - - def build_help_url(self, relativeUrl): - """ - Returns a QUrl pointing to the determined doc source for the given relative URL. - """ - if not relativeUrl or not relativeUrl.lower().endswith((".html", ".htm")): - relativeUrl = "index.html" - - relativeUrl = relativeUrl.lstrip("/") - fullUrlStr = f"{self.get_base_url()}{relativeUrl}" - - url = QUrl(fullUrlStr) + return self._base_url.rstrip("/") + "/" + + def build_help_url(self, relative_url): + if not relative_url or not relative_url.lower().endswith((".html", ".htm")): + relative_url = "index.html" + relative_url = relative_url.lstrip("/") + base = self.get_base_url() + full_url_str = f"{base}{relative_url}" + url = QUrl(full_url_str) if not url.isValid(): - self._logger.warning(f"Constructed invalid URL: {fullUrlStr} from base '{self.get_base_url()}' and relative '{relativeUrl}'") + log.warning(f"Constructed invalid URL: {full_url_str} from base '{base}' and relative '{relative_url}'") return url def get_home_url(self): - """ - Return the 'home' page URL: - - local 'index.html' if local docs are enabled - - online docs homepage otherwise - """ return self.build_help_url("index.html") def create_request_interceptor(self): - """ - Return an appropriate request interceptor: - - LocalRequestInterceptor if local docs are used (for mathjax CORS) - - NoOpRequestInterceptor otherwise - """ - if self._isLocal: - self._logger.debug("Using LocalRequestInterceptor.") + if self._is_local: + log.debug("Using LocalRequestInterceptor.") return LocalRequestInterceptor() else: - self._logger.debug("Using NoOpRequestInterceptor.") + log.debug("Using NoOpRequestInterceptor.") return NoOpRequestInterceptor() diff --git a/qt/python/mantidqt/mantidqt/widgets/helpwindow/helpwindowpresenter.py b/qt/python/mantidqt/mantidqt/widgets/helpwindow/helpwindowpresenter.py index 0d880be4689c..5d5ae01d199f 100644 --- a/qt/python/mantidqt/mantidqt/widgets/helpwindow/helpwindowpresenter.py +++ b/qt/python/mantidqt/mantidqt/widgets/helpwindow/helpwindowpresenter.py @@ -1,21 +1,41 @@ -# Mantid Repository : https://github.com/mantidproject/mantid -# # Copyright © 2017 ISIS Rutherford Appleton Laboratory UKRI, # NScD Oak Ridge National Laboratory, European Spallation Source, # Institut Laue - Langevin & CSNS, Institute of High Energy Physics, CAS # SPDX - License - Identifier: GPL - 3.0 + -import logging -from mantidqt.widgets.helpwindow.helpwindowmodel import HelpWindowModel -from mantidqt.widgets.helpwindow.helpwindowview import HelpWindowView -from qtpy.QtCore import Qt +try: + from mantid.kernel import Logger +except ImportError: + print("Warning: Mantid Logger not found, using basic print for logging.") -class HelpWindowPresenter: - _logger = logging.getLogger(__name__) + class Logger: + def __init__(self, name): + self._name = name + + def warning(self, msg): + print(f"WARNING [{self._name}]: {msg}") + + def debug(self, msg): + print(f"DEBUG [{self._name}]: {msg}") + + def information(self, msg): + print(f"INFO [{self._name}]: {msg}") + + def error(self, msg): + print(f"ERROR [{self._name}]: {msg}") - def __init__(self, parentApp=None, localDocs=None, onlineBaseUrl="https://docs.mantidproject.org/"): - self._logger.debug(f"Initializing with localDocs='{localDocs}', onlineBaseUrl='{onlineBaseUrl}'") - self.model = HelpWindowModel(localDocsBase=localDocs, onlineBase=onlineBaseUrl) + +log = Logger("HelpWindowPresenter") + +from mantidqt.widgets.helpwindow.helpwindowmodel import HelpWindowModel # noqa: E402 +from mantidqt.widgets.helpwindow.helpwindowview import HelpWindowView # noqa: E402 +from qtpy.QtCore import Qt # noqa: E402 + + +class HelpWindowPresenter: + def __init__(self, parentApp=None, onlineBaseUrl="https://docs.mantidproject.org/"): + log.debug(f"Initializing with onlineBaseUrl='{onlineBaseUrl}'") + self.model = HelpWindowModel(online_base=onlineBaseUrl) self.parentApp = parentApp self._view = None self._windowOpen = False @@ -23,24 +43,23 @@ def __init__(self, parentApp=None, localDocs=None, onlineBaseUrl="https://docs.m if self.parentApp: try: self.parentApp.aboutToQuit.connect(self.cleanup) - self._logger.debug("Connected cleanup to parentApp.aboutToQuit signal.") + log.debug("Connected cleanup to parentApp.aboutToQuit signal.") except AttributeError: - self._logger.warning("parentApp provided but does not have aboutToQuit signal.") + log.warning("parentApp provided but does not have aboutToQuit signal.") except Exception as e: - self._logger.error(f"Error connecting aboutToQuit signal: {e}") + log.error(f"Error connecting aboutToQuit signal: {e}") def _ensure_view_created(self): - """ - Creates the View instance if it doesn't exist yet. - """ + """Creates the View instance if it doesn't exist yet.""" if self._view is None: - self._logger.debug("Creating HelpWindowView instance.") + log.debug("Creating HelpWindowView instance.") interceptor = self.model.create_request_interceptor() modeString = self.model.get_mode_string() isLocal = self.model.is_local_docs_mode() + self._view = HelpWindowView(self, interceptor=interceptor) self._view.set_status_indicator(modeString, isLocal) - self._logger.debug("HelpWindowView instance created.") + log.debug("HelpWindowView instance created.") def show_help_page(self, relativeUrl): """ @@ -48,56 +67,52 @@ def show_help_page(self, relativeUrl): Ensures the View is created and visible. """ self._ensure_view_created() - self._logger.debug(f"Requesting help page: '{relativeUrl}'") + log.debug(f"Requesting help page: '{relativeUrl}'") docUrl = self.model.build_help_url(relativeUrl) self._view.set_page_url(docUrl) self.show_help_window() def show_home_page(self): """ - Presenter logic to load the 'Home' page, which might be local or online. - The View calls this when the user hits the Home button. + Presenter logic to load the 'Home' page, based on model's determined home URL. + The View calls this when the user hits the Home button. Ensures View exists. """ self._ensure_view_created() - self._logger.debug("Requesting home page.") + log.debug("Requesting home page.") homeUrl = self.model.get_home_url() self._view.set_page_url(homeUrl) def show_help_window(self): + """Ensure the HelpWindowView is created and visible.""" self._ensure_view_created() if not self._windowOpen: - self._logger.debug("Displaying help window for the first time.") + log.debug("Displaying help window for the first time.") self._windowOpen = True self._view.display() else: - self._logger.debug("Raising existing help window.") + log.debug("Raising existing help window.") self._view.show() self._view.raise_() self._view.activateWindow() def on_close(self): - """ - Called by the View when the window closes. - """ - self._logger.debug("View signaled interactive close.") + """Called by the View when the window closes interactively.""" + log.debug("View signaled interactive close.") self._windowOpen = False def cleanup(self): - """ - Cleanup resources, close the View if open. Called on app quit. - """ - self._logger.debug("Cleanup requested (likely app quitting).") - if self._view: - if self._windowOpen: - self._logger.info("Closing Help Window view during cleanup.") - self._view.setAttribute(Qt.WidgetAttribute.WA_DeleteOnClose, True) - self._view.close() - self._windowOpen = False - else: - self._logger.debug("View exists but wasn't marked open during cleanup, ensuring deletion.") - self._view.setAttribute(Qt.WidgetAttribute.WA_DeleteOnClose, True) - self._view.close() - + """Cleanup resources, close the View if open. Called on app quit.""" + log.debug("Cleanup requested (likely app quitting).") + if self._view is not None and self._windowOpen: + log.information("Closing Help Window view during cleanup.") + self._view.setAttribute(Qt.WidgetAttribute.WA_DeleteOnClose, True) + self._view.close() + self._windowOpen = False + self._view = None + elif self._view is not None: + log.debug("View exists but wasn't marked open during cleanup, ensuring deletion.") + self._view.setAttribute(Qt.WidgetAttribute.WA_DeleteOnClose, True) + self._view.close() self._view = None else: - self._logger.debug("No view instance to clean up.") + log.debug("No view instance to clean up.") diff --git a/qt/python/mantidqt/mantidqt/widgets/helpwindow/helpwindowview.py b/qt/python/mantidqt/mantidqt/widgets/helpwindow/helpwindowview.py index 2b071b9ad461..f193bcffe9b5 100644 --- a/qt/python/mantidqt/mantidqt/widgets/helpwindow/helpwindowview.py +++ b/qt/python/mantidqt/mantidqt/widgets/helpwindow/helpwindowview.py @@ -1,5 +1,3 @@ -# Mantid Repository : https://github.com/mantidproject/mantid -# # Copyright © 2017 ISIS Rutherford Appleton Laboratory UKRI, # NScD Oak Ridge National Laboratory, European Spallation Source, # Institut Laue - Langevin & CSNS, Institute of High Energy Physics, CAS @@ -88,19 +86,16 @@ def __init__(self, presenter, interceptor=None): self.reloadButton.clicked.connect(self.browser.reload) self.toolbar.addWidget(self.reloadButton) - # --- Add Status Indicator --- - # Spacer to push the status label to the right spacer = QWidget() spacer.setSizePolicy(QSizePolicy.Policy.Expanding, QSizePolicy.Policy.Preferred) self.toolbar.addWidget(spacer) # Status Label (Icon + Text) - self.statusLabel = QLabel("Status: Initializing...") # Initial text + self.statusLabel = QLabel("Status: Initializing...") self.statusLabel.setToolTip("Indicates whether documentation is loaded locally (Offline) or from the web (Online)") - # Add some padding/margin for aesthetics + self.statusLabel.setStyleSheet("QLabel { padding-left: 5px; padding-right: 5px; margin-left: 5px; }") self.toolbar.addWidget(self.statusLabel) - # --------------------------- # Connect signals for enabling/disabling buttons self.browser.urlChanged.connect(self.update_navigation_buttons) diff --git a/qt/python/mantidqt/mantidqt/widgets/helpwindow/test/test_helpwindowmodel.py b/qt/python/mantidqt/mantidqt/widgets/helpwindow/test/test_helpwindowmodel.py index 8f9c966497fa..e88034453760 100644 --- a/qt/python/mantidqt/mantidqt/widgets/helpwindow/test/test_helpwindowmodel.py +++ b/qt/python/mantidqt/mantidqt/widgets/helpwindow/test/test_helpwindowmodel.py @@ -1,123 +1,140 @@ -# Mantid Repository : https://github.com/mantidproject/mantid -# # Copyright © 2019 ISIS Rutherford Appleton Laboratory UKRI, # NScD Oak Ridge National Laboratory, European Spallation Source, # Institut Laue - Langevin & CSNS, Institute of High Energy Physics, CAS # SPDX - License - Identifier: GPL - 3.0 + -# This file is part of the mantid workbench. import os import tempfile import shutil import unittest +from unittest.mock import patch -from mantidqt.widgets.helpwindow.helpwindowmodel import HelpWindowModel +from mantidqt.widgets.helpwindow.helpwindowmodel import HelpWindowModel, LocalRequestInterceptor, NoOpRequestInterceptor +# --- Define Constants --- +CONFIG_SERVICE_LOOKUP_PATH = "mantidqt.widgets.helpwindow.helpwindowmodel.ConfigService" +VERSION_FUNC_LOOKUP_PATH = "mantidqt.widgets.helpwindow.helpwindowmodel.getMantidVersionString" +DOCS_ROOT_KEY = "docs.html.root" +ONLINE_BASE_EXAMPLE = "https://example.org" +# ------------------------ -class TestHelpWindowModel(unittest.TestCase): + +class TestHelpWindowModelConfigService(unittest.TestCase): def setUp(self): - # We can create a temporary directory to simulate "local_docs_base" that actually exists self.temp_dir = tempfile.mkdtemp() + self.invalid_path = os.path.join(self.temp_dir, "non_existent_subfolder") + if os.path.exists(self.invalid_path): + os.rmdir(self.invalid_path) def tearDown(self): - # Clean up any temporary directories shutil.rmtree(self.temp_dir, ignore_errors=True) - def test_init_with_local_docs_configures_for_local(self): - model = HelpWindowModel(localDocsBase=self.temp_dir, onlineBase="https://example.org") - - # The model should see this as local - self.assertTrue(model.is_local_docs_mode(), "Expected is_local_docs_mode() to be True for a valid folder") + @patch(CONFIG_SERVICE_LOOKUP_PATH) + def test_init_when_config_returns_valid_path_configures_for_local(self, mock_ConfigService): + """Test local mode when ConfigService returns a valid directory path.""" + mock_instance = mock_ConfigService.Instance.return_value + mock_instance.getString.return_value = self.temp_dir - # Check that the mode string is correctly set to "Local Docs" - self.assertEqual(model.MODE_LOCAL, model.get_mode_string(), "Expected mode string to be 'Local Docs'") + model = HelpWindowModel(online_base=ONLINE_BASE_EXAMPLE) - # build_help_url should produce a file:// URL + self.assertTrue(model.is_local_docs_mode(), "Expected is_local_docs_mode() True") + self.assertEqual(model.MODE_OFFLINE, model.get_mode_string(), "Expected mode string 'Offline Docs'") test_url = model.build_help_url("algorithms/MyAlgorithm-v1.html") self.assertTrue(test_url.isLocalFile(), "Expected a local file:// URL") - # home url also should be local + local_file_path = test_url.toLocalFile() + + expected_prefix = os.path.abspath(self.temp_dir) + os.sep + self.assertTrue( + local_file_path.startswith(expected_prefix), f"URL path '{local_file_path}' should start with temp dir path '{expected_prefix}'" + ) + home_url = model.get_home_url() self.assertTrue(home_url.isLocalFile(), "Expected a local file:// for home URL") - def test_init_with_no_local_docs_uses_online(self): - model = HelpWindowModel(localDocsBase=None, onlineBase="https://example.org") + mock_instance.getString.assert_called_once_with(DOCS_ROOT_KEY, True) + mock_ConfigService.Instance.assert_called_once() - # The model should see this as NOT local - self.assertFalse(model.is_local_docs_mode(), "Expected is_local_docs_mode() to be False") + @patch(CONFIG_SERVICE_LOOKUP_PATH) + def test_init_when_config_returns_none_uses_online(self, mock_ConfigService): + """Test online mode when ConfigService returns None (or empty string) for the path.""" + mock_instance = mock_ConfigService.Instance.return_value + mock_instance.getString.return_value = None - # Check that the mode string is correctly set to "Online Docs" - self.assertEqual(model.MODE_ONLINE, model.get_mode_string(), "Expected mode string to be 'Online Docs'") + model = HelpWindowModel(online_base=ONLINE_BASE_EXAMPLE) - # Print debugging info - print("\nDEBUG INFO:") - print(f"Base URL: {model.get_base_url()}") - - # build_help_url should produce an https://example.org/ URL + self.assertFalse(model.is_local_docs_mode(), "Expected is_local_docs_mode() False") + self.assertEqual(model.MODE_ONLINE, model.get_mode_string(), "Expected mode string 'Online Docs'") test_url = model.build_help_url("algorithms/MyAlgorithm-v1.html") - print(f"Generated URL: {test_url.toString()}") - - # Use a more flexible assertion that just checks it's using the right domain - self.assertTrue("example.org" in test_url.toString(), f"Expected URL to contain 'example.org', got: {test_url.toString()}") - self.assertTrue("/algorithms/" in test_url.toString(), f"Expected URL to contain '/algorithms/', got: {test_url.toString()}") - - # home url also should be online + self.assertFalse(test_url.isLocalFile(), "Expected a non-local (http/https) URL") + self.assertEqual(test_url.scheme(), "https") + self.assertTrue(ONLINE_BASE_EXAMPLE in test_url.toString()) + self.assertTrue("algorithms/MyAlgorithm-v1.html" in test_url.toString()) home_url = model.get_home_url() - self.assertTrue("example.org" in home_url.toString(), "Expected an online docs home URL") - - def test_init_with_invalid_local_docs_path_falls_back_to_online(self): - # Create a path we know doesn't exist anymore - invalid_path = os.path.join(self.temp_dir, "non_existent") - # We do NOT create that subfolder + self.assertFalse(home_url.isLocalFile(), "Expected an online docs home URL") + self.assertTrue(ONLINE_BASE_EXAMPLE in home_url.toString()) - # Model should fall back to online mode without raising an exception - model = HelpWindowModel(localDocsBase=invalid_path, onlineBase="https://example.org") + mock_instance.getString.assert_called_once_with(DOCS_ROOT_KEY, True) + mock_ConfigService.Instance.assert_called_once() - # The model should see this as NOT local - self.assertFalse(model.is_local_docs_mode(), "Expected is_local_docs_mode() to be False with invalid local path") + @patch(CONFIG_SERVICE_LOOKUP_PATH) + def test_init_when_config_returns_invalid_path_falls_back_to_online(self, mock_ConfigService): + """Test fallback to online mode when ConfigService returns an invalid/non-existent directory path.""" + mock_instance = mock_ConfigService.Instance.return_value + mock_instance.getString.return_value = self.invalid_path - # Check that the mode string is correctly set to "Online Docs" as fallback - self.assertEqual(model.MODE_ONLINE, model.get_mode_string(), "Expected mode string to be 'Online Docs' for invalid local path") + model = HelpWindowModel(online_base=ONLINE_BASE_EXAMPLE) - def test_create_request_interceptor_based_on_mode(self): - # Test with local mode - local_model = HelpWindowModel(localDocsBase=self.temp_dir, onlineBase="https://example.org") - local_interceptor = local_model.create_request_interceptor() + self.assertFalse(model.is_local_docs_mode(), "Expected is_local_docs_mode() False") + self.assertEqual(model.MODE_ONLINE, model.get_mode_string(), "Expected mode string 'Online Docs' on fallback") - # Should be a LocalRequestInterceptor - self.assertEqual( - "LocalRequestInterceptor", local_interceptor.__class__.__name__, "Expected LocalRequestInterceptor for local docs mode" - ) + mock_instance.getString.assert_called_once_with(DOCS_ROOT_KEY, True) + mock_ConfigService.Instance.assert_called_once() - # Test with online mode - online_model = HelpWindowModel(localDocsBase=None, onlineBase="https://example.org") + @patch(CONFIG_SERVICE_LOOKUP_PATH) + def test_create_request_interceptor_based_on_mode(self, mock_ConfigService): + """Test that the correct interceptor is created based on the mode determined from ConfigService.""" + # --- Test Local Mode --- + mock_instance_local = mock_ConfigService.Instance.return_value + mock_instance_local.getString.return_value = self.temp_dir + local_model = HelpWindowModel(online_base=ONLINE_BASE_EXAMPLE) + local_interceptor = local_model.create_request_interceptor() + self.assertIsInstance(local_interceptor, LocalRequestInterceptor, "Expected LocalRequestInterceptor for local mode") + mock_instance_local.getString.assert_called_once_with(DOCS_ROOT_KEY, True) + mock_ConfigService.Instance.reset_mock() + + # --- Test Online Mode --- + mock_instance_online = mock_ConfigService.Instance.return_value + mock_instance_online.getString.return_value = None + online_model = HelpWindowModel(online_base=ONLINE_BASE_EXAMPLE) online_interceptor = online_model.create_request_interceptor() - - # Should be a NoOpRequestInterceptor - self.assertEqual( - "NoOpRequestInterceptor", online_interceptor.__class__.__name__, "Expected NoOpRequestInterceptor for online docs mode" - ) - - def test_url_construction_with_trailing_slashes(self): - # Test base URL construction with and without trailing slashes - model1 = HelpWindowModel(localDocsBase=None, onlineBase="https://example.org") - model2 = HelpWindowModel(localDocsBase=None, onlineBase="https://example.org/") - - # Print debugging info - print("\nURL Construction Debug:") - print(f"Model1 base URL: {model1.get_base_url()}") - print(f"Model2 base URL: {model2.get_base_url()}") + self.assertIsInstance(online_interceptor, NoOpRequestInterceptor, "Expected NoOpRequestInterceptor for online mode") + mock_instance_online.getString.assert_called_once_with(DOCS_ROOT_KEY, True) + mock_ConfigService.Instance.assert_called_once() + + @patch(CONFIG_SERVICE_LOOKUP_PATH) + @patch(VERSION_FUNC_LOOKUP_PATH, return_value=None) + def test_online_url_construction_with_trailing_slashes(self, mock_version_func, mock_ConfigService): + """Test online base URL construction handles trailing slashes correctly (forced online).""" + mock_instance = mock_ConfigService.Instance.return_value + mock_instance.getString.return_value = None + + model1 = HelpWindowModel(online_base="https://example.org") + model2 = HelpWindowModel(online_base="https://example.org/") + + self.assertFalse(model1.is_local_docs_mode(), "Model1 should be in online mode") + self.assertFalse(model2.is_local_docs_mode(), "Model2 should be in online mode") + self.assertTrue(model1.get_base_url().endswith("/"), "get_base_url() should add trailing slash") + self.assertTrue(model2.get_base_url().endswith("/"), "get_base_url() should keep trailing slash") + self.assertEqual(model1.get_base_url(), model2.get_base_url(), "Base URLs should be identical") url1 = model1.build_help_url("test.html") url2 = model2.build_help_url("test.html") - print(f"URL1: {url1.toString()}") - print(f"URL2: {url2.toString()}") - - # Test with a more flexible assertion - self.assertEqual(url1.host(), url2.host(), "URLs should have the same host") - self.assertEqual( - url1.path().rstrip("/"), url2.path().rstrip("/"), "URLs should have the same path after normalizing trailing slashes" - ) + self.assertEqual(url1.toString(), url2.toString(), "Generated URLs should be identical") + self.assertEqual(url1.toString(), "https://example.org/test.html", "Generated URL should be correct (no version)") + self.assertTrue(mock_version_func.called) + mock_ConfigService.Instance.assert_called() + mock_instance.getString.assert_called() if __name__ == "__main__": From d804d2d7658feb284482d02d500fa9e088a566c0 Mon Sep 17 00:00:00 2001 From: Darsh Date: Tue, 15 Apr 2025 13:31:52 -0400 Subject: [PATCH 2/4] Updates made based on comments. --- .../widgets/helpwindow/helpwindowmodel.py | 111 ++++++++++++------ 1 file changed, 78 insertions(+), 33 deletions(-) diff --git a/qt/python/mantidqt/mantidqt/widgets/helpwindow/helpwindowmodel.py b/qt/python/mantidqt/mantidqt/widgets/helpwindow/helpwindowmodel.py index 7035ac1561cb..f3912bf5ac1b 100644 --- a/qt/python/mantidqt/mantidqt/widgets/helpwindow/helpwindowmodel.py +++ b/qt/python/mantidqt/mantidqt/widgets/helpwindow/helpwindowmodel.py @@ -1,14 +1,15 @@ +# Mantid Repository : https://github.com/mantidproject/mantid +# # Copyright © 2017 ISIS Rutherford Appleton Laboratory UKRI, # NScD Oak Ridge National Laboratory, European Spallation Source, # Institut Laue - Langevin & CSNS, Institute of High Energy Physics, CAS # SPDX - License - Identifier: GPL - 3.0 + import os - +# --- Logger and ConfigService Setup --- try: from mantid.kernel import Logger from mantid.kernel import ConfigService - except ImportError: print("Warning: Mantid Kernel (Logger/ConfigService) not found, using basic print/dummy.") @@ -28,17 +29,18 @@ def information(self, msg): def error(self, msg): print(f"ERROR [{self._name}]: {msg}") - class ConfigService: + class ConfigService: # Dummy for environments without Mantid @staticmethod def Instance(): class DummyInstance: def getString(self, key, pathAbsolute=True): - return None # Default to None + return None return DummyInstance() log = Logger("HelpWindowModel") +# -------------------------------------- from qtpy.QtCore import QUrl # noqa: E402 from qtpy.QtWebEngineCore import QWebEngineUrlRequestInterceptor, QWebEngineUrlRequestInfo # noqa: E402 @@ -80,89 +82,132 @@ class HelpWindowModel: MODE_ONLINE = "Online Docs" def __init__(self, online_base="https://docs.mantidproject.org/"): - local_docs_path_from_config = None + # Store raw online base early, needed for fallback logic below + self._raw_online_base = online_base.rstrip("/") + + # --- Step 1: Attempt to get local path from ConfigService --- + local_docs_path_from_config = None # Default if lookup fails or path is empty try: + # ConfigService is imported at the top level now config_service = ConfigService.Instance() - raw_path = config_service.getString("docs.html.root", True) - if raw_path: + raw_path = config_service.getString("docs.html.root", True) # pathAbsolute=True + if raw_path: # Only assign if not empty local_docs_path_from_config = raw_path log.debug(f"Retrieved 'docs.html.root' from ConfigService: '{local_docs_path_from_config}'") else: log.debug("'docs.html.root' property is empty or not found in ConfigService.") - except Exception as e: - log.error(f"Error retrieving 'docs.html.root' from ConfigService: {e}") + # Catch potential errors during ConfigService interaction + # This includes cases where the dummy ConfigService might be used + log.error(f"Error retrieving 'docs.html.root' from ConfigService: {e}. Defaulting to online mode.") + # local_docs_path_from_config remains None - self._raw_online_base = online_base.rstrip("/") - self._is_local = False - self._mode_string = self.MODE_ONLINE - self._base_url = self._raw_online_base - self._version_string = None - self._determine_mode_and_base_url(local_docs_path_from_config) + # --- Step 2: Determine final mode and set ALL related state variables --- + # This method now sets _is_local, _mode_string, _base_url, _version_string + self._determine_mode_and_set_state(local_docs_path_from_config) - def _determine_mode_and_base_url(self, local_docs_path): + def _determine_mode_and_set_state(self, local_docs_path): """ - Sets the internal state (_is_local, _mode_string, _base_url, _version_string) - based on the validity of the provided local_docs_path. + Sets the final operational state (_is_local, _mode_string, _base_url, _version_string) + based *only* on the validity of the provided local_docs_path argument, which is the + result of the ConfigService lookup (can be a path string or None). """ - log.debug(f"Determining mode with local_docs_path='{local_docs_path}'") + log.debug(f"Determining final mode and state with local_docs_path='{local_docs_path}'") + + # Check if the path from config is valid and points to an existing directory if local_docs_path and os.path.isdir(local_docs_path): + # --- Configure for LOCAL/OFFLINE Mode --- + log.debug("Valid local docs path found. Configuring for Offline Mode.") self._is_local = True self._mode_string = self.MODE_OFFLINE - abs_local_path = os.path.abspath(local_docs_path) + abs_local_path = os.path.abspath(local_docs_path) # Ensure absolute + # Base URL for local files needs 'file:///' prefix and correct path format self._base_url = QUrl.fromLocalFile(abs_local_path).toString() - self._version_string = None - log.debug(f"Using {self._mode_string} from {self._base_url}") + self._version_string = None # Version string not applicable for local docs mode + log.debug(f"Final state: Mode='{self._mode_string}', Base URL='{self._base_url}'") + else: - if local_docs_path: + # --- Configure for ONLINE Mode --- + # Log reason if applicable + if local_docs_path: # Path was provided but invalid log.warning( - f"Local docs path '{local_docs_path}' from ConfigService ('docs.html.root') is invalid or not found. Falling back to online docs." # noqa: E501 + f"Local docs path '{local_docs_path}' from ConfigService ('docs.html.root') is invalid or not found. Falling back to Online Mode." # noqa: E501 ) - else: - log.debug("No valid local docs path found from ConfigService. Using online docs.") + else: # Path was None (not found in config or error during lookup) + log.debug("No valid local docs path found from ConfigService. Configuring for Online Mode.") self._is_local = False self._mode_string = self.MODE_ONLINE - self._version_string = getMantidVersionString() + # Attempt to get versioned URL for online mode + self._version_string = getMantidVersionString() # Might return None + + # Set final base URL based on online path and version string if self._version_string: base_online = self._raw_online_base if base_online.endswith("/stable"): base_online = base_online[: -len("/stable")] + # Avoid double versioning if base_online already has it if self._version_string not in base_online: self._base_url = f"{base_online.rstrip('/')}/{self._version_string}" - log.debug(f"Using {self._mode_string} (Version: {self._version_string}) from {self._base_url}") - else: + log.debug(f"Using versioned online URL: {self._base_url}") + else: # Use provided base as-is (likely includes 'stable' or version) self._base_url = self._raw_online_base - log.debug(f"Using {self._mode_string} (Using provided base URL, possibly stable/latest): {self._base_url}") - else: + log.debug(f"Using provided online base URL (version/stable implied): {self._base_url}") + else: # No version string found, use raw online base self._base_url = self._raw_online_base - log.debug(f"Using {self._mode_string} (Version: Unknown/Stable) from {self._base_url}") + log.debug(f"Using default online base URL (version unknown): {self._base_url}") + + log.debug(f"Final state: Mode='{self._mode_string}', Base URL='{self._base_url}', Version='{self._version_string}'") + # --- Getter methods remain the same --- def is_local_docs_mode(self): + """ + :return: True if using local docs, False otherwise. Based on state set during init. + """ return self._is_local def get_mode_string(self): + """ + :return: User-friendly string indicating the mode ("Offline Docs" or "Online Docs"). + """ return self._mode_string def get_base_url(self): + """ + :return: The determined base URL (either file:///path/ or https://docs...[/version]/) with trailing slash. + """ + # Ensure trailing slash for correct relative URL joining return self._base_url.rstrip("/") + "/" + # --- URL building methods use the state set during init --- def build_help_url(self, relative_url): + """ + Returns a QUrl pointing to the determined doc source for the given relative URL. + """ if not relative_url or not relative_url.lower().endswith((".html", ".htm")): relative_url = "index.html" + relative_url = relative_url.lstrip("/") - base = self.get_base_url() + base = self.get_base_url() # Uses the final URL determined during init full_url_str = f"{base}{relative_url}" + url = QUrl(full_url_str) if not url.isValid(): log.warning(f"Constructed invalid URL: {full_url_str} from base '{base}' and relative '{relative_url}'") return url def get_home_url(self): + """ + Return the 'home' page URL (index.html) based on the determined mode/base URL. + """ return self.build_help_url("index.html") + # --- Interceptor creation uses the state set during init --- def create_request_interceptor(self): + """ + Return an appropriate request interceptor based on the determined mode (_is_local). + """ if self._is_local: log.debug("Using LocalRequestInterceptor.") return LocalRequestInterceptor() From 5436c638975bece38f1362982351c56f78c99dc5 Mon Sep 17 00:00:00 2001 From: Darsh Date: Tue, 15 Apr 2025 16:12:04 -0400 Subject: [PATCH 3/4] Fix windows test failure. --- .../widgets/helpwindow/test/test_helpwindowmodel.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/qt/python/mantidqt/mantidqt/widgets/helpwindow/test/test_helpwindowmodel.py b/qt/python/mantidqt/mantidqt/widgets/helpwindow/test/test_helpwindowmodel.py index e88034453760..111ee46488c9 100644 --- a/qt/python/mantidqt/mantidqt/widgets/helpwindow/test/test_helpwindowmodel.py +++ b/qt/python/mantidqt/mantidqt/widgets/helpwindow/test/test_helpwindowmodel.py @@ -42,10 +42,12 @@ def test_init_when_config_returns_valid_path_configures_for_local(self, mock_Con self.assertTrue(test_url.isLocalFile(), "Expected a local file:// URL") local_file_path = test_url.toLocalFile() + norm_local_file_path = os.path.normpath(local_file_path) + norm_expected_prefix = os.path.normpath(os.path.abspath(self.temp_dir)) - expected_prefix = os.path.abspath(self.temp_dir) + os.sep self.assertTrue( - local_file_path.startswith(expected_prefix), f"URL path '{local_file_path}' should start with temp dir path '{expected_prefix}'" + norm_local_file_path.startswith(norm_expected_prefix + os.sep), + f"URL path '{norm_local_file_path}' should start with temp dir path '{norm_expected_prefix}{os.sep}'", ) home_url = model.get_home_url() @@ -93,7 +95,6 @@ def test_init_when_config_returns_invalid_path_falls_back_to_online(self, mock_C @patch(CONFIG_SERVICE_LOOKUP_PATH) def test_create_request_interceptor_based_on_mode(self, mock_ConfigService): """Test that the correct interceptor is created based on the mode determined from ConfigService.""" - # --- Test Local Mode --- mock_instance_local = mock_ConfigService.Instance.return_value mock_instance_local.getString.return_value = self.temp_dir local_model = HelpWindowModel(online_base=ONLINE_BASE_EXAMPLE) @@ -102,7 +103,6 @@ def test_create_request_interceptor_based_on_mode(self, mock_ConfigService): mock_instance_local.getString.assert_called_once_with(DOCS_ROOT_KEY, True) mock_ConfigService.Instance.reset_mock() - # --- Test Online Mode --- mock_instance_online = mock_ConfigService.Instance.return_value mock_instance_online.getString.return_value = None online_model = HelpWindowModel(online_base=ONLINE_BASE_EXAMPLE) From 16687ec783a4d597f0f2b1cb51925a86e0eaa0d7 Mon Sep 17 00:00:00 2001 From: Darsh Date: Wed, 16 Apr 2025 15:28:23 -0400 Subject: [PATCH 4/4] Updates to handle file not found error --- .../widgets/helpwindow/helpwindowmodel.py | 56 ++++++-- .../widgets/helpwindow/helpwindowpresenter.py | 95 +++++++++++--- .../widgets/helpwindow/helpwindowview.py | 123 +++++++++++++++++- .../helpwindow/test/test_helpwindowmodel.py | 25 +++- 4 files changed, 268 insertions(+), 31 deletions(-) diff --git a/qt/python/mantidqt/mantidqt/widgets/helpwindow/helpwindowmodel.py b/qt/python/mantidqt/mantidqt/widgets/helpwindow/helpwindowmodel.py index f3912bf5ac1b..1c15b90ad6f2 100644 --- a/qt/python/mantidqt/mantidqt/widgets/helpwindow/helpwindowmodel.py +++ b/qt/python/mantidqt/mantidqt/widgets/helpwindow/helpwindowmodel.py @@ -42,6 +42,7 @@ def getString(self, key, pathAbsolute=True): log = Logger("HelpWindowModel") # -------------------------------------- +# Imports moved below logger/config setup to ensure log is defined from qtpy.QtCore import QUrl # noqa: E402 from qtpy.QtWebEngineCore import QWebEngineUrlRequestInterceptor, QWebEngineUrlRequestInfo # noqa: E402 @@ -49,14 +50,14 @@ def getString(self, key, pathAbsolute=True): def getMantidVersionString(): """Placeholder function to get Mantid version (e.g., 'v6.13.0').""" try: - import mantid + import mantid # Keep import local as it might fail if hasattr(mantid, "__version__"): versionParts = str(mantid.__version__).split(".") if len(versionParts) >= 2: return f"v{versionParts[0]}.{versionParts[1]}.0" except ImportError: - pass + pass # Mantid might not be available log.warning("Could not determine Mantid version for documentation URL.") return None @@ -184,23 +185,60 @@ def get_base_url(self): def build_help_url(self, relative_url): """ Returns a QUrl pointing to the determined doc source for the given relative URL. + Raises FileNotFoundError if building a URL for local mode and the target file doesn't exist. """ + # Default page logic if not relative_url or not relative_url.lower().endswith((".html", ".htm")): relative_url = "index.html" - + # Ensure relative path format relative_url = relative_url.lstrip("/") - base = self.get_base_url() # Uses the final URL determined during init - full_url_str = f"{base}{relative_url}" - url = QUrl(full_url_str) - if not url.isValid(): - log.warning(f"Constructed invalid URL: {full_url_str} from base '{base}' and relative '{relative_url}'") - return url + base = self.get_base_url() # Get base URL (file:/// or https://) + + # --- Check local file existence if in local mode --- + if self._is_local: + # Convert base file:/// URL back to a filesystem path + # Note: QUrl().toLocalFile() handles platform specifics + base_path = QUrl(base).toLocalFile() + if not base_path: # Defensive check + err_msg = f"Cannot determine local base path from URL: {base}" + log.error(err_msg) + # Raise a different error as this indicates a problem with the base URL itself + raise ValueError(err_msg) + + # Construct the full potential path to the target HTML file + full_path = os.path.join(base_path, relative_url) + # Normalize for consistent checking and clearer error messages + norm_full_path = os.path.normpath(full_path) + + log.debug(f"Checking local file existence: {norm_full_path}") + # Check if it exists AND is a file (not a directory) + if not os.path.isfile(norm_full_path): + err_msg = f"Local help file not found: {norm_full_path}" + log.warning(err_msg) + # Raise FileNotFoundError as requested by reviewer suggestion + raise FileNotFoundError(err_msg) + else: + # File exists, return the QUrl for the local file + log.debug(f"Local file found. Returning URL: file:///{norm_full_path}") + return QUrl.fromLocalFile(norm_full_path) + # ------------------------------------------------- + else: # Online mode + # Construct the full online URL string + full_url_str = f"{base}{relative_url}" + url = QUrl(full_url_str) + # Basic validation check + if not url.isValid(): + log.warning(f"Constructed invalid Online URL: {full_url_str} from base '{base}' and relative '{relative_url}'") + log.debug(f"Returning online URL: {url.toString()}") + return url def get_home_url(self): """ Return the 'home' page URL (index.html) based on the determined mode/base URL. + May raise FileNotFoundError if in local mode and index.html does not exist. """ + # This call now incorporates the existence check from build_help_url return self.build_help_url("index.html") # --- Interceptor creation uses the state set during init --- diff --git a/qt/python/mantidqt/mantidqt/widgets/helpwindow/helpwindowpresenter.py b/qt/python/mantidqt/mantidqt/widgets/helpwindow/helpwindowpresenter.py index 5d5ae01d199f..7d8545a3c5a9 100644 --- a/qt/python/mantidqt/mantidqt/widgets/helpwindow/helpwindowpresenter.py +++ b/qt/python/mantidqt/mantidqt/widgets/helpwindow/helpwindowpresenter.py @@ -29,13 +29,18 @@ def error(self, msg): from mantidqt.widgets.helpwindow.helpwindowmodel import HelpWindowModel # noqa: E402 from mantidqt.widgets.helpwindow.helpwindowview import HelpWindowView # noqa: E402 -from qtpy.QtCore import Qt # noqa: E402 + +from qtpy.QtCore import Qt, QUrl # noqa: E402 class HelpWindowPresenter: def __init__(self, parentApp=None, onlineBaseUrl="https://docs.mantidproject.org/"): log.debug(f"Initializing with onlineBaseUrl='{onlineBaseUrl}'") - self.model = HelpWindowModel(online_base=onlineBaseUrl) + try: + self.model = HelpWindowModel(online_base=onlineBaseUrl) + except Exception as e: + log.error(f"Failed to initialize HelpWindowModel: {e}") + self.model = None self.parentApp = parentApp self._view = None self._windowOpen = False @@ -51,40 +56,96 @@ def __init__(self, parentApp=None, onlineBaseUrl="https://docs.mantidproject.org def _ensure_view_created(self): """Creates the View instance if it doesn't exist yet.""" - if self._view is None: + if self._view is None and self.model is not None: log.debug("Creating HelpWindowView instance.") - interceptor = self.model.create_request_interceptor() - modeString = self.model.get_mode_string() - isLocal = self.model.is_local_docs_mode() + try: + interceptor = self.model.create_request_interceptor() + modeString = self.model.get_mode_string() + isLocal = self.model.is_local_docs_mode() - self._view = HelpWindowView(self, interceptor=interceptor) - self._view.set_status_indicator(modeString, isLocal) - log.debug("HelpWindowView instance created.") + self._view = HelpWindowView(self, interceptor=interceptor) + self._view.set_status_indicator(modeString, isLocal) + log.debug("HelpWindowView instance created.") + except Exception as e: + log.error(f"Failed to create HelpWindowView: {e}") + self._view = None + elif self.model is None: + log.error("Cannot create view because model initialization failed.") def show_help_page(self, relativeUrl): """ Build the doc URL from the Model and tell the View to load it. + Handles FileNotFoundError from the model for local files. Ensures the View is created and visible. """ self._ensure_view_created() + if not self._view: + log.error("Cannot show help page, view is not available.") + return + log.debug(f"Requesting help page: '{relativeUrl}'") - docUrl = self.model.build_help_url(relativeUrl) - self._view.set_page_url(docUrl) - self.show_help_window() + try: + docUrl = self.model.build_help_url(relativeUrl) + self._view.set_page_url(docUrl) + self.show_help_window() + except FileNotFoundError as e: + log.error(f"File not found for help page '{relativeUrl}': {e}") + self._view.show_file_not_found_error(str(e), relativeUrl) + self.show_help_window() + except Exception as e: + log.error(f"Error building URL for '{relativeUrl}': {e}") + self._view.show_generic_error(f"Error generating help URL: {e}", relativeUrl) + self.show_help_window() def show_home_page(self): """ - Presenter logic to load the 'Home' page, based on model's determined home URL. - The View calls this when the user hits the Home button. Ensures View exists. + Presenter logic to load the 'Home' page. Handles FileNotFoundError. + Ensures View exists. """ self._ensure_view_created() + if not self._view: + log.error("Cannot show home page, view is not available.") + return + log.debug("Requesting home page.") - homeUrl = self.model.get_home_url() - self._view.set_page_url(homeUrl) + try: + homeUrl = self.model.get_home_url() + self._view.set_page_url(homeUrl) + except FileNotFoundError as e: + log.error(f"File not found for home page: {e}") + self._view.show_file_not_found_error(str(e), "index.html") + self.show_help_window() + except Exception as e: + log.error(f"Error building home URL: {e}") + self._view.show_generic_error(f"Error generating home URL: {e}", "index.html") + self.show_help_window() + + def get_home_url_for_view(self) -> QUrl: + """Provides the home URL (local or online) to the View for links.""" + if not self.model: + return QUrl() + try: + return self.model.get_home_url() + except FileNotFoundError: + log.warning("Home file (index.html) not found for error page link.") + try: + online_home = QUrl(self.model._raw_online_base + "/index.html") + if online_home.isValid(): + return online_home + except Exception: + pass + return QUrl() + except Exception as e: + log.error(f"Error getting home URL for view: {e}") + return QUrl() def show_help_window(self): """Ensure the HelpWindowView is created and visible.""" - self._ensure_view_created() + if self._view is None: + log.error("Cannot show window, view is not created.") + self._ensure_view_created() + if self._view is None: + return if not self._windowOpen: log.debug("Displaying help window for the first time.") self._windowOpen = True diff --git a/qt/python/mantidqt/mantidqt/widgets/helpwindow/helpwindowview.py b/qt/python/mantidqt/mantidqt/widgets/helpwindow/helpwindowview.py index f193bcffe9b5..2e3ae8598d5d 100644 --- a/qt/python/mantidqt/mantidqt/widgets/helpwindow/helpwindowview.py +++ b/qt/python/mantidqt/mantidqt/widgets/helpwindow/helpwindowview.py @@ -3,6 +3,7 @@ # Institut Laue - Langevin & CSNS, Institute of High Energy Physics, CAS # SPDX - License - Identifier: GPL - 3.0 + import logging +from html import escape from qtpy.QtWidgets import ( QMainWindow, QVBoxLayout, @@ -99,9 +100,124 @@ def __init__(self, presenter, interceptor=None): # Connect signals for enabling/disabling buttons self.browser.urlChanged.connect(self.update_navigation_buttons) - self.browser.loadFinished.connect(self.update_navigation_buttons) + # Connect loadFinished to the new handler + self.browser.loadFinished.connect(self.handle_load_finished) self.update_navigation_buttons() + def handle_load_finished(self, ok: bool): + """Slot handling the loadFinished signal. Shows generic error on failure.""" + self._logger.debug(f"Load finished signal received. Success: {ok}") + self.update_navigation_buttons() # Update nav state regardless + + if not ok: + # Load failed for reasons OTHER than local file not found + failed_url = self.browser.url() + failed_url_str = escape(failed_url.toString()) + # Use self._logger consistently + self._logger.error(f"Failed to load page (WebEngine signal): {failed_url_str}") + + # Display a more generic error page now + error_html = f""" + Error Loading Page + + + +

Page Load Error

+

Sorry, the help page could not be loaded:

+

{failed_url_str}

+

This could be due to network issues, online server errors, local file permission problems, or other loading errors.

+ + """ + + # Use baseUrl of the failed request for context + self.browser.setHtml(error_html, baseUrl=failed_url) + + def show_file_not_found_error(self, error_message: str, requested_url: str): + """Displays a specific error page when the Model/Presenter signals a local file wasn't found.""" + self._logger.debug(f"Displaying FileNotFoundError page for requested relative URL: {requested_url}") + error_message_html = escape(error_message) + requested_url_html = escape(requested_url) + + # Try to get home link + home_link_html = "" + try: + home_url_obj = self.presenter.get_home_url_for_view() + if home_url_obj and home_url_obj.isValid(): + # Add target="_self" to ensure it loads in the help window + home_link_html = f"

Go to Home Page

" + except AttributeError: + self._logger.warning("Presenter does not have get_home_url_for_view method.") + except Exception as e: + self._logger.error(f"Error getting home URL for error page: {e}") + + error_html = f""" + + File Not Found + + + +

Help File Not Found

+

Sorry, the requested local documentation file could not be found.

+

Requested: {requested_url_html}

+

Details: {error_message_html}

+

Please check that your local documentation is correctly built + and the path is correctly configured in Mantid ('docs.html.root').

+ {home_link_html} + + + """ + + # Use an empty QUrl or the home URL as the base for setHtml + base_url_for_error = QUrl() + try: + base_url_for_error = self.presenter.get_home_url_for_view() or QUrl() + except Exception: + pass + self.browser.setHtml(error_html, baseUrl=base_url_for_error) + + def show_generic_error(self, error_message: str, requested_url: str): + """Displays a generic error page based on message from presenter (e.g., URL build errors).""" + # Use self._logger consistently + self._logger.debug(f"Displaying generic error page for: {requested_url}") + error_message_html = escape(error_message) + requested_url_html = escape(requested_url) + error_html = f""" + Error + + + +

Help Window Error

+

An error occurred while trying to display help for:

+

{requested_url_html}

+

Details: {error_message_html}

+ + """ + self.browser.setHtml(error_html) + def set_status_indicator(self, modeText: str, isLocal: bool): """ Updates the status label text and icon. @@ -121,8 +237,9 @@ def update_navigation_buttons(self, ok: bool = True): """ Enable/disable back/forward buttons based on browser history. """ - canGoBack = self.browser.history().canGoBack() - canGoForward = self.browser.history().canGoForward() + history = self.browser.history() if hasattr(self.browser, "history") else None + canGoBack = history.canGoBack() if history else False + canGoForward = history.canGoForward() if history else False self.backButton.setEnabled(canGoBack) self.forwardButton.setEnabled(canGoForward) self.reloadButton.setEnabled(True) diff --git a/qt/python/mantidqt/mantidqt/widgets/helpwindow/test/test_helpwindowmodel.py b/qt/python/mantidqt/mantidqt/widgets/helpwindow/test/test_helpwindowmodel.py index 111ee46488c9..bfbe31b259c0 100644 --- a/qt/python/mantidqt/mantidqt/widgets/helpwindow/test/test_helpwindowmodel.py +++ b/qt/python/mantidqt/mantidqt/widgets/helpwindow/test/test_helpwindowmodel.py @@ -38,13 +38,31 @@ def test_init_when_config_returns_valid_path_configures_for_local(self, mock_Con self.assertTrue(model.is_local_docs_mode(), "Expected is_local_docs_mode() True") self.assertEqual(model.MODE_OFFLINE, model.get_mode_string(), "Expected mode string 'Offline Docs'") - test_url = model.build_help_url("algorithms/MyAlgorithm-v1.html") + + dummy_home_path = os.path.join(self.temp_dir, "index.html") + with open(dummy_home_path, "w") as f: + f.write("") + + algo_subdir = os.path.join(self.temp_dir, "algorithms") + os.makedirs(algo_subdir, exist_ok=True) + algo_file_rel_path = "algorithms/MyAlgorithm-v1.html" + dummy_algo_path = os.path.join(self.temp_dir, algo_file_rel_path) + with open(dummy_algo_path, "w") as f: + f.write("") + + test_url = model.build_help_url(algo_file_rel_path) self.assertTrue(test_url.isLocalFile(), "Expected a local file:// URL") local_file_path = test_url.toLocalFile() norm_local_file_path = os.path.normpath(local_file_path) - norm_expected_prefix = os.path.normpath(os.path.abspath(self.temp_dir)) + norm_expected_file_path = os.path.normpath(dummy_algo_path) + self.assertEqual( + norm_local_file_path, + norm_expected_file_path, + f"URL path '{norm_local_file_path}' should match dummy file path '{norm_expected_file_path}'", + ) + norm_expected_prefix = os.path.normpath(os.path.abspath(self.temp_dir)) self.assertTrue( norm_local_file_path.startswith(norm_expected_prefix + os.sep), f"URL path '{norm_local_file_path}' should start with temp dir path '{norm_expected_prefix}{os.sep}'", @@ -52,6 +70,9 @@ def test_init_when_config_returns_valid_path_configures_for_local(self, mock_Con home_url = model.get_home_url() self.assertTrue(home_url.isLocalFile(), "Expected a local file:// for home URL") + norm_home_path = os.path.normpath(home_url.toLocalFile()) + norm_expected_home_path = os.path.normpath(dummy_home_path) + self.assertEqual(norm_home_path, norm_expected_home_path, "Home URL should point to dummy index.html") mock_instance.getString.assert_called_once_with(DOCS_ROOT_KEY, True) mock_ConfigService.Instance.assert_called_once()