|
| 1 | +# Field Test: Media Controls |
| 2 | + |
| 3 | +**Extension**: Media Controls (`mediacontrols@cliffniff.github.com`) |
| 4 | +**Source**: https://github.com/cliffniff/media-controls |
| 5 | +**Category**: Audio/media control (MPRIS) |
| 6 | +**Downloads**: ~437,000 |
| 7 | +**GNOME versions**: 46, 47, 48, 49 |
| 8 | +**Codebase**: 17 JS files, 5,064 lines (hand-written JavaScript, no TypeScript compilation) |
| 9 | +**Layout**: `src/` directory (modern build-system layout) |
| 10 | +**License**: GPL-2.0-or-later |
| 11 | + |
| 12 | +## Why This Extension |
| 13 | + |
| 14 | +First audio/media extension tested. Exercises under-represented patterns: |
| 15 | +- D-Bus proxy lifecycle (MPRIS bus interaction, 3 proxies per player) |
| 16 | +- Signal-heavy architecture (41 manual .connect() calls) |
| 17 | +- `src/` directory layout (build-system pattern, metadata.json in src/) |
| 18 | +- Extensive TypeScript-compatible JSDoc typing strategy |
| 19 | +- Custom St.Widget subclasses with GObject.registerClass |
| 20 | + |
| 21 | +## Architecture Overview |
| 22 | + |
| 23 | +- **extension.js** (787 LOC): Main entry point, player discovery via D-Bus NameOwnerChanged, keybinding lifecycle |
| 24 | +- **PlayerProxy.js** (609 LOC): MPRIS D-Bus wrapper — 3 proxies per player (Mpris, MprisPlayer, Properties), change-listener Map, fallback polling |
| 25 | +- **PanelButton.js** (1,236 LOC): Panel button + popup menu UI, extends PanelMenu.Button, `onDestroy()` cleanup via destroy signal |
| 26 | +- **ScrollingLabel.js** (391 LOC): Animated scrolling text widget, extends St.ScrollView, `destroy()` override with timer cleanup |
| 27 | +- **MenuSlider.js** (278 LOC): Track position slider, extends St.BoxLayout, `onDestroy()` via destroy signal connection |
| 28 | +- **prefs.js** (393 LOC): Preferences with AppChooser, BlacklistedPlayers, ElementList, LabelList widgets |
| 29 | +- **types/dbus.js** (239 LOC): Full MPRIS D-Bus type definitions in JSDoc |
| 30 | + |
| 31 | +Import segregation is clean: shell helpers in `helpers/shell/`, prefs helpers in `helpers/prefs/`, shared utils in `utils/`. |
| 32 | + |
| 33 | +## Raw Results |
| 34 | + |
| 35 | +``` |
| 36 | +432 checks — 135 passed, 3 failed, 246 warnings, 48 skipped |
| 37 | +Exit code: 1 |
| 38 | +``` |
| 39 | + |
| 40 | +### Results by Check Name |
| 41 | + |
| 42 | +| Check | Count | Severity | Assessment | |
| 43 | +|-------|-------|----------|------------| |
| 44 | +| R-SLOP-02 (JSDoc @returns) | 103 | WARN | **FP** — intentional TypeScript-compatible JSDoc | |
| 45 | +| R-SLOP-01 (JSDoc @param) | 70 | WARN | **FP** — same | |
| 46 | +| resource-tracking/no-destroy-method | 45 | WARN | **FP** — `onDestroy()` not recognized as cleanup | |
| 47 | +| R-SLOP-11 (GLib.source_remove) | 10 | WARN | **TP** — should use `GLib.Source.remove()` | |
| 48 | +| R-SLOP-17 (typeof method guard) | 3 | WARN | **Debatable** — defensive check for optional API methods | |
| 49 | +| lifecycle/prototype-override | 2 | WARN | **TP** — MprisSource.prototype._addPlayer not restored | |
| 50 | +| metadata/exists | 1 | FAIL | **FP** — metadata.json in `src/`, no fallback | |
| 51 | +| file-structure/metadata.json | 1 | FAIL | **FP** — same | |
| 52 | +| css/shell-class-override | 1 | FAIL | **TP** — `.panel-button` overrides Shell theme class | |
| 53 | +| lifecycle/signal-balance | 1 | WARN | **Needs analysis** — 41 connects vs 10 disconnects | |
| 54 | +| lifecycle/async-destroyed-guard | 1 | WARN | **TP** | |
| 55 | +| lifecycle/soup-session-abort | 1 | WARN | **TP** — no abort() on disable | |
| 56 | +| async/no-cancellable | 1 | WARN | **TP** — async calls without Gio.Cancellable | |
| 57 | +| async/missing-cancellable | 1 | WARN | **TP** — _async() without cancellable | |
| 58 | +| quality/constructor-resources | 1 | WARN | **TP** — .connect() in AppChooser constructor | |
| 59 | +| quality/comment-density | 1 | WARN | **TP advisory** — utils/common.js 45% comments | |
| 60 | +| quality/private-api | 1 | WARN | **TP** — Main.panel private API access | |
| 61 | +| quality/excessive-null-checks | 1 | WARN | **TP advisory** — 117 null checks, ratio 0.025 | |
| 62 | +| R-QUAL-33 (Gio._promisify) | 1 | WARN | **TP advisory** — 4 files with undocumented promisify | |
| 63 | + |
| 64 | +**Total**: 218 FP (89%), 28 TP/debatable (11%) |
| 65 | + |
| 66 | +## False Positive Analysis |
| 67 | + |
| 68 | +### 1. R-SLOP-01/02 JSDoc Flood (173 WARNs) |
| 69 | + |
| 70 | +**Root cause**: Provenance score 3, threshold >= 4 for suppression. |
| 71 | + |
| 72 | +Media Controls uses intentional TypeScript-compatible JSDoc throughout all 17 files. The project includes `jsconfig.json`, `@girs/*` type packages, and a dedicated `types/dbus.js` with full MPRIS type definitions. This is clearly a deliberate typing strategy, not AI slop. |
| 73 | + |
| 74 | +The provenance score of 3 means "strong hand-written indicators" (domain vocabulary: 12 hits, nontrivial algorithms: 40 hits, consistent naming). The threshold of >= 4 is too conservative for JSDoc suppression. |
| 75 | + |
| 76 | +**Fix**: Lower provenance threshold to >= 3. |
| 77 | + |
| 78 | +### 2. resource-tracking/no-destroy-method (45 WARNs) |
| 79 | + |
| 80 | +**Root cause**: `build-resource-graph.py` recognizes `destroy()`, `disable()`, and `_destroy*()` as cleanup methods, but NOT `onDestroy()`. |
| 81 | + |
| 82 | +Both MenuSlider.js and PanelButton.js use `this.connect("destroy", this.onDestroy.bind(this))` to register cleanup handlers. The `onDestroy()` method contains proper signal disconnection, timer removal, and null assignments. The resource graph doesn't recognize this pattern. |
| 83 | + |
| 84 | +- MenuSlider.js: 8 resource warnings, has `onDestroy()` at line 250 |
| 85 | +- PanelButton.js: 37 resource warnings, has `onDestroy()` at line 1186 |
| 86 | + |
| 87 | +**Fix**: Add `onDestroy()` recognition in build-resource-graph.py cleanup method detection. |
| 88 | + |
| 89 | +### 3. metadata.json Missing (2 FAILs) |
| 90 | + |
| 91 | +**Root cause**: ego-lint.sh has `src/` fallback for extension.js but NOT for metadata.json. check-metadata.py also lacks the fallback. |
| 92 | + |
| 93 | +Media Controls uses a build-system layout with all source in `src/` — metadata.json lives at `src/metadata.json`, which is standard for extensions using gnome-extensions pack with a build step. |
| 94 | + |
| 95 | +**Fix**: Add consistent `src/` fallback for metadata.json in ego-lint.sh and check-metadata.py. |
| 96 | + |
| 97 | +## True Positives |
| 98 | + |
| 99 | +### Lifecycle Issues |
| 100 | +- **prototype-override** (2): `MprisSource.prototype._addPlayer` is monkey-patched but the original is not saved/restored in `disable()`. Genuine concern for multi-extension compatibility. |
| 101 | +- **soup-session-abort** (1): Soup.Session created for album art fetching but no `.abort()` in cleanup path. Pending HTTP requests will continue after disable. |
| 102 | +- **async-destroyed-guard** (1): Multiple async/await chains without `_destroyed` guard. Extension could act on stale state after disable if async operations complete late. |
| 103 | +- **signal-balance** (1): 41 manual `.connect()` vs 10 `.disconnect()`. Needs deeper analysis — some may use `.connectObject()` auto-management, some may be on child widgets (auto-cleaned on destroy). |
| 104 | + |
| 105 | +### Async Safety |
| 106 | +- **no-cancellable + missing-cancellable** (2): D-Bus proxy creation and file operations use async without `Gio.Cancellable`. These should be cancellable via disable() to prevent stale state. |
| 107 | + |
| 108 | +### CSS |
| 109 | +- **shell-class-override** (1 FAIL): `.panel-button` in stylesheet.css overrides GNOME Shell's panel button styling for ALL panel buttons, not just this extension's. Also uses `.popup-menu-container` and `.popup-menu-box` (not flagged but same concern). |
| 110 | + |
| 111 | +### Code Quality |
| 112 | +- **constructor-resources** (1): AppChooser.js:51 has `.connect()` in constructor. Since this is a prefs widget (not extension lifecycle), the impact is limited but still worth noting. |
| 113 | +- **GLib.source_remove** (10): Uses non-idiomatic `GLib.source_remove()` instead of `GLib.Source.remove()`. Functional but not correct GJS API. |
| 114 | + |
| 115 | +## Calibration Lessons |
| 116 | + |
| 117 | +1. **`onDestroy()` is a common cleanup pattern** — build-resource-graph.py should recognize it alongside `destroy()`, `disable()`, and `_destroy*()` |
| 118 | +2. **Provenance threshold of 4 is too conservative for JSDoc suppression** — score 3 is sufficient to indicate hand-written code, especially when JSDoc is extensive and consistent |
| 119 | +3. **`src/` layout support remains inconsistent** — extension.js has fallback, metadata.json does not. Need systematic audit of all src/ fallbacks |
| 120 | +4. **TypeScript-compatible JSDoc is becoming more common** — popular extensions like Media Controls use it intentionally. The R-SLOP-01/02 rules need better calibration for this pattern |
| 121 | +5. **D-Bus proxy lifecycle is well-handled** — no false positives from D-Bus-specific checks. The MPRIS pattern (3 proxies per player, NameOwnerChanged tracking) exercises these checks well |
| 122 | +6. **CSS shell-class-override works correctly** — caught legitimate `.panel-button` override. However, it missed `.popup-menu-container` and `.popup-menu-box` (potential gap in the shell class list) |
| 123 | + |
| 124 | +## Comparison with Prior Tests |
| 125 | + |
| 126 | +| Metric | Media Controls | Avg of Prior 8 | |
| 127 | +|--------|---------------|----------------| |
| 128 | +| FP rate (pre-fix) | 89% | ~40-76% | |
| 129 | +| Dominant FP cause | JSDoc + onDestroy | Varied | |
| 130 | +| True positive rate | 11% | ~24-60% | |
| 131 | +| Security issues | 0 | 0-6 | |
| 132 | +| Code provenance | 3/5 | 2-4/5 | |
| 133 | + |
| 134 | +The high FP rate is driven by two specific gaps. After fixes, the extension's true positive findings are meaningful and actionable. |
0 commit comments