Skip to content

Commit 7e3d1ba

Browse files
NicoHinderlingNicoHinderling
authored
Image Optimization: Ensure AppIcon exclusion logic isn't too aggressive (#418)
* implement proper fix * CI * more feedback * keep asset catalog check
1 parent a6f1ff8 commit 7e3d1ba

File tree

5 files changed

+147
-46
lines changed

5 files changed

+147
-46
lines changed

‎src/launchpad/artifacts/apple/zipped_xcarchive.py‎

Lines changed: 3 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -341,15 +341,9 @@ def _parse_asset_element(self, item: dict[str, Any], parent_path: Path) -> Asset
341341
is_vector = item.get("vector", False)
342342
filename = item.get("filename", "")
343343

344-
# Enhanced asset type handling
345-
if asset_type == 1: # Standard PNG image
346-
full_path = parent_path / f"{image_id}.png"
347-
elif asset_type == 2: # JPEG image
348-
full_path = parent_path / f"{image_id}.jpg"
349-
elif asset_type == 3: # PDF/Vector image
350-
full_path = parent_path / f"{image_id}.pdf"
351-
elif asset_type == 4: # HEIF image
352-
full_path = parent_path / f"{image_id}.heic"
344+
if filename and Path(filename).suffix.lower() in {".png", ".jpg", ".jpeg", ".heic", ".heif"}:
345+
potential_path = parent_path / f"{image_id}.png"
346+
full_path = potential_path if potential_path.exists() else None
353347
else:
354348
full_path = None
355349

‎src/launchpad/size/insights/apple/alternate_icons_optimization.py‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,7 @@ def _resize_icon_for_analysis(self, img: Image.Image) -> Image.Image:
7171
)
7272

7373
def _is_alternate_icon_file(self, file_info: FileInfo, alternate_icon_names: set[str]) -> bool:
74-
return file_info.file_type.lower() in self.OPTIMIZABLE_FORMATS and any(
75-
Path(file_info.path).stem.startswith(name) for name in alternate_icon_names
74+
return (
75+
file_info.file_type.lower() in self.OPTIMIZABLE_FORMATS
76+
and Path(file_info.path).stem in alternate_icon_names
7677
)

‎src/launchpad/size/insights/apple/image_optimization.py‎

Lines changed: 31 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
from PIL import Image, ImageFile
1515

1616
from launchpad.size.insights.insight import Insight, InsightsInput
17+
from launchpad.size.models.apple import AppleAppInfo
1718
from launchpad.size.models.common import FileInfo
1819
from launchpad.size.models.insights import (
1920
ImageOptimizationInsightResult,
@@ -184,18 +185,42 @@ class ImageOptimizationInsight(BaseImageOptimizationInsight):
184185
"""Analyse image optimisation opportunities in iOS apps."""
185186

186187
def _find_images(self, input: InsightsInput) -> List[FileInfo]:
188+
app_info = input.app_info if isinstance(input.app_info, AppleAppInfo) else None
189+
alternate_icon_names = (
190+
set(app_info.alternate_icon_names) if app_info and app_info.alternate_icon_names else set()
191+
)
192+
primary_icon_name = app_info.primary_icon_name if app_info else None
193+
187194
images: List[FileInfo] = []
188195
for fi in input.file_analysis.files:
189196
if fi.file_type == "car":
190-
images.extend(c for c in fi.children if self._is_optimizable_image_file(c))
191-
elif self._is_optimizable_image_file(fi):
197+
images.extend(
198+
c
199+
for c in fi.children
200+
if self._is_optimizable_image_file(
201+
c, alternate_icon_names, primary_icon_name, from_asset_catalog=True
202+
)
203+
)
204+
elif self._is_optimizable_image_file(fi, alternate_icon_names, primary_icon_name, from_asset_catalog=False):
192205
images.append(fi)
193206
return images
194207

195-
def _is_optimizable_image_file(self, file_info: FileInfo) -> bool:
208+
def _is_optimizable_image_file(
209+
self,
210+
file_info: FileInfo,
211+
alternate_icon_names: set[str],
212+
primary_icon_name: str | None,
213+
from_asset_catalog: bool,
214+
) -> bool:
196215
if file_info.file_type.lower() not in self.OPTIMIZABLE_FORMATS:
197216
return False
198-
name = Path(file_info.path).name
199-
if name.startswith(("AppIcon", "iMessage App Icon")):
217+
218+
if any(part.endswith(".stickerpack") for part in file_info.path.split("/")):
200219
return False
201-
return not any(part.endswith(".stickerpack") for part in file_info.path.split("/"))
220+
221+
# We can't guarantee that iMessage app's will have their icons specified in the Info.plist, so be conservative here
222+
if not from_asset_catalog:
223+
return not Path(file_info.path).name.startswith(("AppIcon", "iMessage App Icon"))
224+
225+
stem = Path(file_info.path).stem
226+
return stem != primary_icon_name and stem not in alternate_icon_names

‎tests/integration/test_image_optimization_insight.py‎

Lines changed: 93 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -137,16 +137,107 @@ def test_generates_optimization_results_for_large_images(
137137
assert any("large_unoptimized.png" in path for path in optimizable_paths)
138138
assert any("large_photo.jpg" in path for path in optimizable_paths)
139139

140-
def test_excludes_app_icons_and_sticker_packs(
140+
def test_excludes_loose_app_icons_and_sticker_packs(
141141
self, insight: ImageOptimizationInsight, insights_input: InsightsInput
142142
) -> None:
143-
"""Test that App Icons and sticker pack images are excluded from optimization."""
143+
"""Test that loose AppIcon files and sticker pack images are excluded from optimization."""
144144
result = insight.generate(insights_input)
145145
assert result is not None
146146
optimizable_paths = {f.file_path for f in result.optimizable_files}
147+
# Loose AppIcon files should be excluded
147148
assert not any("AppIcon" in path for path in optimizable_paths)
148149
assert not any("stickerpack" in path for path in optimizable_paths)
149150

151+
def test_includes_appicon_display_variants_from_asset_catalogs(
152+
self, insight: ImageOptimizationInsight, temp_images: Dict[str, Any]
153+
) -> None:
154+
"""Test that AppIcon display variants inside asset catalogs ARE included."""
155+
# Create an asset catalog with an AppIcon display variant
156+
car_path = temp_images["temp_dir"] / "Assets.car"
157+
158+
# Create an alternate icon display variant (should be included)
159+
alternate_icon_display = temp_images["temp_dir"] / "AppIconFuzz_display.png"
160+
icon_img = Image.new("RGB", (1024, 1024), (255, 0, 0))
161+
icon_img.save(alternate_icon_display, format="PNG", optimize=False, compress_level=0)
162+
163+
# Create an actual alternate icon (should be excluded)
164+
actual_icon = temp_images["temp_dir"] / "AppIconFuzz.png"
165+
actual_icon_img = Image.new("RGB", (1024, 1024), (0, 255, 0))
166+
actual_icon_img.save(actual_icon, format="PNG", optimize=False, compress_level=0)
167+
168+
# Create a mock asset catalog file entry
169+
car_file_info = FileInfo(
170+
path="Assets.car",
171+
full_path=car_path,
172+
size=1024,
173+
file_type="car",
174+
hash="test_hash",
175+
treemap_type=TreemapType.ASSETS,
176+
is_dir=False,
177+
children=[
178+
FileInfo(
179+
path="Assets.car/AppIconFuzz_display/AppIconFuzz_display.png",
180+
full_path=alternate_icon_display,
181+
size=alternate_icon_display.stat().st_size,
182+
file_type="png",
183+
hash=calculate_file_hash(alternate_icon_display),
184+
treemap_type=TreemapType.ASSETS,
185+
is_dir=False,
186+
children=[],
187+
),
188+
FileInfo(
189+
path="Assets.car/AppIconFuzz/AppIconFuzz.png",
190+
full_path=actual_icon,
191+
size=actual_icon.stat().st_size,
192+
file_type="png",
193+
hash=calculate_file_hash(actual_icon),
194+
treemap_type=TreemapType.ASSETS,
195+
is_dir=False,
196+
children=[],
197+
),
198+
],
199+
)
200+
201+
file_analysis = FileAnalysis(files=[car_file_info], directories=[])
202+
203+
test_input = InsightsInput(
204+
app_info=AppleAppInfo(
205+
name="TestApp",
206+
app_id="com.test.app",
207+
version="1.0",
208+
build="1",
209+
executable="TestApp",
210+
minimum_os_version="15.0",
211+
supported_platforms=["iphoneos"],
212+
sdk_version=None,
213+
is_simulator=False,
214+
codesigning_type=None,
215+
profile_name=None,
216+
profile_expiration_date=None,
217+
certificate_expiration_date=None,
218+
main_binary_uuid=None,
219+
is_code_signature_valid=True,
220+
code_signature_errors=[],
221+
primary_icon_name=None,
222+
alternate_icon_names=["AppIconFuzz"], # This is the actual icon
223+
),
224+
file_analysis=file_analysis,
225+
binary_analysis=[],
226+
treemap=None,
227+
hermes_reports={},
228+
)
229+
230+
result = insight.generate(test_input)
231+
assert result is not None
232+
assert len(result.optimizable_files) > 0
233+
234+
# The display variant should be included, but not the actual icon
235+
optimizable_paths = {f.file_path for f in result.optimizable_files}
236+
assert any("AppIconFuzz_display" in path for path in optimizable_paths), "Display variant should be included"
237+
assert not any(path.endswith("AppIconFuzz/AppIconFuzz.png") for path in optimizable_paths), (
238+
"Actual icon should be excluded"
239+
)
240+
150241
def test_respects_minimum_savings_threshold(
151242
self, insight: ImageOptimizationInsight, insights_input: InsightsInput
152243
) -> None:

‎tests/unit/test_alternate_icons_optimization.py‎

Lines changed: 17 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -112,7 +112,7 @@ def test_identifies_alternate_icons(self):
112112
executable="TestApp",
113113
minimum_os_version="14.0",
114114
primary_icon_name="AppIcon",
115-
alternate_icon_names=["DarkIcon", "LightIcon"],
115+
alternate_icon_names=["DarkIcon-60@2x", "LightIcon-60@2x"],
116116
)
117117

118118
# Simulate asset catalog with primary and alternate icons
@@ -129,7 +129,7 @@ def test_identifies_alternate_icons(self):
129129
# Primary icon - should be excluded
130130
FileInfo(
131131
full_path=primary_icon_path,
132-
path="Assets.car/AppIcon-60@2x",
132+
path="Assets.car/AppIcon-60@2x.png",
133133
size=primary_icon_path.stat().st_size,
134134
file_type="png",
135135
treemap_type=TreemapType.ASSETS,
@@ -139,7 +139,7 @@ def test_identifies_alternate_icons(self):
139139
# Alternate icons - should be included
140140
FileInfo(
141141
full_path=alt_icon1_path,
142-
path="Assets.car/DarkIcon-60@2x",
142+
path="Assets.car/DarkIcon-60@2x.png",
143143
size=alt_icon1_path.stat().st_size,
144144
file_type="png",
145145
treemap_type=TreemapType.ASSETS,
@@ -148,7 +148,7 @@ def test_identifies_alternate_icons(self):
148148
),
149149
FileInfo(
150150
full_path=alt_icon2_path,
151-
path="Assets.car/LightIcon-60@2x",
151+
path="Assets.car/LightIcon-60@2x.png",
152152
size=alt_icon2_path.stat().st_size,
153153
file_type="png",
154154
treemap_type=TreemapType.ASSETS,
@@ -176,9 +176,9 @@ def test_identifies_alternate_icons(self):
176176

177177
# Verify correct icons are included
178178
paths = [f.file_path for f in result.optimizable_files]
179-
assert any("DarkIcon" in p for p in paths)
180-
assert any("LightIcon" in p for p in paths)
181-
assert not any("AppIcon" in p for p in paths)
179+
assert any("DarkIcon-60@2x" in p for p in paths)
180+
assert any("LightIcon-60@2x" in p for p in paths)
181+
assert not any("PrimaryIcon-60@2x" in p for p in paths)
182182

183183
finally:
184184
# Clean up test files
@@ -199,8 +199,8 @@ def test_excludes_primary_icon(self):
199199
app_id="com.test.app",
200200
executable="TestApp",
201201
minimum_os_version="14.0",
202-
primary_icon_name="PrimaryIcon",
203-
alternate_icon_names=["AlternateIcon"],
202+
primary_icon_name="PrimaryIcon-60@2x",
203+
alternate_icon_names=["AlternateIcon-60@2x"],
204204
)
205205

206206
files = [
@@ -215,7 +215,7 @@ def test_excludes_primary_icon(self):
215215
children=[
216216
FileInfo(
217217
full_path=primary_icon_path,
218-
path="Assets.car/PrimaryIcon-60@2x",
218+
path="Assets.car/PrimaryIcon-60@2x.png",
219219
size=primary_icon_path.stat().st_size,
220220
file_type="png",
221221
treemap_type=TreemapType.ASSETS,
@@ -224,7 +224,7 @@ def test_excludes_primary_icon(self):
224224
),
225225
FileInfo(
226226
full_path=alt_icon_path,
227-
path="Assets.car/AlternateIcon-60@2x",
227+
path="Assets.car/AlternateIcon-60@2x.png",
228228
size=alt_icon_path.stat().st_size,
229229
file_type="png",
230230
treemap_type=TreemapType.ASSETS,
@@ -306,7 +306,7 @@ def test_minimum_savings_threshold(self):
306306
executable="TestApp",
307307
minimum_os_version="14.0",
308308
primary_icon_name="AppIcon",
309-
alternate_icon_names=["TinyIcon"],
309+
alternate_icon_names=["TinyIcon-16"],
310310
)
311311

312312
files = [
@@ -321,7 +321,7 @@ def test_minimum_savings_threshold(self):
321321
children=[
322322
FileInfo(
323323
full_path=small_icon_path,
324-
path="Assets.car/TinyIcon-16",
324+
path="Assets.car/TinyIcon-16.png",
325325
size=small_icon_path.stat().st_size,
326326
file_type="png",
327327
treemap_type=TreemapType.ASSETS,
@@ -363,7 +363,7 @@ def test_icon_name_matching(self):
363363
executable="TestApp",
364364
minimum_os_version="14.0",
365365
primary_icon_name="AppIcon",
366-
alternate_icon_names=["CustomIcon"],
366+
alternate_icon_names=["CustomIcon-60"],
367367
)
368368

369369
files = [
@@ -376,30 +376,20 @@ def test_icon_name_matching(self):
376376
hash="hash_car",
377377
is_dir=False,
378378
children=[
379-
# Should match - starts with CustomIcon
379+
# Should match - CustomIcon
380380
FileInfo(
381381
full_path=icon1_path,
382-
path="Assets.car/CustomIcon-60",
382+
path="Assets.car/CustomIcon-60.png",
383383
size=icon1_path.stat().st_size,
384384
file_type="png",
385385
treemap_type=TreemapType.ASSETS,
386386
hash="hash1",
387387
is_dir=False,
388388
),
389-
# Should match - starts with CustomIcon
390-
FileInfo(
391-
full_path=icon2_path,
392-
path="Assets.car/CustomIcon-60@2x",
393-
size=icon2_path.stat().st_size,
394-
file_type="png",
395-
treemap_type=TreemapType.ASSETS,
396-
hash="hash2",
397-
is_dir=False,
398-
),
399389
# Should not match - different name
400390
FileInfo(
401391
full_path=icon3_path,
402-
path="Assets.car/OtherIcon-60",
392+
path="Assets.car/OtherIcon-60.png",
403393
size=icon3_path.stat().st_size,
404394
file_type="png",
405395
treemap_type=TreemapType.ASSETS,

0 commit comments

Comments
 (0)