From 91bca6bac3c79a767c90cb433c03fca90ded5f64 Mon Sep 17 00:00:00 2001 From: David Rodger Date: Sat, 30 May 2026 22:06:10 -0400 Subject: [PATCH] Polish import/settings UI and fix Share Importer composer - Settings: replace custom logged-out sign-in button with standard ApproachNoteButton, relabel "Sign In / Sign Up" - Import Song shelf (iOS + macOS): drop icon and grey card, de-emphasize MusicBrainz ID, widen fields to full width - Create Song form (iOS + macOS): remove MusicBrainz ID field and the Additional Details section, move Cancel below Create Song, use standard ApproachNoteButton styling; drop unused `key` state - Share Importer JS: resolve composers via the MusicBrainz web service (artist-rels) instead of brittle DOM scraping, with a DOM fallback and a one-shot async completion guard Co-Authored-By: Claude Opus 4.8 --- .../Views/Import/MacSongCreationView.swift | 51 ++------ apps/ShareImporter/CommonViews.swift | 6 +- apps/ShareImporter/ExtractMusicBrainzData.js | 122 ++++++++++++++---- apps/ShareImporter/SongViews.swift | 12 +- apps/ShareImporterMac/MacExtensionViews.swift | 16 +-- apps/iOS/Views/SettingsView.swift | 13 +- apps/iOS/Views/SongCreationView.swift | 46 ++----- 7 files changed, 134 insertions(+), 132 deletions(-) diff --git a/apps/Mac/Views/Import/MacSongCreationView.swift b/apps/Mac/Views/Import/MacSongCreationView.swift index bb148ac2..93a75c75 100644 --- a/apps/Mac/Views/Import/MacSongCreationView.swift +++ b/apps/Mac/Views/Import/MacSongCreationView.swift @@ -18,7 +18,6 @@ struct MacSongCreationView: View { @State private var composer: String @State private var musicbrainzId: String @State private var workType: String - @State private var key: String @State private var isSaving = false @State private var showingError = false @@ -33,30 +32,14 @@ struct MacSongCreationView: View { _composer = State(initialValue: importedData?.composerString ?? "") _musicbrainzId = State(initialValue: importedData?.musicbrainzId ?? "") _workType = State(initialValue: importedData?.workType ?? "") - _key = State(initialValue: importedData?.key ?? "") } var body: some View { VStack(spacing: 0) { // Header HStack { - Button("Cancel") { - dismiss() - } - .keyboardShortcut(.cancelAction) - - Spacer() - Text("Create Song") .font(ApproachNoteTheme.headline()) - - Spacer() - - Button("Save") { - saveSong() - } - .keyboardShortcut(.defaultAction) - .disabled(title.isEmpty || isSaving) } .padding() @@ -70,37 +53,25 @@ struct MacSongCreationView: View { TextField("Composer", text: $composer) .textFieldStyle(.roundedBorder) - - TextField("MusicBrainz ID", text: $musicbrainzId) - .textFieldStyle(.roundedBorder) - .font(.system(.body, design: .monospaced)) } header: { Text("Basic Information") } - - Section { - TextField("Work Type (e.g., Song, Instrumental)", text: $workType) - .textFieldStyle(.roundedBorder) - - TextField("Key (e.g., Eb, F minor)", text: $key) - .textFieldStyle(.roundedBorder) - } header: { - Text("Additional Details") - } - - Section { - Text("Additional details (structure, recordings) can be added later through the app.") - .font(ApproachNoteTheme.caption()) - .foregroundColor(.secondary) - } } .formStyle(.grouped) .padding() - if isSaving { - ProgressView("Saving...") - .padding() + // Footer buttons + VStack(spacing: 12) { + ApproachNoteButton("Create Song", isLoading: isSaving, action: saveSong) + .keyboardShortcut(.defaultAction) + .disabled(title.isEmpty || isSaving) + + ApproachNoteButton("Cancel", style: .secondary) { + dismiss() + } + .keyboardShortcut(.cancelAction) } + .padding() } .frame(minWidth: 400, minHeight: 400) .alert("Error", isPresented: $showingError) { diff --git a/apps/ShareImporter/CommonViews.swift b/apps/ShareImporter/CommonViews.swift index 1be8535d..e0cfdcd4 100644 --- a/apps/ShareImporter/CommonViews.swift +++ b/apps/ShareImporter/CommonViews.swift @@ -170,14 +170,16 @@ struct NotImplementedView: View { struct InfoRow: View { let label: String let value: String - + var deEmphasized: Bool = false + var body: some View { VStack(alignment: .leading, spacing: 4) { Text(label) .font(.caption) .foregroundColor(.secondary) Text(value) - .font(.body) + .font(deEmphasized ? .caption.monospaced() : .body) + .foregroundColor(deEmphasized ? .secondary : .primary) } } } diff --git a/apps/ShareImporter/ExtractMusicBrainzData.js b/apps/ShareImporter/ExtractMusicBrainzData.js index c1afe59a..1bb162e6 100644 --- a/apps/ShareImporter/ExtractMusicBrainzData.js +++ b/apps/ShareImporter/ExtractMusicBrainzData.js @@ -155,17 +155,42 @@ ExtractMusicBrainzData.prototype = { title = pageTitle.replace(/\s*-\s*MusicBrainz\s*$/, '').trim(); } - // Extract composer(s) - try multiple selectors - var composers = []; - - // Look for composer or writer in the details list - var composerElements = document.querySelectorAll('dd.writer a[href*="/artist/"], dd.composer a[href*="/artist/"]'); - composerElements.forEach(function(element) { - var composerName = element.textContent.trim(); - if (composerName && composers.indexOf(composerName) === -1) { - composers.push(composerName); + // Composer(s) are filled in from the MusicBrainz web service below + // (see the artist-rels fetch before completionFunction). The DOM is an + // unreliable source because the relationship markup varies and the work + // page is increasingly served behind a JS challenge. We still keep a + // best-effort DOM scrape as a fallback if the API call fails. + function scrapeComposersFromDOM() { + var found = []; + function addFromDd(dd) { + if (!dd) return; + dd.querySelectorAll('a[href*="/artist/"]').forEach(function(element) { + var name = element.textContent.trim(); + if (name && found.indexOf(name) === -1) { + found.push(name); + } + }); } - }); + document.querySelectorAll('dt').forEach(function(dt) { + var label = dt.textContent.trim().toLowerCase(); + if (label.indexOf('composer') !== -1 || label.indexOf('writer') !== -1) { + var sibling = dt.nextElementSibling; + while (sibling && sibling.tagName === 'DD') { + addFromDd(sibling); + sibling = sibling.nextElementSibling; + } + } + }); + if (found.length === 0) { + document.querySelectorAll('dd.writer a[href*="/artist/"], dd.composer a[href*="/artist/"]').forEach(function(element) { + var name = element.textContent.trim(); + if (name && found.indexOf(name) === -1) { + found.push(name); + } + }); + } + return found; + } // Extract type (song, instrumental, etc.) var workType = ""; @@ -220,36 +245,89 @@ ExtractMusicBrainzData.prototype = { }; // Only add optional fields if they have values - if (composers.length > 0) { - result.composers = composers; - } - if (workType) { result.workType = workType; } - + if (musicalKey) { result.key = musicalKey; } - + if (iswc) { result.iswc = iswc; } - + if (language) { result.language = language; } - + if (annotation) { result.annotation = annotation; } - + if (wikipediaUrl) { result.wikipediaUrl = wikipediaUrl; } - - // Return results to the extension - arguments.completionFunction(result); + + // Resolve composers from the MusicBrainz web service (authoritative), + // then return results to the extension. completionFunction must be + // called exactly once, so guard the timeout/fetch race. Alias the + // extension args because `arguments` is shadowed inside the callbacks + // by their own implicit arguments object. + var extensionArgs = arguments; + var completed = false; + function finish(composerList) { + if (completed) return; + completed = true; + if (composerList && composerList.length > 0) { + result.composers = composerList; + } + extensionArgs.completionFunction(result); + } + + // Safety net: never let a hung request strand the import. + var timeoutId = setTimeout(function() { + finish(scrapeComposersFromDOM()); + }, 4000); + + if (musicbrainzId) { + var apiUrl = "https://musicbrainz.org/ws/2/work/" + musicbrainzId + + "?inc=artist-rels&fmt=json"; + fetch(apiUrl, { headers: { "Accept": "application/json" } }) + .then(function(response) { + return response.ok ? response.json() : null; + }) + .then(function(json) { + clearTimeout(timeoutId); + var names = []; + if (json && json.relations) { + // Prefer "composer"; fall back to writer/lyricist only + // if no composer relationship exists. + var rels = json.relations.filter(function(rel) { + return rel.type === "composer"; + }); + if (rels.length === 0) { + rels = json.relations.filter(function(rel) { + return rel.type === "writer" || rel.type === "lyricist"; + }); + } + rels.forEach(function(rel) { + var name = rel.artist && rel.artist.name ? rel.artist.name.trim() : ""; + if (name && names.indexOf(name) === -1) { + names.push(name); + } + }); + } + finish(names.length > 0 ? names : scrapeComposersFromDOM()); + }) + .catch(function() { + clearTimeout(timeoutId); + finish(scrapeComposersFromDOM()); + }); + } else { + clearTimeout(timeoutId); + finish(scrapeComposersFromDOM()); + } }, extractYouTubeData: function(arguments, url) { diff --git a/apps/ShareImporter/SongViews.swift b/apps/ShareImporter/SongViews.swift index c7c66b99..e65740d8 100644 --- a/apps/ShareImporter/SongViews.swift +++ b/apps/ShareImporter/SongViews.swift @@ -16,10 +16,6 @@ struct SongImportConfirmationView: View { var body: some View { VStack(spacing: 20) { VStack(spacing: 12) { - Image(systemName: "music.note") - .font(.system(size: 50)) - .foregroundColor(.blue) - Text("Import Song") .font(.title2) .bold() @@ -46,13 +42,11 @@ struct SongImportConfirmationView: View { InfoRow(label: "Key", value: key) } - InfoRow(label: "MusicBrainz ID", value: songData.musicbrainzId) + InfoRow(label: "MusicBrainz ID", value: songData.musicbrainzId, deEmphasized: true) } - .padding() + .frame(maxWidth: .infinity, alignment: .leading) + .padding(.horizontal) } - .background(Color(.systemGray6)) - .cornerRadius(12) - .padding(.horizontal) Spacer() diff --git a/apps/ShareImporterMac/MacExtensionViews.swift b/apps/ShareImporterMac/MacExtensionViews.swift index 2d77b737..1f6a1283 100644 --- a/apps/ShareImporterMac/MacExtensionViews.swift +++ b/apps/ShareImporterMac/MacExtensionViews.swift @@ -152,6 +152,7 @@ struct MacNotImplementedView: View { struct MacInfoRow: View { let label: String let value: String + var deEmphasized: Bool = false var body: some View { VStack(alignment: .leading, spacing: 4) { @@ -159,7 +160,8 @@ struct MacInfoRow: View { .font(.caption) .foregroundColor(.secondary) Text(value) - .font(.body) + .font(deEmphasized ? .caption.monospaced() : .body) + .foregroundColor(deEmphasized ? .secondary : .primary) } } } @@ -486,10 +488,6 @@ struct MacSongImportConfirmationView: View { var body: some View { VStack(spacing: 16) { VStack(spacing: 8) { - Image(systemName: "music.note") - .font(.system(size: 40)) - .foregroundColor(.blue) - Text("Import Song") .font(.title2) .bold() @@ -516,13 +514,11 @@ struct MacSongImportConfirmationView: View { MacInfoRow(label: "Key", value: key) } - MacInfoRow(label: "MusicBrainz ID", value: songData.musicbrainzId) + MacInfoRow(label: "MusicBrainz ID", value: songData.musicbrainzId, deEmphasized: true) } - .padding() + .frame(maxWidth: .infinity, alignment: .leading) + .padding(.horizontal) } - .background(Color(NSColor.controlBackgroundColor)) - .cornerRadius(8) - .padding(.horizontal) Spacer() diff --git a/apps/iOS/Views/SettingsView.swift b/apps/iOS/Views/SettingsView.swift index a274bdb4..31b026ed 100644 --- a/apps/iOS/Views/SettingsView.swift +++ b/apps/iOS/Views/SettingsView.swift @@ -268,19 +268,8 @@ struct SettingsView: View { } .padding(.horizontal) } else { - Button(action: { + ApproachNoteButton("Sign In / Sign Up") { isShowingLogin = true - }) { - HStack { - Image(systemName: "person.crop.circle.badge.plus") - .foregroundColor(ApproachNoteTheme.brand) - Text("Sign In or Create Account") - .foregroundColor(ApproachNoteTheme.textPrimary) - Spacer() - } - .padding() - .background(ApproachNoteTheme.surface) - .cornerRadius(8) } .padding(.horizontal) } diff --git a/apps/iOS/Views/SongCreationView.swift b/apps/iOS/Views/SongCreationView.swift index 12443ec0..100b585a 100644 --- a/apps/iOS/Views/SongCreationView.swift +++ b/apps/iOS/Views/SongCreationView.swift @@ -23,7 +23,6 @@ struct SongCreationView: View { @State private var composer: String @State private var musicbrainzId: String @State private var workType: String - @State private var key: String @State private var isSaving = false @State private var showingError = false @@ -41,8 +40,7 @@ struct SongCreationView: View { _composer = State(initialValue: importedData?.composerString ?? "") _musicbrainzId = State(initialValue: importedData?.musicbrainzId ?? "") _workType = State(initialValue: importedData?.workType ?? "") - _key = State(initialValue: importedData?.key ?? "") - + NSLog("✅ Init complete, title state: '%@'", importedData?.title ?? "EMPTY") } @@ -52,47 +50,21 @@ struct SongCreationView: View { Section(header: Text("Basic Information")) { TextField("Song Title", text: $title) TextField("Composer", text: $composer) - TextField("MusicBrainz ID", text: $musicbrainzId) - .textInputAutocapitalization(.never) - .autocorrectionDisabled() - .font(.system(.body, design: .monospaced)) - } - - Section(header: Text("Additional Details")) { - TextField("Work Type (e.g., Song, Instrumental)", text: $workType) - TextField("Key (e.g., Eb, F minor)", text: $key) - } - - Section { - Text("Additional details (structure, recordings) can be added later through the app.") - .font(ApproachNoteTheme.caption()) - .foregroundColor(.secondary) - } - - Section { - Button(action: saveSong) { - if isSaving { - HStack { - ProgressView() - Text("Creating Song...") - } - } else { - Text("Create Song") - .frame(maxWidth: .infinity) - } - } - .disabled(title.isEmpty || isSaving) - .foregroundColor(title.isEmpty ? Color.gray : Color.blue) } } .navigationTitle("Create Song") .navigationBarTitleDisplayMode(.inline) - .toolbar { - ToolbarItem(placement: .navigationBarLeading) { - Button("Cancel") { + .safeAreaInset(edge: .bottom) { + VStack(spacing: 12) { + ApproachNoteButton("Create Song", isLoading: isSaving, action: saveSong) + .disabled(title.isEmpty || isSaving) + + ApproachNoteButton("Cancel", style: .secondary) { dismiss() } } + .padding(.horizontal) + .padding(.bottom, 8) } .onAppear { NSLog("📱 SongCreationView.onAppear()")