Improve `update_icons.dart` script (#93863)

* Update update_icons.dart

* formatting

* more formatting

* simplify safety checks

* add safety checks tests

* formatting

* update copyright

* naming

* revert copyright

* add `fontFamily` option

* tweak diff messages

* Update update_icons.dart

* don't exit if platform adaptive icon not found

* remove trailing spaces

* remove warning and fix insert_chart (outlined) dartdoc

* x

* add error message when exiting due to failed safety checkes

* fix icon reference

* fix rewrite for onetwothree

dev/tools/test/update_icons_test.dart

+// Copyright 2014 The Flutter Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+import 'package:test/test.dart';
+
+import '../update_icons.dart';
+
+Map<String, String> codepointsA = <String, String>{
+  'airplane': '111',
+  'boat': '222',
+};
+Map<String, String> codepointsB = <String, String>{
+  'airplane': '333',
+};
+Map<String, String> codepointsC = <String, String>{
+  'airplane': '111',
+  'train': '444',
+};
+
+void main() {
+  group('safety checks', () {
+    test('superset', () {
+      expect(testIsSuperset(codepointsA, codepointsA), true);
+
+      expect(testIsSuperset(codepointsA, codepointsB), true);
+      expect(testIsSuperset(codepointsB, codepointsA), false);
+    });
+    test('stability', () {
+      expect(testIsStable(codepointsA, codepointsA), true);
+
+      expect(testIsStable(codepointsA, codepointsB), false);
+      expect(testIsStable(codepointsB, codepointsA), false);
+
+      expect(testIsStable(codepointsA, codepointsC), true);
+      expect(testIsStable(codepointsC, codepointsA), true);
+    });
+  });
+}

dev/tools/update_icons.dart

@@ -10,16 +10,21 @@ import 'dart:convert' show LineSplitter;
 import 'dart:io';
 
 import 'package:args/args.dart';
+import 'package:meta/meta.dart';
 import 'package:path/path.dart' as path;
 
+const String _iconsPathOption = 'icons';
+const String _iconsTemplatePathOption = 'icons-template';
 const String _newCodepointsPathOption = 'new-codepoints';
 const String _oldCodepointsPathOption = 'old-codepoints';
-const String _iconsClassPathOption = 'icons';
+const String _fontFamilyOption = 'font-family';
+const String _enforceSafetyChecks = 'enforce-safety-checks';
 const String _dryRunOption = 'dry-run';
 
+const String _defaultIconsPath = 'packages/flutter/lib/src/material/icons.dart';
 const String _defaultNewCodepointsPath = 'codepoints';
 const String _defaultOldCodepointsPath = 'bin/cache/artifacts/material_fonts/codepoints';
-const String _defaultIconsPath = 'packages/flutter/lib/src/material/icons.dart';
+const String _defaultFontFamily = 'MaterialIcons';
 
 const String _beginGeneratedMark = '// BEGIN GENERATED ICONS';
 const String _endGeneratedMark = '// END GENERATED ICONS';
@@ -38,36 +43,37 @@ const Map<String, List<String>> _platformAdaptiveIdentifiers = <String, List<Str
 
 // Rewrite certain Flutter IDs (numbers) using prefix matching.
 const Map<String, String> identifierPrefixRewrites = <String, String>{
-  '_1': 'one_',
-  '_2': 'two_',
-  '_3': 'three_',
-  '_4': 'four_',
-  '_5': 'five_',
-  '_6': 'six_',
-  '_7': 'seven_',
-  '_8': 'eight_',
-  '_9': 'nine_',
-  '_10': 'ten_',
-  '_11': 'eleven_',
-  '_12': 'twelve_',
-  '_13': 'thirteen_',
-  '_14': 'fourteen_',
-  '_15': 'fifteen_',
-  '_16': 'sixteen_',
-  '_17': 'seventeen_',
-  '_18': 'eighteen_',
-  '_19': 'nineteen_',
-  '_20': 'twenty_',
-  '_21': 'twenty_one_',
-  '_22': 'twenty_two_',
-  '_23': 'twenty_three_',
-  '_24': 'twenty_four_',
-  '_30': 'thirty_',
-  '_60': 'sixty_',
-  '_360': 'threesixty',
-  '_2d': 'twod',
-  '_3d': 'threed',
-  '_3d_rotation': 'threed_rotation',
+  '1': 'one_',
+  '2': 'two_',
+  '3': 'three_',
+  '4': 'four_',
+  '5': 'five_',
+  '6': 'six_',
+  '7': 'seven_',
+  '8': 'eight_',
+  '9': 'nine_',
+  '10': 'ten_',
+  '11': 'eleven_',
+  '12': 'twelve_',
+  '13': 'thirteen_',
+  '14': 'fourteen_',
+  '15': 'fifteen_',
+  '16': 'sixteen_',
+  '17': 'seventeen_',
+  '18': 'eighteen_',
+  '19': 'nineteen_',
+  '20': 'twenty_',
+  '21': 'twenty_one_',
+  '22': 'twenty_two_',
+  '23': 'twenty_three_',
+  '24': 'twenty_four_',
+  '30': 'thirty_',
+  '60': 'sixty_',
+  '123': 'onetwothree',
+  '360': 'threesixty',
+  '2d': 'twod',
+  '3d': 'threed',
+  '3d_rotation': 'threed_rotation',
 };
 
 // Rewrite certain Flutter IDs (reserved keywords) using exact matching.
@@ -163,9 +169,14 @@ void main(List<String> args) {
 
   final ArgResults argResults = _handleArguments(args);
 
-  final File iconClassFile = File(path.normalize(path.absolute(argResults[_iconsClassPathOption] as String)));
-  if (!iconClassFile.existsSync()) {
-    stderr.writeln('Error: Icons file not found: ${iconClassFile.path}');
+  final File iconsFile = File(path.normalize(path.absolute(argResults[_iconsPathOption] as String)));
+  if (!iconsFile.existsSync()) {
+    stderr.writeln('Error: Icons file not found: ${iconsFile.path}');
+    exit(1);
+  }
+  final File iconsTemplateFile = File(path.normalize(path.absolute(argResults[_iconsTemplatePathOption] as String)));
+  if (!iconsTemplateFile.existsSync()) {
+    stderr.writeln('Error: Icons template file not found: ${iconsTemplateFile.path}');
     exit(1);
   }
   final File newCodepointsFile = File(argResults[_newCodepointsPathOption] as String);
@@ -185,33 +196,51 @@ void main(List<String> args) {
   final String oldCodepointsString = oldCodepointsFile.readAsStringSync();
   final Map<String, String> oldTokenPairMap = _stringToTokenPairMap(oldCodepointsString);
 
-  _testIsMapSuperset(newTokenPairMap, oldTokenPairMap);
-
-  final String iconClassFileData = iconClassFile.readAsStringSync();
+  stderr.writeln('Performing safety checks');
+  final bool isSuperset = testIsSuperset(newTokenPairMap, oldTokenPairMap);
+  final bool isStable = testIsStable(newTokenPairMap, oldTokenPairMap);
+  if ((!isSuperset || !isStable) && argResults[_enforceSafetyChecks] as bool) {
+    exit(1);
+  }
+  final String iconsTemplateContents = iconsTemplateFile.readAsStringSync();
 
-  stderr.writeln('Generating icons file...');
-  final String newIconData = _regenerateIconsFile(iconClassFileData, newTokenPairMap);
+  stderr.writeln("Generating icons ${argResults[_dryRunOption] as bool ? '' : 'to ${iconsFile.path}'}");
+  final String newIconsContents = _regenerateIconsFile(
+    iconsTemplateContents,
+    newTokenPairMap,
+    argResults[_fontFamilyOption] as String,
+    argResults[_enforceSafetyChecks] as bool,
+  );
 
   if (argResults[_dryRunOption] as bool) {
-    stdout.write(newIconData);
+    stdout.write(newIconsContents);
   } else {
-    stderr.writeln('\nWriting to ${iconClassFile.path}.');
-    iconClassFile.writeAsStringSync(newIconData);
+    iconsFile.writeAsStringSync(newIconsContents);
     _regenerateCodepointsFile(oldCodepointsFile, newTokenPairMap);
   }
 }
 
 ArgResults _handleArguments(List<String> args) {
   final ArgParser argParser = ArgParser()
+    ..addOption(_iconsPathOption,
+        defaultsTo: _defaultIconsPath,
+        help: 'Location of the material icons file')
+    ..addOption(_iconsTemplatePathOption,
+        defaultsTo: _defaultIconsPath,
+        help:
+            'Location of the material icons file template. Usually the same as --$_iconsPathOption')
     ..addOption(_newCodepointsPathOption,
         defaultsTo: _defaultNewCodepointsPath,
         help: 'Location of the new codepoints directory')
     ..addOption(_oldCodepointsPathOption,
         defaultsTo: _defaultOldCodepointsPath,
         help: 'Location of the existing codepoints directory')
-    ..addOption(_iconsClassPathOption,
-        defaultsTo: _defaultIconsPath,
-        help: 'Location of the material icons file')
+    ..addOption(_fontFamilyOption,
+        defaultsTo: _defaultFontFamily,
+        help: 'The font family to use for the IconData constants')
+    ..addFlag(_enforceSafetyChecks,
+        defaultsTo: true,
+        help: 'Whether to exit if safety checks fail (e.g. codepoints are missing or unstable')
     ..addFlag(_dryRunOption);
   argParser.addFlag('help', abbr: 'h', negatable: false, callback: (bool help) {
     if (help) {
@@ -227,7 +256,7 @@ Map<String, String> _stringToTokenPairMap(String codepointData) {
       .map((String line) => line.trim())
       .where((String line) => line.isNotEmpty);
 
-  final Map<String, String> pairs = <String,String>{};
+  final Map<String, String> pairs = <String, String>{};
 
   for (final String line in cleanData) {
     final List<String> tokens = line.split(' ');
@@ -240,40 +269,49 @@ Map<String, String> _stringToTokenPairMap(String codepointData) {
   return pairs;
 }
 
-String _regenerateIconsFile(String iconData, Map<String, String> tokenPairMap) {
+String _regenerateIconsFile(
+    String templateFileContents,
+    Map<String, String> tokenPairMap,
+    String fontFamily,
+    bool enforceSafetyChecks,
+  ) {
   final List<_Icon> newIcons = tokenPairMap.entries
-      .map((MapEntry<String, String> entry) => _Icon(entry))
+      .map((MapEntry<String, String> entry) => _Icon(entry, fontFamily))
       .toList();
   newIcons.sort((_Icon a, _Icon b) => a._compareTo(b));
 
   final StringBuffer buf = StringBuffer();
   bool generating = false;
 
-  for (final String line in LineSplitter.split(iconData)) {
+  for (final String line in LineSplitter.split(templateFileContents)) {
     if (!generating) {
       buf.writeln(line);
     }
 
-    // Generate for _PlatformAdaptiveIcons
+    // Generate for PlatformAdaptiveIcons
     if (line.contains(_beginPlatformAdaptiveGeneratedMark)) {
       generating = true;
       final List<String> platformAdaptiveDeclarations = <String>[];
       _platformAdaptiveIdentifiers.forEach((String flutterId, List<String> ids) {
-        // Automatically finds and generates styled icon declarations.
+        // Automatically finds and generates all icon declarations.
         for (final String style in <String>['', '_outlined', '_rounded', '_sharp']) {
           try {
             final _Icon agnosticIcon = newIcons.firstWhere(
-                    (_Icon icon) => icon.id == '${ids[0]}$style',
+                (_Icon icon) => icon.id == '${ids[0]}$style',
                 orElse: () => throw ids[0]);
             final _Icon iOSIcon = newIcons.firstWhere(
-                    (_Icon icon) => icon.id == '${ids[1]}$style',
+                (_Icon icon) => icon.id == '${ids[1]}$style',
                 orElse: () => throw ids[1]);
-            platformAdaptiveDeclarations.add(_Icon.platformAdaptiveDeclaration('$flutterId$style', agnosticIcon, iOSIcon));
+            platformAdaptiveDeclarations.add(_Icon.platformAdaptiveDeclaration('$flutterId$style', agnosticIcon, iOSIcon),
+            );
           } catch (e) {
             if (style == '') {
-              // Throw an error for regular (unstyled) icons.
-              stderr.writeln("Error while generating platformAdaptiveDeclarations: Icon '$e' not found.");
-              exit(1);
+              // Throw an error for baseline icons.
+              stderr.writeln("❌ Platform adaptive icon '$e' not found.");
+              if (enforceSafetyChecks) {
+                stderr.writeln('Safety checks failed');
+                exit(1);
+              }
             } else {
               // Ignore errors for styled icons since some don't exist.
             }
@@ -299,28 +337,50 @@ String _regenerateIconsFile(String iconData, Map<String, String> tokenPairMap) {
   return buf.toString();
 }
 
-void _testIsMapSuperset(Map<String, String> newCodepoints, Map<String, String> oldCodepoints) {
+@visibleForTesting
+bool testIsSuperset(Map<String, String> newCodepoints, Map<String, String> oldCodepoints) {
   final Set<String> newCodepointsSet = newCodepoints.keys.toSet();
   final Set<String> oldCodepointsSet = oldCodepoints.keys.toSet();
 
+  final int diff = newCodepointsSet.length - oldCodepointsSet.length;
+  if (diff > 0) {
+    stderr.writeln('🆕 $diff new codepoints: ${newCodepointsSet.difference(oldCodepointsSet)}');
+  }
   if (!newCodepointsSet.containsAll(oldCodepointsSet)) {
-    stderr.writeln('''
-Error: New codepoints file does not contain all ${oldCodepointsSet.length} existing codepoints.\n
-        Missing: ${oldCodepointsSet.difference(newCodepointsSet)}
-        ''',
-    );
-    exit(1);
+    stderr.writeln(
+        '❌ new codepoints file does not contain all ${oldCodepointsSet.length} '
+        'existing codepoints. Missing: ${oldCodepointsSet.difference(newCodepointsSet)}');
+    return false;
   } else {
-    final int diff = newCodepointsSet.length - oldCodepointsSet.length;
-    stderr.writeln('New codepoints file contains all ${oldCodepointsSet.length} existing codepoints.');
-    if (diff > 0) {
-      stderr.writeln('It also contains $diff new codepoints: ${newCodepointsSet.difference(oldCodepointsSet)}');
+    stderr.writeln('✅ new codepoints file contains all ${oldCodepointsSet.length} existing codepoints');
+  }
+  return true;
+}
+
+@visibleForTesting
+bool testIsStable(Map<String, String> newCodepoints, Map<String, String> oldCodepoints) {
+  final int oldCodepointsCount = oldCodepoints.length;
+  final List<String> unstable = <String>[];
+
+  oldCodepoints.forEach((String key, String value) {
+    if (newCodepoints.containsKey(key)) {
+      if (value != newCodepoints[key]) {
+        unstable.add(key);
+      }
     }
+  });
+
+  if (unstable.isNotEmpty) {
+    stderr.writeln('❌ out of $oldCodepointsCount existing codepoints, ${unstable.length} were unstable: $unstable');
+    return false;
+  } else {
+    stderr.writeln('✅ all existing $oldCodepointsCount codepoints are stable');
+    return true;
   }
 }
 
 void _regenerateCodepointsFile(File oldCodepointsFile, Map<String, String> newTokenPairMap) {
-  stderr.writeln('Regenerating old codepoints file ${oldCodepointsFile.path}.\n');
+  stderr.writeln('Regenerating old codepoints file ${oldCodepointsFile.path}');
 
   final StringBuffer buf = StringBuffer();
   final SplayTreeMap<String, String> sortedNewTokenPairMap = SplayTreeMap<String, String>.of(newTokenPairMap);
@@ -330,7 +390,7 @@ void _regenerateCodepointsFile(File oldCodepointsFile, Map<String, String> newTo
 
 class _Icon {
   // Parse tokenPair (e.g. {"6_ft_apart_outlined": "e004"}).
-  _Icon(MapEntry<String, String> tokenPair) {
+  _Icon(MapEntry<String, String> tokenPair, this.fontFamily) {
     id = tokenPair.key;
     hexCodepoint = tokenPair.value;
 
@@ -349,14 +409,15 @@ class _Icon {
       htmlSuffix = '-filled';
     } else {
       family = 'material';
-      if (id.endsWith('_outlined') && id != 'insert_chart_outlined') {
+      if (id.endsWith('_baseline')) {
+        id = _removeLast(id, '_baseline');
+        htmlSuffix = '';
+      } else if (id.endsWith('_outlined')) {
         htmlSuffix = '-outlined';
       } else if (id.endsWith('_rounded')) {
         htmlSuffix = '-round';
       } else if (id.endsWith('_sharp')) {
         htmlSuffix = '-sharp';
-      } else {
-        htmlSuffix = '';
       }
     }
 
@@ -379,7 +440,8 @@ class _Icon {
   late String flutterId; // e.g. five_g, five_g_outlined, five_g_rounded, five_g_sharp
   late String family; // e.g. material
   late String hexCodepoint; // e.g. e547
-  late String htmlSuffix; // The suffix for the 'material-icons' HTML class.
+  late String htmlSuffix = ''; // The suffix for the 'material-icons' HTML class.
+  String fontFamily; // The IconData font family.
 
   String get name => shortId.replaceAll('_', ' ').trim();
 
@@ -393,7 +455,7 @@ class _Icon {
       : '';
 
   String get declaration =>
-      "static const IconData $flutterId = IconData(0x$hexCodepoint, fontFamily: 'MaterialIcons'$mirroredInRTL);";
+      "static const IconData $flutterId = IconData(0x$hexCodepoint, fontFamily: '$fontFamily'$mirroredInRTL);";
 
   String get fullDeclaration => '''
 
@@ -419,16 +481,14 @@ class _Icon {
     return shortId.compareTo(b.shortId);
   }
 
-  static String _replaceLast(String string, String toReplace) {
+  static String _removeLast(String string, String toReplace) {
     return string.replaceAll(RegExp('$toReplace\$'), '');
   }
 
   static String _generateShortId(String id) {
     String shortId = id;
     for (final String styleSuffix in _idSuffixes) {
-      if (styleSuffix == '_outlined' && id == 'insert_chart_outlined')
-        continue;
-      shortId = _replaceLast(shortId, styleSuffix);
+      shortId = _removeLast(shortId, styleSuffix);
       if (shortId != id) {
         break;
       }
@@ -440,22 +500,22 @@ class _Icon {
   static String generateFlutterId(String id) {
     String flutterId = id;
     // Exact identifier rewrites.
-    for (final MapEntry<String, String> rewritePair
-    in identifierExactRewrites.entries) {
+    for (final MapEntry<String, String> rewritePair in identifierExactRewrites.entries) {
       final String shortId = _Icon._generateShortId(id);
       if (shortId == rewritePair.key) {
-        flutterId = id.replaceFirst(rewritePair.key, identifierExactRewrites[rewritePair.key]!);
+        flutterId = id.replaceFirst(
+          rewritePair.key,
+          identifierExactRewrites[rewritePair.key]!,
+        );
       }
     }
     // Prefix identifier rewrites.
-    for (final MapEntry<String, String> rewritePair
-    in identifierPrefixRewrites.entries) {
+    for (final MapEntry<String, String> rewritePair in identifierPrefixRewrites.entries) {
       if (id.startsWith(rewritePair.key)) {
-        flutterId = id.replaceFirst(rewritePair.key, identifierPrefixRewrites[rewritePair.key]!);
-      }
-      // TODO(guidezpl): With the next icon update, this won't be necessary, remove it.
-      if (id.startsWith(rewritePair.key.replaceFirst('_', ''))) {
-        flutterId = id.replaceFirst(rewritePair.key.replaceFirst('_', ''), identifierPrefixRewrites[rewritePair.key]!);
+        flutterId = id.replaceFirst(
+          rewritePair.key,
+          identifierPrefixRewrites[rewritePair.key]!,
+        );
       }
     }
     return flutterId;

packages/flutter/lib/src/material/icons.dart

@@ -97,8 +97,7 @@ class PlatformAdaptiveIcons implements Icons {
 ///
 /// Use with the [Icon] class to show specific icons.
 ///
-/// Icons are identified by their name as listed below. **Do not use codepoints
-/// directly, as they are subject to change.**
+/// Icons are identified by their name as listed below, e.g. [Icons.airplanemode_on].
 ///
 /// To use this class, make sure you set `uses-material-design: true` in your
 /// project's `pubspec.yaml` file in the `flutter` section. This ensures that
@@ -10287,7 +10286,7 @@ class Icons {
   /// <i class="material-icons-round md-36">insert_chart</i> &#x2014; material icon named "insert chart" (round).
   static const IconData insert_chart_rounded = IconData(0xf819, fontFamily: 'MaterialIcons');
 
-  /// <i class="material-icons md-36">insert_chart_outlined</i> &#x2014; material icon named "insert chart outlined".
+  /// <i class="material-icons-outlined md-36">insert_chart</i> &#x2014; material icon named "insert chart" (outlined).
   static const IconData insert_chart_outlined = IconData(0xf12a, fontFamily: 'MaterialIcons');
 
   /// <i class="material-icons-sharp md-36">insert_chart_outlined</i> &#x2014; material icon named "insert chart outlined" (sharp).