add webp support for reading APK icon that caused "unknown format" before #90
Conversation
WalkthroughThe changes introduced in this pull request enhance the handling of APK icons in the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
go.mod (1)
3-3: Consider updating the Go version.The current Go version is set to 1.17, which is not the latest version. While this may be intentional for compatibility reasons, it's worth considering an update to a more recent version to take advantage of performance improvements and new features.
apk/apk.go (1)
12-12: LGTM! Consider grouping image format imports.The addition of WebP support through this import statement is correct and aligns with the PR objectives. This change should enable the application to handle WebP format APK icons as intended.
For better organization, consider grouping all image format imports together. You could move the WebP import next to the JPEG and PNG imports:
import ( "archive/zip" "bytes" "fmt" "image" "io" "os" "github.com/shogo82148/androidbinary" - - _ "golang.org/x/image/webp" _ "image/jpeg" // handle jpeg format _ "image/png" // handle png format + _ "golang.org/x/image/webp" // handle webp format )This grouping makes it clearer that these imports are all related to image format support.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (2)
- apk/apk.go (1 hunks)
- go.mod (1 hunks)
🔇 Additional comments (1)
go.mod (1)
5-5: LGTM: New dependency for WebP support.The addition of
golang.org/x/image v0.20.0is appropriate for implementing WebP support as mentioned in the PR objectives. This package provides the necessary functionality for handling WebP images.
| @@ -9,6 +9,7 @@ import ( | |||
| "os" | |||
|
|
|||
| "github.com/shogo82148/androidbinary" | |||
| _ "golang.org/x/image/webp" | |||
There was a problem hiding this comment.
💡 Codebase verification
WebP support is not properly integrated
While the golang.org/x/image/webp package is imported in apk.go, there is no evidence of explicit handling for WebP in image decoding functions. This likely causes the "unknown format" error when processing APKs with WebP icons.
🔗 Analysis chain
Verify WebP support and check for explicit format handling
While this change should theoretically add WebP support, it's important to ensure it fully addresses the issue mentioned in #89.
- Please verify that the application can now successfully read and display WebP icons from APKs.
- Check if there are any other places in the codebase where image formats are explicitly checked or handled. If so, they might need to be updated to include WebP.
Run the following script to search for potential places where image formats are explicitly handled:
If the script returns any relevant results, please review those locations to ensure WebP is properly supported throughout the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for potential explicit image format handling
# Test: Search for image format related keywords
rg --type go -i '\b(image format|png|jpeg|jpg|webp)\b'
Length of output: 256
Script:
#!/bin/bash
# Description: Check if WebP is handled in image decoding functions
# Search for functions that decode images and verify WebP handling
rg --type go 'decode|parse' | rg -i 'webp'
Length of output: 42
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
apk/apk.go (2)
104-104: Remove or address the TODO commentThe TODO comment indicates uncertainty about the implementation's robustness. Either validate the implementation with more test cases or document the specific concerns that need to be addressed.
102-155: Consider handling the background drawableThe current implementation only processes the foreground drawable of the adaptive icon. For complete icon rendering, you might want to consider handling the background drawable as well, as adaptive icons typically consist of both layers.
Would you like me to provide an example implementation that handles both layers?
🧰 Tools
🪛 golangci-lint
135-135: ineffectual assignment to innerForegroundImagePath
(ineffassign)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (2)
apk/apk.go(3 hunks)go.mod(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- go.mod
🧰 Additional context used
🪛 golangci-lint
apk/apk.go
135-135: ineffectual assignment to innerForegroundImagePath
(ineffassign)
🔇 Additional comments (3)
apk/apk.go (3)
6-6: LGTM: WebP support correctly imported
The WebP format support is properly added using the blank identifier import, which is the correct way to register the WebP format decoder.
Also applies to: 14-16
79-86: LGTM: Well-structured AdaptiveIcon definition
The AdaptiveIcon struct is properly defined with correct XML namespace and attributes for Android's adaptive icon format.
16-16: Verify WebP support implementation
The WebP support looks correctly implemented through:
- Registration of the WebP decoder via import
- Usage of
image.Decodewhich automatically handles format detection
Let's verify this implementation:
Also applies to: 156-157
✅ Verification successful
WebP support is properly implemented
The verification confirms correct WebP implementation:
- WebP decoder is registered via blank import in
apk/apk.go - Generic
image.Decodeis used for decoding, which automatically handles format detection - No explicit format checks that could bypass WebP support
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify WebP support implementation
# Test 1: Check if WebP decoder is properly imported
rg --type go "_ \"golang.org/x/image/webp\""
# Test 2: Verify no explicit format checks that might bypass WebP
rg --type go "image\.(PNG|JPEG|GIF)"
# Test 3: Confirm image.Decode is used for generic decoding
rg --type go "image\.Decode"
Length of output: 218
| innerForegroundImagePathTmp, err := k.table.GetResource(resID, nil) | ||
| switch v := innerForegroundImagePathTmp.(type) { | ||
| case string: | ||
| innerForegroundImagePath = v | ||
| default: | ||
| return nil, errorf("failed to get resource %q: %w", adaptiveIcon.Foreground.Drawable, err) | ||
| } | ||
|
|
||
| innerForegroundImagePath = innerForegroundImagePathTmp.(string) | ||
| if err != nil { | ||
| return nil, errorf("failed to get resource %q: %w", adaptiveIcon.Foreground.Drawable, err) | ||
| } |
There was a problem hiding this comment.
Fix redundant error handling and ineffectual assignment
There are several issues in this code segment:
- The assignment at line 140 is ineffectual as the value was already assigned at line 135
- The error check at lines 141-143 is redundant with the type switch error handling above
Apply this diff to fix the issues:
innerForegroundImagePathTmp, err := k.table.GetResource(resID, nil)
switch v := innerForegroundImagePathTmp.(type) {
case string:
innerForegroundImagePath = v
default:
return nil, errorf("failed to get resource %q: %w", adaptiveIcon.Foreground.Drawable, err)
}
-
- innerForegroundImagePath = innerForegroundImagePathTmp.(string)
- if err != nil {
- return nil, errorf("failed to get resource %q: %w", adaptiveIcon.Foreground.Drawable, err)
- }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| innerForegroundImagePathTmp, err := k.table.GetResource(resID, nil) | |
| switch v := innerForegroundImagePathTmp.(type) { | |
| case string: | |
| innerForegroundImagePath = v | |
| default: | |
| return nil, errorf("failed to get resource %q: %w", adaptiveIcon.Foreground.Drawable, err) | |
| } | |
| innerForegroundImagePath = innerForegroundImagePathTmp.(string) | |
| if err != nil { | |
| return nil, errorf("failed to get resource %q: %w", adaptiveIcon.Foreground.Drawable, err) | |
| } | |
| innerForegroundImagePathTmp, err := k.table.GetResource(resID, nil) | |
| switch v := innerForegroundImagePathTmp.(type) { | |
| case string: | |
| innerForegroundImagePath = v | |
| default: | |
| return nil, errorf("failed to get resource %q: %w", adaptiveIcon.Foreground.Drawable, err) | |
| } |
🧰 Tools
🪛 golangci-lint
135-135: ineffectual assignment to innerForegroundImagePath
(ineffassign)
| // read from the inner foreground image location | ||
| imgData, err = k.readZipFile(innerForegroundImagePath) | ||
| if err != nil { | ||
| return nil, errorf("failed to read %q: %w", adaptiveIcon.Foreground, err) |
There was a problem hiding this comment.
Fix error message to use the correct path variable
The error message uses adaptiveIcon.Foreground directly instead of innerForegroundImagePath.
Apply this diff to fix the error message:
- return nil, errorf("failed to read %q: %w", adaptiveIcon.Foreground, err)
+ return nil, errorf("failed to read %q: %w", innerForegroundImagePath, err)Committable suggestion skipped: line range outside the PR's diff.
I had the same problem as in #89 and in my case my APK used an adaptive icon that in the APK ends up as an image encoded using webp format. Adding webp in addition to png and jpg support fixed my problem with "unknown format" for the icon.
Summary by CodeRabbit
golang.org/x/imagelibrary and additional indirect dependencies.