You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@maka.apache.org by "shengfuandqin-stack (via GitHub)" <gi...@apache.org> on 2026/09/10 05:36:42 UTC
[PR] fix(desktop): hand the Windows taskbar the rebuilt ICO as a file path [maka]
shengfuandqin-stack opened a new pull request, #5131:
URL: https://github.com/apache/maka/pull/5131
## Summary
Settings -> Appearance can show a chosen app icon while the Windows taskbar keeps the packaged `sky` tile. `BrowserWindow.setIcon` and the window `icon` option take a NativeImage or a file path; a NativeImage built from the 1024px PNG master is valid, but it does not give `WM_SETICON` the small (16) and large (32) HICONs it asks for, so the taskbar stays on the icon the `.exe` carries.
Hand Windows a path to a rebuilt multi-size ICO instead:
- `encodeWindowsIco` packs 16/24/32/48/64/256 PNG frames into a PNG-in-ICO container (pure function, unit-tested).
- `windowsTaskbarIconFile` writes that ICO to `userData/app-icon-cache/taskbar.ico` and returns the path.
- `readableAppIconPath` resolves to the ICO path on win32 and keeps returning the 1024px PNG master path on macOS and Linux, so no caller changes.
The ICO cannot travel as bytes: `nativeImage` only decodes PNG/JPG, so `createFromBuffer` of an ICO returns an EMPTY image (verified against Electron 43.4.1). Every failure -- unreadable art, an encode or write error -- falls back to the PNG master rather than to a path that decodes to nothing.
## Verification
- `npm --workspace @maka/desktop run build:main` -- pass
- `node --test dist/main/__tests__/app-icon.test.js dist/main/__tests__/client-settings-effects.test.js dist/main/__tests__/startup-progress-window.test.js` -- 20 pass, including the new ICO-frame contract
- `npm --workspace @maka/desktop run typecheck` -- pass
- `npx biome check` on the changed files -- clean
- **Manual, Windows 11, dev build (`npm run dev`)**: switching the icon under Settings -> Appearance updates the taskbar icon, and the run writes `%APPDATA%/Maka Dev/app-icon-cache/taskbar.ico`. The file loads in the Windows icon loader with 16/24/32/48/64/256 frames present.
Not run: a packaged Windows installer, and a live capture against a signed `.exe`. The husky pre-commit Biome stdin hook cannot spawn `biome.cmd` on this Windows shell (`EINVAL`); Biome was run directly on the changed files instead.
## AI use
Select exactly one:
- [ ] No generative tool made a substantive contribution
- [x] Generative tooling made a substantive contribution
Tool(s) and scope: DeepSeek Harness -- located the `setIcon` / HICON gap, wrote the ICO rebuild, the cache write, the tests and this PR text under my direction.
### Screenshots
Before / after of the taskbar tile, plus the picker, to follow -- Windows UI changes are reviewed as diffs otherwise, and the taskbar is the surface this PR is about. (Will attach from my Windows 11 machine.)
## Checklist
- [x] Tests cover the change and fail without it
- [x] Lint, format, typecheck and the affected suites pass locally
Does this PR entail a change in behavior?
- [x] Yes -- described under Summary above
- [ ] No
--
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] fix(desktop): hand the Windows taskbar the rebuilt ICO as a file path [maka]
Posted by "shengfuandqin-stack (via GitHub)" <gi...@apache.org>.
shengfuandqin-stack closed pull request #5131: fix(desktop): hand the Windows taskbar the rebuilt ICO as a file path
URL: https://github.com/apache/maka/pull/5131
--
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] fix(desktop): hand the Windows taskbar the rebuilt ICO as a file path [maka]
Posted by "shengfuandqin-stack (via GitHub)" <gi...@apache.org>.
shengfuandqin-stack commented on PR #5131:
URL: https://github.com/apache/maka/pull/5131#issuecomment-5614610744
Correction to my previous note, plus a follow-up commit (`4724012`).
The previous note claimed the cache-failure path was "covered without needing an Electron process". That overstated it: the extracted cache writer was covered, but nothing asserted the caller's fallback. Specifically, no test showed that a Windows window is handed the rebuilt `.ico` path, that a cache failure leaves it on the PNG master, or that missing artwork never reaches `setIcon`. A plain `node --test` process cannot even load `app-icon-surface.ts` — Electron's package entry exports a path to the binary rather than `app`, so the import fails before any assertion runs — which is why the chain went untested.
`app-icon-apply.test.ts` now drives the real module against a faked Electron with `process.platform` pinned, so the assertions are about the platform's branch rather than about the machine running them (CI is Linux). That requires `--experimental-test-module-mocks` on the workspace `test:dist` script — flagging it explicitly, since it changes how this workspace's tests run. If you would rather not enable an experimental flag, say so and I will move the same three assertions behind an injected-dependency seam instead.
The three assertions fail when the fix is mutated back — guard removed, ICO branch skipped, cache failure returning a path — and pass on this head.
One more honest note on the earlier suite claim: the full desktop run reports the same 36 pre-existing failures as HEAD, but that set wobbles by ±2 between identical runs on this machine (flaky members in the WorkHub / MCP Host form / Rive / managed-host families, none of them icon-related). Treat any single count as approximate.
Screenshots are still outstanding and I have not verified the packaged build.
--
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] fix(desktop): hand the Windows taskbar the rebuilt ICO as a file path [maka]
Posted by "shengfuandqin-stack (via GitHub)" <gi...@apache.org>.
shengfuandqin-stack commented on PR #5131:
URL: https://github.com/apache/maka/pull/5131#issuecomment-5614345358
Follow-up pushed as `daa5428`, on top of the first commit rather than rewriting it, so the earlier one stays reviewable.
**1. The window path had lost its "nothing readable" guard.** The first commit kept the readability check only in the dock branch, so `applyAppIcon` on Windows/Linux handed `setIcon` the fallback path even when no candidate decoded — and an unreadable path blanks the icon a window already has instead of leaving it alone. Resolving the surface can now answer "nothing reads" (`appIconSurfacePath` returns `null`), so that guard is back; window creation keeps the string form, because a window being created has no previous icon to preserve and cannot report an error. Windows still receives the rebuilt multi-size ICO on the happy path.
**2. The ICO test could not have failed.** Its frames were the strings `png-16`, `png-24`, …, so it only walked the container and would have passed with the fix in `app-icon-surface.ts` reverted. The frames are now real PNGs built at the size each directory entry declares, with byte-exact payload and IHDR assertions; and the cache write moved into `app-icon-cache.ts`, where "the write failed, so fall back to the PNG master" is covered without needing an Electron process.
Verification of this commit:
- `node --test` on both icon suites: 10/10, including the three new/changed cases
- the new tests fail when the fix is mutated back — rebuilt and re-run, 2 failures
- `build:main`, `typecheck` and `biome lint` clean
- full desktop main suite: 36 failures, exactly the same count at HEAD before this change (WorkHub / MCP Host form / Rive / managed host; none of them touch the icon modules)
Files touched are now `app-icon.ts`, `app-icon-surface.ts`, `app-icon-cache.ts` (new), `app-icon.test.ts`, `app-icon-cache.test.ts` (new).
Still outstanding, unchanged: **before/after taskbar screenshots** for CONTRIBUTING §87, which need a real desktop session. The switch → taskbar update itself was verified on a Windows 11 dev build; the screenshots are for the record.
CI: the `CI` run on this head is `action_required` (a maintainer approves the workflow for a first-time contributor); `label` passed on the first push.
--
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