Skip to content

refactor: unify Signals plugin (absorbs FilteredCharts)#112

Open
uavinda wants to merge 7 commits into
mainfrom
signals-unified
Open

refactor: unify Signals plugin (absorbs FilteredCharts)#112
uavinda wants to merge 7 commits into
mainfrom
signals-unified

Conversation

@uavinda
Copy link
Copy Markdown
Member

@uavinda uavinda commented Apr 23, 2026

Summary

Absorbs the FilteredCharts plugin into the Signals plugin, replacing the two side-by-side plugins with one unified, data-driven widget. Supersedes PR #111.

What changed

Removed

  • src/Plugins/Tools/FilteredCharts/ — entire directory deleted
  • src/Plugins/Tools/Signals/SignalButton.qml — old static button
  • src/Plugins/Tools/Signals/SignalsView.qml — replaced by ChartsView.qml
  • FilteredCharts entry from src/Plugins/Tools/CMakeLists.txt

New files in src/Plugins/Tools/Signals/

File Purpose
ChartsView.qml QtCharts renderer (replaces both SignalsView + FcChartsView); adds per-page speed cycling [0.2, 0.5, 1, 2, 4]
ColorChooser.qml 12×4 palette grid (was FcColorChooser)
FilterRunningAvg.qml Running-average filter Fact; filterValue(v) / resetState() API
FilterKalmanSimple.qml Simple Kalman filter Fact; same API
MenuFilters.qml Per-item filter selector; applyFilters(v), resetFilterState()
MenuColor.qml Color picker Fact routing to ColorChooser page
MenuItem.qml Per-item editor Fact (bind expr, title, color, filters, warn, alarm, save-to-mandala)
MenuPage.qml Per-page Fact (name, pinned, speed, items list); aggregates warning/alarm state
MenuSet.qml Per-set Fact holding up to 10 pages
SignalsMenu.qml Sets editor (mirrors NumbersMenu pattern); loadSettings() / saveSettings() / buildDefaultSet() / migrateLegacy()
SignalsMenuPopup.qml FactMenuPopup wrapper for the sets editor
PageButton.qml Tab button: alarm→red, warning→orange, checked→yellow, pinned→cyan border

Modified

  • src/Plugins/Tools/Signals/Signals.qml — rewritten as the unified root widget

Architecture

signals.json  ←→  SignalsMenu (sets editor)
                       ↓  builds
                  MenuSet → MenuPage → MenuItem
                                           ↓ evaluates
                                       bind expr → FilterChain → warn/alarm
                                                                      ↓
                                                               PageButton color

Signals.qml (signalsWidget)
  ├── Repeater(pinnedPages) → ChartsView (stacked above)
  ├── ChartsView id:singleChart (main view, active page)
  └── BottomBar
        ├── Repeater(activePages) → PageButton (tabs)
        ├── set-label button → openSetsEditor()
        └── speed button → cycles currentPage.speed

JSON schema (signals.json):

{
  "active": { "signals": 0 },
  "sets": [{
    "title": "default",
    "pages": [{
      "name": "R", "pin": false, "speed": 1.0,
      "items": [{ "bind": "mandala.est.att.roll.value", "title": "roll", "color": "", "filters": [], "warn": "", "alarm": "", "act": "", "save": "" }]
    }]
  }]
}

Legacy migration: old {page, signalas} format is silently converted on first load.

Default set: 10 pages matching the old hardcoded buttons (R/P/Y/Axy/Az/G/Pt/Ctr/RC/Usr) are generated automatically if no signals.json exists.

Testing

  • cmake --build build --target Signals — clean build, no errors (only pre-existing macOS SDK version ld warnings unrelated to this change)

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Unifies the former Signals and FilteredCharts plugins into a single, JSON-configured “Signals” chart widget with per-set/page/item editing and optional filtering.

Changes:

  • Replaces the old static SignalsView/button approach with a data-driven Signals.qml + ChartsView.qml renderer.
  • Adds a full Fact-based editor for sets/pages/items (SignalsMenu*, MenuSet/MenuPage/MenuItem) and filter components (running average + simple Kalman).
  • Introduces UI components for page tabs and color selection (PageButton.qml, ColorChooser.qml, MenuColor.qml).

Reviewed changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
src/Plugins/Tools/Signals/SignalsView.qml Removed legacy chart view implementation.
src/Plugins/Tools/Signals/SignalsMenuPopup.qml Adds pinned Fact menu popup wrapper for the sets editor.
src/Plugins/Tools/Signals/SignalsMenu.qml Adds top-level sets editor that loads/saves signals.json and migrates legacy format.
src/Plugins/Tools/Signals/Signals.qml Rewrites Signals root widget to render active/pinned pages and integrate the sets editor.
src/Plugins/Tools/Signals/REFACTOR_NOTES.md Documents the refactor mapping and intended JSON/Fact-tree design.
src/Plugins/Tools/Signals/PageButton.qml Adds per-page tab button with warning/alarm/pinned styling + tooltip.
src/Plugins/Tools/Signals/MenuSet.qml Implements a set Fact that manages up to 10 pages.
src/Plugins/Tools/Signals/MenuPage.qml Implements per-page Fact editor and aggregation of warning/alarm state.
src/Plugins/Tools/Signals/MenuItem.qml Implements per-chart item editor and telemetry-driven value evaluation/filtering.
src/Plugins/Tools/Signals/MenuFilters.qml Implements filter selection and parameter persistence per item.
src/Plugins/Tools/Signals/MenuColor.qml Refactors color selection into a Fact that opens a chooser page.
src/Plugins/Tools/Signals/FilterRunningAvg.qml Adds running-average filter Fact with stateful filterValue() API.
src/Plugins/Tools/Signals/FilterKalmanSimple.qml Adds simple Kalman filter Fact with stateful filterValue() API.
src/Plugins/Tools/Signals/ColorChooser.qml Adds palette UI page for selecting a chart color.
src/Plugins/Tools/Signals/ChartsView.qml Adds the unified QtCharts renderer (replacement for SignalsView/FcChartsView).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/Plugins/Tools/Signals/Signals.qml Outdated
Comment thread src/Plugins/Tools/Signals/Signals.qml Outdated
Comment thread src/Plugins/Tools/Signals/SignalsMenu.qml Outdated
Comment thread src/Plugins/Tools/Signals/ChartsView.qml Outdated
Comment thread src/Plugins/Tools/Signals/MenuItem.qml Outdated
Comment thread src/Plugins/Tools/Signals/ColorChooser.qml Outdated
@uavinda
Copy link
Copy Markdown
Member Author

uavinda commented Apr 23, 2026

@copilot apply changes based on the comments in this thread

Copy link
Copy Markdown

Copilot AI commented Apr 23, 2026

@copilot apply changes based on the comments in this thread

Applied all six review comments in commit 2346352:

  1. Signals.qml:292 — Removed the direct singleChart.speedFactorValue = next assignment that was breaking the Qt.binding(...). The speed button now only sets currentPage.speed; the existing binding propagates the change automatically.

  2. SignalsMenu.qml:100 — Added if (!c) continue; after createSetFact() in loadSettings(), and if (!c) return; in the "Add set" action, guarding against a null return when MenuSet.qml fails to load.

  3. MenuItem.qml:141-165 — Introduced _bindFn, _warnFn, _alarmFn cached-function properties. Each is compiled once (via eval-created closure for the bind expression, new Function() for warn/alarm) when the expression text changes, and updateValue() now simply calls the cached function instead of re-evaluating the expression string on every telemetry tick.

  4. ColorChooser.qml:1-8 — Added the standard project LGPL license header.

(The duplicate-root-widget and fact.currentValue issues were already resolved in the previous commit.)

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 22 out of 22 changed files in this pull request and generated 4 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

item.color = asString(source.color, "")
item.filters = normalizeFilters(source.filters)
item.warning = asString(source.warning !== undefined ? source.warning : source.warn, "")
item.save = asString(source.save, "")
Copy link

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

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

normalizeItem() normalizes bind but does not normalize save. If a settings file contains mandala.sns.scr.foo.value (or other simple mandala variants), SignalsView will later call mandala.fact(saveTarget, true) with the un-normalized string and likely fail. Consider running normalizeBind() on source.save as well so the persisted/runtime value is consistently the bare path (e.g. sns.scr.foo).

Suggested change
item.save = asString(source.save, "")
item.save = normalizeBind(asString(source.save, ""))

Copilot uses AI. Check for mistakes.
MenuPage.qml
MenuSet.qml
MenuSets.qml
PageButton.qml
Copy link

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

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

PageButton.qml is included in QRC_QML, but it is not referenced anywhere in the repo (no PageButton usages found). If it’s truly unused, removing it from the resource list (and deleting the file) will avoid shipping dead UI variants and reduce maintenance overhead.

Suggested change
PageButton.qml

Copilot uses AI. Check for mistakes.
Comment on lines +28 to +46
TextButton {
Layout.fillHeight: true
checkable: true
ButtonGroup.group: buttonGroup

property var values: []
onActivated: signals.facts=Qt.binding(function(){return values})

toolTip: getToolTip(values)

function getToolTip(facts)
{
var s=[]
for(var i=0;i<facts.length;++i){
var fact=facts[i]
s.push("<font color='"+fact.opts.color+"'>"+fact.descr+"</font>")
}
return s.join("<br>")
}
Copy link

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

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

This file appears to be an unused duplicate of SignalButton.qml (no references to MenuColor/MenuColor.qml found, and it’s not included in CMakeLists.txt resources). If it’s leftover from the refactor, consider removing it to avoid confusion about the active tab/button implementation.

Suggested change
TextButton {
Layout.fillHeight: true
checkable: true
ButtonGroup.group: buttonGroup
property var values: []
onActivated: signals.facts=Qt.binding(function(){return values})
toolTip: getToolTip(values)
function getToolTip(facts)
{
var s=[]
for(var i=0;i<facts.length;++i){
var fact=facts[i]
s.push("<font color='"+fact.opts.color+"'>"+fact.descr+"</font>")
}
return s.join("<br>")
}
// Compatibility wrapper: keep MenuColor as an entry point, but delegate the
// actual tab/button implementation to the canonical SignalButton component.
SignalButton {
property var values: []
onActivated: signals.facts = Qt.binding(function() { return values })

Copilot uses AI. Check for mistakes.
Comment on lines +147 to +157
function evaluateValue(fact, bind)
{
if (fact && fact.value !== undefined)
return fact.value

if (!bind)
return NaN

try {
return eval(normalizeBindText(bind))
} catch (error) {
Copy link

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

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

evaluateValue() currently does eval(normalizeBindText(bind)), but normalizeBindText() strips mandala. and .value, producing paths like est.att.roll. In most of the codebase, mandala facts are accessed via mandala.<path>.value, so this will evaluate to undefined/throw and the chart will fall back to 0. Consider treating simple binds as mandala paths: resolve/caches apxContext.fleet.current.mandala.fact(path, true) in seriesState and read .value (or, at minimum, eval("mandala." + path + ".value")).

Copilot uses AI. Check for mistakes.
uavinda and others added 2 commits April 24, 2026 16:14
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
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.

3 participants