Merge pull request #429 from wordpress-mobile/screenshots/promo-screenshots-improvements

Promo Screenshots action improvements

Author: AliSoftware

CHANGELOG.md

@@ -10,11 +10,13 @@ _None_
 
 ### New Features
 
-* Allow `android_firebase_test` to not crash on failure, letting the caller do custom failure handling (e.g. Buildkite Annotations, etc) on their side. [#430]
+- Allow `android_firebase_test` to not crash on failure, letting the caller do custom failure handling (e.g. Buildkite Annotations, etc) on their side. [#430]
+- `promo_screenshots` now checks that the fonts—referenced via `font-family` in all the stylesheets referenced in the config file—are installed before starting, and prompt to install them if they are not. This check is enabled by default now but can be disabled/skipped if it causes any issue. [#429]
+- `promo_screenshots` now supports config files to be written in `YAML` in addition to still supporting `JSON`. [#429]  
 
 ### Bug Fixes
 
-_None_
+- Fix deprecation warning in `RMagick` call used by `promo_screenshots` action. [#429]
 
 ### Internal Changes
 

Gemfile.lock

@@ -9,7 +9,6 @@ PATH
       diffy (~> 3.3)
       git (~> 1.3)
       google-cloud-storage (~> 1.31)
-      jsonlint (~> 0.3)
       nokogiri (~> 1.11)
       octokit (~> 4.18)
       parallel (~> 1.14)
@@ -264,9 +263,6 @@ GEM
       concurrent-ruby (~> 1.0)
     jmespath (1.6.1)
     json (2.6.2)
-    jsonlint (0.3.0)
-      oj (~> 3)
-      optimist (~> 3)
     jwt (2.5.0)
     kramdown (2.3.1)
       rexml
@@ -292,9 +288,7 @@ GEM
     octokit (4.21.0)
       faraday (>= 0.9)
       sawyer (~> 0.8.0, >= 0.5.3)
-    oj (3.13.20)
     open4 (1.3.4)
-    optimist (3.0.1)
     options (2.3.2)
     optparse (0.1.1)
     os (1.1.4)

docs/screenshot-compositor.md

@@ -37,17 +37,17 @@ By default, this tool will prompt the developer before overwriting any existing
 
 **`config_file`** (optional)
 
-The tool uses a `.json` file for configuration. By default, the tool will attempt to use a file called `screenshots.json` in the same directory as the `Fastfile`. If your file has a different name, you'll need to use this argument.
+The tool uses a `.json` or `.yaml` file for configuration. By default, the tool will attempt to use a file called `screenshots.json` in the same directory as the `Fastfile`. If your file has a different name, you'll need to use this argument.
 
 ## Sample Implementations
 
 The following sample implementations provide a good example of how to create a configuration file:
 
-- [WordPress Android](https://github.com/wordpress-mobile/WordPress-Android/blob/trunk/fastlane/screenshots.json)
+- [WordPress Android](https://github.com/wordpress-mobile/WordPress-Android/blob/trunk/fastlane/screenshots/wordpress-config.json)
 
 ## Creating a configuration file
 
-The `.json` configuration file contains all the information needed for the compositor to do its work. It has three main sections:
+The `.json` (or `.yaml`) configuration file contains all the information needed for the compositor to do its work. It has three main sections:
 
 - Preamble
 - Devices
@@ -69,7 +69,7 @@ This key defines the default background color of each screenshot. This must be a
 
 **`stylesheet`**
 
-This key defines the default stylesheet used for text formatting. It should be a path to the stylesheet, either absolute, or relative to the configuration file.
+This key defines the default stylesheet used for text formatting. It should be a path to the stylesheet, either absolute, or relative to the `fastlane` folder.
 
 A sample stylesheet looks like this:
 
@@ -105,7 +105,7 @@ The device list specifies a set of reusable device frames for the compositor. Th
     "font_size": "70px",
     "screenshot_size": [740, 1480],
     "screenshot_offset": [170, 440],
-    "device_frame": "playstoreres/assets/pixel-2-xl.svg",
+    "device_frame": "screenshots/assets/pixel-2-xl.svg",
     "device_frame_size": [833, 1708],
     "device_frame_offset": [124, 317]
 },
@@ -143,7 +143,7 @@ Sets the origin of the original screenshot image. This must be provided as an ar
 
 **`device_frame`**
 
-A path to the device frame image (if applicable). This path can be absolute or relative to the configuration file.
+A path to the device frame image (if applicable). This path can be absolute or relative to the `fastlane` folder.
 
 **`device_frame_size`**
 
@@ -165,17 +165,17 @@ Each entry looks something like this:
 ```json
 {
     "device": "Pixel 2 XL",
-    "text": "playstoreres/metadata/%s/play_store_screenshot_7.html",
+    "text": "metadata/%s/play_store_screenshot_7.html",
     "screenshot": "images/phoneScreenshots/1-build-and-manage-your-website.png",
-    "background": "playstoreres/assets/backgrounds/01-background-phone.png",
+    "background": "screenshots/assets/backgrounds/01-background-phone.png",
     "attachments": [
         {
-            "file": "playstoreres/assets/attachments/01-phone-website.png",
+            "file": "screenshots/assets/attachments/01-phone-website.png",
             "size": [740, 1480],
             "position": [170, 441]
         },
         {
-            "file": "playstoreres/assets/attachments/01-phone-photos.png",
+            "file": "screenshots/assets/attachments/01-phone-photos.png",
             "size": [918, 975],
             "position": [78, 832]
         }
@@ -194,7 +194,7 @@ Allows you to customize the exported filename for this entry.
 
 **`background`** (optional)
 
-Specifies the path to a background image for this entry. This path can be absolute or relative to the configuration file. The background image **will not** be automatically resized, and it will be placed at `(0,0)`.  If not provided, the image will use the background color defined in the preamble.
+Specifies the path to a background image for this entry. This path can be absolute, or relative to the `fastlane` folder. The background image **will not** be automatically resized, and it will be placed at `(0,0)`.  If not provided, the image will use the background color defined in the preamble.
 
 **`screenshot`** (optional)
 
@@ -253,7 +253,7 @@ Text attachments use the following keys:
 
 **`text`**
 
-Specifies the path to the text file for this attachment. The path should be specified relative to the configuration file, and can contain the `{locale}` placeholder to allow for localization.
+Specifies the path to the text file for this attachment. The path should be specified relative to the `fastlane` folder, and can contain the `{locale}` placeholder to allow for localization.
 
 **`size`**
 
@@ -269,12 +269,12 @@ Specifies the font size for the text of this attachment. If this number is too l
 
 **`stylesheet`**
 
-Specifies a stylesheet used for this attachment. It should contain a path to the stylesheet, either absolute, or relative to the configuration file.
+Specifies a stylesheet used for this attachment. It should contain a path to the stylesheet, either absolute, or relative to the `fastlane` folder.
 
 **Image Attachment**
 ```json
 {
-  "file": "playstoreres/assets/attachments/01-image-name.png",
+  "file": "screenshots/assets/attachments/01-image-name.png",
   "size": [2107, 1414],
   "position": [446, 586]
 }
@@ -284,7 +284,7 @@ Image attachments use the following keys:
 
 **`file`**
 
-Specifies the path to the attachment image. The path should be specified relative to the configuration file, and can contain the `{locale}` placeholder to allow for localization.
+Specifies the path to the attachment image. The path should be specified relative to the `fastlane` folder, and can contain the `{locale}` placeholder to allow for localization.
 
 **`size`**
 

fastlane-plugin-wpmreleasetoolkit.gemspec

@@ -30,7 +30,6 @@ Gem::Specification.new do |spec|
   spec.add_dependency 'chroma', '0.2.0'
   spec.add_dependency 'diffy', '~> 3.3'
   spec.add_dependency 'git', '~> 1.3'
-  spec.add_dependency 'jsonlint', '~> 0.3'
   spec.add_dependency 'nokogiri', '~> 1.11' # Needed for AndroidLocalizeHelper
   spec.add_dependency 'octokit', '~> 4.18'
   spec.add_dependency 'parallel', '~> 1.14'

lib/fastlane/plugin/wpmreleasetoolkit/actions/common/promo_screenshots_action.rb

@@ -11,7 +11,9 @@ def self.run(params)
         UI.message "#{self.check_path(params[:orig_folder])} Original Screenshot Source: #{params[:orig_folder]}"
         UI.message "#{self.check_path(params[:metadata_folder])} Translation source: #{params[:metadata_folder]}"
 
-        config = helper.read_json(params[:config_file])
+        config = helper.read_config(params[:config_file])
+
+        helper.check_fonts_installed!(config: config) unless params[:skip_font_check]
 
         translationDirectories = subdirectories_for_path(params[:metadata_folder])
         imageDirectories = subdirectories_for_path(params[:orig_folder])
@@ -141,31 +143,34 @@ def self.available_options
                                        env_name: 'PROMOSS_ORIG',
                                        description: 'The directory containing the original screenshots',
                                        optional: false,
-                                       is_string: true),
+                                       type: String),
           FastlaneCore::ConfigItem.new(key: :output_folder,
                                        env_name: 'PROMOSS_OUTPUT',
                                        description: 'The path of the folder to save the promo screenshots',
                                        optional: false,
-                                       is_string: true),
-
+                                       type: String),
           FastlaneCore::ConfigItem.new(key: :metadata_folder,
                                        env_name: 'PROMOSS_METADATA_FOLDER',
                                        description: 'The directory containing the translation data',
                                        optional: false,
-                                       is_string: true),
-
+                                       type: String),
           FastlaneCore::ConfigItem.new(key: :config_file,
                                        env_name: 'PROMOSS_CONFIG_FILE',
                                        description: 'The path to the file containing the promo screenshot configuration',
                                        optional: true,
-                                       is_string: true,
+                                       type: String,
                                        default_value: 'screenshots.json'),
-
           FastlaneCore::ConfigItem.new(key: :force,
                                        env_name: 'PROMOSS_FORCE_CREATION',
                                        description: 'Overwrite existing promo screenshots without asking first?',
                                        optional: true,
-                                       is_string: false,
+                                       type: Boolean,
+                                       default_value: false),
+          FastlaneCore::ConfigItem.new(key: :skip_font_check,
+                                       env_name: 'PROMOSS_SKIP_FONT_CHECK',
+                                       description: 'Skip the check verifying that needed fonts are installed and active',
+                                       optional: true,
+                                       type: Boolean,
                                        default_value: false),
         ]
       end

lib/fastlane/plugin/wpmreleasetoolkit/helper/promo_screenshots_helper.rb

@@ -7,11 +7,11 @@
 end
 require 'json'
 require 'tempfile'
+require 'open3'
 require 'optparse'
 require 'pathname'
 require 'progress_bar'
 require 'parallel'
-require 'jsonlint'
 require 'chroma'
 require 'securerandom'
 
@@ -32,17 +32,65 @@ def initialize
         UI.user_error!('`drawText` not found – install it using `brew install automattic/build-tools/drawText`.') unless system('command -v drawText')
       end
 
-      def read_json(configFilePath)
+      def read_config(configFilePath)
         configFilePath = resolve_path(configFilePath)
 
         begin
-          return JSON.parse(open(configFilePath).read)
-        rescue
-          linter = JsonLint::Linter.new
-          linter.check(configFilePath)
-          linter.display_errors
+          # NOTE: While JSON is a subset of YAML and thus YAML.load_file would technically cover both cases at once, in practice
+          # `JSON.parse` is more lenient with JSON files than `YAML.load_file` is — especially, it accepts `// comments` in the
+          # JSON file, despite this not being allowed in the spec — hence why we still try with `JSON.parse` for `.json` files.
+          return File.extname(configFilePath) == '.json' ? JSON.parse(File.read(configFilePath)) : YAML.load_file(configFilePath)
+        rescue StandardError => e
+          UI.error(e)
+          UI.user_error!('Invalid JSON/YAML configuration. Please lint your config file to check for syntax errors.')
+        end
+      end
 
-          UI.user_error!('Invalid JSON configuration. See errors in log.')
+      # Checks that all required fonts are installed
+      #  - Visits the JSON config to find all the stylesheets referenced in it
+      #  - For each stylesheet, extract the first font of each `font-family` attribute found
+      #  - Finally, for each of those fonts, check that they exist and are activated.
+      #
+      # @param [Hash] config The promo screenshots configuration, as returned by #read_config
+      # @raise [UserError] Raises if at least one font is missing.
+      #
+      def check_fonts_installed!(config:)
+        # Find all stylesheets in the config
+        all_stylesheets = ([config['stylesheet']] + config['entries'].flat_map do |entry|
+          entry['attachments']&.map { |att| att['stylesheet'] }
+        end).compact.uniq
+
+        # Parse the first font in each `font-family` attribute found in all found CSS files.
+        # Only the first in each `font-family` font list matters, as others are fallbacks we don't want to use anyway.
+        font_families = all_stylesheets.flat_map do |s|
+          File.readlines(s).flat_map do |line|
+            attr = line.match(/font-family: (.*);/)&.captures&.first
+            attr.split(',').first.strip.gsub(/'(.*)'/, '\1').gsub(/"(.*)"/, '\1') unless attr.nil?
+          end
+        end.compact.uniq
+
+        # Verify that all fonts exists and are active—using a small swift script as there's no nice CLI for that
+        swift_script = <<~SWIFT
+          import AppKit
+
+          var exitCode: Int32 = 0
+          for fontName in CommandLine.arguments.dropFirst() {
+              if NSFont(name: fontName, size: NSFont.systemFontSize) != nil {
+                  print(" ✅ Font \\"\\(fontName)\\" found and active")
+              } else {
+                  print(" ❌ Font \\"\\(fontName)\\" not found, it is either not installed or disabled. Please install it using FontBook first.")
+                  exitCode = 1
+              }
+          }
+          exit(exitCode)
+        SWIFT
+
+        Tempfile.create(['fonts-check-', '.swift']) do |f|
+          f.write(swift_script)
+          f.close
+          oe, s = Open3.capture2e('/usr/bin/env', 'xcrun', 'swift', f.path, *font_families)
+          UI.command_output(oe)
+          UI.user_error!('Some fonts required by your stylesheets are missing. Please install them and try again.') unless s.success?
         end
       end
 
@@ -343,8 +391,8 @@ def crop_image(original, x_position, y_position, width, height)
       def open_image(path)
         path = resolve_path(path)
 
-        Magick::Image.read(path)  do
-          self.background_color = 'transparent'
+        Magick::Image.read(path) do |image|
+          image.background_color = 'transparent'
         end.first
       end