replaced track info string map with a struct
This commit is contained in:
@@ -0,0 +1,274 @@
|
||||
# Plan: Replace `GetCurrentTrackInfo` `map[string]interface{}` with a Typed Struct
|
||||
|
||||
**Refactoring catalog item:** #9
|
||||
**Priority:** P2
|
||||
**Risk:** Low — the player is not in `FEBindings`, so no Wails binding regeneration is needed. All data flows through the event system.
|
||||
|
||||
---
|
||||
|
||||
## Problem Statement
|
||||
|
||||
`player.getCurrentTrackInfoLocked()` returns `map[string]interface{}` — a stringly-typed map with 10 keys. Then `emitTrackChanged()` mutates this map by bolting on 3 additional keys (`trackLength`, `seekPosition`, `trackChangeId`) before emitting it via `runtime.EventsEmit`. This pattern has several issues:
|
||||
|
||||
1. **No compile-time safety** — a typo like `"fileName"` vs `"filename"` is a silent bug.
|
||||
2. **Split construction** — the 13-field payload is built in two places (`getCurrentTrackInfoLocked` builds 10 fields, `emitTrackChanged` appends 3 more via map mutation). The shape of the data is not visible in any single location.
|
||||
3. **Inconsistent nil-file fallback** — when `p.currentFile == nil`, the returned map has 7 keys (missing `coverArtSmall`, `coverArtMedium`, `coverArtLarge`). The error fallback in `emitTrackChanged` has only 3 keys. Both cases produce maps with incomplete field sets that differ from each other and from the happy path (13 keys).
|
||||
4. **Missed opportunity for Wails type generation** — if the player were ever added to `FEBindings`, a struct return type would auto-generate TypeScript bindings. Currently the frontend manually maintains a `TrackInfo` interface that must be kept in sync by hand.
|
||||
5. **Contrast with rest of codebase** — the queue package already uses proper structs with JSON tags (`queue.Track`, `queue.State`, etc.) for all event payloads. The player is an outlier.
|
||||
|
||||
---
|
||||
|
||||
## Design Decisions & Reasoning
|
||||
|
||||
### Decision 1: Define a single `TrackInfo` struct (not two separate types)
|
||||
|
||||
The catalog suggests defining a `TrackInfo` struct. A question arises: should `getCurrentTrackInfoLocked` return a "partial" struct (10 fields) while `emitTrackChanged` extends it with 3 more? No — the whole point is to eliminate the mutation pattern. A single struct with all 13 fields is cleaner. The struct represents "everything the frontend needs to know about the current track for the TrackChanged event."
|
||||
|
||||
**Reasoning:** A single struct means one source of truth for the shape of the data. The zero values for `TrackLength`, `SeekPosition`, and `TrackChangeID` are naturally `0` in Go, which is semantically correct for "no track loaded" or "error" fallback cases.
|
||||
|
||||
### Decision 2: Use `json` struct tags with camelCase keys
|
||||
|
||||
The existing map uses camelCase keys (`"fileName"`, `"coverArtSmall"`, etc.). Wails serializes event payloads as JSON. The struct must use `json:"fileName"` tags to preserve the exact same wire format — otherwise the frontend would break.
|
||||
|
||||
**Reasoning:** This is a behavioral requirement, not a style choice. The frontend `TrackInfo` interface expects camelCase keys. Changing them would require coordinated frontend changes for zero benefit.
|
||||
|
||||
### Decision 3: Keep `getCurrentTrackInfoLocked` but change its return type
|
||||
|
||||
Rather than inlining all logic into `emitTrackChanged`, keep the `getCurrentTrackInfoLocked` helper but have it return `TrackInfo` (with the base 10 fields populated). Then `emitTrackChanged` fills in the remaining 3 fields (`TrackLength`, `SeekPosition`, `TrackChangeID`) on the struct before emitting.
|
||||
|
||||
**Reasoning:** This preserves the separation of concerns — "build metadata from file/DB" vs "compute playback position and emit." It also keeps `GetCurrentTrackInfo()` (the public method) useful: it returns the same struct, just without the playback-timing fields (which are zero-valued). If the player is ever added to `FEBindings`, this method's return type would auto-generate a TypeScript class.
|
||||
|
||||
### Decision 4: Eliminate `GetCurrentTrackInfo()` public method — or keep it?
|
||||
|
||||
`GetCurrentTrackInfo()` has **zero Go callers** and **zero TypeScript callers** (the player is not in `FEBindings`). It exists only as dead code. However, it was likely intended as a Wails binding that hasn't been wired up yet, and it could be useful in the future.
|
||||
|
||||
**Decision: Keep it.** The cost of a single unused method is minimal, and it now returns a proper struct which would be useful if the player is added to `FEBindings` later. If desired, it can be removed as part of a separate cleanup (item #13 addresses dead player methods).
|
||||
|
||||
### Decision 5: Fix the inconsistent nil-file/error fallbacks
|
||||
|
||||
Currently:
|
||||
- **nil file fallback** (line 840-848): returns 7 keys — missing `coverArtSmall`, `coverArtMedium`, `coverArtLarge`
|
||||
- **error fallback** in `emitTrackChanged` (line 319-323): returns only 3 keys — missing most fields
|
||||
|
||||
With a struct, both fallbacks naturally return a fully-populated struct (all fields present, most set to zero values). The `State` field should still be set explicitly in both cases. This eliminates the inconsistency for free.
|
||||
|
||||
### Decision 6: Place the struct in the existing `player.go` file, not a new file
|
||||
|
||||
The player package has only 3 files (`player.go`, `volume.go`, `player_test.go`). The struct is tightly coupled to the player — it describes what the player emits. Creating a separate `trackinfo.go` file for a single ~20-line struct definition would be premature file splitting for such a small package.
|
||||
|
||||
**Reasoning:** Follow the existing pattern — `State` type and playback constants are already defined in `player.go`. The `TrackInfo` struct logically belongs alongside them.
|
||||
|
||||
### Decision 7: Use `State` type (not `string`) in the struct
|
||||
|
||||
Currently the map stores `string(p.state)` — explicitly converting the `State` type to `string`. The struct should use the `State` type with `json:"state"` tag. Since `State` is `type State string`, JSON serialization produces the same string value. This gives us type safety in Go without changing the wire format.
|
||||
|
||||
**Reasoning:** The whole point of this refactoring is compile-time safety. Using `string` in the struct for the state field would undermine that goal.
|
||||
|
||||
### Decision 8: Use `uint64` for `TrackChangeID` (match the field type)
|
||||
|
||||
The `Player` struct defines `trackChangeID uint64`. The struct field should be `TrackChangeID uint64`. The frontend `TrackInfo` interface uses `number` which can safely represent integers up to 2^53 — more than sufficient for a monotonic counter that starts at 0 per session.
|
||||
|
||||
---
|
||||
|
||||
## Implementation Plan
|
||||
|
||||
### Step 1: Define the `TrackInfo` struct in `player.go`
|
||||
|
||||
Add the struct definition near the existing `State` type (around line 58-65), after the sentinel errors:
|
||||
|
||||
```go
|
||||
// TrackInfo contains metadata and playback state for the currently loaded track.
|
||||
type TrackInfo struct {
|
||||
FileName string `json:"fileName"`
|
||||
FilePath string `json:"filePath"`
|
||||
State State `json:"state"`
|
||||
Title string `json:"title"`
|
||||
Artist string `json:"artist"`
|
||||
Album string `json:"album"`
|
||||
CoverArt string `json:"coverArt"`
|
||||
CoverArtSmall string `json:"coverArtSmall"`
|
||||
CoverArtMedium string `json:"coverArtMedium"`
|
||||
CoverArtLarge string `json:"coverArtLarge"`
|
||||
TrackLength int `json:"trackLength"`
|
||||
SeekPosition int `json:"seekPosition"`
|
||||
TrackChangeID uint64 `json:"trackChangeId"`
|
||||
}
|
||||
```
|
||||
|
||||
**Note:** `json:"trackChangeId"` (lowercase `d`) matches the existing frontend interface key `trackChangeId`.
|
||||
|
||||
### Step 2: Refactor `getCurrentTrackInfoLocked` to return `TrackInfo`
|
||||
|
||||
Change the signature from `(map[string]interface{}, error)` to `TrackInfo` (no error needed — see reasoning below).
|
||||
|
||||
**Why remove the error return?** The current function never actually returns an error. It handles all error cases internally (DB lookup failure logs and falls back to defaults). The error in the return signature is unused dead weight. With a struct, the zero-value fallback is even cleaner.
|
||||
|
||||
Updated implementation:
|
||||
|
||||
```go
|
||||
func (p *Player) getCurrentTrackInfoLocked() TrackInfo {
|
||||
info := TrackInfo{
|
||||
State: p.state,
|
||||
}
|
||||
|
||||
if p.currentFile == nil {
|
||||
return info
|
||||
}
|
||||
|
||||
info.FileName = filepath.Base(p.currentFile.Name())
|
||||
info.FilePath = p.currentFile.Name()
|
||||
info.Title = info.FileName // default title
|
||||
|
||||
if p.db != nil {
|
||||
meta, err := p.db.Queries.GetTrackMetadataByPath(
|
||||
p.ctx, info.FilePath,
|
||||
)
|
||||
if err == nil {
|
||||
if meta.Title != "" {
|
||||
info.Title = meta.Title
|
||||
}
|
||||
|
||||
info.Artist = meta.Artist
|
||||
info.Album = meta.Album
|
||||
|
||||
if meta.CoverArtPath != "" {
|
||||
base := filepath.Base(meta.CoverArtPath)
|
||||
info.CoverArt = "/covers/" + base
|
||||
info.CoverArtSmall = "/covers/" +
|
||||
library.SizedFilename(base, "_sm")
|
||||
info.CoverArtMedium = "/covers/" +
|
||||
library.SizedFilename(base, "_md")
|
||||
info.CoverArtLarge = "/covers/" +
|
||||
library.SizedFilename(base, "_lg")
|
||||
}
|
||||
} else {
|
||||
p.logger.Debug(
|
||||
"Could not get track metadata from database",
|
||||
"path", info.FilePath, "err", err,
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
return info
|
||||
}
|
||||
```
|
||||
|
||||
### Step 3: Update `GetCurrentTrackInfo` (public method)
|
||||
|
||||
Change return type from `(map[string]interface{}, error)` to `TrackInfo`:
|
||||
|
||||
```go
|
||||
// GetCurrentTrackInfo returns information about the currently loaded track.
|
||||
func (p *Player) GetCurrentTrackInfo() TrackInfo {
|
||||
p.mu.Lock()
|
||||
defer p.mu.Unlock()
|
||||
|
||||
return p.getCurrentTrackInfoLocked()
|
||||
}
|
||||
```
|
||||
|
||||
**Note:** Dropping the error return is safe — there are zero callers of this method.
|
||||
|
||||
### Step 4: Refactor `emitTrackChanged` to build the struct directly
|
||||
|
||||
Replace map mutation with direct struct field assignment:
|
||||
|
||||
```go
|
||||
func (p *Player) emitTrackChanged() {
|
||||
if p.ctx == nil {
|
||||
p.logger.Error("Context is nil, cannot emit event")
|
||||
|
||||
return
|
||||
}
|
||||
|
||||
trackInfo := p.getCurrentTrackInfoLocked()
|
||||
|
||||
trackLengthSecs, err := p.trackLengthLocked()
|
||||
if err != nil {
|
||||
p.logger.Error("Cannot get track length")
|
||||
}
|
||||
|
||||
trackInfo.TrackLength = trackLengthSecs
|
||||
|
||||
// Compute current seek position in seconds.
|
||||
if p.seeker != nil {
|
||||
speaker.Lock()
|
||||
trackInfo.SeekPosition = p.seeker.Position() /
|
||||
int(p.format.SampleRate)
|
||||
speaker.Unlock()
|
||||
}
|
||||
|
||||
// Increment track change ID so the frontend can detect changes
|
||||
// even when the same file plays consecutively.
|
||||
p.trackChangeID++
|
||||
trackInfo.TrackChangeID = p.trackChangeID
|
||||
|
||||
runtime.EventsEmit(p.ctx, events.TrackChanged, trackInfo)
|
||||
|
||||
p.logger.Info(
|
||||
"Emitting TrackChangedEvent with track info",
|
||||
"trackInfo", trackInfo,
|
||||
)
|
||||
}
|
||||
```
|
||||
|
||||
**Key change:** No more error-fallback map with only 3 keys. If `getCurrentTrackInfoLocked()` returns a zero-valued struct (e.g., when no file is loaded), it still has all 13 fields — the frontend receives a complete, predictable shape every time.
|
||||
|
||||
### Step 5: Verify `UnloadTrack` emits `nil` (no change needed)
|
||||
|
||||
At `player.go:678`, `UnloadTrack` emits:
|
||||
```go
|
||||
runtime.EventsEmit(p.ctx, events.TrackChanged, nil)
|
||||
```
|
||||
|
||||
This is correct and intentional — it signals "no track loaded" to the frontend, which handles `null` in `(trackInfo: TrackInfo | null) => { ... }`. No changes needed here.
|
||||
|
||||
### Step 6: Run `make lint` and `make test`
|
||||
|
||||
Ensure:
|
||||
- No linting violations (line length, godot, nlreturn, etc.)
|
||||
- Tests pass (the existing test is integration-only and skips in CI, but the build itself must succeed with `-tags webkit2_41`)
|
||||
|
||||
### Step 7: (Optional) Update the frontend `TrackInfo` interface comments
|
||||
|
||||
The frontend `TrackInfo` interface in `frontend/src/store/player-store.ts` already matches the struct fields exactly. No field changes are needed. However, a comment noting that it mirrors `player.TrackInfo` from the backend could be helpful for future maintainers:
|
||||
|
||||
```typescript
|
||||
// TrackInfo mirrors the player.TrackInfo struct in the Go backend.
|
||||
// Fields are serialized as camelCase JSON via struct tags.
|
||||
export interface TrackInfo {
|
||||
// ... (existing fields, unchanged)
|
||||
}
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Files Changed
|
||||
|
||||
| File | Change |
|
||||
|------|--------|
|
||||
| `backend/player/player.go` | Add `TrackInfo` struct; refactor `getCurrentTrackInfoLocked`, `GetCurrentTrackInfo`, and `emitTrackChanged` |
|
||||
| `frontend/src/store/player-store.ts` | Add comment noting Go struct mirror (optional) |
|
||||
|
||||
**No other files need changes.** The frontend receives the data via events and the JSON wire format is identical (same keys, same types). No Wails binding regeneration is needed since the player is not in `FEBindings`.
|
||||
|
||||
---
|
||||
|
||||
## Risks & Mitigations
|
||||
|
||||
| Risk | Likelihood | Mitigation |
|
||||
|------|------------|------------|
|
||||
| JSON key mismatch after refactoring | Low | The `json` struct tags are set to exactly match the current map keys. Verify by running the app and checking the frontend receives correct data. |
|
||||
| `slog` logging of struct differs from map | Very low | `slog` will log the struct fields. The output format changes but the information is equivalent. No functional impact. |
|
||||
| Future addition of `player` to `FEBindings` | N/A | This refactoring *enables* that future change — Wails will auto-generate a `player.TrackInfo` TypeScript class from the struct. |
|
||||
|
||||
---
|
||||
|
||||
## Verification
|
||||
|
||||
1. `make lint` passes
|
||||
2. `make build-dev` succeeds
|
||||
3. Manual test: play a track, verify `now-playing` component shows correct title/artist/cover art
|
||||
4. Manual test: verify seek bar shows correct track length and seek position
|
||||
5. Manual test: unload track (stop playback, clear queue), verify frontend clears the now-playing display
|
||||
6. Manual test: play the same track twice consecutively, verify the seek bar resets (trackChangeId detection)
|
||||
@@ -1,477 +0,0 @@
|
||||
# Plan: Consolidate `LibraryScanComplete` Handling
|
||||
|
||||
Addresses refactoring catalog #7. Eliminates redundant direct `LibraryScanComplete` event listeners from components by making stores eagerly re-fetch data after invalidation, so the existing reactive controller subscription (`requestUpdate()`) delivers fresh data automatically.
|
||||
|
||||
---
|
||||
|
||||
## Problem Analysis
|
||||
|
||||
The refactoring catalog describes 10+ components that each independently listen for `LibraryScanComplete` and re-fetch their data. It claims these listeners are redundant because "the stores already invalidate their caches and notify subscribers."
|
||||
|
||||
**This claim is incorrect in the current architecture.** Here is why:
|
||||
|
||||
1. When `LibraryScanComplete` fires, `LibraryStore.invalidate()` nulls out cached data (`tracks`, `albums`, `artists`) and calls `notify()`.
|
||||
2. `notify()` triggers subscriber callbacks, which are `LibraryController.host.requestUpdate()` — a Lit re-render.
|
||||
3. But `requestUpdate()` only re-runs `render()`, and components read from **local `@state()` properties** (e.g., `this.tracks`, `this.albums`), not from the store. The local data is still stale.
|
||||
4. Nobody calls the `loadTracks()`/`loadAlbums()` methods again except the direct `LibraryScanComplete` listener.
|
||||
|
||||
**The root cause:** the stores use **lazy-fetch** — `invalidate()` clears the cache but does not re-fetch. The next `getTracks()` call will hit the backend, but nothing triggers that call except the component's own event listener.
|
||||
|
||||
**The fix:** make stores **eagerly re-fetch** after invalidation, so when the controller calls `requestUpdate()`, the store already has fresh data. Then refactor components to read data reactively from the store/controller instead of from local state populated by imperative load calls.
|
||||
|
||||
---
|
||||
|
||||
## Guiding Principles
|
||||
|
||||
1. **Incremental migration.** The store change (eager refetch) is backwards-compatible. Components are migrated one by one from easiest to hardest. Both patterns (old imperative + new reactive) coexist during migration.
|
||||
|
||||
2. **Preserve existing UX.** Scroll restoration, selection clearing, loading indicators, and search filtering must work identically. No regressions.
|
||||
|
||||
3. **Three categories of listeners.** Not all `LibraryScanComplete` listeners are the same:
|
||||
- **Data refresh listeners** (8 components): re-fetch library/playlist data → these are what we're consolidating.
|
||||
- **UI status listeners** (`config-page`, `library-manager`): update scan progress UI and display metrics → these MUST keep their direct listeners since no store handles scan status.
|
||||
- **Store-bypassing listeners** (`playlist-picker`): calls Go bindings directly → addressed separately.
|
||||
|
||||
4. **Don't fight the `externalAlbums`/`externalTracks` pattern.** Parent-child data delegation (`artist-details` → `cover-grid`, `genre-details` → `track-list`) is a valid pattern. The parent gets migrated; the child already skips the scan listener when driven externally.
|
||||
|
||||
---
|
||||
|
||||
## Phase 0: Store Eager-Refetch
|
||||
|
||||
### 0A. `LibraryStore` — add eager refetch after invalidation
|
||||
|
||||
**File:** `frontend/src/store/library-store.ts`
|
||||
|
||||
Change `invalidate()` to eagerly re-fetch all three data types after clearing the cache. The existing `getTracks()`/`getAlbums()`/`getArtists()` methods already handle concurrent-request coalescing (via `waitFor*()` helpers) and notify subscribers when loading starts/finishes.
|
||||
|
||||
```typescript
|
||||
// Before:
|
||||
private invalidate(): void {
|
||||
this.tracks = null;
|
||||
this.albums = null;
|
||||
this.artists = null;
|
||||
this.scrollPositions = { tracks: 0, albums: 0, artists: 0, genres: 0 };
|
||||
this.notify();
|
||||
}
|
||||
|
||||
// After:
|
||||
private invalidate(): void {
|
||||
this.tracks = null;
|
||||
this.albums = null;
|
||||
this.artists = null;
|
||||
this.scrollPositions = { tracks: 0, albums: 0, artists: 0, genres: 0 };
|
||||
this.notify();
|
||||
this.eagerRefetch();
|
||||
}
|
||||
|
||||
private eagerRefetch(): void {
|
||||
// Fire-and-forget. Each getter handles its own error/loading state
|
||||
// and calls notify() when done, which triggers requestUpdate()
|
||||
// on all subscribed controllers.
|
||||
void this.getTracks();
|
||||
void this.getAlbums();
|
||||
void this.getArtists();
|
||||
}
|
||||
```
|
||||
|
||||
**Why this works:** After `eagerRefetch()`, the store is in a `loading=true` state. When the backend responses arrive, the cache is repopulated and `notify()` fires again (from the `finally` block in each getter). Controllers call `requestUpdate()`, and now any component reading from the store gets fresh data.
|
||||
|
||||
**Why it's backwards-compatible:** Components with direct listeners will still call their `load*()` methods. The store's `waitFor*()` helpers coalesce concurrent requests, so the eager fetch and the component's fetch share the same in-flight promise — no duplicate backend calls.
|
||||
|
||||
**Scroll position reset note:** The `scrollPositions` reset to `0` happens synchronously in `invalidate()`. This is correct — after a library scan, the content has changed and scroll positions are meaningless. Components that read scroll positions during their re-render will see `0`.
|
||||
|
||||
### 0B. `PlaylistStore` — add eager refetch after invalidation
|
||||
|
||||
**File:** `frontend/src/store/playlist-store.ts`
|
||||
|
||||
Same pattern. The `invalidate()` method already exists and is called from multiple event handlers (not just `LibraryScanComplete`).
|
||||
|
||||
```typescript
|
||||
// Before:
|
||||
invalidate(): void {
|
||||
this.playlists = null;
|
||||
this.scrollPosition = 0;
|
||||
this.notify();
|
||||
}
|
||||
|
||||
// After:
|
||||
invalidate(): void {
|
||||
this.playlists = null;
|
||||
this.scrollPosition = 0;
|
||||
this.notify();
|
||||
void this.getPlaylists();
|
||||
}
|
||||
```
|
||||
|
||||
**Note:** `PlaylistStore.invalidate()` is public (called by `PlaylistController.invalidate()`). This eager refetch will also run for `PlaylistCreated`, `PlaylistDeleted`, `PlaylistRenamed`, `PlaylistTracksChanged`, and `PlaylistsRestored` events — which is desirable. Currently those events invalidate the cache and wait for a component to lazily re-fetch. Eager refetch means subscribers see fresh data faster.
|
||||
|
||||
### 0C. Verification
|
||||
|
||||
After Phase 0, both stores eagerly re-fetch on invalidation. Components with existing direct listeners still work (their fetches coalesce with the eager fetch). Components without listeners now get fresh data automatically through the controller subscription path, though they still need to read it reactively (Phase 1+).
|
||||
|
||||
---
|
||||
|
||||
## Phase 1: Migrate `genre-details` and `artist-details` (LOW effort)
|
||||
|
||||
These are thin wrapper components that fetch data, filter/cache it, and pass it to a child via `externalTracks`/`externalAlbums`. The child already skips its own scan listener when receiving external data.
|
||||
|
||||
### 1A. `genre-details.ts`
|
||||
|
||||
**File:** `frontend/src/components/genre-details/genre-details.ts`
|
||||
|
||||
Current flow:
|
||||
1. `connectedCallback()` → `loadTracks()` → `libraryCtrl.getTracks()` → filter by genre → `this.tracks = filtered`
|
||||
2. `LibraryScanComplete` → `loadTracks()` again
|
||||
|
||||
New flow:
|
||||
1. `connectedCallback()` → `loadTracks()` (initial load, unchanged)
|
||||
2. Remove the direct `LibraryScanComplete` listener and its cancellation
|
||||
3. Add reactive consumption in `willUpdate()` or `updated()`: when the store notifies (cache repopulated after eager refetch), the controller calls `requestUpdate()`, triggering a re-render. In `willUpdate()`, detect that the store's cached tracks have changed (or that loading finished) and re-run the genre filtering.
|
||||
|
||||
Implementation approach — use `updated()` to react to controller-triggered re-renders:
|
||||
|
||||
```typescript
|
||||
// Remove from connectedCallback:
|
||||
// this.cancelScanComplete = EventsOn(Events.LibraryScanComplete, () => this.loadTracks());
|
||||
// Remove from disconnectedCallback:
|
||||
// this.cancelScanComplete?.();
|
||||
// Remove the cancelScanComplete field.
|
||||
|
||||
// Add a version counter to detect store changes:
|
||||
private lastStoreVersion = 0;
|
||||
|
||||
override updated() {
|
||||
// The library controller's subscription calls requestUpdate() when the
|
||||
// store notifies. Check if tracks have been refreshed since our last load.
|
||||
const storeVersion = this.libraryCtrl.storeVersion;
|
||||
if (storeVersion !== this.lastStoreVersion && !this.libraryCtrl.tracksLoading) {
|
||||
this.lastStoreVersion = storeVersion;
|
||||
this.loadTracks();
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
**Alternative (simpler):** Instead of a version counter, check if the store's cached tracks reference has changed. Since `invalidate()` sets tracks to `null` and the eager refetch populates a new array, we can compare object identity:
|
||||
|
||||
```typescript
|
||||
private lastTracksRef: library.Track[] | null = null;
|
||||
|
||||
override updated() {
|
||||
const cached = this.libraryCtrl.cachedTracks;
|
||||
if (cached !== null && cached !== this.lastTracksRef) {
|
||||
this.lastTracksRef = cached;
|
||||
this.loadTracks();
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
**Decision: Use the reference-comparison approach.** It's simpler, doesn't require adding version counters to the store, and leverages the fact that each eager refetch creates a new array instance.
|
||||
|
||||
However, there's a subtlety: `updated()` runs after every render, including renders triggered by the component's own `@state()` changes (like `this.tracks` being set). We need to ensure this doesn't create an infinite loop:
|
||||
- `loadTracks()` calls `libraryCtrl.getTracks()`, which if the cache is already populated returns the same reference.
|
||||
- `this.tracks` is set to the filtered result, triggering a render.
|
||||
- `updated()` runs, compares `cachedTracks` — same reference as `lastTracksRef`, so no re-load. Safe.
|
||||
|
||||
But the initial load path: `connectedCallback()` calls `loadTracks()` directly. At that point `cachedTracks` might be `null` (store hasn't loaded yet). After `loadTracks()` finishes, the store cache is populated, and `lastTracksRef` is set. Next `requestUpdate()` from the store won't trigger a re-load because the reference matches. Safe.
|
||||
|
||||
**Required controller addition:** Add a `storeVersion` or expose `cachedTracks` — the controller already exposes `cachedTracks` (line 72-74 of `library-controller.ts`). No changes needed to the controller.
|
||||
|
||||
### 1B. `artist-details.ts`
|
||||
|
||||
**File:** `frontend/src/components/artist-details/artist-details.ts`
|
||||
|
||||
Same approach. This component has a cache-then-fetch dual-load pattern (`getAlbumsByArtistNameCached` then `getAlbumsByArtist`). The scan-complete handler just calls `loadAlbums()`.
|
||||
|
||||
Changes:
|
||||
1. Remove the direct `LibraryScanComplete` listener and its cancellation.
|
||||
2. Add reference comparison in `updated()`:
|
||||
|
||||
```typescript
|
||||
private lastAlbumsRef: library.Album[] | null = null;
|
||||
|
||||
override updated() {
|
||||
const cached = this.libraryCtrl.cachedAlbums;
|
||||
if (cached !== null && cached !== this.lastAlbumsRef) {
|
||||
this.lastAlbumsRef = cached;
|
||||
this.loadAlbums();
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
**Note:** `getAlbumsByArtist(artistId)` is NOT cached by the store — it always hits the backend. But that's fine because `loadAlbums()` already handles this. The reference check on `cachedAlbums` (the full album list) serves as a proxy for "the library data has changed."
|
||||
|
||||
---
|
||||
|
||||
## Phase 2: Migrate `artists-view` (LOW-MEDIUM effort)
|
||||
|
||||
**File:** `frontend/src/components/artists-view/artists-view.ts`
|
||||
|
||||
Current flow:
|
||||
1. `connectedCallback()` → `loadArtists()` → `libraryCtrl.getArtists()` → `this.artists = result`
|
||||
2. `LibraryScanComplete` → `loadArtists()`
|
||||
3. `willUpdate()` → `recomputeArtistCaches()` (filters by search term)
|
||||
4. `render()` reads `cachedGridEntries`
|
||||
|
||||
New flow:
|
||||
1. `connectedCallback()` → `loadArtists()` (initial load, unchanged)
|
||||
2. Remove the direct `LibraryScanComplete` listener.
|
||||
3. React to store changes in `updated()`:
|
||||
|
||||
```typescript
|
||||
private lastArtistsRef: library.Artist[] | null = null;
|
||||
|
||||
override updated() {
|
||||
const cached = this.libraryCtrl.cachedArtists;
|
||||
if (cached !== null && cached !== this.lastArtistsRef) {
|
||||
this.lastArtistsRef = cached;
|
||||
this.loadArtists();
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
**Scroll position consideration:** `loadArtists()` currently restores scroll position at the end. After a scan, scroll positions are reset to `0` by `invalidate()`. The `restoringScroll` flag and `restoreScrollPosition()` call in `loadArtists()` handle this correctly — they'll restore to position `0`, which is a no-op visually.
|
||||
|
||||
**Selection consideration:** `loadArtists()` does not currently clear selection. After migration, selection could reference stale artist IDs. Consider adding `this.selectedArtists.clear()` at the top of `loadArtists()` if not already present. (This is a minor improvement, not a regression from the migration.)
|
||||
|
||||
---
|
||||
|
||||
## Phase 3: Migrate `genres-view` (HIGH effort)
|
||||
|
||||
**File:** `frontend/src/components/genres-view/genres-view.ts`
|
||||
|
||||
This component derives genres from tracks — a transformation the store doesn't provide. The store exposes tracks, not genres.
|
||||
|
||||
Current flow:
|
||||
1. `loadGenres()` → `libraryCtrl.getTracks()` → `extractGenres(tracks)` → `this.genres = result`
|
||||
2. `LibraryScanComplete` → `loadGenres()`
|
||||
|
||||
New flow:
|
||||
1. Same `loadGenres()` for initial load.
|
||||
2. Remove the direct `LibraryScanComplete` listener.
|
||||
3. React to store changes in `updated()` using `cachedTracks` reference comparison:
|
||||
|
||||
```typescript
|
||||
private lastTracksRef: library.Track[] | null = null;
|
||||
|
||||
override updated() {
|
||||
// Existing updated() logic for search term, size properties, etc.
|
||||
// stays unchanged. Add this at the end:
|
||||
const cached = this.libraryCtrl.cachedTracks;
|
||||
if (cached !== null && cached !== this.lastTracksRef) {
|
||||
this.lastTracksRef = cached;
|
||||
this.loadGenres();
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
**Why not move genre extraction to the store?** The store's job is to cache backend data, not derive view-specific aggregations. Genres are only needed by `genres-view` and `genre-details`. Adding genre derivation to the store would couple it to a specific UI concern. The component is the right place for this derivation.
|
||||
|
||||
**Scroll/selection considerations:** Same as `artists-view`. `loadGenres()` handles scroll restoration. Consider adding `this.selectedGenres.clear()` if not already present.
|
||||
|
||||
---
|
||||
|
||||
## Phase 4: Migrate `track-list` (MEDIUM effort)
|
||||
|
||||
**File:** `frontend/src/components/track-list/track-list.ts`
|
||||
|
||||
This has a dual-source pattern (`externalTracks` vs store fetch). The scan listener is already conditionally registered:
|
||||
|
||||
```typescript
|
||||
if (this.externalTracks) {
|
||||
this.tracks = this.externalTracks;
|
||||
} else {
|
||||
this.loadTracks();
|
||||
this.cancelScanComplete = EventsOn(Events.LibraryScanComplete, () => this.loadTracks());
|
||||
}
|
||||
```
|
||||
|
||||
New flow:
|
||||
1. Keep the `externalTracks` path unchanged — when a parent provides tracks, the parent is responsible for refreshing (and the parent's migration in Phase 1/3 handles this).
|
||||
2. For the standalone path (no `externalTracks`):
|
||||
- `connectedCallback()` → `loadTracks()` (initial load, unchanged)
|
||||
- Remove the `LibraryScanComplete` listener registration
|
||||
- Add reactive consumption in `updated()`, guarded by `!this.externalTracks`:
|
||||
|
||||
```typescript
|
||||
private lastTracksRef: library.Track[] | null = null;
|
||||
|
||||
override updated() {
|
||||
// ... existing updated() logic ...
|
||||
|
||||
if (!this.externalTracks) {
|
||||
const cached = this.libraryCtrl.cachedTracks;
|
||||
if (cached !== null && cached !== this.lastTracksRef) {
|
||||
this.lastTracksRef = cached;
|
||||
this.loadTracks();
|
||||
}
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
**Selection consideration:** `loadTracks()` already clears selection. Safe.
|
||||
|
||||
---
|
||||
|
||||
## Phase 5: Migrate `playlist-view` (HIGH effort)
|
||||
|
||||
**File:** `frontend/src/components/playlist-view/playlist-view.ts`
|
||||
|
||||
This component reshapes `playlist.WithTracks[]` into `PlaylistEntry[]` with an `expanded` boolean per entry. It has a `refreshPlaylists()` method that preserves expanded state across refetches.
|
||||
|
||||
Current flow:
|
||||
1. `loadPlaylists()` → `playlistCtrl.getPlaylists()` → map to `PlaylistEntry[]` → `this.entries = result`
|
||||
2. `LibraryScanComplete` → `loadPlaylists()`
|
||||
|
||||
New flow:
|
||||
1. `connectedCallback()` → `loadPlaylists()` (initial load, unchanged)
|
||||
2. Remove the direct `LibraryScanComplete` listener.
|
||||
3. React to store changes in `updated()`:
|
||||
|
||||
```typescript
|
||||
private lastPlaylistsRef: playlist.WithTracks[] | null = null;
|
||||
|
||||
override updated() {
|
||||
// ... existing updated() logic ...
|
||||
|
||||
const cached = this.playlistCtrl.cachedPlaylists;
|
||||
if (cached !== null && cached !== this.lastPlaylistsRef) {
|
||||
this.lastPlaylistsRef = cached;
|
||||
this.refreshPlaylists(); // preserves expanded state
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
**Key choice: use `refreshPlaylists()` instead of `loadPlaylists()`.** The `refreshPlaylists()` method preserves which playlists are expanded, providing a better UX after a scan completes. `loadPlaylists()` resets all to collapsed. The current scan-complete handler uses `loadPlaylists()` (collapsing everything), but since we're improving the architecture anyway, switching to `refreshPlaylists()` is a UX improvement.
|
||||
|
||||
**Alternative consideration:** If `loadPlaylists()` is preferred (to reset UI state after a scan), that works too. The choice is a UX decision, not a technical constraint.
|
||||
|
||||
---
|
||||
|
||||
## Phase 6: Migrate `cover-grid` (VERY HIGH effort)
|
||||
|
||||
**File:** `frontend/src/components/cover-grid/cover-grid.ts`
|
||||
|
||||
The most complex component. Dual-source pattern, split-mode scroll management, sort/filter pipeline.
|
||||
|
||||
Current flow:
|
||||
1. `loadAlbums()` → `libraryCtrl.getAlbums()` or `externalAlbums` → `this.albums = result`
|
||||
2. Only registers scan listener when `!this.externalAlbums`
|
||||
3. `LibraryScanComplete` → `loadAlbums()`
|
||||
|
||||
New flow:
|
||||
1. Keep the `externalAlbums` path unchanged.
|
||||
2. For the standalone path:
|
||||
- `connectedCallback()` → `loadAlbums()` (initial load, unchanged)
|
||||
- Remove the `LibraryScanComplete` listener registration
|
||||
- Add reactive consumption in `updated()`, guarded by `!this.externalAlbums`:
|
||||
|
||||
```typescript
|
||||
private lastAlbumsRef: library.Album[] | null = null;
|
||||
|
||||
override updated() {
|
||||
// ... existing updated() logic (size properties, wheel listener,
|
||||
// grid layout, search term selection clearing) ...
|
||||
|
||||
if (!this.externalAlbums) {
|
||||
const cached = this.libraryCtrl.cachedAlbums;
|
||||
if (cached !== null && cached !== this.lastAlbumsRef) {
|
||||
this.lastAlbumsRef = cached;
|
||||
this.loadAlbums();
|
||||
}
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
**Split-mode consideration:** If the album dropdown is open (`expandedAlbumId !== null`) when a scan completes, `loadAlbums()` will close it (resets `expandedAlbumId` and `expandedTracks`). This is the same behavior as the current direct listener. The split-mode transition logic in `willUpdate()` will handle the layout change.
|
||||
|
||||
**Selection consideration:** `loadAlbums()` already clears album selection. Safe.
|
||||
|
||||
---
|
||||
|
||||
## Phase 7: Cleanup and Documentation
|
||||
|
||||
### 7A. Remove unused imports
|
||||
|
||||
After all data-refresh components are migrated, remove unused `EventsOn` and `Events` imports from migrated components (only if no other events are listened to in that component).
|
||||
|
||||
### 7B. Components that KEEP their direct listeners
|
||||
|
||||
These components are explicitly excluded from migration and should be documented:
|
||||
|
||||
| Component | Reason |
|
||||
|---|---|
|
||||
| `config-page.ts` | Uses event for UI status (scan progress, metrics display), not data refresh. No store handles scan status. |
|
||||
| `library-manager.ts` | Same as config-page — UI status listener for scan progress/metrics. |
|
||||
| `playlist-picker.ts` | Bypasses store entirely, calls `GetAllPlaylists()` directly for lightweight summary data. See refactoring catalog #19 for a future plan to route this through a store. |
|
||||
|
||||
### 7C. Update refactoring catalog
|
||||
|
||||
Mark item #7 as solved in `.opencode/plans/refactoring-catalog.md`.
|
||||
|
||||
---
|
||||
|
||||
## Migration Order Summary
|
||||
|
||||
| Phase | Component(s) | Effort | Depends On |
|
||||
|---|---|---|---|
|
||||
| 0 | `LibraryStore`, `PlaylistStore` (eager refetch) | Low | — |
|
||||
| 1 | `genre-details`, `artist-details` | Low | Phase 0 |
|
||||
| 2 | `artists-view` | Low-Medium | Phase 0 |
|
||||
| 3 | `genres-view` | High | Phase 0 |
|
||||
| 4 | `track-list` | Medium | Phase 0 |
|
||||
| 5 | `playlist-view` | High | Phase 0 |
|
||||
| 6 | `cover-grid` | Very High | Phase 0 |
|
||||
| 7 | Cleanup + docs | Low | Phases 1-6 |
|
||||
|
||||
Each phase after 0 is independent of the others and can be done in any order. The ordering above goes from easiest to hardest as a recommended sequence.
|
||||
|
||||
---
|
||||
|
||||
## Reactive Pattern: Reference Comparison
|
||||
|
||||
All component migrations use the same pattern to detect store data changes:
|
||||
|
||||
```typescript
|
||||
private lastDataRef: T[] | null = null;
|
||||
|
||||
override updated() {
|
||||
const cached = this.controller.cachedData;
|
||||
if (cached !== null && cached !== this.lastDataRef) {
|
||||
this.lastDataRef = cached;
|
||||
this.loadData(); // existing imperative load method
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
**Why reference comparison instead of a version counter or dirty flag:**
|
||||
- **Simplicity:** No store API changes needed. `cachedTracks`/`cachedAlbums`/`cachedArtists`/`cachedPlaylists` are already exposed by controllers.
|
||||
- **Correctness:** Each backend fetch creates a new array instance. `invalidate()` sets cache to `null`. The reference comparison catches both "new data arrived" and "data was cleared and refetched."
|
||||
- **No infinite loops:** Setting `this.lastDataRef = cached` before calling `loadData()` prevents re-triggering. The `loadData()` call may set local `@state()` which triggers another `updated()`, but by then `lastDataRef` matches `cached` and the guard short-circuits.
|
||||
- **No store changes needed:** The controllers already expose `cachedTracks`, `cachedAlbums`, `cachedArtists`, `cachedPlaylists`.
|
||||
|
||||
**Why not move everything into `render()`:** Components do significant local work beyond just displaying store data — filtering, sorting, scroll restoration, selection management. Keeping the imperative `loadData()` call but triggering it reactively is the minimal change that achieves the goal.
|
||||
|
||||
---
|
||||
|
||||
## Risk Assessment
|
||||
|
||||
| Risk | Mitigation |
|
||||
|---|---|
|
||||
| Double fetch on scan complete (eager + component listener during migration) | Store's `waitFor*()` helpers coalesce concurrent requests. Only one backend call actually fires. |
|
||||
| Infinite `updated()` loop | Reference comparison with `lastDataRef` assignment prevents re-triggering. Each migration should be tested for this. |
|
||||
| Stale selection after scan | `loadData()` methods already clear selection in most components. Verify for each migration. |
|
||||
| Scroll position regression | `invalidate()` resets scroll to `0`. `loadData()` methods handle scroll restoration. The `0` position means "start from top", which is correct after a scan. |
|
||||
| `externalAlbums`/`externalTracks` components don't refresh | Parent components (`artist-details`, `genre-details`) are migrated first. They re-fetch and update the `external*` property, which triggers the child's `willUpdate()` change detection. |
|
||||
| `playlist-picker` left unmigrated | Intentional. It uses a different API (`GetAllPlaylists` vs `GetAllPlaylistsWithTracks`). See catalog item #19. |
|
||||
|
||||
---
|
||||
|
||||
## Testing Strategy
|
||||
|
||||
For each phase:
|
||||
1. **Manual test:** Trigger a library scan while each affected view is visible. Verify data refreshes without stale content.
|
||||
2. **Manual test:** Trigger a scan while a detail view is open (`genre-details`, `artist-details`). Verify child components (`track-list`, `cover-grid`) refresh via the parent's external data update.
|
||||
3. **Manual test:** Verify scroll position resets to top after scan.
|
||||
4. **Manual test:** Verify that adding/removing tracks from the library directory and scanning updates all views correctly.
|
||||
5. **Verify no console errors** — especially no infinite loop warnings or unhandled promise rejections.
|
||||
6. **Run `pnpm exec tsc --noEmit`** — ensure no TypeScript errors after each phase.
|
||||
@@ -42,30 +42,11 @@ Prioritized list of architectural improvements identified during a full codebase
|
||||
|
||||
---
|
||||
|
||||
### 8. Type the WebAwesome popup interactions (eliminate 49x `as any`)
|
||||
|
||||
**Problem:** Every component with a context menu uses `(popup as any).anchor = ...` and `(popup as any).active = true`. This pattern appears 49 times across `track-list.ts`, `cover-grid.ts`, `queue-panel.ts`, `playlist-view.ts`, `genres-view.ts`, `artists-view.ts`.
|
||||
|
||||
**Why it matters:** Type safety is completely bypassed for a core interaction pattern. Typos in property names (`actve` instead of `active`) would silently fail.
|
||||
|
||||
**Approach:** Create a type declaration for the WebAwesome popup element (or find one in their package). Alternatively, write a small typed utility:
|
||||
|
||||
```typescript
|
||||
function openPopup(popup: Element, anchor: Element | VirtualAnchor): void
|
||||
function closePopup(popup: Element): void
|
||||
```
|
||||
|
||||
Replace all 49 `as any` casts with calls to these utilities.
|
||||
### ~~8. Type the WebAwesome popup interactions (eliminate 49x `as any`)~~ — solved
|
||||
|
||||
---
|
||||
|
||||
### 9. Replace `GetCurrentTrackInfo` `map[string]interface{}` with a struct
|
||||
|
||||
**Problem:** `player.GetCurrentTrackInfo()` returns `map[string]interface{}` with stringly-typed keys (`"fileName"`, `"filePath"`, `"state"`, `"title"`, etc.). The `emitTrackChanged()` method mutates this map by adding keys after the fact.
|
||||
|
||||
**Why it matters:** No compile-time safety — typos in key names are silent bugs. The Wails binding generator would produce typed TypeScript if given a struct.
|
||||
|
||||
**Approach:** Define a `TrackInfo` struct in the player package with all the fields. Return it from `GetCurrentTrackInfo`. Update `emitTrackChanged` to build the struct directly instead of mutating a map.
|
||||
### ~~9. Replace `GetCurrentTrackInfo` `map[string]interface{}` with a struct~~ — solved
|
||||
|
||||
---
|
||||
|
||||
|
||||
@@ -1,411 +0,0 @@
|
||||
# Plan: Split `cover-grid.ts` + Extract Shared Context Menu
|
||||
|
||||
Addresses refactoring catalog #4 (split `cover-grid.ts`, 3774 lines) and partially addresses #8 (49× `as any` casts on popups).
|
||||
|
||||
## Current State
|
||||
|
||||
`frontend/src/components/cover-grid/cover-grid.ts` is the largest frontend file at 3774 lines. It contains a single `CoverGrid` LitElement that handles:
|
||||
|
||||
- Virtualized album grid rendering (single + split mode with inline dropdown)
|
||||
- Album/track selection (custom inline logic with Ctrl/Shift/range)
|
||||
- Context menus (album + track, with playlist submenu) — **duplicated across 6 components**
|
||||
- Drag-and-drop source (albums + tracks)
|
||||
- Sort controls (toolbar + dropdown)
|
||||
- Ctrl+scroll zoom
|
||||
- Scroll position save/restore (index-based + pixel-based resize-aware)
|
||||
- Transition overlays (DOM snapshots during layout transitions)
|
||||
- Album filtering/sorting (memoized)
|
||||
- 319 lines of CSS
|
||||
|
||||
The context menu logic is copy-pasted into 6 components: `cover-grid.ts`, `track-list.ts`, `playlist-view.ts`, `queue-panel.ts`, `genres-view.ts`, `artists-view.ts`. Each duplicates ~200 lines of state, open/close methods, submenu timers, document event listeners, and render templates.
|
||||
|
||||
---
|
||||
|
||||
## Guiding Principles
|
||||
|
||||
1. **Extract logic modules, not sub-components.** The grid is one visual component. Splitting it into multiple custom elements would create artificial boundaries and state-forwarding complexity. Instead, extract plain TS files (classes/functions) that the component imports.
|
||||
|
||||
2. **Follow existing patterns.** The codebase has `SelectionController` in `utils/`, `drag-controller.ts`, `drag-image.ts`. New extractions follow these conventions.
|
||||
|
||||
3. **Shared context menu is the highest-value extraction.** Duplicated across 6 components, it benefits the whole codebase.
|
||||
|
||||
4. **Don't over-split.** Lifecycle methods, render methods, and data loading are inherently tied to component state and stay in the main file. Some code density is fine for orchestration.
|
||||
|
||||
---
|
||||
|
||||
## Part 1: Types and Constants → `cover-grid-types.ts`
|
||||
|
||||
**New file:** `frontend/src/components/cover-grid/cover-grid-types.ts` (~85 lines)
|
||||
|
||||
**Move from `cover-grid.ts` lines 49-132:**
|
||||
- `ContextMenuTarget` discriminated union type
|
||||
- `GridEntry` interface
|
||||
- `SCROLL_DEBOUNCE_MS`, `ZOOM_STEP` constants
|
||||
- `SORT_FIELD_KEY`, `SORT_DIR_KEY` localStorage key constants
|
||||
- `AlbumSortField` type, `SortDirection` type
|
||||
- `AlbumSortOption` interface
|
||||
- `ALBUM_SORT_OPTIONS` array (3 sort options with comparator functions)
|
||||
|
||||
**Rationale:** Pure data definitions with zero component dependency. Multiple files in the directory will import these (scroll-manager needs `SCROLL_DEBOUNCE_MS`, main file needs sort options, etc.).
|
||||
|
||||
---
|
||||
|
||||
## Part 2: CSS Styles → `cover-grid-styles.ts`
|
||||
|
||||
**New file:** `frontend/src/components/cover-grid/cover-grid-styles.ts` (~270 lines)
|
||||
|
||||
**Move from `cover-grid.ts` lines 343-661**, minus the context-menu styles (~47 lines at 615-661) which move to the shared context menu utility in Part 4.
|
||||
|
||||
Export as a tagged template:
|
||||
```typescript
|
||||
import { css } from 'lit';
|
||||
export const coverGridStyles = css`...`;
|
||||
```
|
||||
|
||||
Main file uses:
|
||||
```typescript
|
||||
import { coverGridStyles } from './cover-grid-styles.js';
|
||||
import { contextMenuStyles } from '@utils/context-menu-controller.js';
|
||||
// ...
|
||||
static override styles = [coverGridStyles, contextMenuStyles];
|
||||
```
|
||||
|
||||
**Rationale:** Standard Lit pattern for large style blocks. Reduces visual noise. The style array composition pattern is idiomatic Lit.
|
||||
|
||||
---
|
||||
|
||||
## Part 3: Scroll Manager → `scroll-manager.ts`
|
||||
|
||||
**New file:** `frontend/src/components/cover-grid/scroll-manager.ts` (~450 lines)
|
||||
|
||||
**Move from `cover-grid.ts`:**
|
||||
- Scroll position persistence: `restoreScrollPosition()` (line 1580), `onVisibilityChanged` (line 1607)
|
||||
- Resize-aware scroll preservation: `setupResizeObserver()` (line 1668), `captureFocusPoint()` (line 1803)
|
||||
- Layout helpers: `getColumnCount()` (line 1865), `getContainerWidth()` (line 1887), `getGridRowWidth()` (line 1901), `getCaratOffset()` (line 1916), `computeSplitIndex()` (line 1949)
|
||||
- Transition overlay: `captureOverlay()` (line 2039), `removeOverlay()` (line 2090)
|
||||
- Scroll positioning: `awaitBeforeLayout()` (line 2116), `computeAdjustedScrollTop()` (line 2135), `restoreScrollTop()` (line 2197), `scrollToShowDropdown()` (line 2265)
|
||||
- Associated fields: `resizeObserver`, `resizeDebounceTimer`, `pendingFocus`, `currentColumnCount`, `isResizing`, `savedScrollTop`, `needsScrollRestore`, `showDropdownAfterRestore`, `scrollRestoreGeneration`, `scrollRestoreResolved`, `savedAlbumViewportOffset`, `transitionOverlay`, `scrollDebounceTimer`
|
||||
|
||||
**Shape:** Plain class with a host interface (not a ReactiveController — scroll management is imperative/async, not reactive).
|
||||
|
||||
```typescript
|
||||
export interface ScrollManagerHost {
|
||||
readonly libraryCtrl: LibraryController;
|
||||
readonly cachedFilteredAlbums: library.Album[];
|
||||
readonly expandedAlbumId: number | null;
|
||||
readonly expandedTracks: library.Track[];
|
||||
readonly splitMode: boolean;
|
||||
readonly splitIndex: number;
|
||||
readonly cardWidth: number;
|
||||
readonly cardHeight: number;
|
||||
readonly cardTextHeight: number;
|
||||
shadowRoot: ShadowRoot | null;
|
||||
updateComplete: Promise<boolean>;
|
||||
requestUpdate(): void;
|
||||
}
|
||||
|
||||
export class ScrollManager {
|
||||
constructor(host: ScrollManagerHost, gridConstants: GridConstants);
|
||||
|
||||
// Called from component lifecycle
|
||||
setup(): void; // from connectedCallback
|
||||
teardown(): void; // from disconnectedCallback
|
||||
|
||||
// Scroll save/restore
|
||||
onVisibilityChanged(e: VisibilityChangedEvent): void;
|
||||
restoreScrollPosition(): void;
|
||||
|
||||
// Resize handling
|
||||
setupResizeObserver(): void;
|
||||
|
||||
// Split/single mode transitions
|
||||
captureOverlay(): void;
|
||||
removeOverlay(): void;
|
||||
computeAdjustedScrollTop(): number;
|
||||
async restoreScrollTop(target: number): Promise<void>;
|
||||
async scrollToShowDropdown(): Promise<void>;
|
||||
awaitBeforeLayout(): Promise<void>;
|
||||
|
||||
// Layout geometry
|
||||
getColumnCount(): number;
|
||||
getContainerWidth(): number;
|
||||
getGridRowWidth(): number;
|
||||
getCaratOffset(): number;
|
||||
computeSplitIndex(): number;
|
||||
|
||||
// State exposed to component
|
||||
needsScrollRestore: boolean;
|
||||
showDropdownAfterRestore: boolean;
|
||||
savedScrollTop: number;
|
||||
savedAlbumViewportOffset: number | null;
|
||||
isResizing: boolean;
|
||||
splitIndex: number;
|
||||
}
|
||||
```
|
||||
|
||||
**Rationale:** Scroll management is the largest concern (~800 raw lines, consolidated to ~450 without the grid constants that stay on the component). It's completely self-contained — reads component state but doesn't modify selection, context menus, or rendering. The host interface decouples it from the concrete class. A plain class (not ReactiveController) is honest about the imperative nature of scroll management.
|
||||
|
||||
---
|
||||
|
||||
## Part 4: Shared Context Menu Controller → `utils/context-menu-controller.ts`
|
||||
|
||||
**New file:** `frontend/src/utils/context-menu-controller.ts` (~200 lines)
|
||||
|
||||
This is the highest cross-cutting value extraction. The same context menu pattern is duplicated in 6 components.
|
||||
|
||||
**Extract the common pattern from all 6 components:**
|
||||
|
||||
```typescript
|
||||
import type { ReactiveController, ReactiveControllerHost } from 'lit';
|
||||
|
||||
export interface ContextMenuHost extends ReactiveControllerHost {
|
||||
// Query accessors — each component provides its own popup element refs
|
||||
getContextMenuPopup(): HTMLElement | undefined;
|
||||
getPlaylistSubmenuPopup(): HTMLElement | undefined;
|
||||
updateComplete: Promise<boolean>;
|
||||
shadowRoot: ShadowRoot | null;
|
||||
}
|
||||
|
||||
export class ContextMenuController implements ReactiveController {
|
||||
// Reactive state (component reads these for rendering)
|
||||
contextMenuOpen = false;
|
||||
playlistSubmenuOpen = false;
|
||||
playlistFilePaths: string[] = [];
|
||||
|
||||
constructor(host: ContextMenuHost);
|
||||
|
||||
// Lifecycle — registers/removes document-level listeners
|
||||
hostConnected(): void;
|
||||
hostDisconnected(): void;
|
||||
|
||||
// Actions
|
||||
openAt(clientX: number, clientY: number): void;
|
||||
close(): void;
|
||||
showPlaylistSubmenu(filePaths: string[]): Promise<void>;
|
||||
closePlaylistSubmenu(): void;
|
||||
onPlaylistActionComplete(): void;
|
||||
}
|
||||
```
|
||||
|
||||
**Also extract** shared context menu CSS styles as:
|
||||
```typescript
|
||||
export const contextMenuStyles = css`
|
||||
#context-menu { ... }
|
||||
.context-menu-panel { ... }
|
||||
wa-dropdown-item { ... }
|
||||
.submenu-item { ... }
|
||||
.submenu-arrow { ... }
|
||||
#playlist-submenu { ... }
|
||||
`;
|
||||
```
|
||||
|
||||
**What stays in each component:**
|
||||
- The `renderContextMenu()` method — menu items differ per component (cover-grid has conditional "Track Details", queue-panel has "Remove" instead of "Add to Queue", etc.)
|
||||
- The `onContextMenuAction(action)` handler — file path resolution differs per component
|
||||
- The `@query` decorators for popup elements (passed to controller via host interface)
|
||||
|
||||
**Components to update (6):**
|
||||
1. `cover-grid.ts` — Remove ~200 lines of inline context menu code
|
||||
2. `track-list.ts` — Remove ~200 lines
|
||||
3. `playlist-view.ts` — Remove ~200 lines (keep the second playlist-level context menu as-is or also migrate)
|
||||
4. `queue-panel.ts` — Remove ~200 lines
|
||||
5. `genres-view.ts` — Remove ~200 lines
|
||||
6. `artists-view.ts` — Remove ~200 lines
|
||||
|
||||
**Bonus:** All 49× `(popup as any).anchor = ...` and `(popup as any).active = ...` casts are now centralized in one file. This partially addresses catalog item #8 — adding proper typing to the controller's internals eliminates the `as any` from all 6 components.
|
||||
|
||||
**Rationale:** ReactiveController is the right shape here (unlike ScrollManager) because it manages document-level event listeners tied to the component lifecycle via `hostConnected`/`hostDisconnected`. This matches the existing `SelectionController` pattern in `utils/`.
|
||||
|
||||
---
|
||||
|
||||
## Part 5: Album Selection Manager → `album-selection.ts`
|
||||
|
||||
**New file:** `frontend/src/components/cover-grid/album-selection.ts` (~250 lines)
|
||||
|
||||
**Move from `cover-grid.ts`:**
|
||||
- Album selection: `selectAlbumRange()` (line 2378), `getSelectedAlbumFilePaths()` (line 2398), `getContextMenuAlbumFilePaths()` (line 2422), `getAlbumFilePaths()` (line 2449)
|
||||
- Drag cache: `warmAlbumFilePathCache()` (line 2471), `getCachedSelectedAlbumFilePaths()` (line 2505), `albumFilePathCache` Map
|
||||
- Track selection: `selectTrackRange()` (line 2528), `getSelectedTrackFilePaths()` (line 2547)
|
||||
- Dropdown coupling: `closeDropdown()` (line 2561), `openDropdown()` (line 2575), `syncDropdownToSelection()` (line 2607)
|
||||
|
||||
**Shape:**
|
||||
```typescript
|
||||
export class AlbumSelectionManager {
|
||||
selectedAlbums = new Set<number>();
|
||||
selectedTracks = new Set<string>();
|
||||
expandedAlbumId: number | null = null;
|
||||
expandedTracks: library.Track[] = [];
|
||||
lastSelectedAlbumIndex: number | null = null;
|
||||
lastSelectedTrackIndex: number | null = null;
|
||||
|
||||
private albumFilePathCache = new Map<number, string[]>();
|
||||
|
||||
// Album selection
|
||||
selectAlbumRange(from: number, to: number, filteredAlbums: library.Album[]): Set<number>;
|
||||
async getSelectedAlbumFilePaths(albums: library.Album[]): Promise<string[]>;
|
||||
async getContextMenuAlbumFilePaths(contextMenuAlbumId: number | null, albums: library.Album[]): Promise<string[]>;
|
||||
|
||||
// Drag cache
|
||||
async warmCache(albums: library.Album[]): Promise<void>;
|
||||
getCachedSelectedPaths(albums: library.Album[]): string[];
|
||||
|
||||
// Track selection
|
||||
selectTrackRange(from: number, to: number): Set<string>;
|
||||
getSelectedTrackFilePaths(): string[];
|
||||
|
||||
// Dropdown
|
||||
async openDropdown(album: library.Album): Promise<void>;
|
||||
closeDropdown(): void;
|
||||
syncDropdownToSelection(filteredAlbums: library.Album[]): void;
|
||||
|
||||
// Reset
|
||||
clear(): void;
|
||||
}
|
||||
```
|
||||
|
||||
**Why not use the existing `SelectionController`?** The existing controller:
|
||||
- Uses string keys only; album selection uses numeric IDs
|
||||
- Manages a single selection set; cover-grid has separate album and track selections
|
||||
- Has no concept of dropdown coupling (selecting 1 album → opens dropdown)
|
||||
- Has no file path caching for drag
|
||||
|
||||
Retrofitting `SelectionController` to handle all of this would make it overly complex for its other consumers (`track-list.ts`, `playlist-view.ts`, `queue-panel.ts`). A dedicated manager for cover-grid's dual album/track model is cleaner.
|
||||
|
||||
**Rationale:** Selection state + file path resolution is a coherent concern (~250 lines) that doesn't need access to the DOM, making it easy to extract. The main component's event handlers become thin wrappers that call into this manager.
|
||||
|
||||
---
|
||||
|
||||
## What Stays in `cover-grid.ts`
|
||||
|
||||
After all extractions and improvements, the main file will be approximately **~1700 lines** (down from 3774):
|
||||
|
||||
| Section | ~Lines | Why it stays |
|
||||
|---------|--------|-------------|
|
||||
| Imports and class declaration | 60 | Structural |
|
||||
| Properties, state, queries, controllers | 100 | Component-specific reactive state (fewer `@state` props) |
|
||||
| Grid layout creation + memoization | 80 | Tightly coupled to virtualizer |
|
||||
| Lifecycle (connectedCallback, disconnectedCallback, willUpdate, updated) | 350 | Orchestration — wires managers together (debug logs removed) |
|
||||
| Dynamic size properties + zoom | 70 | Simple, component-specific |
|
||||
| Data loading | 30 | Simple async fetch |
|
||||
| Virtualizer item builders | 40 | Depends on component state (memoized) |
|
||||
| Event handlers (album + track + drag) | 340 | Thin delegation to managers |
|
||||
| Render methods | 430 | Templates reference component state |
|
||||
| Sort toolbar logic | 120 | Small, self-contained |
|
||||
|
||||
~1700 lines is still substantial, but the *complexity* is dramatically reduced because the three hardest subsystems (scroll management, context menus, selection/file-path resolution) are encapsulated in dedicated modules. The remaining code is pure orchestration and rendering.
|
||||
|
||||
---
|
||||
|
||||
## What This Does NOT Do
|
||||
|
||||
- **Does not split into multiple custom elements** — Artificial component boundaries would add event-forwarding complexity for no UX benefit.
|
||||
- **Does not refactor the split/single virtualizer architecture** — That's the core rendering strategy; changing it is a separate effort.
|
||||
- **Does not retrofit `SelectionController` for albums** — The existing controller serves different consumers with simpler needs. See Part 5 rationale.
|
||||
- **Does not touch `album-dropdown.ts`** — Already a well-scoped 410-line sub-component.
|
||||
- **Does not extract drag handlers** — ~165 lines of glue code that delegates to existing `drag-controller.ts`/`drag-image.ts`. Diminishing returns.
|
||||
|
||||
---
|
||||
|
||||
## Part 6: Code Quality and Performance Improvements
|
||||
|
||||
These improvements are applied during the extraction steps that touch the relevant code. They don't change behavior — they make the same behavior more efficient and clean.
|
||||
|
||||
### 6a. Remove 13 `console.log` debug statements
|
||||
|
||||
**Lines:** 1133, 1151, 1192, 1226, 1303, 1320, 1328, 1390, 1432, 1437, 2158, 2176, 2345
|
||||
|
||||
The scroll restoration and transition overlay code contains 13 `console.log` calls that are clearly development debugging artifacts (e.g., `[willUpdate] exit split (tracks empty)`, `[updated] scroll restore start`, `[adjustScroll]`, `[restoreScrollTop] attempt ${i}`).
|
||||
|
||||
**Action:** Remove all 13 `console.log` calls. Keep the 3 `console.error` (actual failures) and 1 `console.warn` (retry exhaustion).
|
||||
|
||||
**Applied during:** Part 3 (scroll-manager extraction) and Part 5 lifecycle cleanup.
|
||||
|
||||
### 6b. Memoize `buildGridEntries()` — eliminates 3-5 redundant array allocations per render
|
||||
|
||||
**Problem:** `buildGridEntries()` allocates a new `GridEntry[]` array on every call. In split-mode rendering, it's called up to 5 times per render cycle:
|
||||
- `getBeforeEntries()` → `buildGridEntries().slice(0, splitIndex)` (line 2003)
|
||||
- `getAfterEntries()` called **twice** in `renderSplitGrid()` — once for `.length > 0` check (line 3612), once for `.items` (line 3616) — each rebuilding the full array
|
||||
- `onVisibilityChanged` scroll handler also rebuilds it (line 1637)
|
||||
|
||||
There's even a placeholder comment on line 340: `// buildGridEntries() memoization cache.` — but no cache was ever implemented.
|
||||
|
||||
**Action:**
|
||||
1. Cache the `GridEntry[]` result, keyed on `cachedFilteredAlbums` reference identity. Invalidate in `recomputeAlbumCache()`.
|
||||
2. In `renderSplitGrid()`, compute `const afterEntries = this.getAfterEntries()` once and reuse for both the length check and the `.items` binding.
|
||||
|
||||
**Applied during:** Part 1 (types — `GridEntry` moves) and main file cleanup.
|
||||
|
||||
### 6c. Cache expanded album index — eliminates 6 redundant O(n) scans
|
||||
|
||||
**Problem:** `cachedFilteredAlbums.findIndex((a) => a.ID === this.expandedAlbumId)` appears at 6 call sites (lines 1077, 1364, 1813, 1919, 1959, 2276). Each is a linear scan of the album array for the same ID.
|
||||
|
||||
**Action:** Compute `expandedAlbumIndex` in `recomputeAlbumCache()` (or in `willUpdate` when `expandedAlbumId` changes). All 6 call sites become a direct property read. Invalidate when either `expandedAlbumId` or `cachedFilteredAlbums` changes.
|
||||
|
||||
**Applied during:** Part 3 (scroll-manager extraction — 4 of the 6 sites are in scroll code) and main file cleanup.
|
||||
|
||||
### 6d. Build `albumById` Map for O(1) selection lookups
|
||||
|
||||
**Problem:** `getSelectedAlbumFilePaths()` (line 2401) and `warmAlbumFilePathCache()` (line 2472) both call `this.albums.filter(a => selectedAlbums.has(a.ID))` to find selected albums — an O(n) scan of the full album list. `resolveTrackCoverArt()` (line 3181) does `this.albums.find(a => a.Name === albumName)` — an O(n) name-based scan that could also match the wrong album if names collide.
|
||||
|
||||
**Action:** Build a `Map<number, library.Album>` (keyed by album ID) when `albums` changes. Selection lookups iterate `selectedAlbums` and do O(1) map lookups. `resolveTrackCoverArt()` uses the map with `expandedAlbumId` instead of name-based search.
|
||||
|
||||
**Applied during:** Part 5 (album-selection extraction).
|
||||
|
||||
### 6e. Remove unnecessary `@state()` from 2 properties
|
||||
|
||||
**Problem:** 15 properties have `@state()`. Two don't need it:
|
||||
- `playlistFilePaths` (line 695) — only rendered inside the playlist submenu, which is conditionally shown when `playlistSubmenuOpen` is true. Since `showPlaylistSubmenu()` sets `playlistFilePaths` before setting `playlistSubmenuOpen`, the reactive update from `playlistSubmenuOpen` will render with the correct paths. `playlistFilePaths` itself doesn't need to trigger a re-render.
|
||||
- `splitIndex` (line 737) — only used to compute `getBeforeEntries()`/`getAfterEntries()`. It's always set before `splitMode` changes (which triggers the render), so it doesn't need independent reactivity.
|
||||
|
||||
**Action:** Remove `@state()` decorator from both. Make them plain private fields.
|
||||
|
||||
**Applied during:** Main file cleanup after extractions.
|
||||
|
||||
### 6f. Single-pass `onGridClick` path traversal
|
||||
|
||||
**Problem:** `onGridClick` (line 3071) calls `composedPath()` once, then iterates it twice with `.some()` — once for `.album-card` and once for `.album-dropdown`.
|
||||
|
||||
**Action:** Single loop checking both classes:
|
||||
```typescript
|
||||
for (const el of e.composedPath()) {
|
||||
if (!(el instanceof HTMLElement)) continue;
|
||||
if (el.classList.contains('album-card') ||
|
||||
el.classList.contains('album-dropdown')) return;
|
||||
}
|
||||
```
|
||||
|
||||
**Applied during:** Main file cleanup.
|
||||
|
||||
### 6g. Use expanded album directly for cover art resolution
|
||||
|
||||
**Problem:** `resolveTrackCoverArt(albumName)` (line 3176) does an O(n) `.find()` on `this.albums` by `Name` to get cover art URLs. But we already know which album is expanded (`expandedAlbumId`), and all tracks in the dropdown belong to that album. Name-based lookup has a theoretical collision risk if two albums share the same name.
|
||||
|
||||
**Action:** Replace the name-based search with a direct lookup using `expandedAlbumId` and the `albumById` map from improvement 6d. Falls back gracefully if the album isn't found.
|
||||
|
||||
**Applied during:** Part 5 (album-selection extraction) or main file cleanup.
|
||||
|
||||
### 6h. Prune `albumFilePathCache` to prevent unbounded growth
|
||||
|
||||
**Problem:** The `albumFilePathCache` (Map<number, string[]>) is warmed when albums are selected and read during dragstart, but entries are never removed. Over a session, it grows without bound.
|
||||
|
||||
**Action:**
|
||||
1. Clear the entire cache when `albums` changes (library rescan).
|
||||
2. After `warmAlbumFilePathCache()` completes, remove entries whose album ID is no longer in `selectedAlbums`.
|
||||
|
||||
**Applied during:** Part 5 (album-selection extraction — the cache moves to `AlbumSelectionManager`).
|
||||
|
||||
---
|
||||
|
||||
## Execution Order
|
||||
|
||||
| Step | File(s) | Risk | Notes |
|
||||
|------|---------|------|-------|
|
||||
| 1 | `cover-grid-types.ts` | Minimal | Pure move, no logic changes |
|
||||
| 2 | `cover-grid-styles.ts` | Minimal | Pure move, verify `static styles` array works |
|
||||
| 3 | `utils/context-menu-controller.ts` | Medium | Widest blast radius — update 6 components |
|
||||
| 4 | `album-selection.ts` + improvements 6d, 6g, 6h | Low | Contained to cover-grid |
|
||||
| 5 | `scroll-manager.ts` + improvements 6a, 6c | Medium | Largest extraction, deep state interaction |
|
||||
| 6 | Main file cleanup: improvements 6b, 6e, 6f | Low | After extractions, clean up remaining code |
|
||||
| 7 | Verify: `pnpm build` + `pnpm exec tsc --noEmit` | — | Ensure no type errors or build failures |
|
||||
|
||||
Steps 1-2 are safe warmups. Step 3 has the highest cross-cutting value. Steps 4-5 are the structural wins for cover-grid itself. Step 6 is polish. Each step should be independently verifiable with `tsc --noEmit`.
|
||||
@@ -1,267 +0,0 @@
|
||||
# Plan: Split and Refactor `backend/queue/queue.go`
|
||||
|
||||
Addresses refactoring catalog #3 (split `queue.go`), #14 (unused sentinels), and #18 (custom `sortInts`), plus two bug fixes and four DRY improvements discovered during analysis.
|
||||
|
||||
## Current State
|
||||
|
||||
`backend/queue/queue.go` is a single 2297-line file containing:
|
||||
- Type definitions (9 types/constants)
|
||||
- Constructor and lifecycle methods
|
||||
- 11 event handler methods (~310 lines of boilerplate)
|
||||
- 15+ queue operation methods (add, insert, remove, move, play, etc.)
|
||||
- 6 navigation/shuffle functions
|
||||
- 7 database I/O functions
|
||||
- 4 event emission helpers
|
||||
|
||||
The file is hard to navigate, hard to review, and mixes unrelated concerns.
|
||||
|
||||
---
|
||||
|
||||
## Part 1: File Split
|
||||
|
||||
### 1a. `queue.go` (~1200 lines) — Types, struct, constructor, business logic
|
||||
|
||||
**Keep:**
|
||||
- Package doc comment
|
||||
- All type/const definitions: `RepeatMode`, `PreviousRestartThreshold`, `maxSQLiteVars`, `initialBatchSize`, `trackMeta`, `TrackLoader`, `Track`, `State`, `IndexChanged`, `ModeChanged`, `TracksModified`, `Queue` struct
|
||||
- Constructor: `NewQueue`
|
||||
- Lifecycle: `SetContext`, `SetPlayer`
|
||||
- All public queue operations: `SetQueue`, `resolveRemainingTracks`, `AddTrack`, `AddTracks`, `InsertNext`, `InsertNextTracks`, `InsertTracksAt`, `MoveQueueTracks`, `RemoveTrack`, `RemoveTracks`, `Play`, `playFromStart`, `PlayIndex`, `ToggleShuffle`, `CycleRepeat`, `GetState`, `Clear`, `EmitCurrentState`
|
||||
- Playback helpers: `playOrLoadCurrentTrack`, `loadCurrentTrack`, `playCurrentTrack`, `handleCurrentTrackRemoved`, `onQueueExhausted`, `reindexPositions`
|
||||
- New helpers: `trackMeta.toTrack()`, `commitMutation()`
|
||||
|
||||
**Imports:** `context`, `log/slog`, `slices`, `sync`, `sync/atomic`, `github.com/wailsapp/wails/v2/pkg/runtime`, `yellowjacket/backend/database`, `yellowjacket/backend/profiling`
|
||||
|
||||
### 1b. `handlers.go` (~280 lines) — Event handlers and external callbacks
|
||||
|
||||
**Move:**
|
||||
- `OnPlaybackFinished` (external callback from player — same dispatch pattern as event handlers)
|
||||
- `registerEventHandlers`
|
||||
- All 10 `handle*` methods
|
||||
- New helpers: `toStringSlice()`, `toIntSlice()`
|
||||
|
||||
**Imports:** `github.com/wailsapp/wails/v2/pkg/runtime`, `yellowjacket/backend/events`
|
||||
|
||||
**Rationale:** Pure dispatch boilerplate. Adding/modifying event handlers only touches this file plus event constants. `OnPlaybackFinished` is included because it's an inbound callback invoked from outside (the player), same conceptual layer as the event handlers.
|
||||
|
||||
### 1c. `persistence.go` (~330 lines) — All database I/O
|
||||
|
||||
**Move:**
|
||||
- `lookupTrackMetaBatch`, `lookupChunk` (metadata lookup)
|
||||
- `persistTracks`, `insertTrackBatch` (track persistence)
|
||||
- `persistState` (state persistence)
|
||||
- `SaveState` (public wrapper)
|
||||
- `RestoreState` (public, loads from DB)
|
||||
|
||||
**Imports:** `database/sql`, `encoding/json`, `fmt`, `strings`, `yellowjacket/backend/database/sql/sqlcgen`, `yellowjacket/backend/profiling`
|
||||
|
||||
**Rationale:** All database interaction in one place. Schema changes, query optimizations, or persistence strategy changes only affect this file.
|
||||
|
||||
### 1d. `navigation.go` (~130 lines) — Index navigation and shuffle order
|
||||
|
||||
**Move:**
|
||||
- `nextIndex`, `previousIndex` (linear/shuffled dispatch with repeat logic)
|
||||
- `nextShuffledIndex`, `previousShuffledIndex`
|
||||
- `currentShufflePosition`
|
||||
- `generateShuffleOrder` (Fisher-Yates)
|
||||
|
||||
**Imports:** `math/rand/v2`
|
||||
|
||||
**Rationale:** The catalog suggested `shuffle.go`, but these 6 functions form a cohesive "navigation" group — `nextIndex`/`previousIndex` contain both the linear (repeat-aware) and the shuffle dispatching logic. Naming it `shuffle.go` would be misleading since half the file handles non-shuffle navigation. These functions only access `q.tracks`, `q.currentIndex`, `q.shuffleOrder`, and `q.repeatMode` — a cleanly bounded dependency set.
|
||||
|
||||
### 1e. `emit.go` (~75 lines) — Event emission helpers
|
||||
|
||||
**Move:**
|
||||
- `emitQueueChanged`
|
||||
- `emitIndexChanged`
|
||||
- `emitModeChanged`
|
||||
- `emitTracksModified`
|
||||
|
||||
**Imports:** `github.com/wailsapp/wails/v2/pkg/runtime`, `yellowjacket/backend/events`
|
||||
|
||||
**Rationale:** Clean boundary — the rest of the code calls `q.emit*()` without knowing event names or payload shapes.
|
||||
|
||||
---
|
||||
|
||||
## Part 2: Bug Fixes (behavior-preserving — fixing existing broken behavior)
|
||||
|
||||
### 2a. Fix `InsertNext` empty-queue bug
|
||||
|
||||
**Location:** `queue.go:991-1041` (current)
|
||||
|
||||
**Problem:** `InsertNext` does not handle the empty-queue case. When called on an empty queue:
|
||||
- `insertPos = currentIndex + 1 = 0 + 1 = 1` (out of bounds clamped to 0 by the guard)
|
||||
- A track is inserted, but `currentIndex` stays at 0 and `loadCurrentTrack` is never called
|
||||
- The user sees a queue with one track but nothing loaded
|
||||
|
||||
Compare with `InsertNextTracks` (line 977-980) which correctly checks `wasEmpty` and loads the first track.
|
||||
|
||||
**Fix:** Add after the persist calls in `InsertNext`:
|
||||
```go
|
||||
wasEmpty := len(q.tracks) == 0
|
||||
// ... existing insert logic ...
|
||||
// After commitMutation:
|
||||
if wasEmpty && len(q.tracks) > 0 {
|
||||
q.currentIndex = 0
|
||||
q.loadCurrentTrack()
|
||||
}
|
||||
```
|
||||
|
||||
### 2b. Fix `AddTracks` persist-before-index ordering
|
||||
|
||||
**Location:** `queue.go:906-912` (current)
|
||||
|
||||
**Problem:** `AddTracks` calls `persistTracks()` + `persistState()` at lines 906-907, then sets `currentIndex = 0` and calls `loadCurrentTrack()` at lines 909-912. If the app crashes between persist and index update, the restored state has the wrong `currentIndex`. `AddTrack` does this correctly (sets index before persist).
|
||||
|
||||
**Fix:** Move the `wasEmpty` check and `currentIndex = 0` assignment to before the `commitMutation()` call, matching the pattern in `AddTrack`.
|
||||
|
||||
---
|
||||
|
||||
## Part 3: DRY Improvements (behavior-preserving)
|
||||
|
||||
### 3a. Extract `toStringSlice` and `toIntSlice` helpers (in `handlers.go`)
|
||||
|
||||
**Problem:** The `[]interface{} -> []string` conversion is copy-pasted in 4 handlers (`handleSetQueue`, `handleAddTracksToQueue`, `handleInsertTracksAtIndex`, `handlePlayTracksNext`). The `[]interface{} -> []int` conversion is in 2 handlers (`handleRemoveTracksFromQueue`, `handleMoveQueueTracks`).
|
||||
|
||||
**New helpers:**
|
||||
```go
|
||||
// toStringSlice extracts strings from a Wails event argument.
|
||||
func toStringSlice(raw []interface{}) []string {
|
||||
result := make([]string, 0, len(raw))
|
||||
for _, v := range raw {
|
||||
if s, ok := v.(string); ok {
|
||||
result = append(result, s)
|
||||
}
|
||||
}
|
||||
return result
|
||||
}
|
||||
|
||||
// toIntSlice extracts ints (from float64) from a Wails event argument.
|
||||
func toIntSlice(raw []interface{}) []int {
|
||||
result := make([]int, 0, len(raw))
|
||||
for _, v := range raw {
|
||||
if f, ok := v.(float64); ok {
|
||||
result = append(result, int(f))
|
||||
}
|
||||
}
|
||||
return result
|
||||
}
|
||||
```
|
||||
|
||||
Eliminates ~30 lines of repetition, centralizes type-coercion logic.
|
||||
|
||||
### 3b. Extract `trackMeta.toTrack(position)` method (in `queue.go`)
|
||||
|
||||
**Problem:** The `trackMeta` -> `Track` struct literal appears 7 times across `SetQueue`, `resolveRemainingTracks`, `AddTrack`, `AddTracks`, `InsertNextTracks`, `InsertNext`, `InsertTracksAt`.
|
||||
|
||||
**New method:**
|
||||
```go
|
||||
// toTrack converts metadata lookup results into a queue Track.
|
||||
func (m trackMeta) toTrack(position int64) Track {
|
||||
return Track{
|
||||
AudioFileID: m.AudioFileID,
|
||||
FilePath: m.FilePath,
|
||||
Position: position,
|
||||
Title: m.Title,
|
||||
Artist: m.Artist,
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
Eliminates ~35 lines. Creates one authoritative mapping point — if a field is added to `Track`, only one place needs updating.
|
||||
|
||||
### 3c. Extract `commitMutation(reindex bool)` helper (in `queue.go`)
|
||||
|
||||
**Problem:** The post-mutation epilogue (reindex positions → regenerate shuffle order → persist tracks → persist state) is repeated in 8+ methods: `InsertNextTracks`, `InsertNext`, `InsertTracksAt`, `MoveQueueTracks`, `RemoveTrack`, `RemoveTracks`, `AddTracks`, `SetQueue` (small-batch path), `AddTrack` (after unification).
|
||||
|
||||
**New helper:**
|
||||
```go
|
||||
// commitMutation persists the current queue state after a mutation.
|
||||
// When reindex is true, track positions are renumbered first.
|
||||
func (q *Queue) commitMutation(reindex bool) {
|
||||
if reindex {
|
||||
q.reindexPositions()
|
||||
}
|
||||
if q.shuffleMode {
|
||||
q.generateShuffleOrder()
|
||||
}
|
||||
q.persistTracks()
|
||||
q.persistState()
|
||||
}
|
||||
```
|
||||
|
||||
Eliminates ~40 lines. Ensures every mutation consistently applies the full epilogue — no risk of forgetting one of the steps.
|
||||
|
||||
### 3d. Use `slices.Insert` for slice insertions (in `queue.go`)
|
||||
|
||||
**Problem:** The manual tail-copy insertion pattern appears 3 times:
|
||||
```go
|
||||
tail := make([]Track, len(q.tracks[insertPos:]))
|
||||
copy(tail, q.tracks[insertPos:])
|
||||
q.tracks = append(q.tracks[:insertPos], newTracks...)
|
||||
q.tracks = append(q.tracks, tail...)
|
||||
```
|
||||
in `InsertNextTracks`, `InsertTracksAt`, and `MoveQueueTracks`. `InsertNext` has a variant.
|
||||
|
||||
**Fix:** Replace all with `q.tracks = slices.Insert(q.tracks, insertPos, newTracks...)`. The `slices` package is already imported.
|
||||
|
||||
### 3e. Unify `AddTrack` persistence strategy (in `queue.go`)
|
||||
|
||||
**Problem:** `AddTrack` is the only method that uses a single-row `InsertQueueTrack` DB call (line 834), while every other mutating method uses `persistTracks` (full table rewrite). This dual strategy means:
|
||||
- If the single-row insert fails, the in-memory state diverges from the DB
|
||||
- `AddTrack` has different error recovery behavior than all other methods
|
||||
- The shuffle order append (line 847) is an optimization that `AddTracks` doesn't share, creating inconsistency
|
||||
|
||||
**Fix:** Replace `AddTrack`'s custom DB insert with `commitMutation(false)` (no reindex needed since it appends). This makes it consistent with every other method. The performance cost of a full table rewrite for a single-track add is negligible for music-player queue sizes (typically <10K tracks).
|
||||
|
||||
---
|
||||
|
||||
## Part 4: Cleanup (bundled from catalog #14 and #18)
|
||||
|
||||
### 4a. Delete `sortInts`, use `slices.Sort` (catalog #18)
|
||||
|
||||
**Location:** `queue.go:1274-1281` (current)
|
||||
|
||||
Delete the hand-rolled insertion sort. Replace its one call site in `MoveQueueTracks` (`sortInts(sorted)` → `slices.Sort(sorted)`). `slices.Sort` is already used elsewhere in the same file (line 1364).
|
||||
|
||||
### 4b. Remove exported `PlayFromStart` wrapper
|
||||
|
||||
**Location:** `queue.go:1521-1530` (current)
|
||||
|
||||
`PlayFromStart` is exported but has zero callers outside the package. The unexported `playFromStart` already exists. Remove the exported wrapper — if external access is ever needed, it can be re-added.
|
||||
|
||||
---
|
||||
|
||||
## Execution Order
|
||||
|
||||
The order matters because later steps depend on earlier ones:
|
||||
|
||||
1. **Replace `sortInts` with `slices.Sort`** — single-line change, eliminates a function before the split
|
||||
2. **Remove `PlayFromStart`** — eliminates dead code before the split
|
||||
3. **Add `trackMeta.toTrack()` method** — replace all 7 call sites
|
||||
4. **Add `commitMutation()` helper** — replace all 8+ call sites
|
||||
5. **Fix `InsertNext` empty-queue bug** — add `wasEmpty` guard
|
||||
6. **Fix `AddTracks` persist ordering** — move index assignment before persist
|
||||
7. **Unify `AddTrack` persistence** — replace custom insert with `commitMutation`
|
||||
8. **Use `slices.Insert`** — replace 3-4 manual insertion patterns
|
||||
9. **Extract `handlers.go`** — move `OnPlaybackFinished`, `registerEventHandlers`, all `handle*` methods; add `toStringSlice`/`toIntSlice` helpers; update all 6 call sites
|
||||
10. **Extract `emit.go`** — move all 4 `emit*` methods
|
||||
11. **Extract `navigation.go`** — move all 6 navigation/shuffle functions
|
||||
12. **Extract `persistence.go`** — move all 7 persistence/lookup functions
|
||||
13. **Clean up `queue.go` imports** — remove now-unused imports (`encoding/json`, `fmt`, `strings`, `math/rand/v2`, `errors`, `yellowjacket/backend/events`, `yellowjacket/backend/database/sql/sqlcgen`)
|
||||
14. **Delete `ErrEmptyQueue` and `ErrNoPlayer`** — unused sentinels (catalog #14)
|
||||
15. **Run `make lint`** — fix any formatting/import-order issues
|
||||
16. **Run `make test`** — verify nothing is broken (note: no queue-specific tests exist, but this catches compilation errors and any tests that depend on queue indirectly)
|
||||
17. **Update refactoring catalog** — mark #3, #14, #18 as solved
|
||||
|
||||
## Risk Assessment
|
||||
|
||||
**Very low risk.** All files remain in the same `queue` package — field access, unexported methods, and mutex sharing work identically across files within a package. The Go compiler catches any missing imports or broken references at build time. The two bug fixes change behavior only in edge cases that are currently broken. The DRY extractions are mechanical transformations that preserve identical behavior.
|
||||
|
||||
## What This Does NOT Change
|
||||
|
||||
- No changes to the public API surface (except removing unused `PlayFromStart` and the unused sentinels)
|
||||
- No changes to the mutex strategy or locking granularity
|
||||
- No changes to the event system or frontend
|
||||
- No changes to database schema or query logic
|
||||
- No new dependencies
|
||||
+48
-65
@@ -64,6 +64,26 @@ const (
|
||||
Stopped State = "stopped"
|
||||
)
|
||||
|
||||
// TrackInfo contains metadata and playback state for the currently
|
||||
// loaded track. It is emitted as the payload of the TrackChanged
|
||||
// event and serialized as camelCase JSON to match the frontend
|
||||
// TrackInfo interface in player-store.ts.
|
||||
type TrackInfo struct {
|
||||
FileName string `json:"fileName"`
|
||||
FilePath string `json:"filePath"`
|
||||
State State `json:"state"`
|
||||
Title string `json:"title"`
|
||||
Artist string `json:"artist"`
|
||||
Album string `json:"album"`
|
||||
CoverArt string `json:"coverArt"`
|
||||
CoverArtSmall string `json:"coverArtSmall"`
|
||||
CoverArtMedium string `json:"coverArtMedium"`
|
||||
CoverArtLarge string `json:"coverArtLarge"`
|
||||
TrackLength int `json:"trackLength"`
|
||||
SeekPosition int `json:"seekPosition"`
|
||||
TrackChangeID uint64 `json:"trackChangeId"`
|
||||
}
|
||||
|
||||
// Sentinel errors for player operations.
|
||||
var (
|
||||
errNoControlStreamer = errors.New("no control streamer")
|
||||
@@ -307,28 +327,19 @@ func (p *Player) emitTrackChanged() {
|
||||
return
|
||||
}
|
||||
|
||||
trackInfo := p.getCurrentTrackInfoLocked()
|
||||
|
||||
trackLengthSecs, err := p.trackLengthLocked()
|
||||
if err != nil {
|
||||
p.logger.Error("Cannot get track length")
|
||||
}
|
||||
|
||||
trackInfo, err := p.getCurrentTrackInfoLocked()
|
||||
if err != nil {
|
||||
p.logger.Error("Cannot get track info")
|
||||
|
||||
trackInfo = map[string]interface{}{
|
||||
"fileName": "",
|
||||
"filePath": "",
|
||||
"state": string(p.state),
|
||||
}
|
||||
}
|
||||
trackInfo.TrackLength = trackLengthSecs
|
||||
|
||||
// Compute current seek position in seconds.
|
||||
seekPosition := 0
|
||||
|
||||
if p.seeker != nil {
|
||||
speaker.Lock()
|
||||
seekPosition = p.seeker.Position() /
|
||||
trackInfo.SeekPosition = p.seeker.Position() /
|
||||
int(p.format.SampleRate)
|
||||
speaker.Unlock()
|
||||
}
|
||||
@@ -336,12 +347,11 @@ func (p *Player) emitTrackChanged() {
|
||||
// Increment track change ID so the frontend can detect changes
|
||||
// even when the same file plays consecutively.
|
||||
p.trackChangeID++
|
||||
trackInfo.TrackChangeID = p.trackChangeID
|
||||
|
||||
// Emit comprehensive track info.
|
||||
trackInfo["trackLength"] = trackLengthSecs
|
||||
trackInfo["seekPosition"] = seekPosition
|
||||
trackInfo["trackChangeId"] = p.trackChangeID
|
||||
runtime.EventsEmit(p.ctx, events.TrackChanged, trackInfo)
|
||||
runtime.EventsEmit(
|
||||
p.ctx, events.TrackChanged, trackInfo,
|
||||
)
|
||||
|
||||
p.logger.Info(
|
||||
"Emitting TrackChangedEvent with track info",
|
||||
@@ -824,85 +834,58 @@ func (p *Player) seekLocked(targetSeconds int) error {
|
||||
|
||||
// GetCurrentTrackInfo returns information about the currently
|
||||
// loaded track.
|
||||
func (p *Player) GetCurrentTrackInfo() (
|
||||
map[string]interface{}, error,
|
||||
) {
|
||||
func (p *Player) GetCurrentTrackInfo() TrackInfo {
|
||||
p.mu.Lock()
|
||||
defer p.mu.Unlock()
|
||||
|
||||
return p.getCurrentTrackInfoLocked()
|
||||
}
|
||||
|
||||
func (p *Player) getCurrentTrackInfoLocked() (
|
||||
map[string]interface{}, error,
|
||||
) {
|
||||
if p.currentFile == nil {
|
||||
return map[string]interface{}{
|
||||
"fileName": "",
|
||||
"filePath": "",
|
||||
"state": string(p.state),
|
||||
"title": "",
|
||||
"artist": "",
|
||||
"album": "",
|
||||
"coverArt": "",
|
||||
}, nil
|
||||
func (p *Player) getCurrentTrackInfoLocked() TrackInfo {
|
||||
info := TrackInfo{
|
||||
State: p.state,
|
||||
}
|
||||
|
||||
fileName := filepath.Base(p.currentFile.Name())
|
||||
filePath := p.currentFile.Name()
|
||||
if p.currentFile == nil {
|
||||
return info
|
||||
}
|
||||
|
||||
// Default values.
|
||||
title := fileName
|
||||
artist := ""
|
||||
album := ""
|
||||
coverArt := ""
|
||||
coverArtSmall := ""
|
||||
coverArtMedium := ""
|
||||
coverArtLarge := ""
|
||||
info.FileName = filepath.Base(p.currentFile.Name())
|
||||
info.FilePath = p.currentFile.Name()
|
||||
info.Title = info.FileName // default title is the filename
|
||||
|
||||
// Try to get metadata from database.
|
||||
if p.db != nil {
|
||||
meta, err := p.db.Queries.GetTrackMetadataByPath(
|
||||
p.ctx, filePath,
|
||||
p.ctx, info.FilePath,
|
||||
)
|
||||
if err == nil {
|
||||
if meta.Title != "" {
|
||||
title = meta.Title
|
||||
info.Title = meta.Title
|
||||
}
|
||||
|
||||
artist = meta.Artist
|
||||
album = meta.Album
|
||||
info.Artist = meta.Artist
|
||||
info.Album = meta.Album
|
||||
|
||||
if meta.CoverArtPath != "" {
|
||||
base := filepath.Base(meta.CoverArtPath)
|
||||
coverArt = "/covers/" + base
|
||||
coverArtSmall = "/covers/" +
|
||||
info.CoverArt = "/covers/" + base
|
||||
info.CoverArtSmall = "/covers/" +
|
||||
library.SizedFilename(base, "_sm")
|
||||
coverArtMedium = "/covers/" +
|
||||
info.CoverArtMedium = "/covers/" +
|
||||
library.SizedFilename(base, "_md")
|
||||
coverArtLarge = "/covers/" +
|
||||
info.CoverArtLarge = "/covers/" +
|
||||
library.SizedFilename(base, "_lg")
|
||||
}
|
||||
} else {
|
||||
p.logger.Debug(
|
||||
"Could not get track metadata from database",
|
||||
"path", filePath, "err", err,
|
||||
"path", info.FilePath, "err", err,
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
return map[string]interface{}{
|
||||
"fileName": fileName,
|
||||
"filePath": filePath,
|
||||
"state": string(p.state),
|
||||
"title": title,
|
||||
"artist": artist,
|
||||
"album": album,
|
||||
"coverArt": coverArt,
|
||||
"coverArtSmall": coverArtSmall,
|
||||
"coverArtMedium": coverArtMedium,
|
||||
"coverArtLarge": coverArtLarge,
|
||||
}, nil
|
||||
return info
|
||||
}
|
||||
|
||||
// TrackLengthInSeconds returns the duration of the current track.
|
||||
|
||||
@@ -25,6 +25,7 @@ import {
|
||||
import type { ContextMenuHost } from '@utils/context-menu-controller.js';
|
||||
import '@awesome.me/webawesome/dist/components/icon/icon.js';
|
||||
import '@awesome.me/webawesome/dist/components/popup/popup.js';
|
||||
import type WaPopup from '@awesome.me/webawesome/dist/components/popup/popup.js';
|
||||
import '@awesome.me/webawesome/dist/components/dropdown-item/dropdown-item.js';
|
||||
import '@components/playlist-picker/playlist-picker.js';
|
||||
|
||||
@@ -99,17 +100,17 @@ export class ArtistsView
|
||||
private contextMenuArtistId: number | null = null;
|
||||
|
||||
@query('#context-menu')
|
||||
private contextMenuPopup!: HTMLElement;
|
||||
private contextMenuPopup!: WaPopup;
|
||||
|
||||
@query('#playlist-submenu')
|
||||
private playlistSubmenuPopup!: HTMLElement;
|
||||
private playlistSubmenuPopup!: WaPopup;
|
||||
|
||||
getContextMenuPopup(): HTMLElement | undefined {
|
||||
getContextMenuPopup(): WaPopup | undefined {
|
||||
return this.contextMenuPopup;
|
||||
}
|
||||
|
||||
getPlaylistSubmenuPopup():
|
||||
| HTMLElement
|
||||
| WaPopup
|
||||
| undefined {
|
||||
return this.playlistSubmenuPopup;
|
||||
}
|
||||
|
||||
@@ -17,6 +17,7 @@ import { LibraryController } from '@store/controllers/library-controller';
|
||||
import { SearchController } from '@store/controllers/search-controller';
|
||||
import { queueStore } from '@store/queue-store';
|
||||
import '@awesome.me/webawesome/dist/components/popup/popup.js';
|
||||
import type WaPopup from '@awesome.me/webawesome/dist/components/popup/popup.js';
|
||||
import '@awesome.me/webawesome/dist/components/dropdown-item/dropdown-item.js';
|
||||
import '@awesome.me/webawesome/dist/components/icon/icon.js';
|
||||
import '@components/playlist-picker/playlist-picker.js';
|
||||
@@ -290,7 +291,7 @@ export class CoverGrid
|
||||
private sortDropdownOpen = false;
|
||||
|
||||
@query('#sort-dropdown')
|
||||
private sortDropdownPopup!: HTMLElement;
|
||||
private sortDropdownPopup!: WaPopup;
|
||||
|
||||
/** ID of the album whose dropdown is currently open, or null. */
|
||||
@state()
|
||||
@@ -321,17 +322,17 @@ export class CoverGrid
|
||||
splitIndex = 0;
|
||||
|
||||
@query('#context-menu')
|
||||
private contextMenuPopup!: HTMLElement;
|
||||
private contextMenuPopup!: WaPopup;
|
||||
|
||||
@query('#playlist-submenu')
|
||||
private playlistSubmenuPopup!: HTMLElement;
|
||||
private playlistSubmenuPopup!: WaPopup;
|
||||
|
||||
// ContextMenuHost interface.
|
||||
getContextMenuPopup(): HTMLElement | undefined {
|
||||
getContextMenuPopup(): WaPopup | undefined {
|
||||
return this.contextMenuPopup;
|
||||
}
|
||||
|
||||
getPlaylistSubmenuPopup(): HTMLElement | undefined {
|
||||
getPlaylistSubmenuPopup(): WaPopup | undefined {
|
||||
return this.playlistSubmenuPopup;
|
||||
}
|
||||
|
||||
@@ -435,8 +436,8 @@ export class CoverGrid
|
||||
);
|
||||
|
||||
if (popup && anchor) {
|
||||
(popup as any).anchor = anchor;
|
||||
(popup as any).active = true;
|
||||
popup.anchor = anchor;
|
||||
popup.active = true;
|
||||
}
|
||||
}
|
||||
|
||||
@@ -448,7 +449,7 @@ export class CoverGrid
|
||||
const popup = this.sortDropdownPopup;
|
||||
|
||||
if (popup) {
|
||||
(popup as any).active = false;
|
||||
popup.active = false;
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -21,6 +21,7 @@ import {
|
||||
import type { ContextMenuHost } from '@utils/context-menu-controller.js';
|
||||
import '@awesome.me/webawesome/dist/components/icon/icon.js';
|
||||
import '@awesome.me/webawesome/dist/components/popup/popup.js';
|
||||
import type WaPopup from '@awesome.me/webawesome/dist/components/popup/popup.js';
|
||||
import '@awesome.me/webawesome/dist/components/dropdown-item/dropdown-item.js';
|
||||
import '@components/playlist-picker/playlist-picker.js';
|
||||
|
||||
@@ -102,19 +103,19 @@ export class GenresView
|
||||
private contextMenuGenreName: string | null = null;
|
||||
|
||||
@query('#context-menu')
|
||||
private contextMenuPopup!: HTMLElement;
|
||||
private contextMenuPopup!: WaPopup;
|
||||
|
||||
@query('#playlist-submenu')
|
||||
private playlistSubmenuPopup!: HTMLElement;
|
||||
private playlistSubmenuPopup!: WaPopup;
|
||||
|
||||
// ----- ContextMenuHost interface -----
|
||||
|
||||
getContextMenuPopup(): HTMLElement | undefined {
|
||||
getContextMenuPopup(): WaPopup | undefined {
|
||||
return this.contextMenuPopup;
|
||||
}
|
||||
|
||||
getPlaylistSubmenuPopup():
|
||||
| HTMLElement
|
||||
| WaPopup
|
||||
| undefined {
|
||||
return this.playlistSubmenuPopup;
|
||||
}
|
||||
|
||||
@@ -2,6 +2,7 @@ import { LitElement, html, css, nothing } from 'lit';
|
||||
import { customElement, state } from 'lit/decorators.js';
|
||||
import '@awesome.me/webawesome/dist/components/icon/icon.js';
|
||||
import '@awesome.me/webawesome/dist/components/popup/popup.js';
|
||||
import type WaPopup from '@awesome.me/webawesome/dist/components/popup/popup.js';
|
||||
import { PlayerController } from '@store/controllers/player-controller';
|
||||
|
||||
const MIN_WIDTH = 120;
|
||||
@@ -220,15 +221,16 @@ export class NowPlaying extends LitElement {
|
||||
this.showCoverPreview = true;
|
||||
|
||||
this.updateComplete.then(() => {
|
||||
const popup = this.shadowRoot?.querySelector(
|
||||
'#cover-preview',
|
||||
);
|
||||
const popup =
|
||||
this.shadowRoot?.querySelector<WaPopup>(
|
||||
'#cover-preview',
|
||||
);
|
||||
const anchor = this.shadowRoot?.querySelector(
|
||||
'.cover-art',
|
||||
);
|
||||
|
||||
if (popup && anchor) {
|
||||
(popup as any).anchor = anchor;
|
||||
popup.anchor = anchor;
|
||||
}
|
||||
});
|
||||
};
|
||||
|
||||
@@ -2,6 +2,7 @@ import { LitElement, html, css, nothing } from 'lit';
|
||||
import { customElement, state, query } from 'lit/decorators.js';
|
||||
import '@awesome.me/webawesome/dist/components/icon/icon.js';
|
||||
import '@awesome.me/webawesome/dist/components/popup/popup.js';
|
||||
import type WaPopup from '@awesome.me/webawesome/dist/components/popup/popup.js';
|
||||
import '@awesome.me/webawesome/dist/components/dropdown-item/dropdown-item.js';
|
||||
|
||||
import {
|
||||
@@ -63,12 +64,12 @@ export class PlaylistView
|
||||
private selection = new SelectionController(this);
|
||||
private ctxMenu = new ContextMenuController(this);
|
||||
|
||||
getContextMenuPopup(): HTMLElement | undefined {
|
||||
getContextMenuPopup(): WaPopup | undefined {
|
||||
return this.contextMenuPopup;
|
||||
}
|
||||
|
||||
getPlaylistSubmenuPopup():
|
||||
| HTMLElement
|
||||
| WaPopup
|
||||
| undefined {
|
||||
return this.playlistSubmenuPopup;
|
||||
}
|
||||
@@ -199,13 +200,13 @@ export class PlaylistView
|
||||
private dragImageEl: HTMLElement | null = null;
|
||||
|
||||
@query('#context-menu')
|
||||
private contextMenuPopup!: HTMLElement;
|
||||
private contextMenuPopup!: WaPopup;
|
||||
|
||||
@query('#playlist-submenu')
|
||||
private playlistSubmenuPopup!: HTMLElement;
|
||||
private playlistSubmenuPopup!: WaPopup;
|
||||
|
||||
@query('#playlist-context-menu')
|
||||
private playlistContextMenuPopup!: HTMLElement;
|
||||
private playlistContextMenuPopup!: WaPopup;
|
||||
|
||||
@query('track-details')
|
||||
private trackDetailsDialog!: TrackDetails;
|
||||
@@ -1438,21 +1439,17 @@ export class PlaylistView
|
||||
this.playlistContextMenuPopup;
|
||||
|
||||
if (popup) {
|
||||
(popup as any).anchor = {
|
||||
popup.anchor = {
|
||||
getBoundingClientRect() {
|
||||
return {
|
||||
width: 0,
|
||||
height: 0,
|
||||
x: e.clientX,
|
||||
y: e.clientY,
|
||||
top: e.clientY,
|
||||
left: e.clientX,
|
||||
right: e.clientX,
|
||||
bottom: e.clientY,
|
||||
};
|
||||
return new DOMRect(
|
||||
e.clientX,
|
||||
e.clientY,
|
||||
0,
|
||||
0,
|
||||
);
|
||||
},
|
||||
};
|
||||
(popup as any).active = true;
|
||||
popup.active = true;
|
||||
}
|
||||
});
|
||||
};
|
||||
@@ -1467,7 +1464,7 @@ export class PlaylistView
|
||||
this.playlistContextMenuPopup;
|
||||
|
||||
if (popup) {
|
||||
(popup as any).active = false;
|
||||
popup.active = false;
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -7,6 +7,7 @@ import {
|
||||
} from 'lit/decorators.js';
|
||||
import '@awesome.me/webawesome/dist/components/icon/icon.js';
|
||||
import '@awesome.me/webawesome/dist/components/popup/popup.js';
|
||||
import type WaPopup from '@awesome.me/webawesome/dist/components/popup/popup.js';
|
||||
import '@awesome.me/webawesome/dist/components/dropdown-item/dropdown-item.js';
|
||||
import { QueueController } from '@store/controllers/queue-controller';
|
||||
import '@components/playlist-picker/playlist-picker.js';
|
||||
@@ -73,13 +74,13 @@ export class QueuePanel
|
||||
private dragImageEl: HTMLElement | null = null;
|
||||
|
||||
@query('#add-to-playlist-popup')
|
||||
private addToPlaylistPopup!: HTMLElement;
|
||||
private addToPlaylistPopup!: WaPopup;
|
||||
|
||||
@query('#context-menu')
|
||||
private contextMenuPopup!: HTMLElement;
|
||||
private contextMenuPopup!: WaPopup;
|
||||
|
||||
@query('#playlist-submenu')
|
||||
private playlistSubmenuPopup!: HTMLElement;
|
||||
private playlistSubmenuPopup!: WaPopup;
|
||||
|
||||
@query('lit-virtualizer')
|
||||
private virtualizer!: LitVirtualizer;
|
||||
@@ -158,11 +159,11 @@ export class QueuePanel
|
||||
// ContextMenuHost interface
|
||||
// =================================================================
|
||||
|
||||
getContextMenuPopup(): HTMLElement | undefined {
|
||||
getContextMenuPopup(): WaPopup | undefined {
|
||||
return this.contextMenuPopup;
|
||||
}
|
||||
|
||||
getPlaylistSubmenuPopup(): HTMLElement | undefined {
|
||||
getPlaylistSubmenuPopup(): WaPopup | undefined {
|
||||
return this.playlistSubmenuPopup;
|
||||
}
|
||||
|
||||
@@ -536,8 +537,8 @@ export class QueuePanel
|
||||
);
|
||||
|
||||
if (popup && btn) {
|
||||
(popup as any).anchor = btn;
|
||||
(popup as any).active = this.playlistPickerOpen;
|
||||
popup.anchor = btn;
|
||||
popup.active = this.playlistPickerOpen;
|
||||
}
|
||||
|
||||
if (this.playlistPickerOpen) {
|
||||
@@ -557,7 +558,7 @@ export class QueuePanel
|
||||
const popup = this.addToPlaylistPopup;
|
||||
|
||||
if (popup) {
|
||||
(popup as any).active = false;
|
||||
popup.active = false;
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -39,6 +39,7 @@ import type {
|
||||
} from '@lit-labs/virtualizer';
|
||||
import { flow } from '@lit-labs/virtualizer/layouts/flow.js';
|
||||
import '@awesome.me/webawesome/dist/components/popup/popup.js';
|
||||
import type WaPopup from '@awesome.me/webawesome/dist/components/popup/popup.js';
|
||||
import '@awesome.me/webawesome/dist/components/dropdown-item/dropdown-item.js';
|
||||
import '@awesome.me/webawesome/dist/components/icon/icon.js';
|
||||
import '@components/playlist-picker/playlist-picker.js';
|
||||
@@ -104,18 +105,18 @@ export class TrackList extends LitElement implements SelectionHost, ContextMenuH
|
||||
private tracks: library.Track[] = [];
|
||||
|
||||
@query('#context-menu')
|
||||
private contextMenuPopup!: HTMLElement;
|
||||
private contextMenuPopup!: WaPopup;
|
||||
|
||||
@query('#playlist-submenu')
|
||||
private playlistSubmenuPopup!: HTMLElement;
|
||||
private playlistSubmenuPopup!: WaPopup;
|
||||
|
||||
// -- ContextMenuHost interface --
|
||||
|
||||
getContextMenuPopup(): HTMLElement | undefined {
|
||||
getContextMenuPopup(): WaPopup | undefined {
|
||||
return this.contextMenuPopup;
|
||||
}
|
||||
|
||||
getPlaylistSubmenuPopup(): HTMLElement | undefined {
|
||||
getPlaylistSubmenuPopup(): WaPopup | undefined {
|
||||
return this.playlistSubmenuPopup;
|
||||
}
|
||||
|
||||
@@ -169,7 +170,7 @@ export class TrackList extends LitElement implements SelectionHost, ContextMenuH
|
||||
private sortDropdownOpen = false;
|
||||
|
||||
@query('#sort-dropdown')
|
||||
private sortDropdownPopup!: HTMLElement;
|
||||
private sortDropdownPopup!: WaPopup;
|
||||
|
||||
private resizingColumn: number | null = null;
|
||||
private resizeStartX = 0;
|
||||
@@ -1371,8 +1372,8 @@ export class TrackList extends LitElement implements SelectionHost, ContextMenuH
|
||||
);
|
||||
|
||||
if (popup && anchor) {
|
||||
(popup as any).anchor = anchor;
|
||||
(popup as any).active = true;
|
||||
popup.anchor = anchor;
|
||||
popup.active = true;
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1384,7 +1385,7 @@ export class TrackList extends LitElement implements SelectionHost, ContextMenuH
|
||||
const popup = this.sortDropdownPopup;
|
||||
|
||||
if (popup) {
|
||||
(popup as any).active = false;
|
||||
popup.active = false;
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -1,7 +1,8 @@
|
||||
import { EventsOn, EventsEmit } from '@runtime/runtime';
|
||||
import { Events } from '../events';
|
||||
|
||||
// Types
|
||||
// TrackInfo mirrors the player.TrackInfo struct in the Go backend.
|
||||
// Fields are serialized as camelCase JSON via struct tags.
|
||||
export interface TrackInfo {
|
||||
fileName: string;
|
||||
filePath: string;
|
||||
|
||||
@@ -3,6 +3,7 @@ import type {
|
||||
ReactiveController,
|
||||
ReactiveControllerHost,
|
||||
} from 'lit';
|
||||
import type WaPopup from '@awesome.me/webawesome/dist/components/popup/popup.js';
|
||||
|
||||
/**
|
||||
* Host interface for components using the ContextMenuController.
|
||||
@@ -15,9 +16,9 @@ export interface ContextMenuHost
|
||||
updateComplete: Promise<boolean>;
|
||||
shadowRoot: ShadowRoot | null;
|
||||
/** Return the main context-menu popup element. */
|
||||
getContextMenuPopup(): HTMLElement | undefined;
|
||||
getContextMenuPopup(): WaPopup | undefined;
|
||||
/** Return the playlist submenu popup element. */
|
||||
getPlaylistSubmenuPopup(): HTMLElement | undefined;
|
||||
getPlaylistSubmenuPopup(): WaPopup | undefined;
|
||||
/**
|
||||
* Called when the context menu is closed by an
|
||||
* outside click/contextmenu/mousedown. Components
|
||||
@@ -144,21 +145,17 @@ export class ContextMenuController
|
||||
|
||||
if (!popup) return;
|
||||
|
||||
(popup as any).anchor = {
|
||||
popup.anchor = {
|
||||
getBoundingClientRect() {
|
||||
return {
|
||||
width: 0,
|
||||
height: 0,
|
||||
x: clientX,
|
||||
y: clientY,
|
||||
top: clientY,
|
||||
left: clientX,
|
||||
right: clientX,
|
||||
bottom: clientY,
|
||||
};
|
||||
return new DOMRect(
|
||||
clientX,
|
||||
clientY,
|
||||
0,
|
||||
0,
|
||||
);
|
||||
},
|
||||
};
|
||||
(popup as any).active = true;
|
||||
popup.active = true;
|
||||
});
|
||||
}
|
||||
|
||||
@@ -178,7 +175,7 @@ export class ContextMenuController
|
||||
this.host.getContextMenuPopup();
|
||||
|
||||
if (popup) {
|
||||
(popup as any).active = false;
|
||||
popup.active = false;
|
||||
}
|
||||
|
||||
this.host.onContextMenuClose?.();
|
||||
@@ -220,8 +217,8 @@ export class ContextMenuController
|
||||
);
|
||||
|
||||
if (submenu && trigger) {
|
||||
(submenu as any).anchor = trigger;
|
||||
(submenu as any).active = true;
|
||||
submenu.anchor = trigger;
|
||||
submenu.active = true;
|
||||
}
|
||||
|
||||
const picker =
|
||||
@@ -246,7 +243,7 @@ export class ContextMenuController
|
||||
this.host.getPlaylistSubmenuPopup();
|
||||
|
||||
if (submenu) {
|
||||
(submenu as any).active = false;
|
||||
submenu.active = false;
|
||||
}
|
||||
|
||||
this.host.requestUpdate();
|
||||
|
||||
Reference in New Issue
Block a user