Merge pull request #153 from opengisch/param-ux

Improve parameters handling

Author: 3nids

oqtopus/core/package_prepare_task.py

@@ -90,6 +90,14 @@ def run(self):
             self.__destination_directory = self.__prepare_destination_directory()
             logger.info(f"Destination directory: {self.__destination_directory}")
 
+            # For branches/PRs, always fetch the latest commit SHA before
+            # checking the cache so we detect new commits
+            if self.module_package.type in (
+                ModulePackage.Type.BRANCH,
+                ModulePackage.Type.PULL_REQUEST,
+            ):
+                self.module_package.fetch_commit_sha()
+
             # Reset progress tracking
             self.__download_total_expected = 0
             self.__download_total_received = 0
@@ -128,9 +136,14 @@ def __prepare_module_assets(self, module_package):
         self.__prefetch_download_sizes(module_package)
 
         # Download the source or use from zip
-        zip_file = self.from_zip_file or self.__download_module_asset(
-            module_package.download_url, "source.zip", module_package
-        )
+        if self.from_zip_file:
+            zip_file = self.from_zip_file
+        else:
+            zip_file, was_downloaded = self.__download_module_asset(
+                module_package.download_url, "source.zip", module_package
+            )
+            if was_downloaded:
+                self.__remove_extracted_dir("src")
 
         module_package.source_package_zip = zip_file
         package_dir = self.__extract_zip_file(zip_file, "src")
@@ -139,22 +152,26 @@ def __prepare_module_assets(self, module_package):
         # Download the release assets
         self.__checkForCanceled()
         if module_package.asset_project is not None:
-            zip_file = self.__download_module_asset(
+            zip_file, was_downloaded = self.__download_module_asset(
                 module_package.asset_project.download_url,
                 module_package.asset_project.type.value + ".zip",
                 module_package,
             )
+            if was_downloaded:
+                self.__remove_extracted_dir("project")
             package_dir = self.__extract_zip_file(zip_file, "project")
             module_package.asset_project.package_zip = zip_file
             module_package.asset_project.package_dir = package_dir
 
         self.__checkForCanceled()
         if module_package.asset_plugin is not None:
-            zip_file = self.__download_module_asset(
+            zip_file, was_downloaded = self.__download_module_asset(
                 module_package.asset_plugin.download_url,
                 module_package.asset_plugin.type.value + ".zip",
                 module_package,
             )
+            if was_downloaded:
+                self.__remove_extracted_dir("plugin")
             package_dir = self.__extract_zip_file(zip_file, "plugin")
             module_package.asset_plugin.package_zip = zip_file
             module_package.asset_plugin.package_dir = package_dir
@@ -256,6 +273,26 @@ def __get_cache_filename(self, base_filename: str, module_package):
                 return f"{name}-{int(time.time())}{ext}"
         return base_filename
 
+    def __remove_extracted_dir(self, subdir):
+        """Remove an extracted directory to force re-extraction."""
+        package_dir = os.path.join(self.__destination_directory, subdir)
+        if os.path.exists(package_dir):
+            shutil.rmtree(package_dir)
+            logger.info(f"Removed stale extracted directory: {subdir}")
+
+    def __cleanup_old_cached_files(self, base_filename, current_cache_filename):
+        """Remove old SHA-versioned cache files for the same asset."""
+        name, ext = os.path.splitext(base_filename)
+        prefix = f"{name}-"
+        for f in os.listdir(self.__destination_directory):
+            if f.startswith(prefix) and f.endswith(ext) and f != current_cache_filename:
+                old_file = os.path.join(self.__destination_directory, f)
+                try:
+                    os.remove(old_file)
+                    logger.info(f"Removed old cached file: {f}")
+                except OSError as e:
+                    logger.warning(f"Failed to remove old cached file {f}: {e}")
+
     def __download_module_asset(self, url: str, filename: str, module_package):
 
         cache_filename = self.__get_cache_filename(filename, module_package)
@@ -273,14 +310,17 @@ def __download_module_asset(self, url: str, filename: str, module_package):
                     logger.info(f"Using cached: {os.path.basename(zip_file)}")
                     # Still emit some progress to show we're not stuck
                     self.signalPackagingProgress.emit(-1.0, 0)
-                    return zip_file
+                    return zip_file, False
             except (zipfile.BadZipFile, OSError, Exception) as e:
                 logger.warning(f"Existing file '{zip_file}' is invalid ({e}), will re-download")
                 try:
                     os.remove(zip_file)
                 except OSError:
                     pass
 
+        # Clean up old SHA-versioned files for the same asset
+        self.__cleanup_old_cached_files(filename, cache_filename)
+
         # Streaming, so we can iterate over the response.
         timeout = 60
         logger.info(f"Downloading: {os.path.basename(zip_file)}")
@@ -322,7 +362,7 @@ def __download_module_asset(self, url: str, filename: str, module_package):
         # Ensure final progress reflects completion
         self.__emit_progress(force=True)
 
-        return zip_file
+        return zip_file, True
 
     def __emit_progress(self, force: bool = False):
         """Emit download progress as percentage (0-100) or -1 for indeterminate."""

oqtopus/gui/database_connection_widget.py

@@ -223,24 +223,29 @@ def refreshInstalledModules(self):
             beta_text = " \u26a0\ufe0f" if info["beta_testing"] else ""
             display = f"\u2022 <b>{module_label}</b> ({version}){beta_text}"
 
-            # Build tooltip with details
+            # Build tooltip with details (rich HTML for bigger text)
             tooltip_lines = []
-            tooltip_lines.append(f"Module: {module_label}")
-            tooltip_lines.append(f"Schema: {schema}")
-            tooltip_lines.append(f"Version: {version}")
+            tooltip_lines.append(f"<b>Module:</b> {module_label}")
+            tooltip_lines.append(f"<b>Schema:</b> {schema}")
+            tooltip_lines.append(f"<b>Version:</b> {version}")
             if info["beta_testing"]:
-                tooltip_lines.append("\u26a0\ufe0f Beta testing")
+                tooltip_lines.append("\u26a0\ufe0f <b>Beta testing</b>")
             if info["installed_date"]:
                 tooltip_lines.append(
-                    f"Installed: {info['installed_date'].strftime('%Y-%m-%d %H:%M')}"
+                    f"<b>Installed:</b> {info['installed_date'].strftime('%Y-%m-%d %H:%M')}"
                 )
             if info["upgrade_date"]:
                 tooltip_lines.append(
-                    f"Last upgrade: {info['upgrade_date'].strftime('%Y-%m-%d %H:%M')}"
+                    f"<b>Last upgrade:</b> {info['upgrade_date'].strftime('%Y-%m-%d %H:%M')}"
                 )
+            if info.get("parameters") and isinstance(info["parameters"], dict):
+                tooltip_lines.append("<br><b>Parameters:</b>")
+                for param_name, param_value in info["parameters"].items():
+                    tooltip_lines.append(f"&nbsp;&nbsp;{param_name} = {param_value}")
 
+            tooltip_html = "<p style='font-size:11pt'>" + "<br>".join(tooltip_lines) + "</p>"
             label = QLabel(display)
-            label.setToolTip("\n".join(tooltip_lines))
+            label.setToolTip(tooltip_html)
             layout.addWidget(label)
 
         self.installed_modules_groupbox.setVisible(True)

oqtopus/gui/module_selection_widget.py

@@ -267,15 +267,21 @@ def __loadModuleFromZip(self, filename):
     def __packagePrepareTaskFinished(self):
         logger.info("Load package task finished")
 
-        self.module_progressBar.setVisible(False)
-
         if isinstance(self.__packagePrepareTask.lastError, PackagePrepareTaskCanceled):
             logger.info("Load package task was canceled by user.")
-            self.module_information_label.setText(self.tr("Package loading canceled."))
-            QtUtils.setForegroundColor(self.module_information_label, PluginUtils.COLOR_WARNING)
+            # A new task may already be running (user switched versions).
+            # Only update UI if no new task has started.
+            if not self.__packagePrepareTask.isRunning():
+                self.module_progressBar.setVisible(False)
+                self.module_information_label.setText(self.tr("Package loading canceled."))
+                QtUtils.setForegroundColor(
+                    self.module_information_label, PluginUtils.COLOR_WARNING
+                )
             # Don't emit signal_loadingFinished when cancelled - a new load may be starting
             return
 
+        self.module_progressBar.setVisible(False)
+
         if self.__packagePrepareTask.lastError is not None:
             error_text = self.tr("Can't load module package:")
             CriticalMessageBox(

oqtopus/gui/module_widget.py

@@ -13,6 +13,7 @@
 from ..libs.pum.schema_migrations import SchemaMigrations
 from ..utils.plugin_utils import PluginUtils, logger
 from ..utils.qt_utils import CriticalMessageBox, QtUtils
+from .recreate_app_dialog import RecreateAppDialog
 
 DIALOG_UI = PluginUtils.get_ui_class("module_widget.ui")
 
@@ -217,7 +218,13 @@ def __packagePrepareGetPUMConfig(self):
         logger.info(f"PUM config loaded from '{pumConfigFilename}'")
 
         try:
-            self.parameters_groupbox.setParameters(self.__pum_config.parameters())
+            all_params = self.__pum_config.parameters()
+            standard_params = [p for p in all_params if not p.app_only]
+            app_only_params = [p for p in all_params if p.app_only]
+            self.parameters_groupbox.setParameters(standard_params)
+            self.parameters_app_only_groupbox.setParameters(app_only_params)
+            self.parameters_groupbox_upgrade.setParameters(standard_params)
+            self.parameters_app_only_groupbox_upgrade.setParameters(app_only_params)
         except Exception as exception:
             CriticalMessageBox(
                 self.tr("Error"),
@@ -266,7 +273,7 @@ def __installModuleClicked(self):
             return
 
         try:
-            parameters = self.parameters_groupbox.parameters_values()
+            parameters = self.__get_all_parameters()
 
             beta_testing = self.beta_testing_checkbox_pageInstall.isChecked()
             if beta_testing:
@@ -383,7 +390,7 @@ def __upgradeModuleClicked(self):
                         return
 
             try:
-                parameters = self.parameters_groupbox.parameters_values()
+                parameters = self.__get_all_parameters()
 
                 beta_testing = self.beta_testing_checkbox_pageUpgrade.isChecked()
                 if beta_testing:
@@ -485,7 +492,7 @@ def __uninstallModuleClicked(self):
             return
 
         try:
-            parameters = self.parameters_groupbox.parameters_values()
+            parameters = self.__get_all_parameters()
 
             # Start background uninstall operation
             self.__startOperation("uninstall", parameters, {})
@@ -517,7 +524,7 @@ def __rolesClicked(self):
             return
 
         try:
-            parameters = self.parameters_groupbox.parameters_values()
+            parameters = self.__get_all_parameters()
 
             # Start background roles operation
             self.__startOperation("roles", parameters, {})
@@ -563,7 +570,7 @@ def __dropAppClicked(self):
             return
 
         try:
-            parameters = self.parameters_groupbox.parameters_values()
+            parameters = self.__get_all_parameters()
 
             # Start background drop app operation
             self.__startOperation("drop_app", parameters, {})
@@ -594,22 +601,22 @@ def __recreateAppClicked(self):
             ).exec()
             return
 
-        reply = QMessageBox.question(
-            self,
-            self.tr("(Re)create app"),
-            self.tr(
-                "Are you sure you want to recreate the application?\n\n"
-                "This will first drop the app and then create it again, executing the corresponding handlers."
-            ),
-            QMessageBox.StandardButton.Yes | QMessageBox.StandardButton.No,
-            QMessageBox.StandardButton.No,
-        )
+        try:
+            all_params = self.__pum_config.parameters()
+            standard_params = [p for p in all_params if not p.app_only]
+            app_only_params = [p for p in all_params if p.app_only]
+        except Exception as exception:
+            CriticalMessageBox(
+                self.tr("Error"), self.tr("Can't load parameters:"), exception, self
+            ).exec()
+            return
 
-        if reply != QMessageBox.StandardButton.Yes:
+        dialog = RecreateAppDialog(standard_params, app_only_params, self)
+        if dialog.exec() != RecreateAppDialog.DialogCode.Accepted:
             return
 
         try:
-            parameters = self.parameters_groupbox.parameters_values()
+            parameters = dialog.parameters()
 
             # Start background recreate app operation
             self.__startOperation("recreate_app", parameters, {})
@@ -620,6 +627,24 @@ def __recreateAppClicked(self):
             ).exec()
             return
 
+    def __get_all_parameters(self) -> dict:
+        """Collect parameter values from both standard and app_only groupboxes.
+
+        Uses the upgrade-specific groupboxes when on the upgrade page,
+        otherwise uses the install page groupboxes.
+        """
+        values = {}
+        if (
+            self.moduleInfo_stackedWidget.currentWidget()
+            == self.moduleInfo_stackedWidget_pageUpgrade
+        ):
+            values.update(self.parameters_groupbox_upgrade.parameters_values())
+            values.update(self.parameters_app_only_groupbox_upgrade.parameters_values())
+        else:
+            values.update(self.parameters_groupbox.parameters_values())
+            values.update(self.parameters_app_only_groupbox.parameters_values())
+        return values
+
     def __show_error_state(self, message: str, on_label=None):
         """Display an error state and hide the widget content."""
         label = on_label or self.moduleInfo_selected_label
@@ -653,6 +678,10 @@ def __show_install_page(self, version: str):
         # Configure beta testing checkbox based on package source
         self.__configure_beta_testing_checkbox(self.beta_testing_checkbox_pageInstall)
 
+        # On install, both standard and app_only parameters are editable
+        self.parameters_groupbox.setEnabled(True)
+        self.parameters_app_only_groupbox.setEnabled(True)
+
         self.moduleInfo_stackedWidget.setCurrentWidget(self.moduleInfo_stackedWidget_pageInstall)
         # Ensure the stacked widget is visible when showing a valid page
         self.moduleInfo_stackedWidget.setVisible(True)
@@ -727,6 +756,10 @@ def __show_upgrade_page(
         # Configure beta testing checkbox based on package source
         self.__configure_beta_testing_checkbox(self.beta_testing_checkbox_pageUpgrade)
 
+        # On upgrade, standard parameters cannot be changed but remain scrollable
+        self.parameters_groupbox_upgrade.setParametersEnabled(False)
+        self.parameters_app_only_groupbox_upgrade.setParametersEnabled(True)
+
         self.moduleInfo_stackedWidget.setCurrentWidget(self.moduleInfo_stackedWidget_pageUpgrade)
         self.moduleInfo_stackedWidget.setVisible(True)