Skip to content

只在未被打包为 App 时设置 macOS Dock 栏图标#4698

Merged
Glavo merged 7 commits intoHMCL-dev:mainfrom
AnemoFlower:macos-set-icon
Oct 27, 2025
Merged

只在未被打包为 App 时设置 macOS Dock 栏图标#4698
Glavo merged 7 commits intoHMCL-dev:mainfrom
AnemoFlower:macos-set-icon

Conversation

@AnemoFlower
Copy link
Copy Markdown
Contributor

@AnemoFlower AnemoFlower commented Oct 19, 2025

不在 HMCL 被打包为 App 时设置 macOS Dock 栏图标,以支持 ShulkerSakura/HMCL-MacOS 自定义图标。

Copilot AI review requested due to automatic review settings October 19, 2025 02:08
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

为 macOS 引入环境变量 HMCL_MACOS_SET_ICON,用于控制是否设置 Dock 图标,便于 HMCL-MacOS 自定义图标集成。

  • 在 EntryPoint 中仅当 macOS 且 HMCL_MACOS_SET_ICON=true 时才调用 initIcon
  • 在中文调试文档中新增 HMCL_MACOS_SET_ICON 的说明,默认值为 true

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
docs/Debug_zh.md 新增 HMCL_MACOS_SET_ICON 环境变量的文档条目,说明默认值
HMCL/src/main/java/org/jackhuang/hmcl/EntryPoint.java 使用环境变量开关控制 macOS 下 initIcon 的执行

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Copy link
Copy Markdown
Member

@burningtnt burningtnt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot 的建议是对的。为避免与“应用图标/文件图标”混淆,建议明确为“程序坞图标(Dock)”。当然,不区分大小写这种东西…… 你得看看 HMCL 其他地方支不支持大小写混用

@Glavo
Copy link
Copy Markdown
Member

Glavo commented Oct 22, 2025

我感觉可以不用一个环境变量来控制,而是让 HMCL 主动检测自己是否被打包为了 dmg 等格式。

@AnemoFlower AnemoFlower changed the title 添加 HMCL_MACOS_SET_ICON 环境变量 只在未打包为 App 时设置 macOS Dock 栏图标 Oct 23, 2025
@AnemoFlower
Copy link
Copy Markdown
Contributor Author

当前逻辑为:如果 HMCL 目录路径中包含 Contents,且其前一个路径组件以 .app 结尾,则不会设置 macOS Dock 栏图标。

@WhatDamon
Copy link
Copy Markdown
Contributor

当前逻辑为:如果 HMCL 目录路径中包含 Contents,且其前一个路径组件以 .app 结尾,则不会设置 macOS Dock 栏图标。

可以进一步添加一个逻辑,检查Contents目录是否包含Info.plist,没有该文件macOS无法正确处理App Bundle,并且让此检测逻辑只在macOS下生效,避免未来与此更改有关的Issue(虽然概率极低)

@AnemoFlower AnemoFlower changed the title 只在未打包为 App 时设置 macOS Dock 栏图标 只在未被打包为 App 时设置 macOS Dock 栏图标 Oct 25, 2025
}

private static boolean isMacDockIconDisabled() {
Path directory = JarUtils.thisJarPath();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

为什么要叫 directory?这明明是 JAR 的路径。

return false;

for (int i = 1; i < directory.getNameCount(); i++) {
Path subpath = directory.getRoot().resolve(directory.subpath(0, i));
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

为什么要这样遍历,而不是从 JAR 开始逐级向父目录查询?

@AnemoFlower AnemoFlower requested a review from Glavo October 25, 2025 13:18
@Glavo Glavo requested a review from Copilot October 27, 2025 13:25
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.


while (jarPath != null && jarPath.getParent() != null) {
if ("Contents".equals(jarPath.getFileName().toString())
&& jarPath.getParent().getFileName().toString().endsWith(".app")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这里不安全,要是 getParent() 返回 / 的话,那么它的 getFileName() 会返回 null,这里就会抛出 NPE 了。

@Glavo Glavo requested a review from Copilot October 27, 2025 13:36
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +171 to +174
current != null && current.getParent() != null;
current = current.getParent()
) {
if ("Contents".equals(FileUtils.getName(current))
Copy link

Copilot AI Oct 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The loop condition current.getParent() != null may cause the loop to terminate prematurely when checking paths near the root directory. If the JAR is located in /Applications/MyApp.app/Contents/Java/app.jar, the loop should check the MyApp.app level, but it will stop before reaching it because it requires the parent of MyApp.app (which is /Applications) to also have a parent. Remove the current.getParent() != null condition from the loop.

Suggested change
current != null && current.getParent() != null;
current = current.getParent()
) {
if ("Contents".equals(FileUtils.getName(current))
current != null;
current = current.getParent()
) {
if ("Contents".equals(FileUtils.getName(current))
&& current.getParent() != null

Copilot uses AI. Check for mistakes.
Comment on lines +174 to +175
if ("Contents".equals(FileUtils.getName(current))
&& FileUtils.getName(current.getParent()).endsWith(".app")
Copy link

Copilot AI Oct 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potential NullPointerException when calling current.getParent() on line 175. Although the loop condition checks current != null, it doesn't guarantee that current.getParent() is non-null when the condition on line 174 is true. Add a null check before accessing current.getParent().

Suggested change
if ("Contents".equals(FileUtils.getName(current))
&& FileUtils.getName(current.getParent()).endsWith(".app")
Path parent = current.getParent();
if (parent != null
&& "Contents".equals(FileUtils.getName(current))
&& FileUtils.getName(parent).endsWith(".app")

Copilot uses AI. Check for mistakes.
@Glavo Glavo merged commit 322e296 into HMCL-dev:main Oct 27, 2025
2 checks passed
@AnemoFlower AnemoFlower deleted the macos-set-icon branch March 2, 2026 07:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants