You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@maka.apache.org by "ARE404 (via GitHub)" <gi...@apache.org> on 2026/08/21 14:29:23 UTC
[PR] feat(desktop): let the user switch the app icon [maka]
ARE404 opened a new pull request, #3431:
URL: https://github.com/apache/maka/pull/3431
Draft — opening it early to check the direction before polishing.
## What
Settings → 外观 gains an **应用图标** picker. Picking one changes the dock tile on macOS and the window / taskbar icon on Windows and Linux, live, and it survives a restart.
Two icons ship today: the existing brand mark, and a grayscale variant derived from it with `sips`. **The variant is placeholder artwork** — the mechanism is what this PR is about. Adding another is one entry in `APP_ICONS` plus one 1024px PNG, so if the project wants real alternate marks, they drop straight in.
## How
- `appearance.appIcon` is a **closed enum** (`packages/core/src/settings.ts`) normalized fail-closed exactly the way `appearance.palette` is. The renderer names an id, never a path, so a hand-edited `settings.json` can only ever select artwork that ships with the build.
- Applying it hangs off `client-settings-effects.ts`, which already owns the OS-level side effects of a settings write (keep-awake, bot channels). That gets live switching and dedupe against the settings file watcher echoing the same write back, for free.
- Startup still sets the **default** mark synchronously in `desktop-shell-presentation.ts`. Awaiting the settings store there would put the generic Electron rocket in the dock for the first frames — the exact regression PR-GRAY-CARD-LIFT-0 fixed — so the persisted choice lands a tick later from the same effect.
- New windows are born with the chosen icon (`main-window.ts`), which is what Windows and Linux need since they draw the icon per window.
- A missing or undecodable variant falls back to the brand mark. `nativeImage.createFromPath` reports that case as an *empty* image rather than throwing, and handing an empty image to `setIcon` blanks the dock tile.
- The picker's thumbnails come from the main process as ids plus 128px data URLs, computed once — the 1024px masters stay out of the renderer bundle.
## Tests
- `packages/core`: `appearance.appIcon` defaults, accepts a shipped id, and falls back for `undefined` / unknown / non-string — without disturbing `theme` or `palette`.
- `app-icon.test.ts`: every id in `APP_ICONS` resolves to a file that is really a square 1024px PNG (checked against the bytes, since a bad file shows up as a blank dock tile, not an error), plus the path layout and the fallback order.
- `client-settings-effects.test.ts`: a chosen icon is applied once, not re-applied when the watcher echoes the same write, and a run that never leaves the default never touches the OS icon at all.
Local: `typecheck`, `lint`, `format:check`, `@maka/core` (577) and `@maka/desktop` (1027) suites all green.
## Open questions
1. **Is this wanted at all right now?** Happy to park it if the incubator move means features are on hold.
2. **The artwork.** A grayscale derivative is a placeholder, not a design proposal — if the icon set is a brand decision, say the word and I will wire whatever art you want to ship instead.
3. **Packaged builds.** `app.dock.setIcon` overrides the bundle icon at runtime on macOS, so the picker works in a packaged app, but the Finder / .app bundle icon still comes from `electron-builder.config.mjs`. Out of scope here; worth deciding whether they should ever agree.
4. **A custom image** ("pick any PNG") is the obvious follow-up and needs a file dialog plus validation and workspace storage — deliberately left out to keep this reviewable.
## Not verified
I could not launch the Electron GUI in this environment (headless), so the dock switch itself is not screenshotted. Everything below the Electron API boundary is covered by tests; the `app.dock.setIcon` / `win.setIcon` calls themselves are not.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] feat(desktop): let the user choose or import the app icon [maka]
Posted by "ARE404 (via GitHub)" <gi...@apache.org>.
ARE404 commented on code in PR #3431:
URL: https://github.com/apache/maka/pull/3431#discussion_r3837830607
##########
apps/desktop/src/main/app-icon-ipc.ts:
##########
@@ -0,0 +1,213 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import type {
+ IpcMain,
+ OpenDialogOptions,
+ OpenDialogReturnValue,
+} from "electron";
+import {
+ customAppIconId,
+ isAppIconChoice,
+ isCustomAppIcon,
+ toAppIconChoice,
+ type AppIconChoice,
+ type AppSettings,
+} from "@maka/core/settings";
+import type { SettingsStore } from "@maka/storage";
+import type { AppIconPreview } from "./app-icon-surface.js";
+import {
+ CustomAppIconError,
+ listCustomAppIconIds,
+ removeCustomAppIcon,
+ type CustomAppIconImportReason,
+} from "./custom-app-icon-store.js";
+
+export type AppIconImportResult =
+ | { readonly ok: true; readonly icon: AppIconChoice }
+ | { readonly ok: false; readonly reason: CustomAppIconImportReason };
+
+export type AppIconSelectResult =
+ | { readonly ok: true; readonly selection: AppIconChoice }
+ | {
+ readonly ok: false;
+ readonly reason: "invalid_id" | "missing_artwork" | "write_failed";
+ };
+
+export type AppIconRemoveResult =
+ | { readonly ok: true; readonly selection: AppIconChoice }
+ | {
+ readonly ok: false;
+ readonly reason: "invalid_id" | "reset_failed" | "remove_failed";
+ };
+
+/**
+ * The icon surface's one main-process owner.
+ *
+ * Selection and artwork are two pieces of state that must not disagree, so the
+ * operation that can break the pair lives here rather than being sequenced by
+ * the renderer: a renderer that deletes and then persists leaves the setting
+ * pointing at a file that is gone whenever the second call fails.
+ */
+/**
+ * Everything that touches Electron is injected rather than imported: this
+ * module holds the policy, and keeping `electron` out of its import graph is
+ * what lets the removal contract be tested without booting a browser process.
+ */
+export function registerAppIconIpc(input: {
+ readonly ipcMain: Pick<IpcMain, "handle">;
+ readonly showOpenDialog: (
+ options: OpenDialogOptions,
+ ) => Promise<OpenDialogReturnValue>;
+ readonly listPreviews: () => Promise<readonly AppIconPreview[]>;
+ readonly importArtwork: (source: {
+ readonly sourcePath: string;
+ readonly userDataPath: string;
+ }) => Promise<AppIconChoice>;
+ readonly settingsStore: Pick<SettingsStore, "update" | "updateIf">;
+ readonly applySettings: (settings: AppSettings) => Promise<void>;
+ readonly userDataPath: () => string;
+}): void {
+ const { userDataPath } = input;
+
+ // Every operation that can move the selection or the artwork runs here, one
+ // at a time. A compare-and-set around the settings write alone was not
+ // enough: applying the icon yields, and a selection arriving in that window
+ // would be left pointing at a file this sequence is about to delete. Making
+ // the three operations a single serialized owner removes the window itself,
+ // rather than guarding each one that gets noticed.
+ let queue: Promise<unknown> = Promise.resolve();
+ function serialize<T>(operation: () => Promise<T>): Promise<T> {
+ const run = queue.then(operation, operation);
+ queue = run.then(
+ () => undefined,
+ () => undefined,
+ );
+ return run;
+ }
+
+ /**
+ * Selection lives behind this seam too, so it queues with removal instead of
+ * racing it through the generic settings channel. It is also the only place
+ * that can refuse a choice whose artwork is gone — the generic channel would
+ * happily persist an id with nothing behind it.
+ */
+ input.ipcMain.handle("app:selectIcon", (_event, icon: unknown) =>
+ serialize(async (): Promise<AppIconSelectResult> => {
+ if (!isAppIconChoice(icon)) return { ok: false, reason: "invalid_id" };
+ const custom = customAppIconId(icon);
+ if (custom !== undefined) {
+ const present = await listCustomAppIconIds(userDataPath());
+ if (!present.includes(custom))
+ return { ok: false, reason: "missing_artwork" };
+ }
+ try {
+ const settings = await input.settingsStore.update({
+ appearance: { appIcon: icon },
+ });
+ await input.applySettings(settings);
+ return {
+ ok: true,
+ selection: toAppIconChoice(settings.appearance.appIcon),
+ };
+ } catch {
+ return { ok: false, reason: "write_failed" };
+ }
+ }),
+ );
+
+ // The picker asks for the whole set at once; there is no per-id request, so
+ // no id from the renderer ever reaches the filesystem.
+ input.ipcMain.handle(
+ "app:iconPreviews",
+ (): Promise<readonly AppIconPreview[]> => input.listPreviews(),
+ );
+
+ // The dialog runs here and the file it returns is the only path this sees.
+ input.ipcMain.handle("app:importIcon", () =>
Review Comment:
Fixed in d925da1e. The dialog now runs before `serialize`, and only the copy-into-place is queued.
Your framing is the part I had wrong — user time versus work time. The queue exists to order state changes, and a dialog changes nothing until a file comes back, so holding the owner across it bought no invariant and cost every other window.
Covered by a test that leaves a dialog deliberately open and selects a different icon while it hangs: the selection completes rather than waiting.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] feat(desktop): let the user choose or import the app icon [maka]
Posted by "ARE404 (via GitHub)" <gi...@apache.org>.
ARE404 commented on code in PR #3431:
URL: https://github.com/apache/maka/pull/3431#discussion_r3835052431
##########
apps/desktop/src/main/custom-app-icons.ts:
##########
@@ -0,0 +1,63 @@
+import { randomUUID } from 'node:crypto';
+import { mkdir, stat, writeFile } from 'node:fs/promises';
+import { nativeImage } from 'electron';
+import { CUSTOM_APP_ICON_PREFIX, type CustomAppIcon } from '@maka/core/settings';
+import {
+ CustomAppIconError,
+ CUSTOM_ICON_EDGE,
+ CUSTOM_ICON_MAX_INPUT_BYTES,
+ CUSTOM_ICON_MIN_EDGE,
+ customAppIconDirectory,
+ resolveCustomAppIconPath,
+} from './custom-app-icon-store.js';
+
+/**
+ * Decode, square, scale, store. Non-square art is centre-cropped rather than
+ * letterboxed: an icon that keeps its own transparent margin is the caller's
+ * business, and silently adding one would change art the user already framed.
+ */
+export async function importCustomAppIcon(input: {
+ readonly sourcePath: string;
+ readonly userDataPath: string;
+}): Promise<CustomAppIcon> {
+ const info = await stat(input.sourcePath).catch(() => undefined);
+ if (!info?.isFile()) throw new CustomAppIconError('unreadable', 'not a file');
+ if (info.size > CUSTOM_ICON_MAX_INPUT_BYTES) {
+ throw new CustomAppIconError('too_large', `over ${CUSTOM_ICON_MAX_INPUT_BYTES} bytes`);
+ }
+
+ // An undecodable file comes back EMPTY rather than throwing — the same trap
+ // the shipped set falls into, and the reason emptiness is the failure test.
+ const source = nativeImage.createFromPath(input.sourcePath);
Review Comment:
Fixed in 6cffc61. Both halves of this were real.
The file is now read once into a capped buffer, the header is parsed out of those bytes, dimensions are rejected there (max 4096 per edge), and the *same buffer* is decoded via `createFromBuffer`. That removes the second read entirely, so there is no window to swap the path in, and no decoder sees data whose declared size has not been checked.
The header parser is pure and tested, including the fixture you asked for: a sub-1 KB PNG declaring 30000×30000.
##########
apps/desktop/src/main/app-ipc-main.ts:
##########
@@ -57,6 +65,40 @@ export function registerAppClientIpc(
targetIpc.handle('window:setTitleBarOverlayTheme', (event, theme: unknown): void => {
mainWindowController.setTitleBarOverlayTheme(event.sender, theme);
});
+ // The picker asks for the whole set at once; there is no per-id request, so
+ // no id from the renderer ever reaches the filesystem.
+ targetIpc.handle('app:iconPreviews', (): Promise<readonly AppIconPreview[]> =>
+ listAppIconPreviews(),
+ );
+ // The picker hands back a choice it was given, never a path: the dialog runs
+ // here and the file it returns is the only path this ever sees.
+ targetIpc.handle('app:importIcon', async (): Promise<CustomAppIconImportResult> => {
+ const picked = await mainWindowController.showOpenDialog({
+ properties: ['openFile'],
+ filters: [{ name: 'Images', extensions: ['png', 'jpg', 'jpeg', 'tiff', 'webp'] }],
Review Comment:
Fixed in 6cffc61 — the dialog now offers PNG and JPEG only.
The handler moved to `app-icon-ipc.ts` in the meantime. Worth noting the two ends can no longer drift: the header parser recognises exactly PNG and JPEG, so a format the dialog offered but the decoder would refuse is now a compile-and-test-level impossibility rather than a comment.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] feat(desktop): let the user choose or import the app icon [maka]
Posted by "ARE404 (via GitHub)" <gi...@apache.org>.
ARE404 commented on PR #3431:
URL: https://github.com/apache/maka/pull/3431#issuecomment-5384678452
Sure, here are some material.
before:
<img width="1650" height="1068" alt="Snipaste_2026-08-23_14-28-06" src="https://github.com/user-attachments/assets/ee363c46-9215-46f2-a09c-e5644d1baed9" />
after:
<img width="1650" height="1070" alt="Snipaste_2026-08-23_14-41-42" src="https://github.com/user-attachments/assets/2f1058b8-12e4-48e8-9bbb-882a3003cd1a" />
a short screen recording showing customize icon:
https://github.com/user-attachments/assets/f198af94-c138-4606-8e95-93a3da48bb54
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] feat(desktop): let the user choose or import the app icon [maka]
Posted by "ARE404 (via GitHub)" <gi...@apache.org>.
ARE404 commented on code in PR #3431:
URL: https://github.com/apache/maka/pull/3431#discussion_r3835052953
##########
apps/desktop/src/main/main-window.ts:
##########
@@ -263,12 +265,16 @@ export function createMainWindowController(deps: MainWindowControllerDeps): Main
...(bounds.x !== undefined && bounds.y !== undefined ? { x: bounds.x, y: bounds.y } : {}),
title: 'Maka',
// PR-GRAY-CARD-LIFT-0 (WAWQAQ msg `0eb99429` 2026-06-20): the
- // app icon ships as a 1024px PNG under apps/desktop/assets/icon.png.
- // BrowserWindow accepts a PNG path directly on macOS for the dock
- // / window title bar; .icns / .ico packaging will come with the
- // installer build pass. The asset path resolves from the built
- // dist/main/main.js (two levels up to apps/desktop, then assets).
- icon: join(import.meta.dirname, '..', '..', 'assets', 'icon.png'),
+ // app icon ships as a 1024px PNG under apps/desktop/assets/. BrowserWindow
+ // accepts a PNG path directly on macOS for the dock / window title bar;
+ // .icns / .ico packaging will come with the installer build pass. The
+ // asset path resolves from the built dist/main/main.js (two levels up to
+ // apps/desktop, then assets).
+ //
+ // Windows and Linux draw this icon per window, so a window opened after
+ // the user switched icons must be born with the chosen one — waiting for
+ // the client-settings effect to catch up would show the default first.
+ icon: appIconPath(persistedAppearance?.appIcon ?? 'default'),
Review Comment:
Fixed in 6cffc61. A well-formed path says nothing about the file existing, and per-window icons on Windows/Linux are exactly where that shows.
Window creation now walks the same fallback the dock does, through a pure `pickReadableAppIconPath(icon, toPath, isReadable)`. Keeping the readability probe injectable means the missing-file case is covered by a test rather than needing a browser process: a persisted custom id whose file disappeared resolves to the brand mark, and an id whose file is present is still honoured.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] feat(desktop): let the user choose or import the app icon [maka]
Posted by "ARE404 (via GitHub)" <gi...@apache.org>.
ARE404 commented on PR #3431:
URL: https://github.com/apache/maka/pull/3431#issuecomment-5380097536
Status on head `3fdfac7c1`: **`test` passes** (14m13s — first full green run on this branch). **`package` / Release Windows check fails**, and I believe that failure is not from this PR. Evidence rather than a claim:
```
FilesystemWorkerClientError: The target was created while this call waited for the lock; re-read before writing.
code: 'SANDBOX_FILESYSTEM_OPERATION_FAILED' reason: 'path_changed'
at verifyWindowsSandboxWorkerE2E (scripts/verify-windows-sandbox-e2e.mjs:97:5)
at verifyPackagedWindowsApp (scripts/verify-windows-x64.mjs:192:5)
```
- `git log -S "created while this call waited for the lock" --all` returns exactly one commit: `ec1579bc7` — "fix(runtime): close the path-replacement window for filesystem mutations (#2600) (#3001)". That guard and its message landed on `main` at 10:50; this run started at 11:18.
- This branch touches `verify-windows-x64.mjs` in one place — a `requireAppIconCatalog` flag on the resource assertions around line 110. The failure is at line 192, in the sandbox filesystem-worker E2E, which the run never reaches through anything I changed.
- Release Windows check has not run on `main` since #3001 merged, so this PR looks like the first execution of that workflow on top of it. Its previous `main` run (2026-08-21) also failed, though on a different step — an automatic-update version expectation (`0.1.12` vs `0.1.11`) — so I am not offering that as the same failure.
I have not tried to fix it here: it is upstream runtime behaviour, it belongs with #3001, and folding it into this PR would only make this branch harder to review. Happy to open a separate issue with the log if that is useful.
For completeness, local verification on this exact head: full workspace typecheck, Biome lint and format, Knip on both workspaces, the Astryx inventory and theme checks, 1130 desktop main-process tests, 592 in `@maka/core`, the packaging-contract tests, and a Storybook build plus visual smoke.
Windows and Linux runtime behaviour remains unverified by me — this machine cannot start an Electron GUI, so the picker, dock switching and import were exercised by hand on macOS only.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] feat(desktop): let the user choose or import the app icon [maka]
Posted by "Astro-Han (via GitHub)" <gi...@apache.org>.
Astro-Han commented on code in PR #3431:
URL: https://github.com/apache/maka/pull/3431#discussion_r3835378020
##########
scripts/verify-packaged-app.mjs:
##########
@@ -718,6 +728,18 @@ export async function assertPackagedResources(
join('licenses', 'git', 'LICENSE.txt'),
join('licenses', 'git', 'SOURCE_OFFER.txt'),
join('workers', 'filesystem-worker.js'),
+ // The app icon and the picker's catalog are read at runtime, and Electron
+ // reports a missing file as an empty image rather than an error — a
+ // packaging change that drops them would ship a blank dock tile silently.
+ //
+ // The list comes from the artwork directory rather than from `APP_ICONS`,
+ // because that enum only exists as build output: importing it would make
+ // this verifier — and its test — unable to even load before a compile.
+ // The other half of the chain is already covered where it belongs, by the
+ // desktop test that walks `APP_ICONS` and requires a 1024px master for
+ // every id.
+ join('assets', 'icon.png'),
+ ...(await packagedIconCatalog()),
Review Comment:
**[P1] Preserve the legacy artifact contract**
The Windows upgrade lifecycle verifies the previously released installer with `artifactContract: 'legacy-baseline'`, but this catalog is derived from the current checkout. All 15 `assets/app-icons/*.png` files were introduced by this PR, so every previous release necessarily lacks them and the normal upgrade verification fails before the upgrade is attempted.
Please make the new icon catalog requirement part of the current artifact contract only—for example, thread a `requireAppIconCatalog` option from `requiresCurrentContract` into `assertPackagedResources()`—while keeping the current package check strict. A contract-level test covering both current and legacy artifacts would close this completely without weakening present-day verification.
##########
apps/desktop/src/renderer/settings/appearance-settings-page.tsx:
##########
@@ -227,6 +367,81 @@ export function AppearanceSettingsPage(props: {
</VStack>
))}
</SettingsSection>
+ <SettingsSection
+ variant="bare"
+ titleId={APP_ICON_SECTION_HEADING_ID}
+ title={sections.appIcon}
+ description={sections.appIconHelp}
+ >
+ {appIconLoadFailed ? (
+ <Text type="supporting" size="sm" color="secondary">{copy.appIconUnavailable}</Text>
+ ) : (
+ <VStack gap={3}>
+ {appIconGroupsToRender.map((group) => (
+ <VStack key={group.id} gap={1.5}>
+ <Text
+ id={appIconGroupLabelId(group.id)}
+ type="label"
+ size="sm"
+ color="secondary"
+ weight="medium"
+ >
+ {copy.appIconGroups[group.id]}
+ </Text>
+ <Grid
+ columns={{ minWidth: 180 }}
+ gap={2}
+ role="group"
+ aria-labelledby={appIconGroupLabelId(group.id)}
+ >
+ {group.options.map((option) => (
+ <SelectableCard
Review Comment:
**[P2] Fence selection while an icon is being removed**
The remove button becomes disabled, but the icon cards remain selectable while `app:removeIcon` is awaiting its main-process read/reset/delete sequence. A quick selection of the still-visible icon can therefore persist it after the removal handler has read the old selection, leaving the setting pointed at a deleted file. The extra renderer-side `setAppIcon(result.selection)` can also overwrite a newer selection even though `SettingsSurface` already subscribes to `settings:clientChanged`.
The clean final state is to disable every icon card while `appIconBusy` is true and let the existing client-settings change notification reload the authoritative selection, removing the second renderer write.
##########
apps/desktop/src/renderer/settings/appearance-settings-page.tsx:
##########
@@ -227,6 +367,81 @@ export function AppearanceSettingsPage(props: {
</VStack>
))}
</SettingsSection>
+ <SettingsSection
+ variant="bare"
+ titleId={APP_ICON_SECTION_HEADING_ID}
+ title={sections.appIcon}
+ description={sections.appIconHelp}
+ >
+ {appIconLoadFailed ? (
+ <Text type="supporting" size="sm" color="secondary">{copy.appIconUnavailable}</Text>
+ ) : (
+ <VStack gap={3}>
+ {appIconGroupsToRender.map((group) => (
+ <VStack key={group.id} gap={1.5}>
+ <Text
+ id={appIconGroupLabelId(group.id)}
+ type="label"
+ size="sm"
+ color="secondary"
+ weight="medium"
+ >
+ {copy.appIconGroups[group.id]}
+ </Text>
+ <Grid
+ columns={{ minWidth: 180 }}
+ gap={2}
+ role="group"
Review Comment:
**[P2] Expose this single-choice picker as a radio group**
These choices are mutually exclusive, but `SelectableCard` exposes a hidden checkbox and the container only has `role="group"`. Screen-reader and keyboard users therefore receive independent toggle semantics rather than one-of-many selection semantics.
Please use a real radio/radiogroup contract, or extend the shared card component with a single-select mode and radio keyboard behavior. The visual card treatment can stay unchanged.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] feat(desktop): let the user choose or import the app icon [maka]
Posted by "Astro-Han (via GitHub)" <gi...@apache.org>.
Astro-Han commented on PR #3431:
URL: https://github.com/apache/maka/pull/3431#issuecomment-5382276630
One request before this lands, and it's about the PR rather than the code.
This change is user-visible — it adds fifteen preset icons plus a custom-icon import path — but the PR body has no visuals. Could you add:
1. **Before / after** — the current fixed icon next to the new picker, so the change is legible without checking out the branch;
2. **A short visual walkthrough of the feature** — the picker itself, choosing a preset, and importing a custom file.
Screenshots are enough; a short clip is better for the import flow. The reason is practical: a UI change that only exists as a diff is reviewed as a diff, and reviewers end up guessing at the part that actually matters to users. It also gives whoever reads this PR in six months something to compare against.
The functional review is still in progress on our side and I'll post it separately — this one is independent, so you can start on it now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] feat(desktop): let the user choose or import the app icon [maka]
Posted by "Astro-Han (via GitHub)" <gi...@apache.org>.
Astro-Han commented on code in PR #3431:
URL: https://github.com/apache/maka/pull/3431#discussion_r3835467096
##########
apps/desktop/src/renderer/settings/appearance-settings-page.tsx:
##########
@@ -227,6 +367,81 @@ export function AppearanceSettingsPage(props: {
</VStack>
))}
</SettingsSection>
+ <SettingsSection
+ variant="bare"
+ titleId={APP_ICON_SECTION_HEADING_ID}
+ title={sections.appIcon}
+ description={sections.appIconHelp}
+ >
+ {appIconLoadFailed ? (
+ <Text type="supporting" size="sm" color="secondary">{copy.appIconUnavailable}</Text>
+ ) : (
+ <VStack gap={3}>
+ {appIconGroupsToRender.map((group) => (
+ <VStack key={group.id} gap={1.5}>
+ <Text
+ id={appIconGroupLabelId(group.id)}
+ type="label"
+ size="sm"
+ color="secondary"
+ weight="medium"
+ >
+ {copy.appIconGroups[group.id]}
+ </Text>
+ <Grid
+ columns={{ minWidth: 180 }}
+ gap={2}
+ role="group"
Review Comment:
Thanks — I agree with the proposed boundary. Since the same shared component governs the theme, palette, and app-icon single-choice pickers, fixing only the icon picker here would create inconsistent semantics on the same page. A focused shared-component follow-up that converts all three together is the cleaner final state. Treating this as a non-blocking P2 for this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] feat(desktop): let the user choose or import the app icon [maka]
Posted by "ARE404 (via GitHub)" <gi...@apache.org>.
ARE404 commented on code in PR #3431:
URL: https://github.com/apache/maka/pull/3431#discussion_r3837830164
##########
apps/desktop/src/main/app-icon-ipc.ts:
##########
@@ -0,0 +1,213 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import type {
+ IpcMain,
+ OpenDialogOptions,
+ OpenDialogReturnValue,
+} from "electron";
+import {
+ customAppIconId,
+ isAppIconChoice,
+ isCustomAppIcon,
+ toAppIconChoice,
+ type AppIconChoice,
+ type AppSettings,
+} from "@maka/core/settings";
+import type { SettingsStore } from "@maka/storage";
+import type { AppIconPreview } from "./app-icon-surface.js";
+import {
+ CustomAppIconError,
+ listCustomAppIconIds,
+ removeCustomAppIcon,
+ type CustomAppIconImportReason,
+} from "./custom-app-icon-store.js";
+
+export type AppIconImportResult =
+ | { readonly ok: true; readonly icon: AppIconChoice }
+ | { readonly ok: false; readonly reason: CustomAppIconImportReason };
+
+export type AppIconSelectResult =
+ | { readonly ok: true; readonly selection: AppIconChoice }
+ | {
+ readonly ok: false;
+ readonly reason: "invalid_id" | "missing_artwork" | "write_failed";
+ };
+
+export type AppIconRemoveResult =
+ | { readonly ok: true; readonly selection: AppIconChoice }
+ | {
+ readonly ok: false;
+ readonly reason: "invalid_id" | "reset_failed" | "remove_failed";
+ };
+
+/**
+ * The icon surface's one main-process owner.
+ *
+ * Selection and artwork are two pieces of state that must not disagree, so the
+ * operation that can break the pair lives here rather than being sequenced by
+ * the renderer: a renderer that deletes and then persists leaves the setting
+ * pointing at a file that is gone whenever the second call fails.
+ */
+/**
+ * Everything that touches Electron is injected rather than imported: this
+ * module holds the policy, and keeping `electron` out of its import graph is
+ * what lets the removal contract be tested without booting a browser process.
+ */
+export function registerAppIconIpc(input: {
+ readonly ipcMain: Pick<IpcMain, "handle">;
+ readonly showOpenDialog: (
+ options: OpenDialogOptions,
+ ) => Promise<OpenDialogReturnValue>;
+ readonly listPreviews: () => Promise<readonly AppIconPreview[]>;
+ readonly importArtwork: (source: {
+ readonly sourcePath: string;
+ readonly userDataPath: string;
+ }) => Promise<AppIconChoice>;
+ readonly settingsStore: Pick<SettingsStore, "update" | "updateIf">;
+ readonly applySettings: (settings: AppSettings) => Promise<void>;
+ readonly userDataPath: () => string;
+}): void {
+ const { userDataPath } = input;
+
+ // Every operation that can move the selection or the artwork runs here, one
+ // at a time. A compare-and-set around the settings write alone was not
+ // enough: applying the icon yields, and a selection arriving in that window
+ // would be left pointing at a file this sequence is about to delete. Making
+ // the three operations a single serialized owner removes the window itself,
+ // rather than guarding each one that gets noticed.
+ let queue: Promise<unknown> = Promise.resolve();
+ function serialize<T>(operation: () => Promise<T>): Promise<T> {
+ const run = queue.then(operation, operation);
+ queue = run.then(
+ () => undefined,
+ () => undefined,
+ );
+ return run;
+ }
+
+ /**
+ * Selection lives behind this seam too, so it queues with removal instead of
+ * racing it through the generic settings channel. It is also the only place
+ * that can refuse a choice whose artwork is gone — the generic channel would
Review Comment:
Fixed in d925da1e. You were right that the comment named a gap the PR then left open — and pointing at `personalization` right beside it was the useful part, because the shape was already there to copy.
`clientOwnedSettingsPatch` now filters `appearance` field by field and drops `appIcon`. A patch that carried nothing else drops the section entirely rather than forwarding an empty object, so it does not register as a write on that channel at all. Three tests pin it: the icon is stripped, an icon-only patch vanishes, and the rest of `appearance` still travels.
The comment on the seam is now true as written rather than aspirational.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] feat(desktop): let the user choose or import the app icon [maka]
Posted by "Astro-Han (via GitHub)" <gi...@apache.org>.
Astro-Han commented on code in PR #3431:
URL: https://github.com/apache/maka/pull/3431#discussion_r3835675580
##########
apps/desktop/src/main/app-icon-ipc.ts:
##########
@@ -0,0 +1,125 @@
+import type { IpcMain, OpenDialogOptions, OpenDialogReturnValue } from 'electron';
+import {
+ customAppIconId,
+ isCustomAppIcon,
+ toAppIconChoice,
+ type AppIconChoice,
+ type AppSettings,
+} from '@maka/core/settings';
+import type { SettingsStore } from '@maka/storage';
+import type { AppIconPreview } from './app-icon-surface.js';
+import {
+ CustomAppIconError,
+ removeCustomAppIcon,
+ type CustomAppIconImportReason,
+} from './custom-app-icon-store.js';
+
+export type AppIconImportResult =
+ | { readonly ok: true; readonly icon: AppIconChoice }
+ | { readonly ok: false; readonly reason: CustomAppIconImportReason };
+
+export type AppIconRemoveResult =
+ | { readonly ok: true; readonly selection: AppIconChoice }
+ | {
+ readonly ok: false;
+ readonly reason: 'invalid_id' | 'reset_failed' | 'remove_failed';
+ };
+
+/**
+ * The icon surface's one main-process owner.
+ *
+ * Selection and artwork are two pieces of state that must not disagree, so the
+ * operation that can break the pair lives here rather than being sequenced by
+ * the renderer: a renderer that deletes and then persists leaves the setting
+ * pointing at a file that is gone whenever the second call fails.
+ */
+/**
+ * Everything that touches Electron is injected rather than imported: this
+ * module holds the policy, and keeping `electron` out of its import graph is
+ * what lets the removal contract be tested without booting a browser process.
+ */
+export function registerAppIconIpc(input: {
+ readonly ipcMain: Pick<IpcMain, 'handle'>;
+ readonly showOpenDialog: (options: OpenDialogOptions) => Promise<OpenDialogReturnValue>;
+ readonly listPreviews: () => Promise<readonly AppIconPreview[]>;
+ readonly importArtwork: (source: {
+ readonly sourcePath: string;
+ readonly userDataPath: string;
+ }) => Promise<AppIconChoice>;
+ readonly settingsStore: Pick<SettingsStore, 'updateIf'>;
+ readonly applySettings: (settings: AppSettings) => Promise<void>;
+ readonly userDataPath: () => string;
+}): void {
+ const { userDataPath } = input;
+
+ // The picker asks for the whole set at once; there is no per-id request, so
+ // no id from the renderer ever reaches the filesystem.
+ input.ipcMain.handle(
+ 'app:iconPreviews',
+ (): Promise<readonly AppIconPreview[]> => input.listPreviews(),
+ );
+
+ // The dialog runs here and the file it returns is the only path this sees.
+ input.ipcMain.handle('app:importIcon', async (): Promise<AppIconImportResult> => {
+ const picked = await input.showOpenDialog({
+ properties: ['openFile'],
+ // Only what `nativeImage` guarantees on every platform. Offering TIFF
+ // or WebP would let a Windows or Linux user pick a file that then fails
+ // to decode, with the dialog having implied otherwise.
+ filters: [{ name: 'PNG and JPEG', extensions: ['png', 'jpg', 'jpeg'] }],
+ });
+ const sourcePath = picked.canceled ? undefined : picked.filePaths[0];
+ if (!sourcePath) return { ok: false, reason: 'cancelled' };
+ try {
+ return {
+ ok: true,
+ icon: await input.importArtwork({ sourcePath, userDataPath: userDataPath() }),
+ };
+ } catch (error) {
+ return {
+ ok: false,
+ reason: error instanceof CustomAppIconError ? error.reason : 'unreadable',
+ };
+ }
+ });
+
+ input.ipcMain.handle(
+ 'app:removeIcon',
+ async (_event, icon: unknown): Promise<AppIconRemoveResult> => {
+ // Only imported artwork is the user's to delete, and only a well-formed
+ // reference names any of it.
+ if (!isCustomAppIcon(icon)) return { ok: false, reason: 'invalid_id' };
+ const id = customAppIconId(icon);
+ if (!id) return { ok: false, reason: 'invalid_id' };
+
+ // Compare-and-set, not read-then-write. Between a read and an
+ // unconditional write another surface can select a different icon, and
+ // resetting anyway would stamp `default` over that newer choice. The
+ // store runs the predicate and the write on one queue, so this is the
+ // only place the pair can be made atomic — no renderer busy flag
+ // reaches across the IPC boundary to do it.
+ //
+ // When the predicate no longer holds, the newer selection stands and the
+ // file is still deleted: it is no longer the one in use, which is
+ // exactly the state the caller asked for.
+ let settings: AppSettings;
+ try {
+ const outcome = await input.settingsStore.updateIf(
Review Comment:
[P2] This CAS protects only the settings write. After it returns, `applySettings()` yields before the file deletion; during that window another renderer or an already queued generic `settings:client:update` can select this same custom icon again. Removal then deletes the file and leaves the newest setting pointing at missing artwork. Please do not add another post-delete CAS: the clean final state is to put app-icon selection behind this main-process icon seam as well, serialize select/import/remove under one owner, validate that a selected custom asset still exists, and remove the renderer's generic `appearance.appIcon` write path. Then one interleaving test can prove a selection cannot enter between reset/apply/delete, rather than continuing to patch individual windows.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] feat(desktop): let the user choose or import the app icon [maka]
Posted by "ARE404 (via GitHub)" <gi...@apache.org>.
ARE404 commented on code in PR #3431:
URL: https://github.com/apache/maka/pull/3431#discussion_r3835860095
##########
apps/desktop/src/main/app-icon-ipc.ts:
##########
@@ -0,0 +1,125 @@
+import type { IpcMain, OpenDialogOptions, OpenDialogReturnValue } from 'electron';
+import {
+ customAppIconId,
+ isCustomAppIcon,
+ toAppIconChoice,
+ type AppIconChoice,
+ type AppSettings,
+} from '@maka/core/settings';
+import type { SettingsStore } from '@maka/storage';
+import type { AppIconPreview } from './app-icon-surface.js';
+import {
+ CustomAppIconError,
+ removeCustomAppIcon,
+ type CustomAppIconImportReason,
+} from './custom-app-icon-store.js';
+
+export type AppIconImportResult =
+ | { readonly ok: true; readonly icon: AppIconChoice }
+ | { readonly ok: false; readonly reason: CustomAppIconImportReason };
+
+export type AppIconRemoveResult =
+ | { readonly ok: true; readonly selection: AppIconChoice }
+ | {
+ readonly ok: false;
+ readonly reason: 'invalid_id' | 'reset_failed' | 'remove_failed';
+ };
+
+/**
+ * The icon surface's one main-process owner.
+ *
+ * Selection and artwork are two pieces of state that must not disagree, so the
+ * operation that can break the pair lives here rather than being sequenced by
+ * the renderer: a renderer that deletes and then persists leaves the setting
+ * pointing at a file that is gone whenever the second call fails.
+ */
+/**
+ * Everything that touches Electron is injected rather than imported: this
+ * module holds the policy, and keeping `electron` out of its import graph is
+ * what lets the removal contract be tested without booting a browser process.
+ */
+export function registerAppIconIpc(input: {
+ readonly ipcMain: Pick<IpcMain, 'handle'>;
+ readonly showOpenDialog: (options: OpenDialogOptions) => Promise<OpenDialogReturnValue>;
+ readonly listPreviews: () => Promise<readonly AppIconPreview[]>;
+ readonly importArtwork: (source: {
+ readonly sourcePath: string;
+ readonly userDataPath: string;
+ }) => Promise<AppIconChoice>;
+ readonly settingsStore: Pick<SettingsStore, 'updateIf'>;
+ readonly applySettings: (settings: AppSettings) => Promise<void>;
+ readonly userDataPath: () => string;
+}): void {
+ const { userDataPath } = input;
+
+ // The picker asks for the whole set at once; there is no per-id request, so
+ // no id from the renderer ever reaches the filesystem.
+ input.ipcMain.handle(
+ 'app:iconPreviews',
+ (): Promise<readonly AppIconPreview[]> => input.listPreviews(),
+ );
+
+ // The dialog runs here and the file it returns is the only path this sees.
+ input.ipcMain.handle('app:importIcon', async (): Promise<AppIconImportResult> => {
+ const picked = await input.showOpenDialog({
+ properties: ['openFile'],
+ // Only what `nativeImage` guarantees on every platform. Offering TIFF
+ // or WebP would let a Windows or Linux user pick a file that then fails
+ // to decode, with the dialog having implied otherwise.
+ filters: [{ name: 'PNG and JPEG', extensions: ['png', 'jpg', 'jpeg'] }],
+ });
+ const sourcePath = picked.canceled ? undefined : picked.filePaths[0];
+ if (!sourcePath) return { ok: false, reason: 'cancelled' };
+ try {
+ return {
+ ok: true,
+ icon: await input.importArtwork({ sourcePath, userDataPath: userDataPath() }),
+ };
+ } catch (error) {
+ return {
+ ok: false,
+ reason: error instanceof CustomAppIconError ? error.reason : 'unreadable',
+ };
+ }
+ });
+
+ input.ipcMain.handle(
+ 'app:removeIcon',
+ async (_event, icon: unknown): Promise<AppIconRemoveResult> => {
+ // Only imported artwork is the user's to delete, and only a well-formed
+ // reference names any of it.
+ if (!isCustomAppIcon(icon)) return { ok: false, reason: 'invalid_id' };
+ const id = customAppIconId(icon);
+ if (!id) return { ok: false, reason: 'invalid_id' };
+
+ // Compare-and-set, not read-then-write. Between a read and an
+ // unconditional write another surface can select a different icon, and
+ // resetting anyway would stamp `default` over that newer choice. The
+ // store runs the predicate and the write on one queue, so this is the
+ // only place the pair can be made atomic — no renderer busy flag
+ // reaches across the IPC boundary to do it.
+ //
+ // When the predicate no longer holds, the newer selection stands and the
+ // file is still deleted: it is no longer the one in use, which is
+ // exactly the state the caller asked for.
+ let settings: AppSettings;
+ try {
+ const outcome = await input.settingsStore.updateIf(
Review Comment:
Fixed in 3fdfac7 — and you were right that guarding the window would only move it.
Selection now goes through the seam rather than racing it. `app:selectIcon` persists and applies the choice, select/import/remove run one at a time under a single owner, and the renderer no longer writes `appearance.appIcon` on the generic settings channel at all. That write path was the thing that could interleave, so it is gone rather than fenced.
Selection can also be refused now: the generic channel would happily persist an id whose file is missing, while this seam checks the artwork is still there first.
The regression test issues a selection mid-removal without awaiting it, and pins the whole sequence rather than one boundary — the selection runs after the removal instead of inside it, is refused as `missing_artwork`, the setting ends on the brand mark, the file is gone, and nothing ran between the reset and the delete.
Also rebased onto current main; the conflict was the generated Astryx surface inventory, which #3369 had regenerated, so I re-ran the generator rather than hand-merging it.
Local verification on this head: full workspace typecheck, Biome lint and format, Knip, the Astryx inventory and theme checks, 1130 desktop main-process tests, 592 in `@maka/core`, the packaging-contract tests, and the Storybook build plus visual smoke — the 外观 story renders.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] feat(desktop): let the user choose or import the app icon [maka]
Posted by "ARE404 (via GitHub)" <gi...@apache.org>.
ARE404 commented on code in PR #3431:
URL: https://github.com/apache/maka/pull/3431#discussion_r3835648180
##########
apps/desktop/src/main/app-icon-ipc.ts:
##########
@@ -0,0 +1,118 @@
+import type { IpcMain, OpenDialogOptions, OpenDialogReturnValue } from 'electron';
+import {
+ customAppIconId,
+ isCustomAppIcon,
+ toAppIconChoice,
+ type AppIconChoice,
+ type AppSettings,
+} from '@maka/core/settings';
+import type { SettingsStore } from '@maka/storage';
+import type { AppIconPreview } from './app-icon-surface.js';
+import {
+ CustomAppIconError,
+ removeCustomAppIcon,
+ type CustomAppIconImportReason,
+} from './custom-app-icon-store.js';
+
+export type AppIconImportResult =
+ | { readonly ok: true; readonly icon: AppIconChoice }
+ | { readonly ok: false; readonly reason: CustomAppIconImportReason };
+
+export type AppIconRemoveResult =
+ | { readonly ok: true; readonly selection: AppIconChoice }
+ | {
+ readonly ok: false;
+ readonly reason: 'invalid_id' | 'reset_failed' | 'remove_failed';
+ };
+
+/**
+ * The icon surface's one main-process owner.
+ *
+ * Selection and artwork are two pieces of state that must not disagree, so the
+ * operation that can break the pair lives here rather than being sequenced by
+ * the renderer: a renderer that deletes and then persists leaves the setting
+ * pointing at a file that is gone whenever the second call fails.
+ */
+/**
+ * Everything that touches Electron is injected rather than imported: this
+ * module holds the policy, and keeping `electron` out of its import graph is
+ * what lets the removal contract be tested without booting a browser process.
+ */
+export function registerAppIconIpc(input: {
+ readonly ipcMain: Pick<IpcMain, 'handle'>;
+ readonly showOpenDialog: (options: OpenDialogOptions) => Promise<OpenDialogReturnValue>;
+ readonly listPreviews: () => Promise<readonly AppIconPreview[]>;
+ readonly importArtwork: (source: {
+ readonly sourcePath: string;
+ readonly userDataPath: string;
+ }) => Promise<AppIconChoice>;
+ readonly settingsStore: Pick<SettingsStore, 'get' | 'update'>;
+ readonly applySettings: (settings: AppSettings) => Promise<void>;
+ readonly userDataPath: () => string;
+}): void {
+ const { userDataPath } = input;
+
+ // The picker asks for the whole set at once; there is no per-id request, so
+ // no id from the renderer ever reaches the filesystem.
+ input.ipcMain.handle(
+ 'app:iconPreviews',
+ (): Promise<readonly AppIconPreview[]> => input.listPreviews(),
+ );
+
+ // The dialog runs here and the file it returns is the only path this sees.
+ input.ipcMain.handle('app:importIcon', async (): Promise<AppIconImportResult> => {
+ const picked = await input.showOpenDialog({
+ properties: ['openFile'],
+ // Only what `nativeImage` guarantees on every platform. Offering TIFF
+ // or WebP would let a Windows or Linux user pick a file that then fails
+ // to decode, with the dialog having implied otherwise.
+ filters: [{ name: 'PNG and JPEG', extensions: ['png', 'jpg', 'jpeg'] }],
+ });
+ const sourcePath = picked.canceled ? undefined : picked.filePaths[0];
+ if (!sourcePath) return { ok: false, reason: 'cancelled' };
+ try {
+ return {
+ ok: true,
+ icon: await input.importArtwork({ sourcePath, userDataPath: userDataPath() }),
+ };
+ } catch (error) {
+ return {
+ ok: false,
+ reason: error instanceof CustomAppIconError ? error.reason : 'unreadable',
+ };
+ }
+ });
+
+ input.ipcMain.handle(
+ 'app:removeIcon',
+ async (_event, icon: unknown): Promise<AppIconRemoveResult> => {
+ // Only imported artwork is the user's to delete, and only a well-formed
+ // reference names any of it.
+ if (!isCustomAppIcon(icon)) return { ok: false, reason: 'invalid_id' };
+ const id = customAppIconId(icon);
+ if (!id) return { ok: false, reason: 'invalid_id' };
+
+ const current = await input.settingsStore.get();
+ let selection = toAppIconChoice(current.appearance.appIcon);
+ if (selection === icon) {
+ // Hand the OS back to the brand mark BEFORE the file goes away. The
+ // other order leaves a persisted choice pointing at nothing whenever
+ // this write fails.
+ try {
+ const next = await input.settingsStore.update({ appearance: { appIcon: 'default' } });
Review Comment:
Fixed in 3ce30b6. You are right, and the busy flag was never going to reach across the IPC boundary to close it.
`updateIf` runs its predicate and the write on one queue, so the reset now applies only while the current choice is still the icon being removed. When it is not, the newer selection stands and is what the caller is told; the OS surface is re-applied only when the selection actually moved.
The file is deleted either way — at that point it is no longer the one in use, which is the state the caller asked for. Worth confirming that reading is what you intended, since the alternative would be to refuse the delete and make the user click again on an icon that is no longer selected.
This also removed a call rather than adding a path: `updateIf` reports the settings whether or not it applied, so the separate `get()` is gone and the dependency is narrowed to `Pick<SettingsStore, 'updateIf'>`.
Regression test added as asked — a selection lands while the compare-and-set is being evaluated, and it pins all four outcomes: the newer choice wins, it is what is returned, nothing is re-applied, and the artwork is still gone.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] feat(desktop): let the user choose or import the app icon [maka]
Posted by "ARE404 (via GitHub)" <gi...@apache.org>.
ARE404 commented on code in PR #3431:
URL: https://github.com/apache/maka/pull/3431#discussion_r3835442654
##########
apps/desktop/src/renderer/settings/appearance-settings-page.tsx:
##########
@@ -227,6 +367,81 @@ export function AppearanceSettingsPage(props: {
</VStack>
))}
</SettingsSection>
+ <SettingsSection
+ variant="bare"
+ titleId={APP_ICON_SECTION_HEADING_ID}
+ title={sections.appIcon}
+ description={sections.appIconHelp}
+ >
+ {appIconLoadFailed ? (
+ <Text type="supporting" size="sm" color="secondary">{copy.appIconUnavailable}</Text>
+ ) : (
+ <VStack gap={3}>
+ {appIconGroupsToRender.map((group) => (
+ <VStack key={group.id} gap={1.5}>
+ <Text
+ id={appIconGroupLabelId(group.id)}
+ type="label"
+ size="sm"
+ color="secondary"
+ weight="medium"
+ >
+ {copy.appIconGroups[group.id]}
+ </Text>
+ <Grid
+ columns={{ minWidth: 180 }}
+ gap={2}
+ role="group"
Review Comment:
Agreed on the semantics — a hidden checkbox for a one-of-many choice is wrong, and `role="group"` does not repair it.
I would rather not fix it only here, and I want to explain why rather than just deferring. `SelectableCard` hard-codes `type="checkbox"`, and this file uses it eight times: theme, palettes, and now icons. All three are single-choice, so the gap is identical in all three and predates this branch. Fixing it in the icon picker alone would leave the same page announcing three visually identical pickers with two different contracts, which seems worse than the current consistent-but-wrong state.
The right shape looks like a single-select mode on the shared card — native radios also give the arrow-key behavior for free — applied to all three pickers at once. I can see `patches/@astryxdesign+core+0.4.0.patch`, so extending the component from this repo is established practice here, and I am happy to do that work.
Leaving this thread open rather than resolving it, since nothing changed. Two ways I can go, your call:
1. a separate PR that adds the single-select mode and converts theme, palettes and icons together — my preference, since it is a design-system change and reviewable on its own
2. do it inside this PR if you would rather not ship a new picker with the old semantics
If you want (2), say so and I will fold it in.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] feat(desktop): let the user choose or import the app icon [maka]
Posted by "ARE404 (via GitHub)" <gi...@apache.org>.
ARE404 commented on PR #3431:
URL: https://github.com/apache/maka/pull/3431#issuecomment-5377756545
Thanks — the two runtime findings were both real, and I confirmed them before fixing rather than taking them on faith.
Rebased onto current `main` (which now carries #3451, so the duplicate commit is gone and the conflict with it).
**Path escape — confirmed and fixed.** `normalizeSettings` runs on *read*; `SettingsStore.update` merges and writes without it, so the snapshot the effects act on carries whatever the patch put there. `appIconPath` then cast it. Every runtime ingress now coerces through a new `toAppIconChoice` — `appIconPath`, `applyAppIcon`, and the settings effect — and the declared type is no longer treated as a guarantee. Regression tests at both layers: the coercion itself in `@maka/core`, and a path-level assertion that a malformed choice cannot resolve outside the asset root.
**Delete ordering — confirmed and fixed.** The code did the opposite of its comment. Removal is now one main-process operation that resets the selection, applies it, then deletes; the renderer no longer sequences anything. `app-icon-ipc.ts` takes its Electron-touching capabilities as parameters so the contract is testable without a browser process — three tests cover reset-then-delete, the untouched-selection case, and refusal of shipped ids and malformed references.
**Packaged-resource verifier.** `assertPackagedResources` now requires `assets/icon.png` and every `APP_ICONS` file, plus a test that the check fails when one is dropped. You are right that the source tests proved the wrong thing: they showed the repository was correct, not that the payload survives packaging.
**`desktopAssetPath`** — removed along with the redundant assertion; its only production caller became `appIconPath` in this branch.
**Trailing blank line** — fixed, `git diff --check` is clean.
On the 14 variants: I have raised the brand question with the maintainers separately and was told to go ahead, so they stay for now. If the PMC would rather decide that on its own thread, say the word and I will drop the artwork commit — the picker and the import path stand without it.
Not fixed, and I want to be straight about it: I still cannot verify Windows/Linux icon updates or run a packaged smoke check here. This machine cannot start an Electron GUI, so the picker and dock switching were verified by hand on macOS only, and the cross-platform format filter you flagged is untested by me.
Verification this round: `tsc --noEmit` clean for main/preload/renderer; Biome lint and format clean; 17 main-process tests, 2 packaging-boundary tests, 591 in `@maka/core`; `git diff --check` clean.
Re-requesting review.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] feat(desktop): let the user choose or import the app icon [maka]
Posted by "hqhq1025 (via GitHub)" <gi...@apache.org>.
hqhq1025 commented on code in PR #3431:
URL: https://github.com/apache/maka/pull/3431#discussion_r3832993828
##########
apps/desktop/src/main/app-icon-surface.ts:
##########
@@ -0,0 +1,136 @@
+import { app, BrowserWindow, nativeImage } from 'electron';
+import { join } from 'node:path';
+import {
+ APP_ICONS,
+ CUSTOM_APP_ICON_PREFIX,
+ customAppIconId,
+ type AppIcon,
+ type AppIconChoice,
+} from '@maka/core/settings';
+import {
+ customAppIconDirectory,
+ listCustomAppIconIds,
+ resolveCustomAppIconPath,
+} from './custom-app-icon-store.js';
+import { appIconLoadOrder, resolveAppIconPath } from './app-icon.js';
+import { desktopAssetRoot } from './desktop-assets.js';
+
+/**
+ * One choice's artwork path — shipped art under the asset root, imported art
+ * under the directory the app owns. Never throws: a malformed id can only come
+ * from a settings file that dodged normalization, and a window being created
+ * is no place to raise. The brand mark is the answer to every such question.
+ */
+export function appIconPath(choice: AppIconChoice): string {
+ const custom = customAppIconId(choice);
+ if (custom === undefined) return resolveAppIconPath(currentAssetRoot(), choice as AppIcon);
Review Comment:
[P1] Reject non-catalog values before constructing a shipped asset path. `AppIconChoice` is only a compile-time type: `settings:client:update` accepts renderer input at runtime, `clientOwnedSettingsPatch()` forwards `appearance` unchanged, and `SettingsStore.update()` merges without normalization. Therefore `appearance.appIcon = "../../../../tmp/owned"` reaches this cast and resolves outside the asset root (`/tmp/owned.png` in a direct reproduction), after which Electron's native decoder reads it. Use `isAppIcon(choice)` here (falling back or rejecting) and normalize/validate the generic settings update boundary as defense in depth; add a test that sends a malformed runtime settings patch.
##########
apps/desktop/src/main/custom-app-icons.ts:
##########
@@ -0,0 +1,63 @@
+import { randomUUID } from 'node:crypto';
+import { mkdir, stat, writeFile } from 'node:fs/promises';
+import { nativeImage } from 'electron';
+import { CUSTOM_APP_ICON_PREFIX, type CustomAppIcon } from '@maka/core/settings';
+import {
+ CustomAppIconError,
+ CUSTOM_ICON_EDGE,
+ CUSTOM_ICON_MAX_INPUT_BYTES,
+ CUSTOM_ICON_MIN_EDGE,
+ customAppIconDirectory,
+ resolveCustomAppIconPath,
+} from './custom-app-icon-store.js';
+
+/**
+ * Decode, square, scale, store. Non-square art is centre-cropped rather than
+ * letterboxed: an icon that keeps its own transparent margin is the caller's
+ * business, and silently adding one would change art the user already framed.
+ */
+export async function importCustomAppIcon(input: {
+ readonly sourcePath: string;
+ readonly userDataPath: string;
+}): Promise<CustomAppIcon> {
+ const info = await stat(input.sourcePath).catch(() => undefined);
+ if (!info?.isFile()) throw new CustomAppIconError('unreadable', 'not a file');
+ if (info.size > CUSTOM_ICON_MAX_INPUT_BYTES) {
+ throw new CustomAppIconError('too_large', `over ${CUSTOM_ICON_MAX_INPUT_BYTES} bytes`);
+ }
+
+ // An undecodable file comes back EMPTY rather than throwing — the same trap
+ // the shipped set falls into, and the reason emptiness is the failure test.
+ const source = nativeImage.createFromPath(input.sourcePath);
Review Comment:
[P2] Bound decoded dimensions before decoding untrusted image data in the main process. The 16 MB `stat()` limit only caps compressed bytes; a highly compressed image can expand to a very large bitmap and exhaust the Electron main process before `getSize()` runs. There is also a TOCTOU window because the path can be replaced between `stat()` and `createFromPath()`. Read one size-capped file snapshot, validate its format and pixel dimensions before native decoding (or use a decoder with explicit resource limits), then decode that same buffer. Add a compressed oversized-dimension fixture to exercise the limit.
##########
apps/desktop/src/main/main-window.ts:
##########
@@ -263,12 +265,16 @@ export function createMainWindowController(deps: MainWindowControllerDeps): Main
...(bounds.x !== undefined && bounds.y !== undefined ? { x: bounds.x, y: bounds.y } : {}),
title: 'Maka',
// PR-GRAY-CARD-LIFT-0 (WAWQAQ msg `0eb99429` 2026-06-20): the
- // app icon ships as a 1024px PNG under apps/desktop/assets/icon.png.
- // BrowserWindow accepts a PNG path directly on macOS for the dock
- // / window title bar; .icns / .ico packaging will come with the
- // installer build pass. The asset path resolves from the built
- // dist/main/main.js (two levels up to apps/desktop, then assets).
- icon: join(import.meta.dirname, '..', '..', 'assets', 'icon.png'),
+ // app icon ships as a 1024px PNG under apps/desktop/assets/. BrowserWindow
+ // accepts a PNG path directly on macOS for the dock / window title bar;
+ // .icns / .ico packaging will come with the installer build pass. The
+ // asset path resolves from the built dist/main/main.js (two levels up to
+ // apps/desktop, then assets).
+ //
+ // Windows and Linux draw this icon per window, so a window opened after
+ // the user switched icons must be born with the chosen one — waiting for
+ // the client-settings effect to catch up would show the default first.
+ icon: appIconPath(persistedAppearance?.appIcon ?? 'default'),
Review Comment:
[P2] Use the same readable-image fallback used by `applyAppIcon()` when creating a window. `appIconPath()` returns a syntactically valid custom path even when that file is missing or corrupt, so a newly created Windows/Linux window can receive an invalid icon path while existing windows fall back to the default through `loadAppIcon()`. Resolve and validate the image once through the shared fallback path, rather than giving `BrowserWindow` the raw path. Add a test for a persisted custom ID whose file has disappeared.
##########
apps/desktop/src/renderer/settings/appearance-settings-page.tsx:
##########
@@ -90,6 +118,70 @@ export function AppearanceSettingsPage(props: {
const toast = useToast();
const themePageMountedRef = useMountedRef();
const themePersistTicketRef = useRef(0);
+ // The picker draws real artwork, so the option set arrives from the main
+ // process (ids plus thumbnails) rather than from a list held here: the icons
+ // are 1024px masters that only main can read, and the renderer is never
+ // handed a path. `undefined` is "still asking", not "none shipped".
+ const [appIconOptions, setAppIconOptions] = useState<
+ ReadonlyArray<{ id: AppIconChoice; dataUrl: string; removable?: boolean }> | undefined
+ >(undefined);
+ const [appIconLoadFailed, setAppIconLoadFailed] = useState(false);
+ const [appIconBusy, setAppIconBusy] = useState(false);
+
+ async function refreshAppIcons() {
+ const options = await window.maka.app.iconPreviews().catch(() => undefined);
+ if (options) setAppIconOptions(options);
+ }
+
+ async function importAppIcon() {
+ setAppIconBusy(true);
+ try {
+ const result = await window.maka.app.importIcon();
+ if (!result.ok) {
+ // Closing the dialog is an answer, not a failure worth a toast.
+ if (result.reason !== 'cancelled') toast.error(copy.appIconImportFailed[result.reason]);
+ return;
+ }
+ await refreshAppIcons();
+ await setAppIcon(result.icon);
+ } catch (error) {
+ // Reasons above describe the *file*; landing here instead means the call
+ // itself failed — a stale preload bundle with no `importIcon` on the
+ // bridge looks exactly like this — and calling that "unreadable image"
+ // would send the user off inspecting a file that was never the problem.
+ toast.error(copy.appIconImportError, settingsActionErrorMessage(error, locale));
+ } finally {
+ setAppIconBusy(false);
+ }
+ }
+
+ async function removeAppIcon(icon: AppIconChoice) {
+ setAppIconBusy(true);
+ try {
+ await window.maka.app.removeIcon(icon);
Review Comment:
[P2] Reset the selected setting before deleting its file, and do not swallow the persistence result in this destructive sequence. The comment below says to hand the selection back first, but deletion happens here first. `setAppIcon()` calls `persistAppearance()`, which catches update failures and resolves, so a disk/IPC failure leaves the saved setting pointing at an already-deleted icon while this function continues as if it succeeded. This also races because the selection cards remain enabled while `appIconBusy` is true. Prefer one main-process operation that coordinates the setting reset and deletion, or at minimum confirm the reset succeeded before deletion and disable all icon choices during the operation.
##########
apps/desktop/src/main/app-ipc-main.ts:
##########
@@ -57,6 +65,40 @@ export function registerAppClientIpc(
targetIpc.handle('window:setTitleBarOverlayTheme', (event, theme: unknown): void => {
mainWindowController.setTitleBarOverlayTheme(event.sender, theme);
});
+ // The picker asks for the whole set at once; there is no per-id request, so
+ // no id from the renderer ever reaches the filesystem.
+ targetIpc.handle('app:iconPreviews', (): Promise<readonly AppIconPreview[]> =>
+ listAppIconPreviews(),
+ );
+ // The picker hands back a choice it was given, never a path: the dialog runs
+ // here and the file it returns is the only path this ever sees.
+ targetIpc.handle('app:importIcon', async (): Promise<CustomAppIconImportResult> => {
+ const picked = await mainWindowController.showOpenDialog({
+ properties: ['openFile'],
+ filters: [{ name: 'Images', extensions: ['png', 'jpg', 'jpeg', 'tiff', 'webp'] }],
Review Comment:
[P2] Do not advertise TIFF and WebP when the decoder used below does not guarantee them cross-platform. Electron's current `nativeImage` documentation guarantees PNG and JPEG across all platforms (and ICO paths on Windows); a Windows/Linux user can select one of these advertised extensions and immediately get an unreadable-image failure. Restrict the dialog to PNG/JPEG, or add an explicit cross-platform decoder and tests for every advertised format.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] feat(desktop): let the user choose or import the app icon [maka]
Posted by "ARE404 (via GitHub)" <gi...@apache.org>.
ARE404 commented on code in PR #3431:
URL: https://github.com/apache/maka/pull/3431#discussion_r3835051891
##########
apps/desktop/src/main/app-icon-surface.ts:
##########
@@ -0,0 +1,136 @@
+import { app, BrowserWindow, nativeImage } from 'electron';
+import { join } from 'node:path';
+import {
+ APP_ICONS,
+ CUSTOM_APP_ICON_PREFIX,
+ customAppIconId,
+ type AppIcon,
+ type AppIconChoice,
+} from '@maka/core/settings';
+import {
+ customAppIconDirectory,
+ listCustomAppIconIds,
+ resolveCustomAppIconPath,
+} from './custom-app-icon-store.js';
+import { appIconLoadOrder, resolveAppIconPath } from './app-icon.js';
+import { desktopAssetRoot } from './desktop-assets.js';
+
+/**
+ * One choice's artwork path — shipped art under the asset root, imported art
+ * under the directory the app owns. Never throws: a malformed id can only come
+ * from a settings file that dodged normalization, and a window being created
+ * is no place to raise. The brand mark is the answer to every such question.
+ */
+export function appIconPath(choice: AppIconChoice): string {
+ const custom = customAppIconId(choice);
+ if (custom === undefined) return resolveAppIconPath(currentAssetRoot(), choice as AppIcon);
Review Comment:
Confirmed and fixed in 17d37be. You are right that the type was doing no work at runtime: `normalizeSettings` runs on read, while `SettingsStore.update` merges and writes without it, so the snapshot the effects act on carries whatever the patch put there.
Added `toAppIconChoice` in `@maka/core` and coerced at every runtime ingress — `appIconPath`, `applyAppIcon`, and the settings effect — so no path is built from an unchecked value. Two regressions: the coercion itself, and a path-level assertion that a malformed choice cannot resolve outside the asset root.
##########
apps/desktop/src/renderer/settings/appearance-settings-page.tsx:
##########
@@ -90,6 +118,70 @@ export function AppearanceSettingsPage(props: {
const toast = useToast();
const themePageMountedRef = useMountedRef();
const themePersistTicketRef = useRef(0);
+ // The picker draws real artwork, so the option set arrives from the main
+ // process (ids plus thumbnails) rather than from a list held here: the icons
+ // are 1024px masters that only main can read, and the renderer is never
+ // handed a path. `undefined` is "still asking", not "none shipped".
+ const [appIconOptions, setAppIconOptions] = useState<
+ ReadonlyArray<{ id: AppIconChoice; dataUrl: string; removable?: boolean }> | undefined
+ >(undefined);
+ const [appIconLoadFailed, setAppIconLoadFailed] = useState(false);
+ const [appIconBusy, setAppIconBusy] = useState(false);
+
+ async function refreshAppIcons() {
+ const options = await window.maka.app.iconPreviews().catch(() => undefined);
+ if (options) setAppIconOptions(options);
+ }
+
+ async function importAppIcon() {
+ setAppIconBusy(true);
+ try {
+ const result = await window.maka.app.importIcon();
+ if (!result.ok) {
+ // Closing the dialog is an answer, not a failure worth a toast.
+ if (result.reason !== 'cancelled') toast.error(copy.appIconImportFailed[result.reason]);
+ return;
+ }
+ await refreshAppIcons();
+ await setAppIcon(result.icon);
+ } catch (error) {
+ // Reasons above describe the *file*; landing here instead means the call
+ // itself failed — a stale preload bundle with no `importIcon` on the
+ // bridge looks exactly like this — and calling that "unreadable image"
+ // would send the user off inspecting a file that was never the problem.
+ toast.error(copy.appIconImportError, settingsActionErrorMessage(error, locale));
+ } finally {
+ setAppIconBusy(false);
+ }
+ }
+
+ async function removeAppIcon(icon: AppIconChoice) {
+ setAppIconBusy(true);
+ try {
+ await window.maka.app.removeIcon(icon);
Review Comment:
Confirmed and fixed in 17d37be. The code did the opposite of its own comment.
Selection and artwork are one pair of state, so the operation moved to a single main-process owner (`app-icon-ipc.ts`): it resets the selection, applies it, and only then deletes. The renderer sequences nothing and no longer swallows the result. It takes its Electron-touching capabilities as parameters, so three tests cover reset-then-delete, the untouched-selection case, and refusal of shipped ids and malformed references.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] feat(desktop): let the user choose or import the app icon [maka]
Posted by "ARE404 (via GitHub)" <gi...@apache.org>.
ARE404 commented on code in PR #3431:
URL: https://github.com/apache/maka/pull/3431#discussion_r3835441449
##########
scripts/verify-packaged-app.mjs:
##########
@@ -718,6 +728,18 @@ export async function assertPackagedResources(
join('licenses', 'git', 'LICENSE.txt'),
join('licenses', 'git', 'SOURCE_OFFER.txt'),
join('workers', 'filesystem-worker.js'),
+ // The app icon and the picker's catalog are read at runtime, and Electron
+ // reports a missing file as an empty image rather than an error — a
+ // packaging change that drops them would ship a blank dock tile silently.
+ //
+ // The list comes from the artwork directory rather than from `APP_ICONS`,
+ // because that enum only exists as build output: importing it would make
+ // this verifier — and its test — unable to even load before a compile.
+ // The other half of the chain is already covered where it belongs, by the
+ // desktop test that walks `APP_ICONS` and requires a 1024px master for
+ // every id.
+ join('assets', 'icon.png'),
+ ...(await packagedIconCatalog()),
Review Comment:
Fixed in 0660b5f. You are right, and it would have failed the upgrade check before the upgrade was attempted.
`requireAppIconCatalog` is now threaded from `requiresCurrentContract` in `verify-windows-x64.mjs`, exactly alongside `requireDisclaimer`, with the current contract left strict.
One thing worth flagging beyond the report: the gate has to cover `assets/icon.png` as well, not only the catalog. `assets/` was not packaged at all before the picker landed — that directory arrived with #3451 — so a legacy baseline lacks the brand mark too, and gating only `app-icons/` would still have failed on `icon.png`.
Contract-level coverage added both ways: the current contract requires every file present in the artwork directory, and a `legacy-baseline` run asks for none of `assets/` while still verifying everything the old contract did carry.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] feat(desktop): let the user choose or import the app icon [maka]
Posted by "ARE404 (via GitHub)" <gi...@apache.org>.
ARE404 commented on code in PR #3431:
URL: https://github.com/apache/maka/pull/3431#discussion_r3835442028
##########
apps/desktop/src/renderer/settings/appearance-settings-page.tsx:
##########
@@ -227,6 +367,81 @@ export function AppearanceSettingsPage(props: {
</VStack>
))}
</SettingsSection>
+ <SettingsSection
+ variant="bare"
+ titleId={APP_ICON_SECTION_HEADING_ID}
+ title={sections.appIcon}
+ description={sections.appIconHelp}
+ >
+ {appIconLoadFailed ? (
+ <Text type="supporting" size="sm" color="secondary">{copy.appIconUnavailable}</Text>
+ ) : (
+ <VStack gap={3}>
+ {appIconGroupsToRender.map((group) => (
+ <VStack key={group.id} gap={1.5}>
+ <Text
+ id={appIconGroupLabelId(group.id)}
+ type="label"
+ size="sm"
+ color="secondary"
+ weight="medium"
+ >
+ {copy.appIconGroups[group.id]}
+ </Text>
+ <Grid
+ columns={{ minWidth: 180 }}
+ gap={2}
+ role="group"
+ aria-labelledby={appIconGroupLabelId(group.id)}
+ >
+ {group.options.map((option) => (
+ <SelectableCard
Review Comment:
Fixed in 0660b5f — every card is fenced while a removal is in flight, not just the remove button.
The second write is gone too, and thank you for pointing at `settings:clientChanged`: I checked, the effects fingerprint is a stringify of the whole settings object, so an icon-only change does emit it and the surface reloads on its own. That write came out of my own review pass a few hours ago, where I found `settings:externalChanged`, concluded nothing notified this surface, and stopped looking. It was both unnecessary and a way to stamp a stale value over a newer choice — exactly as you describe.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] feat(desktop): let the user choose or import the app icon [maka]
Posted by "Astro-Han (via GitHub)" <gi...@apache.org>.
Astro-Han merged PR #3431:
URL: https://github.com/apache/maka/pull/3431
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] feat(desktop): let the user choose or import the app icon [maka]
Posted by "Astro-Han (via GitHub)" <gi...@apache.org>.
Astro-Han commented on PR #3431:
URL: https://github.com/apache/maka/pull/3431#issuecomment-5384733232
Nice Feature Addition! LGTM.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] feat(desktop): let the user choose or import the app icon [maka]
Posted by "Astro-Han (via GitHub)" <gi...@apache.org>.
Astro-Han commented on code in PR #3431:
URL: https://github.com/apache/maka/pull/3431#discussion_r3835635218
##########
apps/desktop/src/main/app-icon-ipc.ts:
##########
@@ -0,0 +1,118 @@
+import type { IpcMain, OpenDialogOptions, OpenDialogReturnValue } from 'electron';
+import {
+ customAppIconId,
+ isCustomAppIcon,
+ toAppIconChoice,
+ type AppIconChoice,
+ type AppSettings,
+} from '@maka/core/settings';
+import type { SettingsStore } from '@maka/storage';
+import type { AppIconPreview } from './app-icon-surface.js';
+import {
+ CustomAppIconError,
+ removeCustomAppIcon,
+ type CustomAppIconImportReason,
+} from './custom-app-icon-store.js';
+
+export type AppIconImportResult =
+ | { readonly ok: true; readonly icon: AppIconChoice }
+ | { readonly ok: false; readonly reason: CustomAppIconImportReason };
+
+export type AppIconRemoveResult =
+ | { readonly ok: true; readonly selection: AppIconChoice }
+ | {
+ readonly ok: false;
+ readonly reason: 'invalid_id' | 'reset_failed' | 'remove_failed';
+ };
+
+/**
+ * The icon surface's one main-process owner.
+ *
+ * Selection and artwork are two pieces of state that must not disagree, so the
+ * operation that can break the pair lives here rather than being sequenced by
+ * the renderer: a renderer that deletes and then persists leaves the setting
+ * pointing at a file that is gone whenever the second call fails.
+ */
+/**
+ * Everything that touches Electron is injected rather than imported: this
+ * module holds the policy, and keeping `electron` out of its import graph is
+ * what lets the removal contract be tested without booting a browser process.
+ */
+export function registerAppIconIpc(input: {
+ readonly ipcMain: Pick<IpcMain, 'handle'>;
+ readonly showOpenDialog: (options: OpenDialogOptions) => Promise<OpenDialogReturnValue>;
+ readonly listPreviews: () => Promise<readonly AppIconPreview[]>;
+ readonly importArtwork: (source: {
+ readonly sourcePath: string;
+ readonly userDataPath: string;
+ }) => Promise<AppIconChoice>;
+ readonly settingsStore: Pick<SettingsStore, 'get' | 'update'>;
+ readonly applySettings: (settings: AppSettings) => Promise<void>;
+ readonly userDataPath: () => string;
+}): void {
+ const { userDataPath } = input;
+
+ // The picker asks for the whole set at once; there is no per-id request, so
+ // no id from the renderer ever reaches the filesystem.
+ input.ipcMain.handle(
+ 'app:iconPreviews',
+ (): Promise<readonly AppIconPreview[]> => input.listPreviews(),
+ );
+
+ // The dialog runs here and the file it returns is the only path this sees.
+ input.ipcMain.handle('app:importIcon', async (): Promise<AppIconImportResult> => {
+ const picked = await input.showOpenDialog({
+ properties: ['openFile'],
+ // Only what `nativeImage` guarantees on every platform. Offering TIFF
+ // or WebP would let a Windows or Linux user pick a file that then fails
+ // to decode, with the dialog having implied otherwise.
+ filters: [{ name: 'PNG and JPEG', extensions: ['png', 'jpg', 'jpeg'] }],
+ });
+ const sourcePath = picked.canceled ? undefined : picked.filePaths[0];
+ if (!sourcePath) return { ok: false, reason: 'cancelled' };
+ try {
+ return {
+ ok: true,
+ icon: await input.importArtwork({ sourcePath, userDataPath: userDataPath() }),
+ };
+ } catch (error) {
+ return {
+ ok: false,
+ reason: error instanceof CustomAppIconError ? error.reason : 'unreadable',
+ };
+ }
+ });
+
+ input.ipcMain.handle(
+ 'app:removeIcon',
+ async (_event, icon: unknown): Promise<AppIconRemoveResult> => {
+ // Only imported artwork is the user's to delete, and only a well-formed
+ // reference names any of it.
+ if (!isCustomAppIcon(icon)) return { ok: false, reason: 'invalid_id' };
+ const id = customAppIconId(icon);
+ if (!id) return { ok: false, reason: 'invalid_id' };
+
+ const current = await input.settingsStore.get();
+ let selection = toAppIconChoice(current.appearance.appIcon);
+ if (selection === icon) {
+ // Hand the OS back to the brand mark BEFORE the file goes away. The
+ // other order leaves a persisted choice pointing at nothing whenever
+ // this write fails.
+ try {
+ const next = await input.settingsStore.update({ appearance: { appIcon: 'default' } });
Review Comment:
[P2] Thanks for moving this lifecycle into the main process. There is still a small compare-and-set gap here: `get()` can observe the icon as selected, then another renderer or settings-sync path can select a different icon before this unconditional `update()`, causing the stale removal flow to overwrite that newer choice with `default`. The renderer busy flag cannot make this Host-side sequence atomic. Could we use the existing `settingsStore.updateIf()` seam to reset only while the current choice still equals `icon`; if it no longer does, keep the returned latest selection and just delete the now-unselected file. A `get → concurrent select → remove` regression test would pin the final authority without adding another state path.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] feat(desktop): let the user choose or import the app icon [maka]
Posted by "Astro-Han (via GitHub)" <gi...@apache.org>.
Astro-Han commented on code in PR #3431:
URL: https://github.com/apache/maka/pull/3431#discussion_r3836879928
##########
apps/desktop/src/main/app-icon-ipc.ts:
##########
@@ -0,0 +1,213 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import type {
+ IpcMain,
+ OpenDialogOptions,
+ OpenDialogReturnValue,
+} from "electron";
+import {
+ customAppIconId,
+ isAppIconChoice,
+ isCustomAppIcon,
+ toAppIconChoice,
+ type AppIconChoice,
+ type AppSettings,
+} from "@maka/core/settings";
+import type { SettingsStore } from "@maka/storage";
+import type { AppIconPreview } from "./app-icon-surface.js";
+import {
+ CustomAppIconError,
+ listCustomAppIconIds,
+ removeCustomAppIcon,
+ type CustomAppIconImportReason,
+} from "./custom-app-icon-store.js";
+
+export type AppIconImportResult =
+ | { readonly ok: true; readonly icon: AppIconChoice }
+ | { readonly ok: false; readonly reason: CustomAppIconImportReason };
+
+export type AppIconSelectResult =
+ | { readonly ok: true; readonly selection: AppIconChoice }
+ | {
+ readonly ok: false;
+ readonly reason: "invalid_id" | "missing_artwork" | "write_failed";
+ };
+
+export type AppIconRemoveResult =
+ | { readonly ok: true; readonly selection: AppIconChoice }
+ | {
+ readonly ok: false;
+ readonly reason: "invalid_id" | "reset_failed" | "remove_failed";
+ };
+
+/**
+ * The icon surface's one main-process owner.
+ *
+ * Selection and artwork are two pieces of state that must not disagree, so the
+ * operation that can break the pair lives here rather than being sequenced by
+ * the renderer: a renderer that deletes and then persists leaves the setting
+ * pointing at a file that is gone whenever the second call fails.
+ */
+/**
+ * Everything that touches Electron is injected rather than imported: this
+ * module holds the policy, and keeping `electron` out of its import graph is
+ * what lets the removal contract be tested without booting a browser process.
+ */
+export function registerAppIconIpc(input: {
+ readonly ipcMain: Pick<IpcMain, "handle">;
+ readonly showOpenDialog: (
+ options: OpenDialogOptions,
+ ) => Promise<OpenDialogReturnValue>;
+ readonly listPreviews: () => Promise<readonly AppIconPreview[]>;
+ readonly importArtwork: (source: {
+ readonly sourcePath: string;
+ readonly userDataPath: string;
+ }) => Promise<AppIconChoice>;
+ readonly settingsStore: Pick<SettingsStore, "update" | "updateIf">;
+ readonly applySettings: (settings: AppSettings) => Promise<void>;
+ readonly userDataPath: () => string;
+}): void {
+ const { userDataPath } = input;
+
+ // Every operation that can move the selection or the artwork runs here, one
+ // at a time. A compare-and-set around the settings write alone was not
+ // enough: applying the icon yields, and a selection arriving in that window
+ // would be left pointing at a file this sequence is about to delete. Making
+ // the three operations a single serialized owner removes the window itself,
+ // rather than guarding each one that gets noticed.
+ let queue: Promise<unknown> = Promise.resolve();
+ function serialize<T>(operation: () => Promise<T>): Promise<T> {
+ const run = queue.then(operation, operation);
+ queue = run.then(
+ () => undefined,
+ () => undefined,
+ );
+ return run;
+ }
+
+ /**
+ * Selection lives behind this seam too, so it queues with removal instead of
+ * racing it through the generic settings channel. It is also the only place
+ * that can refuse a choice whose artwork is gone — the generic channel would
+ * happily persist an id with nothing behind it.
+ */
+ input.ipcMain.handle("app:selectIcon", (_event, icon: unknown) =>
+ serialize(async (): Promise<AppIconSelectResult> => {
+ if (!isAppIconChoice(icon)) return { ok: false, reason: "invalid_id" };
+ const custom = customAppIconId(icon);
+ if (custom !== undefined) {
+ const present = await listCustomAppIconIds(userDataPath());
+ if (!present.includes(custom))
+ return { ok: false, reason: "missing_artwork" };
+ }
+ try {
+ const settings = await input.settingsStore.update({
+ appearance: { appIcon: icon },
+ });
+ await input.applySettings(settings);
+ return {
+ ok: true,
+ selection: toAppIconChoice(settings.appearance.appIcon),
+ };
+ } catch {
+ return { ok: false, reason: "write_failed" };
+ }
+ }),
+ );
+
+ // The picker asks for the whole set at once; there is no per-id request, so
+ // no id from the renderer ever reaches the filesystem.
+ input.ipcMain.handle(
+ "app:iconPreviews",
+ (): Promise<readonly AppIconPreview[]> => input.listPreviews(),
+ );
+
+ // The dialog runs here and the file it returns is the only path this sees.
+ input.ipcMain.handle("app:importIcon", () =>
Review Comment:
[P3] The file dialog runs inside `serialize`, so the queue is held for as long as the dialog is open — which is user time, not work time. While one window has the picker up, `select` and `remove` from any other window are blocked behind it.
The dialog itself changes no state; only the copy-into-place and the subsequent select do. Awaiting `showOpenDialog` before entering `serialize`, and serializing just the write path, keeps the invariant this owner exists for while removing an unbounded hold. Minor, since the practical case is a single window.
##########
apps/desktop/src/main/app-icon-ipc.ts:
##########
@@ -0,0 +1,213 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import type {
+ IpcMain,
+ OpenDialogOptions,
+ OpenDialogReturnValue,
+} from "electron";
+import {
+ customAppIconId,
+ isAppIconChoice,
+ isCustomAppIcon,
+ toAppIconChoice,
+ type AppIconChoice,
+ type AppSettings,
+} from "@maka/core/settings";
+import type { SettingsStore } from "@maka/storage";
+import type { AppIconPreview } from "./app-icon-surface.js";
+import {
+ CustomAppIconError,
+ listCustomAppIconIds,
+ removeCustomAppIcon,
+ type CustomAppIconImportReason,
+} from "./custom-app-icon-store.js";
+
+export type AppIconImportResult =
+ | { readonly ok: true; readonly icon: AppIconChoice }
+ | { readonly ok: false; readonly reason: CustomAppIconImportReason };
+
+export type AppIconSelectResult =
+ | { readonly ok: true; readonly selection: AppIconChoice }
+ | {
+ readonly ok: false;
+ readonly reason: "invalid_id" | "missing_artwork" | "write_failed";
+ };
+
+export type AppIconRemoveResult =
+ | { readonly ok: true; readonly selection: AppIconChoice }
+ | {
+ readonly ok: false;
+ readonly reason: "invalid_id" | "reset_failed" | "remove_failed";
+ };
+
+/**
+ * The icon surface's one main-process owner.
+ *
+ * Selection and artwork are two pieces of state that must not disagree, so the
+ * operation that can break the pair lives here rather than being sequenced by
+ * the renderer: a renderer that deletes and then persists leaves the setting
+ * pointing at a file that is gone whenever the second call fails.
+ */
+/**
+ * Everything that touches Electron is injected rather than imported: this
+ * module holds the policy, and keeping `electron` out of its import graph is
+ * what lets the removal contract be tested without booting a browser process.
+ */
+export function registerAppIconIpc(input: {
+ readonly ipcMain: Pick<IpcMain, "handle">;
+ readonly showOpenDialog: (
+ options: OpenDialogOptions,
+ ) => Promise<OpenDialogReturnValue>;
+ readonly listPreviews: () => Promise<readonly AppIconPreview[]>;
+ readonly importArtwork: (source: {
+ readonly sourcePath: string;
+ readonly userDataPath: string;
+ }) => Promise<AppIconChoice>;
+ readonly settingsStore: Pick<SettingsStore, "update" | "updateIf">;
+ readonly applySettings: (settings: AppSettings) => Promise<void>;
+ readonly userDataPath: () => string;
+}): void {
+ const { userDataPath } = input;
+
+ // Every operation that can move the selection or the artwork runs here, one
+ // at a time. A compare-and-set around the settings write alone was not
+ // enough: applying the icon yields, and a selection arriving in that window
+ // would be left pointing at a file this sequence is about to delete. Making
+ // the three operations a single serialized owner removes the window itself,
+ // rather than guarding each one that gets noticed.
+ let queue: Promise<unknown> = Promise.resolve();
+ function serialize<T>(operation: () => Promise<T>): Promise<T> {
+ const run = queue.then(operation, operation);
+ queue = run.then(
+ () => undefined,
+ () => undefined,
+ );
+ return run;
+ }
+
+ /**
+ * Selection lives behind this seam too, so it queues with removal instead of
+ * racing it through the generic settings channel. It is also the only place
+ * that can refuse a choice whose artwork is gone — the generic channel would
Review Comment:
[P3] This comment names a gap that the PR then leaves open. It says the seam "is also the only place that can refuse a choice whose artwork is gone — the generic channel would happily persist an id with nothing behind it." The generic channel is still reachable: `clientOwnedSettingsPatch` in `apps/desktop/src/shared/settings-ownership.ts` forwards `appearance` as one whole object, so a well-formed `custom:<32hex>` can be written through `settings:client:update` without queueing behind this owner — from a second window, an older preload, or a direct `settings.update({ appearance: { appIcon } })`.
The id shape is still validated, so the failure is not a malformed value; it is a valid id landing between `remove`'s settings apply and its file deletion, leaving the setting pointing at artwork that no longer exists. `normalizeSettings:700` documents that case and fails over to the brand mark, so the user-visible result is a graceful fallback rather than a broken tile — which is why this is P3 and not higher.
The reason it is still worth closing: the same function already filters `personalization` field by field rather than forwarding the section wholesale. Dropping `appearance.appIcon` there would make `appearance` consistent with its own neighbour and make this comment true as written, without adding another compare-and-set.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] feat(desktop): let the user choose or import the app icon [maka]
Posted by "Astro-Han (via GitHub)" <gi...@apache.org>.
Astro-Han commented on PR #3431:
URL: https://github.com/apache/maka/pull/3431#issuecomment-5382301517
Correction to my review above, since I stated it more strongly than the code supports.
I wrote that `mergeSettings` has exactly one production caller and that the shape gate is therefore unavoidable. That is wrong. `FileSettingsStore.update` and `updateIf` (`packages/storage/src/settings-store.ts:102` and `:120`) merge and write directly; `normalizeSettings` runs on the **read** side, in `readOrCreate`. So a malformed `appIcon` string can reach `settings.json` on disk — it just cannot survive being read back, and the OS surfaces resolve through `toAppIconChoice` regardless, so nothing downstream sees it.
The practical consequence is unchanged and both P3s stand as written: no path-traversal or malformed-value reaches an icon path, and the ownership point at `app-icon-ipc.ts:108` is still the one worth acting on. But "cannot be persisted" and "cannot be read back" are different claims, and I asserted the stronger one without checking `packages/storage`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org