From a5744d0937e1bf4e97d5d14fd1aec7ac41401914 Mon Sep 17 00:00:00 2001 From: Nico Hinderling Date: Thu, 16 Oct 2025 11:43:44 -0700 Subject: [PATCH 1/4] implement proper fix --- .../artifacts/apple/zipped_xcarchive.py | 16 +--- .../apple/alternate_icons_optimization.py | 5 +- .../size/insights/apple/image_optimization.py | 37 ++++++-- .../test_image_optimization_insight.py | 95 ++++++++++++++++++- 4 files changed, 132 insertions(+), 21 deletions(-) diff --git a/src/launchpad/artifacts/apple/zipped_xcarchive.py b/src/launchpad/artifacts/apple/zipped_xcarchive.py index 76b7e411..2c04b3c2 100644 --- a/src/launchpad/artifacts/apple/zipped_xcarchive.py +++ b/src/launchpad/artifacts/apple/zipped_xcarchive.py @@ -341,17 +341,11 @@ def _parse_asset_element(self, item: dict[str, Any], parent_path: Path) -> Asset is_vector = item.get("vector", False) filename = item.get("filename", "") - # Enhanced asset type handling - if asset_type == 1: # Standard PNG image - full_path = parent_path / f"{image_id}.png" - elif asset_type == 2: # JPEG image - full_path = parent_path / f"{image_id}.jpg" - elif asset_type == 3: # PDF/Vector image - full_path = parent_path / f"{image_id}.pdf" - elif asset_type == 4: # HEIF image - full_path = parent_path / f"{image_id}.heic" - else: - full_path = None + full_path = ( + parent_path / f"{image_id}.png" + if filename and Path(filename).suffix.lower() in {".png", ".jpg", ".jpeg", ".heic", ".heif"} + else None + ) return AssetCatalogElement( name=name, diff --git a/src/launchpad/size/insights/apple/alternate_icons_optimization.py b/src/launchpad/size/insights/apple/alternate_icons_optimization.py index 2a182a6c..ca5d3261 100644 --- a/src/launchpad/size/insights/apple/alternate_icons_optimization.py +++ b/src/launchpad/size/insights/apple/alternate_icons_optimization.py @@ -71,6 +71,7 @@ def _resize_icon_for_analysis(self, img: Image.Image) -> Image.Image: ) def _is_alternate_icon_file(self, file_info: FileInfo, alternate_icon_names: set[str]) -> bool: - return file_info.file_type.lower() in self.OPTIMIZABLE_FORMATS and any( - 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 ) diff --git a/src/launchpad/size/insights/apple/image_optimization.py b/src/launchpad/size/insights/apple/image_optimization.py index ac511ce8..0e71a319 100644 --- a/src/launchpad/size/insights/apple/image_optimization.py +++ b/src/launchpad/size/insights/apple/image_optimization.py @@ -14,6 +14,7 @@ from PIL import Image, ImageFile from launchpad.size.insights.insight import Insight, InsightsInput +from launchpad.size.models.apple import AppleAppInfo from launchpad.size.models.common import FileInfo from launchpad.size.models.insights import ( ImageOptimizationInsightResult, @@ -184,18 +185,42 @@ class ImageOptimizationInsight(BaseImageOptimizationInsight): """Analyse image optimisation opportunities in iOS apps.""" def _find_images(self, input: InsightsInput) -> List[FileInfo]: + alternate_icon_names = ( + set(input.app_info.alternate_icon_names) + if isinstance(input.app_info, AppleAppInfo) and input.app_info.alternate_icon_names + else set() + ) + primary_icon_name = input.app_info.primary_icon_name if isinstance(input.app_info, AppleAppInfo) else None + images: List[FileInfo] = [] for fi in input.file_analysis.files: if fi.file_type == "car": - images.extend(c for c in fi.children if self._is_optimizable_image_file(c)) - elif self._is_optimizable_image_file(fi): + images.extend( + c + for c in fi.children + if self._is_optimizable_image_file( + c, alternate_icon_names, primary_icon_name, from_asset_catalog=True + ) + ) + elif self._is_optimizable_image_file(fi, alternate_icon_names, primary_icon_name, from_asset_catalog=False): images.append(fi) return images - def _is_optimizable_image_file(self, file_info: FileInfo) -> bool: + def _is_optimizable_image_file( + self, + file_info: FileInfo, + alternate_icon_names: set[str], + primary_icon_name: str | None, + from_asset_catalog: bool, + ) -> bool: if file_info.file_type.lower() not in self.OPTIMIZABLE_FORMATS: return False - name = Path(file_info.path).name - if name.startswith(("AppIcon", "iMessage App Icon")): + + if any(part.endswith(".stickerpack") for part in file_info.path.split("/")): return False - return not any(part.endswith(".stickerpack") for part in file_info.path.split("/")) + + if not from_asset_catalog: + 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 diff --git a/tests/integration/test_image_optimization_insight.py b/tests/integration/test_image_optimization_insight.py index e350ac06..4555a23d 100644 --- a/tests/integration/test_image_optimization_insight.py +++ b/tests/integration/test_image_optimization_insight.py @@ -137,16 +137,107 @@ def test_generates_optimization_results_for_large_images( assert any("large_unoptimized.png" in path for path in optimizable_paths) assert any("large_photo.jpg" in path for path in optimizable_paths) - def test_excludes_app_icons_and_sticker_packs( + def test_excludes_loose_app_icons_and_sticker_packs( self, insight: ImageOptimizationInsight, insights_input: InsightsInput ) -> None: - """Test that App Icons and sticker pack images are excluded from optimization.""" + """Test that loose AppIcon files and sticker pack images are excluded from optimization.""" result = insight.generate(insights_input) assert result is not None optimizable_paths = {f.file_path for f in result.optimizable_files} + # Loose AppIcon files should be excluded assert not any("AppIcon" in path for path in optimizable_paths) assert not any("stickerpack" in path for path in optimizable_paths) + def test_includes_appicon_display_variants_from_asset_catalogs( + self, insight: ImageOptimizationInsight, temp_images: Dict[str, Any] + ) -> None: + """Test that AppIcon display variants inside asset catalogs ARE included.""" + # Create an asset catalog with an AppIcon display variant + car_path = temp_images["temp_dir"] / "Assets.car" + + # Create an alternate icon display variant (should be included) + alternate_icon_display = temp_images["temp_dir"] / "AppIconFuzz_display.png" + icon_img = Image.new("RGB", (1024, 1024), (255, 0, 0)) + icon_img.save(alternate_icon_display, format="PNG", optimize=False, compress_level=0) + + # Create an actual alternate icon (should be excluded) + actual_icon = temp_images["temp_dir"] / "AppIconFuzz.png" + actual_icon_img = Image.new("RGB", (1024, 1024), (0, 255, 0)) + actual_icon_img.save(actual_icon, format="PNG", optimize=False, compress_level=0) + + # Create a mock asset catalog file entry + car_file_info = FileInfo( + path="Assets.car", + full_path=car_path, + size=1024, + file_type="car", + hash="test_hash", + treemap_type=TreemapType.ASSETS, + is_dir=False, + children=[ + FileInfo( + path="Assets.car/AppIconFuzz_display/AppIconFuzz_display.png", + full_path=alternate_icon_display, + size=alternate_icon_display.stat().st_size, + file_type="png", + hash=calculate_file_hash(alternate_icon_display), + treemap_type=TreemapType.ASSETS, + is_dir=False, + children=[], + ), + FileInfo( + path="Assets.car/AppIconFuzz/AppIconFuzz.png", + full_path=actual_icon, + size=actual_icon.stat().st_size, + file_type="png", + hash=calculate_file_hash(actual_icon), + treemap_type=TreemapType.ASSETS, + is_dir=False, + children=[], + ), + ], + ) + + file_analysis = FileAnalysis(files=[car_file_info], directories=[]) + + test_input = InsightsInput( + app_info=AppleAppInfo( + name="TestApp", + app_id="com.test.app", + version="1.0", + build="1", + executable="TestApp", + minimum_os_version="15.0", + supported_platforms=["iphoneos"], + sdk_version=None, + is_simulator=False, + codesigning_type=None, + profile_name=None, + profile_expiration_date=None, + certificate_expiration_date=None, + main_binary_uuid=None, + is_code_signature_valid=True, + code_signature_errors=[], + primary_icon_name=None, + alternate_icon_names=["AppIconFuzz"], # This is the actual icon + ), + file_analysis=file_analysis, + binary_analysis=[], + treemap=None, + hermes_reports={}, + ) + + result = insight.generate(test_input) + assert result is not None + assert len(result.optimizable_files) > 0 + + # The display variant should be included, but not the actual icon + optimizable_paths = {f.file_path for f in result.optimizable_files} + assert any("AppIconFuzz_display" in path for path in optimizable_paths), "Display variant should be included" + assert not any(path.endswith("AppIconFuzz/AppIconFuzz.png") for path in optimizable_paths), ( + "Actual icon should be excluded" + ) + def test_respects_minimum_savings_threshold( self, insight: ImageOptimizationInsight, insights_input: InsightsInput ) -> None: From ea157b8ebe1485e6103d154aa3688a16c507962b Mon Sep 17 00:00:00 2001 From: Nico Hinderling Date: Thu, 16 Oct 2025 11:59:45 -0700 Subject: [PATCH 2/4] CI --- .../artifacts/apple/zipped_xcarchive.py | 10 +++---- .../unit/test_alternate_icons_optimization.py | 28 ++++++------------- 2 files changed, 14 insertions(+), 24 deletions(-) diff --git a/src/launchpad/artifacts/apple/zipped_xcarchive.py b/src/launchpad/artifacts/apple/zipped_xcarchive.py index 2c04b3c2..d916543d 100644 --- a/src/launchpad/artifacts/apple/zipped_xcarchive.py +++ b/src/launchpad/artifacts/apple/zipped_xcarchive.py @@ -341,11 +341,11 @@ def _parse_asset_element(self, item: dict[str, Any], parent_path: Path) -> Asset is_vector = item.get("vector", False) filename = item.get("filename", "") - full_path = ( - parent_path / f"{image_id}.png" - if filename and Path(filename).suffix.lower() in {".png", ".jpg", ".jpeg", ".heic", ".heif"} - else None - ) + if filename and Path(filename).suffix.lower() in {".png", ".jpg", ".jpeg", ".heic", ".heif"}: + potential_path = parent_path / f"{image_id}.png" + full_path = potential_path if potential_path.exists() else None + else: + full_path = None return AssetCatalogElement( name=name, diff --git a/tests/unit/test_alternate_icons_optimization.py b/tests/unit/test_alternate_icons_optimization.py index 0023ca31..751ded75 100644 --- a/tests/unit/test_alternate_icons_optimization.py +++ b/tests/unit/test_alternate_icons_optimization.py @@ -129,7 +129,7 @@ def test_identifies_alternate_icons(self): # Primary icon - should be excluded FileInfo( full_path=primary_icon_path, - path="Assets.car/AppIcon-60@2x", + path="Assets.car/AppIcon.png", size=primary_icon_path.stat().st_size, file_type="png", treemap_type=TreemapType.ASSETS, @@ -139,7 +139,7 @@ def test_identifies_alternate_icons(self): # Alternate icons - should be included FileInfo( full_path=alt_icon1_path, - path="Assets.car/DarkIcon-60@2x", + path="Assets.car/DarkIcon.png", size=alt_icon1_path.stat().st_size, file_type="png", treemap_type=TreemapType.ASSETS, @@ -148,7 +148,7 @@ def test_identifies_alternate_icons(self): ), FileInfo( full_path=alt_icon2_path, - path="Assets.car/LightIcon-60@2x", + path="Assets.car/LightIcon.png", size=alt_icon2_path.stat().st_size, file_type="png", treemap_type=TreemapType.ASSETS, @@ -215,7 +215,7 @@ def test_excludes_primary_icon(self): children=[ FileInfo( full_path=primary_icon_path, - path="Assets.car/PrimaryIcon-60@2x", + path="Assets.car/PrimaryIcon.png", size=primary_icon_path.stat().st_size, file_type="png", treemap_type=TreemapType.ASSETS, @@ -224,7 +224,7 @@ def test_excludes_primary_icon(self): ), FileInfo( full_path=alt_icon_path, - path="Assets.car/AlternateIcon-60@2x", + path="Assets.car/AlternateIcon.png", size=alt_icon_path.stat().st_size, file_type="png", treemap_type=TreemapType.ASSETS, @@ -321,7 +321,7 @@ def test_minimum_savings_threshold(self): children=[ FileInfo( full_path=small_icon_path, - path="Assets.car/TinyIcon-16", + path="Assets.car/TinyIcon.png", size=small_icon_path.stat().st_size, file_type="png", treemap_type=TreemapType.ASSETS, @@ -376,30 +376,20 @@ def test_icon_name_matching(self): hash="hash_car", is_dir=False, children=[ - # Should match - starts with CustomIcon + # Should match - CustomIcon FileInfo( full_path=icon1_path, - path="Assets.car/CustomIcon-60", + path="Assets.car/CustomIcon.png", size=icon1_path.stat().st_size, file_type="png", treemap_type=TreemapType.ASSETS, hash="hash1", is_dir=False, ), - # Should match - starts with CustomIcon - FileInfo( - full_path=icon2_path, - path="Assets.car/CustomIcon-60@2x", - size=icon2_path.stat().st_size, - file_type="png", - treemap_type=TreemapType.ASSETS, - hash="hash2", - is_dir=False, - ), # Should not match - different name FileInfo( full_path=icon3_path, - path="Assets.car/OtherIcon-60", + path="Assets.car/OtherIcon.png", size=icon3_path.stat().st_size, file_type="png", treemap_type=TreemapType.ASSETS, From 9628179f0095e25f4d12c0842653713865f6274a Mon Sep 17 00:00:00 2001 From: Nico Hinderling Date: Thu, 16 Oct 2025 13:37:24 -0700 Subject: [PATCH 3/4] more feedback --- .../size/insights/apple/image_optimization.py | 19 +++++------ .../unit/test_alternate_icons_optimization.py | 32 +++++++++---------- 2 files changed, 24 insertions(+), 27 deletions(-) diff --git a/src/launchpad/size/insights/apple/image_optimization.py b/src/launchpad/size/insights/apple/image_optimization.py index 0e71a319..01c1cf3c 100644 --- a/src/launchpad/size/insights/apple/image_optimization.py +++ b/src/launchpad/size/insights/apple/image_optimization.py @@ -185,12 +185,11 @@ class ImageOptimizationInsight(BaseImageOptimizationInsight): """Analyse image optimisation opportunities in iOS apps.""" def _find_images(self, input: InsightsInput) -> List[FileInfo]: + app_info = input.app_info if isinstance(input.app_info, AppleAppInfo) else None alternate_icon_names = ( - set(input.app_info.alternate_icon_names) - if isinstance(input.app_info, AppleAppInfo) and input.app_info.alternate_icon_names - else set() + set(app_info.alternate_icon_names) if app_info and app_info.alternate_icon_names else set() ) - primary_icon_name = input.app_info.primary_icon_name if isinstance(input.app_info, AppleAppInfo) else None + primary_icon_name = app_info.primary_icon_name if app_info else None images: List[FileInfo] = [] for fi in input.file_analysis.files: @@ -198,11 +197,9 @@ def _find_images(self, input: InsightsInput) -> List[FileInfo]: images.extend( c for c in fi.children - if self._is_optimizable_image_file( - c, alternate_icon_names, primary_icon_name, from_asset_catalog=True - ) + if self._is_optimizable_image_file(c, alternate_icon_names, primary_icon_name) ) - elif self._is_optimizable_image_file(fi, alternate_icon_names, primary_icon_name, from_asset_catalog=False): + elif self._is_optimizable_image_file(fi, alternate_icon_names, primary_icon_name): images.append(fi) return images @@ -211,7 +208,6 @@ def _is_optimizable_image_file( file_info: FileInfo, alternate_icon_names: set[str], primary_icon_name: str | None, - from_asset_catalog: bool, ) -> bool: if file_info.file_type.lower() not in self.OPTIMIZABLE_FORMATS: return False @@ -219,8 +215,9 @@ def _is_optimizable_image_file( if any(part.endswith(".stickerpack") for part in file_info.path.split("/")): return False - if not from_asset_catalog: - return not Path(file_info.path).name.startswith(("AppIcon", "iMessage App Icon")) + # We can't guarantee that iMessage app's will have their icons specified in the Info.plist, so be conservative here + if Path(file_info.path).name.startswith("iMessage App Icon"): + return False stem = Path(file_info.path).stem return stem != primary_icon_name and stem not in alternate_icon_names diff --git a/tests/unit/test_alternate_icons_optimization.py b/tests/unit/test_alternate_icons_optimization.py index 751ded75..3b655d0c 100644 --- a/tests/unit/test_alternate_icons_optimization.py +++ b/tests/unit/test_alternate_icons_optimization.py @@ -112,7 +112,7 @@ def test_identifies_alternate_icons(self): executable="TestApp", minimum_os_version="14.0", primary_icon_name="AppIcon", - alternate_icon_names=["DarkIcon", "LightIcon"], + alternate_icon_names=["DarkIcon-60@2x", "LightIcon-60@2x"], ) # Simulate asset catalog with primary and alternate icons @@ -129,7 +129,7 @@ def test_identifies_alternate_icons(self): # Primary icon - should be excluded FileInfo( full_path=primary_icon_path, - path="Assets.car/AppIcon.png", + path="Assets.car/AppIcon-60@2x.png", size=primary_icon_path.stat().st_size, file_type="png", treemap_type=TreemapType.ASSETS, @@ -139,7 +139,7 @@ def test_identifies_alternate_icons(self): # Alternate icons - should be included FileInfo( full_path=alt_icon1_path, - path="Assets.car/DarkIcon.png", + path="Assets.car/DarkIcon-60@2x.png", size=alt_icon1_path.stat().st_size, file_type="png", treemap_type=TreemapType.ASSETS, @@ -148,7 +148,7 @@ def test_identifies_alternate_icons(self): ), FileInfo( full_path=alt_icon2_path, - path="Assets.car/LightIcon.png", + path="Assets.car/LightIcon-60@2x.png", size=alt_icon2_path.stat().st_size, file_type="png", treemap_type=TreemapType.ASSETS, @@ -176,9 +176,9 @@ def test_identifies_alternate_icons(self): # Verify correct icons are included paths = [f.file_path for f in result.optimizable_files] - assert any("DarkIcon" in p for p in paths) - assert any("LightIcon" in p for p in paths) - assert not any("AppIcon" in p for p in paths) + assert any("DarkIcon-60@2x" in p for p in paths) + assert any("LightIcon-60@2x" in p for p in paths) + assert not any("PrimaryIcon-60@2x" in p for p in paths) finally: # Clean up test files @@ -199,8 +199,8 @@ def test_excludes_primary_icon(self): app_id="com.test.app", executable="TestApp", minimum_os_version="14.0", - primary_icon_name="PrimaryIcon", - alternate_icon_names=["AlternateIcon"], + primary_icon_name="PrimaryIcon-60@2x", + alternate_icon_names=["AlternateIcon-60@2x"], ) files = [ @@ -215,7 +215,7 @@ def test_excludes_primary_icon(self): children=[ FileInfo( full_path=primary_icon_path, - path="Assets.car/PrimaryIcon.png", + path="Assets.car/PrimaryIcon-60@2x.png", size=primary_icon_path.stat().st_size, file_type="png", treemap_type=TreemapType.ASSETS, @@ -224,7 +224,7 @@ def test_excludes_primary_icon(self): ), FileInfo( full_path=alt_icon_path, - path="Assets.car/AlternateIcon.png", + path="Assets.car/AlternateIcon-60@2x.png", size=alt_icon_path.stat().st_size, file_type="png", treemap_type=TreemapType.ASSETS, @@ -306,7 +306,7 @@ def test_minimum_savings_threshold(self): executable="TestApp", minimum_os_version="14.0", primary_icon_name="AppIcon", - alternate_icon_names=["TinyIcon"], + alternate_icon_names=["TinyIcon-16"], ) files = [ @@ -321,7 +321,7 @@ def test_minimum_savings_threshold(self): children=[ FileInfo( full_path=small_icon_path, - path="Assets.car/TinyIcon.png", + path="Assets.car/TinyIcon-16.png", size=small_icon_path.stat().st_size, file_type="png", treemap_type=TreemapType.ASSETS, @@ -363,7 +363,7 @@ def test_icon_name_matching(self): executable="TestApp", minimum_os_version="14.0", primary_icon_name="AppIcon", - alternate_icon_names=["CustomIcon"], + alternate_icon_names=["CustomIcon-60"], ) files = [ @@ -379,7 +379,7 @@ def test_icon_name_matching(self): # Should match - CustomIcon FileInfo( full_path=icon1_path, - path="Assets.car/CustomIcon.png", + path="Assets.car/CustomIcon-60.png", size=icon1_path.stat().st_size, file_type="png", treemap_type=TreemapType.ASSETS, @@ -389,7 +389,7 @@ def test_icon_name_matching(self): # Should not match - different name FileInfo( full_path=icon3_path, - path="Assets.car/OtherIcon.png", + path="Assets.car/OtherIcon-60.png", size=icon3_path.stat().st_size, file_type="png", treemap_type=TreemapType.ASSETS, From 902310b243475c14618369cf5739d3d937d6f143 Mon Sep 17 00:00:00 2001 From: Nico Hinderling Date: Thu, 16 Oct 2025 15:07:02 -0700 Subject: [PATCH 4/4] keep asset catalog check --- .../size/insights/apple/image_optimization.py | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/launchpad/size/insights/apple/image_optimization.py b/src/launchpad/size/insights/apple/image_optimization.py index 01c1cf3c..2e8171b6 100644 --- a/src/launchpad/size/insights/apple/image_optimization.py +++ b/src/launchpad/size/insights/apple/image_optimization.py @@ -197,9 +197,11 @@ def _find_images(self, input: InsightsInput) -> List[FileInfo]: images.extend( c for c in fi.children - if self._is_optimizable_image_file(c, alternate_icon_names, primary_icon_name) + if self._is_optimizable_image_file( + c, alternate_icon_names, primary_icon_name, from_asset_catalog=True + ) ) - elif self._is_optimizable_image_file(fi, alternate_icon_names, primary_icon_name): + elif self._is_optimizable_image_file(fi, alternate_icon_names, primary_icon_name, from_asset_catalog=False): images.append(fi) return images @@ -208,6 +210,7 @@ def _is_optimizable_image_file( file_info: FileInfo, alternate_icon_names: set[str], primary_icon_name: str | None, + from_asset_catalog: bool, ) -> bool: if file_info.file_type.lower() not in self.OPTIMIZABLE_FORMATS: return False @@ -216,8 +219,8 @@ def _is_optimizable_image_file( return False # We can't guarantee that iMessage app's will have their icons specified in the Info.plist, so be conservative here - if Path(file_info.path).name.startswith("iMessage App Icon"): - return False + if not from_asset_catalog: + 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