Skip to content

Expose basemap controls in Settings#806

Merged
ebelo merged 2 commits intomainfrom
fix/settings-basemap-controls
May 6, 2026
Merged

Expose basemap controls in Settings#806
ebelo merged 2 commits intomainfrom
fix/settings-basemap-controls

Conversation

@ebelo
Copy link
Copy Markdown
Owner

@ebelo ebelo commented May 6, 2026

Summary

  • Move the existing Mapbox basemap controls into the visible local-first Settings tab.
  • Keep the legacy backing controls and settings bindings intact for this small consolidation slice.
  • Cover the basemap-control move in pure dock tests and update the QGIS smoke expectation for the local-first Settings placement.

Closes #803.

Tests

  • python3 -m pytest tests/test_qfit_dockwidget_analysis_pure.py -q
  • python3 -m pytest tests/test_qfit_dockwidget_analysis_pure.py tests/test_qgis_smoke.py::QgisSmokeTests::test_dock_widget_contextual_help_smoke tests/test_qgis_smoke.py::QgisSmokeTests::test_dock_widget_defaults_to_local_first_live_path -q
  • python3 -m pytest tests/ -x -q --tb=short

@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented May 6, 2026

Greptile Summary

This PR moves the existing backgroundGroupBox (Mapbox basemap controls) from its legacy position in the scroll dock into the Settings tab of the local-first wizard shell, using the same idempotency-guarded reparent-and-show pattern already established by _install_wizard_filter_controls.

  • qfit_dockwidget.py: A new _install_local_first_basemap_controls method mirrors the filter-controls helper: it guards against double-installation via an id()-keyed flag, removes the widget from its old layout, reparents it to connection_content, renames its title to "Mapbox basemap", and appends it to the Settings tab's outer_layout.
  • tests/test_qfit_dockwidget_analysis_pure.py: A pure-Python test verifies the move (idempotency, removal, reparenting, title, visibility) without requiring a live QGIS session.
  • tests/test_qgis_smoke.py: Updates the live-QGIS smoke assertion for the new basemap location, and separately corrects a pre-existing wrong assertion about the analysisModeComboBox default (see inline comment).

Confidence Score: 4/5

Safe to merge; the basemap reparenting is a contained UI reorganization with no data-path changes.

The production change is a straightforward widget move that mirrors an already-tested pattern. The only thing worth a second look is the bundled analysisModeComboBox smoke-test correction — it is correct, but it is unrelated to the basemap feature and is not explained in the PR description, which makes it slightly harder to audit independently.

tests/test_qgis_smoke.py — contains a second, unrelated assertion fix that deserves a brief explanation alongside the basemap changes.

Important Files Changed

Filename Overview
qfit_dockwidget.py Adds _install_local_first_basemap_controls following the identical guard/reparent/show pattern used by _install_wizard_filter_controls; inserted between the two existing install calls with no side effects on other controls.
tests/test_qfit_dockwidget_analysis_pure.py Adds a pure unit test for the basemap control move, covering idempotency (double-call), widget removal from the old layout, re-parenting, title rename, and the show() call; all assertions are correct.
tests/test_qgis_smoke.py Updates two smoke expectations: the basemap location correctly moves from styleSectionContentWidget to connection_content, and the analysisModeComboBox default is corrected from "None" to "Most frequent starting points" — the latter fixing a pre-existing incorrect assertion unrelated to this feature.

Sequence Diagram

sequenceDiagram
    participant Init as QfitDockWidget.__init__
    participant StartupCoord as DockStartupCoordinator
    participant LocalFirst as _install_live_local_first_dock
    participant Build as _build_local_first_dock_composition
    participant BasemapInstall as _install_local_first_basemap_controls
    participant Layout as connection_content.outer_layout

    Init->>StartupCoord: run() → _configure_analysis_mode_options()
    Note over StartupCoord: analysisModeComboBox created with "None" as default
    Init->>LocalFirst: _install_live_local_first_dock()
    LocalFirst->>Build: _build_local_first_dock_composition()
    Build->>BasemapInstall: _install_local_first_basemap_controls(composition)
    BasemapInstall->>BasemapInstall: guard: already installed for same settings_content?
    BasemapInstall->>Layout: removeWidget(backgroundGroupBox) from old layout
    BasemapInstall->>Layout: backgroundGroupBox.setParent(connection_content)
    BasemapInstall->>Layout: backgroundGroupBox.setTitle("Mapbox basemap")
    BasemapInstall->>Layout: outer_layout().addWidget(backgroundGroupBox)
    BasemapInstall->>Layout: backgroundGroupBox.show()
    BasemapInstall->>BasemapInstall: set _local_first_basemap_controls_installed = True
    Build->>Build: _bind_wizard_analysis_mode_controls(composition)
    Note over Build: promotes analysisModeComboBox from "None" to "Most frequent starting points"
Loading

Reviews (1): Last reviewed commit: "Expose basemap controls in Settings" | Re-trigger Greptile

Comment thread tests/test_qgis_smoke.py Outdated
Comment on lines +325 to +328
self.assertEqual(
dock.analysisModeComboBox.currentText(),
"Most frequent starting points",
)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Unrelated test-expectation change bundled silently

The assertion for analysisModeComboBox.currentText() is changed from "None" to "Most frequent starting points", but _install_local_first_basemap_controls never touches the analysis mode combo. The real reason the old assertion was wrong is that _bind_wizard_analysis_mode_controls (already called before this PR) explicitly promotes "None" to options[0] when the current value is not in the valid options list, so the effective default has always been "Most frequent starting points" after full initialization. This fix is correct, but it is a pre-existing test bug that is unrelated to the basemap move and has no mention in the PR description — a note explaining this would help reviewers distinguish it from a behavior regression.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in cf34b23: I removed the unrelated analysis-mode expectation from this smoke test instead of changing its expected value. The test now stays focused on the local-first basemap placement introduced by this PR.

@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud Bot commented May 6, 2026

@ebelo ebelo merged commit 6783bc2 into main May 6, 2026
8 checks passed
@ebelo ebelo deleted the fix/settings-basemap-controls branch May 6, 2026 20:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Move Mapbox background-map configuration to Settings

1 participant