Image Optimization: Ensure AppIcon exclusion logic isn't too aggressive#418
Image Optimization: Ensure AppIcon exclusion logic isn't too aggressive#418NicoHinderling merged 4 commits intomainfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #418 +/- ##
==========================================
+ Coverage 80.48% 80.56% +0.07%
==========================================
Files 147 147
Lines 11917 11935 +18
Branches 1211 1209 -2
==========================================
+ Hits 9591 9615 +24
+ Misses 1885 1882 -3
+ Partials 441 438 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
NicoHinderling
force-pushed
the
missing-image-optimization-insights
branch
from
e73ec1e to
a5744d0
Compare
| parent_path / f"{image_id}.png" | ||
| if filename and Path(filename).suffix.lower() in {".png", ".jpg", ".jpeg", ".heic", ".heif"} | ||
| else None | ||
| ) |
There was a problem hiding this comment.
FYI: we were never parsing PDFs here correctly because they are never included in ParsedAssets and after talking with @noahsmartin I had a wip PR to add them but he pointed out that it isn't current functionality in emerge so probably not worth spending much time on that, so i punted
| Path(file_info.path).stem.startswith(name) for name in alternate_icon_names | ||
| return ( | ||
| file_info.file_type.lower() in self.OPTIMIZABLE_FORMATS | ||
| and Path(file_info.path).stem in alternate_icon_names |
There was a problem hiding this comment.
dont include images just because "AppIcon" is a prefix in the image's name
| return not Path(file_info.path).name.startswith(("AppIcon", "iMessage App Icon")) | ||
|
|
||
| stem = Path(file_info.path).stem | ||
| return stem != primary_icon_name and stem not in alternate_icon_names |
There was a problem hiding this comment.
don't include any images that should be considered for the "alternative icon" insight for the "image optimization" insight
| return False | ||
| return not any(part.endswith(".stickerpack") for part in file_info.path.split("/")) | ||
|
|
||
| if not from_asset_catalog: |
There was a problem hiding this comment.
Is from_asset_catalog needed? Seems like an edge case if someone put in a top-level AppIcon
There was a problem hiding this comment.
yes, otherwise I can remove it actually. I don't think we need the app icon check anymore since I've included the primary_icon_name and alternate_icon_names values into the context, but ill keep the imessage one just in case, since i guess imessage apps are structured slightly differently and don't have a guarantee of a CFBundlePrimaryIcon value in their info.plist
There was a problem hiding this comment.
turns out we do need to keep this actually. found some corner cases between testing patreon and hackernews.
2 participants
The previous logic excluded all of these images from being considered eligible for the image optimization insight