Open
Conversation
- Add useOsdPreview composable for reactive preview buffer generation - Port searchLimitsElement and drawByOrder helpers from legacy OSD - Generate previewRows computed property for grid-based rendering - Use FONT.draw() for character image generation - Watch store state for automatic re-rendering - Add useOsdRuler composable for ruler overlay drawing - Port rulerConfig, axis drawing helpers from legacy OSD - Accept Vue refs instead of DOM queries - Handle window resize with debounced requestAnimationFrame - Update OsdTab.vue for grid-based preview rendering - Replace absolute-positioned preview-char with row/cell grid structure - Integrate useOsdPreview and useOsdRuler composables - Remove legacy OSD.updateOsdView() call - Add zoom CSS and grid cell highlighting styles - Update stores/osd.js with virtual mode MSP support - Extract osd_constants.js for shared constants
- Port full drag/drop logic from legacy OSD.GUI.preview - onDragStart: pass x/y coordinates and field index via dataTransfer - onDropCell: position clamping for string and array-type previews - Boundary detection for wrap-around and edge overflow - Drag image support (disabled on Linux per legacy behavior) - Add per-cell drag events (onDragOverCell, onDragLeaveCell) - Visual feedback with background highlight on drag over - Add mouse hover cross-highlighting - onCellMouseEnter/onCellMouseLeave update highlightedField - Preview cells and element list highlight simultaneously - Export searchLimitsElement from useOsdPreview for position clamping
- Create osd_positions.js with 15-position positionConfigs (3x5 grid) - Refactored to accept displaySize parameter instead of global reference - getPresetGridCells helper for building selector UI - Add preset position popover to OsdTab.vue - Click "..." button toggles grid popover - 3x5 grid with dot indicators and hover effects - Click-outside handler for popover dismissal - Port applyPresetPosition from legacy with full logic - Element size calculation (string/array preview) - Boundary clamping - Growth direction search for collision avoidance - Array offset adjustment for advanced elements
- Vue-rendered font character preview using reactive computed property
- fontDataVersion trigger for non-reactive FONT.data updates
- v-for rendering of character image URLs (replaces jQuery DOM append)
- Font preset loading via fetch API
- Loads MCM files from resources/osd/2/{file}.mcm
- Watches selectedFontPreset for dropdown changes
- Updates font version info label
- Font upload with progress tracking
- jQuery progress shim (val() method) for FONT.upload compatibility
- Reactive uploadProgress and uploadProgressLabel refs
- LogoManager integration for custom logo
- Initializes on font manager open
- replaceLogoImage: open image, replace in font, redraw preview
- Import FONT, SYM, and LogoManager into OsdTab.vue
Auto-fix 219 eslint errors (indentation, trailing commas) in: - OsdTab.vue - useOsdPreview.js - useOsdRuler.js - osd_positions.js
The Pinia OSD store's `isSupported` computed property checks for `haveMax7456Video || isMspDevice`, but setupVirtualOSD() was not setting either flag. This caused the OSD tab to show the "unsupported" message when connected in virtual mode.
- Rulers: add dynamic margins/padding when rulers are toggled ON, matching legacy initializeRulers() behavior (paddingTop on container, marginLeft/Right on preview) so ruler labels sit outside the preview - Rulers: decouple ruler visibility from drag/click state—rulers now only show when the Rulers checkbox is checked, not on mousedown - Grid: cell border grid still appears on mousedown via :active CSS - Toolbar: add position:fixed to bottom toolbar matching other Vue tabs - Toggles: use togglesmall class for warnings/stats toggle switches - Font: call updatePreviewBuffer() after font load for correct render - Font: add selectedFont watcher to sync font dropdown with preview - Preview: always render <img> with SVG fallback to prevent grid collapse - Preview: fix highlighted class to skip null field comparisons - UI: various i18n, context menu, timer layout, and styling improvements
- Ported all styles from `osd.less` to `OsdTab.vue` scoped styles - Removed `src/css/tabs/osd.less` - Removed legacy `src/tabs/osd.html` - Removed `osd.less` import from `browserMain.js`
- Mount OSD tab as Vue component in main.js - Initialize font data in store - Handle missing LogoManager gracefully in legacy code - Prevent overwriting FC features in virtual mode
Contributor
WalkthroughAdds a new Vue OSD tab (OsdTab.vue), Pinia OSD store, two composables (useOsdPreview, useOsdRuler), static OSD constants/positions, updates legacy osd JS exports and app tab wiring, removes legacy OSD HTML/CSS, and adjusts virtual/backend handling for Max7456 and virtual mode initialization. Changes
Sequence DiagramsequenceDiagram
participant User as User
participant OsdTab as OsdTab.vue
participant Store as useOsdStore
participant Preview as useOsdPreview
participant Ruler as useOsdRuler
participant Backend as FlightController
User->>OsdTab: open OSD tab
OsdTab->>Store: fetchOsdConfig()
Store->>Backend: request OSD config (MSP / virtual)
Backend-->>Store: OSD data
Store->>Store: decode & init state
Store-->>OsdTab: state ready
OsdTab->>Preview: updatePreviewBuffer()
Preview->>Store: read displayItems / profiles / displaySize
Preview->>Preview: build z-ordered buffer
Preview-->>OsdTab: previewRows
OsdTab->>Ruler: drawRulers()
Ruler->>Ruler: compute grid geometry & draw axes
Ruler-->>OsdTab: overlay rendered
User->>OsdTab: move element / change settings
OsdTab->>Store: updateDisplayItemPosition / updateDisplayItemVisibility
Store->>Preview: trigger updatePreviewBuffer
Preview-->>OsdTab: updated preview
User->>OsdTab: save
OsdTab->>Store: saveOsdConfig()
Store->>Backend: send encoded MSP packets
Backend-->>Store: ack
Store-->>OsdTab: save complete
OsdTab->>User: notify
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/js/tabs/osd.js (1)
3052-3056:⚠️ Potential issue | 🟠 MajorAvoid +1 in advanced element width/height; it shifts centering.
The legacy positioning math uses span calculations without the +1 to keep elements visually centered. This change will move elements off-center for advanced previews.
🛠️ Suggested fix
- // Note: Using +1 for inclusive range calculation, which is mathematically - // correct for counting occupied positions. An element spanning coordinates - // -2 to 2 occupies 5 positions: [-2, -1, 0, 1, 2] - elementWidth = limits.maxX - limits.minX + 1; - elementHeight = limits.maxY - limits.minY + 1; + // Use legacy span calculation to preserve visual centering. + elementWidth = limits.maxX - limits.minX; + elementHeight = limits.maxY - limits.minY;Based on learnings: In OSD positioning systems, element width/height should use
limits.maxX - limits.minXandlimits.maxY - limits.minYbecause adding +1 breaks centering.
🤖 Fix all issues with AI agents
In `@src/components/tabs/OsdTab.vue`:
- Around line 1517-1542: The CSS ruleset for .preset-pos-btn (including its
:hover and :active states) is duplicated; remove the second occurrence of the
entire block (the later .preset-pos-btn, .preset-pos-btn:hover, and
.preset-pos-btn:active rules) so only one definition remains, ensuring no other
selectors or differences are accidentally deleted and preserving the original
spacing and comments around the first .preset-pos-btn block.
- Line 114: In OsdTab.vue replace the hardcoded user-facing strings "Align to
position" and "Choose Position" with i18n keys using this.$t or $t (e.g.
$t('osd.alignToPosition') and $t('osd.choosePosition')) wherever they appear in
the template, and then add matching entries ("osd.alignToPosition" and
"osd.choosePosition") to locales/en/messages.json; ensure you use the same key
names referenced in the template so the translations render correctly.
In `@src/js/tabs/osd_constants.js`:
- Around line 1-3: Remove the unused imported symbol i18n from the top of the
module (the import from the localization module) to satisfy linting; locate the
import statement that brings in i18n and delete it (or remove i18n from the
named import if other symbols are used) so the module no longer has an unused
import.
In `@src/js/tabs/osd_positions.js`:
- Around line 81-89: RMC's coords function can produce x <= 0; change its x
calculation to use the same guard as the other right-column entries by replacing
the direct expression ds.x - 1 - w with Math.max(1, ds.x - w - 1) inside the
coords arrow function for RMC so the x coordinate never drops below 1 while
leaving the y calculation and other properties unchanged.
In `@src/stores/osd.js`:
- Around line 325-326: The ESLint indentation warnings are caused by misaligned
lines around the comment and the statement using Object.assign(state,
OSD.data.state); fix them by re-indenting the comment ("// Update reactive state
object") and the Object.assign line to match the surrounding block indentation
level (same indent as adjacent statements in the function or block where state
is updated), ensuring consistent spacing/tabs per the project's ESLint rules;
apply the same adjustment to the other occurrence at the noted location (also
where Object.assign and OSD.data.state are used).
- Around line 9-13: The import list in src/stores/osd.js includes an unused
symbol API_VERSION_1_45 which triggers a linter warning; remove API_VERSION_1_45
from the named imports (leaving API_VERSION_1_46 and API_VERSION_1_47) in the
import statement that references "../js/data_storage" so only actually-used
symbols are imported.
- Around line 51-55: The store defines parameters as a reactive object but never
initializes or restores parameters.overlayRadioMode even though encodeOther()
pushes it; add overlayRadioMode to the reactive parameters (with an appropriate
default) and restore/sync it when loading legacy or saved data so it isn't reset
on save — update the initializer that builds parameters and the load/deserialize
path that reads legacy values (the same place that handles other parameters) to
assign parameters.overlayRadioMode, and ensure any save/serialize logic still
includes it (encodeOther() already does).
🧹 Nitpick comments (5)
src/js/tabs/osd_positions.js (1)
150-165:getPresetGridCellsperforms a linear scan per cell — fine for 15 entries but could be simplified.This is a minor nit. A reverse-lookup map would be cleaner, but given the small dataset size this is perfectly acceptable as-is.
Optional: build a lookup map
export function getPresetGridCells() { + const lookup = new Map(); + for (const [key, cfg] of Object.entries(positionConfigs)) { + lookup.set(`${cfg.gridPos[0]},${cfg.gridPos[1]}`, { key, label: cfg.label }); + } const cells = []; for (let row = 0; row < 5; row++) { for (let col = 0; col < 3; col++) { - const entry = Object.entries(positionConfigs).find( - ([, cfg]) => cfg.gridPos[0] === col && cfg.gridPos[1] === row, - ); + const entry = lookup.get(`${col},${row}`); cells.push({ col, row, - key: entry ? entry[0] : null, - label: entry ? entry[1].label : "", + key: entry?.key ?? null, + label: entry?.label ?? "", }); } } return cells; }src/components/tabs/OsdTab.vue (4)
946-947: Duplicate comments onloadConfigandsaveConfig.Both functions have their leading comment duplicated.
Fix
-// Load OSD configuration from FC // Load OSD configuration from FC async function loadConfig() {-// Save OSD configuration to FC // Save OSD configuration to FC async function saveConfig() {Also applies to: 963-964
676-680:onPositionChangeis defined but never called.This function isn't referenced anywhere in the template or script. If it's intended for future use, consider removing it to avoid dead code; otherwise, wire it up where needed.
#!/bin/bash # Verify onPositionChange is not referenced elsewhere rg -n 'onPositionChange' --type=vue --type=js
235-235: Consider extracting the inline SVG fallback to a constant.The long inline data URI for the empty SVG placeholder makes the template harder to read.
Extract to a constant
In the
<script setup>section:const EMPTY_CELL_SRC = "data:image/svg+xml;utf8,<svg width='12' height='18' xmlns='http://www.w3.org/2000/svg'></svg>";In the template:
-<img :src="cell.img || 'data:image/svg+xml;utf8,<svg width=\'12\' height=\'18\' xmlns=\'http://www.w3.org/2000/svg\'></svg>'" draggable="false" /> +<img :src="cell.img || EMPTY_CELL_SRC" draggable="false" />
570-574:effectiveShowRulersis a redundant computed — it just wrapsshowRulers.Unless there's a plan to add additional logic (e.g., combining with another flag), you could pass
showRulersdirectly touseOsdRuler.
| class="context-menu-item-display" | ||
| @click="showPresetSubmenu = !showPresetSubmenu" | ||
| > | ||
| <span>Align to position</span> |
Contributor
There was a problem hiding this comment.
Hardcoded English strings should use i18n.
"Align to position" (line 114) and "Choose Position" (line 125) are hardcoded instead of using $t(). All other user-facing text in this component uses the i18n system.
Proposed fix
- <span>Align to position</span>
+ <span>{{ $t('osdPresetAlignToPosition') }}</span>- <div class="preset-popover-title">Choose Position</div>
+ <div class="preset-popover-title">{{ $t('osdPresetChoosePosition') }}</div>Don't forget to add the corresponding keys to locales/en/messages.json.
Also applies to: 125-125
🤖 Prompt for AI Agents
In `@src/components/tabs/OsdTab.vue` at line 114, In OsdTab.vue replace the
hardcoded user-facing strings "Align to position" and "Choose Position" with
i18n keys using this.$t or $t (e.g. $t('osd.alignToPosition') and
$t('osd.choosePosition')) wherever they appear in the template, and then add
matching entries ("osd.alignToPosition" and "osd.choosePosition") to
locales/en/messages.json; ensure you use the same key names referenced in the
template so the translations render correctly.
Comment on lines
51
to
55
| const parameters = reactive({ | ||
| cameraFrameWidth: 24, | ||
| cameraFrameHeight: 11, | ||
| }); | ||
|
|
Contributor
There was a problem hiding this comment.
Persist overlayRadioMode; it’s encoded but never stored.
encodeOther() pushes parameters.overlayRadioMode, but the store never initializes or syncs it from legacy data. This will reset the value on save.
🛠️ Suggested fix
- const parameters = reactive({
- cameraFrameWidth: 24,
- cameraFrameHeight: 11,
- });
+ const parameters = reactive({
+ overlayRadioMode: 0,
+ cameraFrameWidth: 24,
+ cameraFrameHeight: 11,
+ });- if (OSD.data.parameters) {
- parameters.cameraFrameWidth = OSD.data.parameters.cameraFrameWidth;
- parameters.cameraFrameHeight = OSD.data.parameters.cameraFrameHeight;
- }
+ if (OSD.data.parameters) {
+ parameters.overlayRadioMode = OSD.data.parameters.overlayRadioMode ?? 0;
+ parameters.cameraFrameWidth = OSD.data.parameters.cameraFrameWidth;
+ parameters.cameraFrameHeight = OSD.data.parameters.cameraFrameHeight;
+ }🤖 Prompt for AI Agents
In `@src/stores/osd.js` around lines 51 - 55, The store defines parameters as a
reactive object but never initializes or restores parameters.overlayRadioMode
even though encodeOther() pushes it; add overlayRadioMode to the reactive
parameters (with an appropriate default) and restore/sync it when loading legacy
or saved data so it isn't reset on save — update the initializer that builds
parameters and the load/deserialize path that reads legacy values (the same
place that handles other parameters) to assign parameters.overlayRadioMode, and
ensure any save/serialize logic still includes it (encodeOther() already does).
src/stores/osd.js
Outdated
Comment on lines
325
to
326
| // Update reactive state object | ||
| Object.assign(state, OSD.data.state); |
Contributor
There was a problem hiding this comment.
Fix eslint indentation warnings.
🧹 Suggested fix
- // Update reactive state object
- Object.assign(state, OSD.data.state);
+ // Update reactive state object
+ Object.assign(state, OSD.data.state);- promises.push(MSP.promise(MSPCodes.MSP_SET_OSD_CONFIG, encodeStatistics(stat)));
+ promises.push(MSP.promise(MSPCodes.MSP_SET_OSD_CONFIG, encodeStatistics(stat)));Also applies to: 501-501
🧰 Tools
🪛 ESLint
[error] 325-325: Expected indentation of 16 spaces but found 17.
(indent)
[error] 326-326: Expected indentation of 16 spaces but found 17.
(indent)
🤖 Prompt for AI Agents
In `@src/stores/osd.js` around lines 325 - 326, The ESLint indentation warnings
are caused by misaligned lines around the comment and the statement using
Object.assign(state, OSD.data.state); fix them by re-indenting the comment ("//
Update reactive state object") and the Object.assign line to match the
surrounding block indentation level (same indent as adjacent statements in the
function or block where state is updated), ensuring consistent spacing/tabs per
the project's ESLint rules; apply the same adjustment to the other occurrence at
the noted location (also where Object.assign and OSD.data.state are used).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/components/tabs/OsdTab.vue`:
- Around line 1084-1087: Replace the hardcoded English string "Upload failed"
used in the catch block that sets uploadProgressLabel.value with the i18n
message lookup (use the component's i18n method or $t) so the label uses a
localization key (e.g., "osd.uploadFailed"); then add that key and its English
value to locales/en/messages.json. Locate the catch block around
uploadProgressLabel in OsdTab.vue and update it to set uploadProgressLabel.value
using the i18n key, and ensure locales/en/messages.json contains the
corresponding "osd.uploadFailed": "Upload failed" entry.
In `@src/stores/osd.js`:
- Around line 90-101: The initData function resets alarms to an array but alarms
was originally created as a ref to an object and downstream code (encodeOther,
code that reads alarms.value.rssi / alarms.value.cap etc.) expects an object;
change initData so alarms.value is reset to an object with the expected keys (or
an empty object) instead of [], e.g. set alarms.value = {} (or { rssi: ..., cap:
... } if defaults are needed), and ensure any other initialization in initData
(videoSystem, unitMode, displayItems, etc.) remains consistent with their
original ref types so fetchOsdConfig and encodeOther see the correct shapes.
🧹 Nitpick comments (2)
src/components/tabs/OsdTab.vue (2)
651-653:isFieldVisibleandonPositionChangeare defined but never referenced.Neither function is called anywhere in the template or other script code. They appear to be dead code from the migration.
Proposed cleanup
-// Check if a field is visible in current preview profile -function isFieldVisible(field) { - return field.isVisible[previewProfile.value]; -} - // Check if field is highlighted-// Handle position change -function onPositionChange(field) { - trackChange("position", field.name); - updatePreview(); -} - // Handle video system changeAlso applies to: 686-689
956-957: Duplicate comments on consecutive lines.Lines 956–957 and 973–974 each have the same comment repeated twice.
Proposed fix
-// Load OSD configuration from FC // Load OSD configuration from FC async function loadConfig() {-// Save OSD configuration to FC // Save OSD configuration to FC async function saveConfig() {Also applies to: 973-974
Comment on lines
+1084
to
+1087
| } catch (err) { | ||
| console.error("Font upload failed:", err); | ||
| uploadProgressLabel.value = "Upload failed"; | ||
| } |
Contributor
There was a problem hiding this comment.
Hardcoded English "Upload failed" string.
Line 1086 uses a raw English string instead of an i18n key, inconsistent with the rest of the component.
Proposed fix
} catch (err) {
console.error("Font upload failed:", err);
- uploadProgressLabel.value = "Upload failed";
+ uploadProgressLabel.value = i18n.getMessage("osdSetupUploadFontFailed");
}Add the corresponding key to locales/en/messages.json.
🤖 Prompt for AI Agents
In `@src/components/tabs/OsdTab.vue` around lines 1084 - 1087, Replace the
hardcoded English string "Upload failed" used in the catch block that sets
uploadProgressLabel.value with the i18n message lookup (use the component's i18n
method or $t) so the label uses a localization key (e.g., "osd.uploadFailed");
then add that key and its English value to locales/en/messages.json. Locate the
catch block around uploadProgressLabel in OsdTab.vue and update it to set
uploadProgressLabel.value using the i18n key, and ensure
locales/en/messages.json contains the corresponding "osd.uploadFailed": "Upload
failed" entry.
Comment on lines
+90
to
+101
| function initData() { | ||
| videoSystem.value = null; | ||
| unitMode.value = null; | ||
| alarms.value = []; | ||
| statItems.value = []; | ||
| warnings.value = []; | ||
| displayItems.value = []; | ||
| timers.value = []; | ||
| lastPositions.value = {}; | ||
| preview.value = []; | ||
| tooltips.value = []; | ||
| osdProfiles.value = { number: 1, selected: 0 }; |
Contributor
There was a problem hiding this comment.
alarms type inconsistency: initialized as ref({}) but reset to [] in initData.
Line 15 initializes alarms as ref({}) (object), but initData sets it to [] (array). Downstream code in encodeOther accesses alarms.value.rssi, alarms.value.cap, etc., which expects an object. After initData() runs, these would be undefined on an array — safe in practice since fetchOsdConfig overwrites it, but the inconsistency is confusing.
Proposed fix
function initData() {
videoSystem.value = null;
unitMode.value = null;
- alarms.value = [];
+ alarms.value = {};
statItems.value = [];📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function initData() { | |
| videoSystem.value = null; | |
| unitMode.value = null; | |
| alarms.value = []; | |
| statItems.value = []; | |
| warnings.value = []; | |
| displayItems.value = []; | |
| timers.value = []; | |
| lastPositions.value = {}; | |
| preview.value = []; | |
| tooltips.value = []; | |
| osdProfiles.value = { number: 1, selected: 0 }; | |
| function initData() { | |
| videoSystem.value = null; | |
| unitMode.value = null; | |
| alarms.value = {}; | |
| statItems.value = []; | |
| warnings.value = []; | |
| displayItems.value = []; | |
| timers.value = []; | |
| lastPositions.value = {}; | |
| preview.value = []; | |
| tooltips.value = []; | |
| osdProfiles.value = { number: 1, selected: 0 }; |
🤖 Prompt for AI Agents
In `@src/stores/osd.js` around lines 90 - 101, The initData function resets alarms
to an array but alarms was originally created as a ref to an object and
downstream code (encodeOther, code that reads alarms.value.rssi /
alarms.value.cap etc.) expects an object; change initData so alarms.value is
reset to an object with the expected keys (or an empty object) instead of [],
e.g. set alarms.value = {} (or { rssi: ..., cap: ... } if defaults are needed),
and ensure any other initialization in initData (videoSystem, unitMode,
displayItems, etc.) remains consistent with their original ref types so
fetchOsdConfig and encodeOther see the correct shapes.
…aracter code retrieval to `codePointAt`, and remove unused offset calculations.
|
Contributor
|
🎉 Do you want to test this code? 🎉 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/components/tabs/OsdTab.vue`:
- Around line 1124-1143: The selectedFont watcher is causing loadFontPreset to
run twice because assigning selectedFontPreset.value triggers its watcher (which
calls loadFontPreset) and the watcher also calls loadFontPreset directly; change
the selectedFont watcher (the block that sets selectedFontPreset.value and then
calls loadFontPreset) to first check whether selectedFontPreset.value already
equals newVal and only call loadFontPreset directly when they are already equal
(otherwise just set selectedFontPreset.value and let the selectedFontPreset
watcher handle loading), referencing the selectedFont watcher,
selectedFontPreset watcher, and the loadFontPreset function to locate where to
add the guard.
- Around line 560-562: The variables _fcStore, _navigationStore, and _reboot are
declared but unused: either remove the declarations (the calls to
useFlightControllerStore(), useNavigationStore(), and useReboot()) or keep them
and add a one-line comment explaining their intended future side-effect/usage;
locate the declarations near the top of the component (symbols: _fcStore,
_navigationStore, _reboot) and apply the change so no unused imports/variables
remain and linter warnings disappear.
🧹 Nitpick comments (6)
src/components/tabs/OsdTab.vue (6)
529-529: Inconsistent i18n usage:i18n.getMessage()vs$t().The rest of the template consistently uses
$t()(i18next-vue's composition API), but this line directly callsi18n.getMessage(). This should use$t()for consistency and to ensure proper reactivity if locale changes.Proposed fix
- v-html="i18n.getMessage('osdSetupFontManagerTitle')" + v-html="$t('osdSetupFontManagerTitle')"
698-701:onPositionChangeis defined but never called.This handler is not wired to any template event or called from other functions. In the legacy code, this likely handled a position
<input>change event. If the position input was intentionally removed (per the comment on line 91), this function should also be removed.Remove dead code
-// Handle position change -function onPositionChange(field) { - trackChange("position", field.name); - updatePreview(); -}
1032-1039: Bare expressionfontDataVersion.value;for reactivity tracking is fragile.While this works to establish a reactive dependency, linters or future contributors may remove this "no-op" expression. A more explicit pattern makes the intent clear.
Proposed fix
const fontCharacterUrls = computed(() => { - // Trigger reactivity on fontDataVersion - fontDataVersion.value; + // eslint-disable-next-line no-unused-expressions -- trigger reactive dependency + void fontDataVersion.value; if (!FONT.data?.character_image_urls?.length) { return []; }
1663-1663: Orphaned comment from duplicate CSS removal.This comment
/* Preset Button - Explicitly Legacy */has no associated CSS rules — it's a leftover from the duplicate block removal.Remove orphaned comment
-/* Preset Button - Explicitly Legacy */ - /* Preset Popover Grid Container */
980-980: Duplicate comments on lines 980 and 997.Both
loadConfigandsaveConfighave their leading comment duplicated (e.g.,// Load OSD configuration from FCappears twice). Remove the extra lines.Proposed fix
-// Load OSD configuration from FC // Load OSD configuration from FC async function loadConfig() {-// Save OSD configuration to FC // Save OSD configuration to FC async function saveConfig() {
663-665: Remove unusedisFieldVisiblefunction — this function is defined but never referenced anywhere in the codebase. It appears to be leftover code that can be safely deleted.
Comment on lines
+560
to
+562
| const _fcStore = useFlightControllerStore(); | ||
| const _navigationStore = useNavigationStore(); | ||
| const { reboot: _reboot } = useReboot(); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for all references to _fcStore, _navigationStore, and _reboot in OsdTab.vue
echo "=== Searching for _fcStore, _navigationStore, _reboot references ==="
rg '_fcStore|_navigationStore|_reboot' src/components/tabs/OsdTab.vue -n
echo ""
echo "=== File size check ==="
wc -l src/components/tabs/OsdTab.vueRepository: betaflight/betaflight-configurator
Length of output: 352
🏁 Script executed:
#!/bin/bash
# Get context around the unused variables and check the surrounding code
echo "=== Context around lines 560-565 ==="
sed -n '555,570p' src/components/tabs/OsdTab.vue
echo ""
echo "=== Check if these composables are called elsewhere in the setup ==="
rg 'useFlightControllerStore|useNavigationStore|useReboot' src/components/tabs/OsdTab.vue -nRepository: betaflight/betaflight-configurator
Length of output: 1027
🏁 Script executed:
#!/bin/bash
# Double-check by looking at the full setup function/script section
echo "=== Full script setup section (lines 540-600) ==="
sed -n '540,600p' src/components/tabs/OsdTab.vue
echo ""
echo "=== Verify these stores are not accessed through other means ==="
# Check if _fc, _navigation, _reboot are used as methods or in computed
rg '\b_fc[A-Za-z]*\b|\b_navigation[A-Za-z]*\b' src/components/tabs/OsdTab.vue -n | grep -v 'const _'Repository: betaflight/betaflight-configurator
Length of output: 2488
🏁 Script executed:
#!/bin/bash
# Let's check if these composables might be doing side effects when called
echo "=== Checking useFlightControllerStore implementation ==="
fd -e ts -e js 'fc.*store' src/stores/ | head -5
echo ""
echo "=== Checking useReboot implementation ==="
fd -e ts -e js 'useReboot' src/composables/ | head -5
echo ""
echo "=== Look at imports and usage context more broadly ==="
sed -n '1,50p' src/components/tabs/OsdTab.vueRepository: betaflight/betaflight-configurator
Length of output: 2834
🏁 Script executed:
#!/bin/bash
# Check the useReboot composable to see if it has side effects
echo "=== useReboot composable implementation ==="
cat src/composables/useReboot.js
echo ""
echo "=== Search for fc store definition ==="
fd 'fc.js\|fc.ts' src/stores/ --exec cat {}Repository: betaflight/betaflight-configurator
Length of output: 4000
🏁 Script executed:
#!/bin/bash
# Search for the stores in the stores directory
echo "=== Find all store files ==="
find src/stores -type f \( -name "*.js" -o -name "*.ts" \) | sort
echo ""
echo "=== Search specifically for fc store ==="
rg 'useFlightControllerStore' src/stores/ -lRepository: betaflight/betaflight-configurator
Length of output: 351
🏁 Script executed:
#!/bin/bash
# Check the fc.js store implementation
echo "=== fc.js store implementation ==="
head -100 src/stores/fc.js
echo ""
echo "=== Check if it has any side effects or initialization ==="
grep -n 'export.*useFlightControllerStore\|const useFlightControllerStore\|function useFlightControllerStore' src/stores/fc.js -A 20Repository: betaflight/betaflight-configurator
Length of output: 3691
Remove unused store and composable instances or clarify their purpose.
Lines 560-562 declare _fcStore, _navigationStore, and _reboot but none are referenced elsewhere in the component. If retained for future use or side effects, add a brief explanatory comment; otherwise, remove them.
🤖 Prompt for AI Agents
In `@src/components/tabs/OsdTab.vue` around lines 560 - 562, The variables
_fcStore, _navigationStore, and _reboot are declared but unused: either remove
the declarations (the calls to useFlightControllerStore(), useNavigationStore(),
and useReboot()) or keep them and add a one-line comment explaining their
intended future side-effect/usage; locate the declarations near the top of the
component (symbols: _fcStore, _navigationStore, _reboot) and apply the change so
no unused imports/variables remain and linter warnings disappear.
Comment on lines
+1124
to
+1143
| // Watch for font preset changes | ||
| watch(selectedFontPreset, (newVal) => { | ||
| if (newVal >= 0) { | ||
| loadFontPreset(newVal); | ||
| } | ||
| }); | ||
|
|
||
| // Watch for profile changes | ||
| watch(previewProfile, (newVal) => { | ||
| osdStore.setSelectedPreviewProfile(newVal); | ||
| updatePreview(); | ||
| }); | ||
|
|
||
| // Watch for font selection in header | ||
| watch(selectedFont, (newVal) => { | ||
| // Sync preset selection for UI consistency | ||
| selectedFontPreset.value = newVal; | ||
| // Directly load the font (avoids issue if selectedFontPreset was already this value) | ||
| loadFontPreset(newVal); | ||
| }); |
Contributor
There was a problem hiding this comment.
loadFontPreset is called twice when selectedFont changes to a new value.
When selectedFont changes:
- Line 1140 sets
selectedFontPreset.value = newVal→ triggers theselectedFontPresetwatcher (line 1125) → callsloadFontPreset(newVal) - Line 1142 directly calls
loadFontPreset(newVal)
This results in the font being fetched and parsed twice. The direct call on line 1142 is only needed when selectedFontPreset already equals newVal (watcher won't fire). Consider guarding to avoid the double call:
Proposed fix
watch(selectedFont, (newVal) => {
- // Sync preset selection for UI consistency
- selectedFontPreset.value = newVal;
- // Directly load the font (avoids issue if selectedFontPreset was already this value)
- loadFontPreset(newVal);
+ if (selectedFontPreset.value === newVal) {
+ // Watcher won't fire if value is the same, so load directly
+ loadFontPreset(newVal);
+ } else {
+ // Setting the value will trigger the selectedFontPreset watcher
+ selectedFontPreset.value = newVal;
+ }
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Watch for font preset changes | |
| watch(selectedFontPreset, (newVal) => { | |
| if (newVal >= 0) { | |
| loadFontPreset(newVal); | |
| } | |
| }); | |
| // Watch for profile changes | |
| watch(previewProfile, (newVal) => { | |
| osdStore.setSelectedPreviewProfile(newVal); | |
| updatePreview(); | |
| }); | |
| // Watch for font selection in header | |
| watch(selectedFont, (newVal) => { | |
| // Sync preset selection for UI consistency | |
| selectedFontPreset.value = newVal; | |
| // Directly load the font (avoids issue if selectedFontPreset was already this value) | |
| loadFontPreset(newVal); | |
| }); | |
| // Watch for font preset changes | |
| watch(selectedFontPreset, (newVal) => { | |
| if (newVal >= 0) { | |
| loadFontPreset(newVal); | |
| } | |
| }); | |
| // Watch for profile changes | |
| watch(previewProfile, (newVal) => { | |
| osdStore.setSelectedPreviewProfile(newVal); | |
| updatePreview(); | |
| }); | |
| // Watch for font selection in header | |
| watch(selectedFont, (newVal) => { | |
| if (selectedFontPreset.value === newVal) { | |
| // Watcher won't fire if value is the same, so load directly | |
| loadFontPreset(newVal); | |
| } else { | |
| // Setting the value will trigger the selectedFontPreset watcher | |
| selectedFontPreset.value = newVal; | |
| } | |
| }); |
🤖 Prompt for AI Agents
In `@src/components/tabs/OsdTab.vue` around lines 1124 - 1143, The selectedFont
watcher is causing loadFontPreset to run twice because assigning
selectedFontPreset.value triggers its watcher (which calls loadFontPreset) and
the watcher also calls loadFontPreset directly; change the selectedFont watcher
(the block that sets selectedFontPreset.value and then calls loadFontPreset) to
first check whether selectedFontPreset.value already equals newVal and only call
loadFontPreset directly when they are already equal (otherwise just set
selectedFontPreset.value and let the selectedFontPreset watcher handle loading),
referencing the selectedFont watcher, selectedFontPreset watcher, and the
loadFontPreset function to locate where to add the guard.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Migrates OSD tab to Vue SFC as part of #4812. Includes virtual mode support and style migration.
Summary by CodeRabbit
New Features
Bug Fixes