Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Load icons for custom boot entries #91

Merged
merged 3 commits into from Jul 16, 2020
Merged

Load icons for custom boot entries #91

merged 3 commits into from Jul 16, 2020

Conversation

tkashkin
Copy link
Contributor

@tkashkin tkashkin commented Jul 13, 2020

This PR adds support for icons for custom entries (acidanthera/bugtracker#1042).

It should try to load an icon for custom entry from the same directory and fallback to trying to load .VolumeIcon.icns.

For entry with Path = .../\EFI\path\to\bootloader.efi it should try to load .../\EFI\path\to\bootloader.efi.icns.

I'm not familiar with either UEFI programming nor with OpenCore code, so let me know if I somehow managed to do something wrong in a few lines of code.

@@ -459,6 +459,25 @@ OcGetBootEntryIcon (
return Status;
}

if (BootEntry->Type == OC_BOOT_EXTERNAL_OS && BootEntry->PathName != NULL) {
// Try to load the icon from the same path with appended .icns extension
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use EDK II-style comments with extra surrodunging // here and below, i.e.:

//
// Something 
//

@vit9696
Copy link
Contributor

vit9696 commented Jul 13, 2020

Thanks! This looks good to me, @Download-Fritz also does not mind. @usr-sse2 is the change ok to you? If so, we could merge this after the minor codestyle issue is addressed. Other than that, please update the Configuration.tex in regards to the new behaviour for the icon paths. Do not regenerate the PDFs to reduce the conflict possibility though.

Update comments code style
@vit9696
Copy link
Contributor

vit9696 commented Jul 13, 2020

According to documentation we have .icns suffix variant used for custom entries, and root volume icns for everything else. Yet currently the former will fallback to the latter, which is inconsistent. I do not mind, which route we take: fixing up the docs or updating the code to stop using .VolumeIcon.icns, yet please do either.

@tkashkin
Copy link
Contributor Author

@vit9696 I've tried to update docs but I'm very bad at explaining and documenting things.

Yet currently the former will fallback to the latter, which is inconsistent.

It looks like more correct behaviour to me and, more importantly, it does not change current behaviour unless <ENTRY_PATH>.icns actually exists and loads correctly.

@vit9696 vit9696 merged commit a3d191e into acidanthera:master Jul 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants