Promo screenshots action refinements #685
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,7 +33,13 @@ def initialize | |
| UI.user_error!(message) | ||
| end | ||
|
|
||
| UI.user_error!('`drawText` not found – install it using `brew install automattic/build-tools/drawText`.') unless self.class.draw_text_available? | ||
| end | ||
|
|
||
| def self.draw_text_available? | ||
| return @draw_text_available if defined?(@draw_text_available) | ||
|
|
||
| @draw_text_available = system('command -v drawText', %i[out err] => File::NULL) | ||
| end | ||
|
|
||
| def read_config(config_file_path) | ||
|
|
@@ -287,7 +293,17 @@ def draw_text_to_canvas(canvas, text, width, height, x_position, y_position, fon | |
| begin | ||
| temp_text_file = Tempfile.new | ||
|
|
||
| Actions.sh( | ||
| 'drawText', | ||
| "html=#{text}", | ||
| "maxWidth=#{width}", | ||
| "maxHeight=#{height}", | ||
| "output=#{temp_text_file.path}", | ||
| "fontSize=#{font_size}", | ||
| "stylesheet=#{stylesheet_path}", | ||
| "alignment=#{position}", | ||
| log: false | ||
| ) | ||
|
|
||
| text_content = open_image(temp_text_file.path).trim | ||
| text_frame = create_image(width, height) | ||
|
|
@@ -407,8 +423,8 @@ def create_image(width, height, background = 'transparent') | |
| working_background = background.frozen? ? background.dup : background | ||
| working_background.paint.to_hex | ||
|
|
||
| Image.new(width, height) do |info| | ||
| info.background_color = working_background | ||
| end | ||
| end | ||
|
Comment on lines
-410
to
429
Contributor
Author
There was a problem hiding this comment. This addresses a deprecation warning. See commit message for more details. |
||
|
|
||
|
|
@@ -432,8 +448,12 @@ def resolve_path(path) | |
| return resolved_path if !resolved_path.nil? && resolved_path.exist? | ||
| end | ||
|
|
||
| message = <<~MESSAGE | ||
| Unable to locate #{path}. | ||
|
|
||
| Did you run the automation to generate the screenshots? | ||
|
Contributor
Author
There was a problem hiding this comment. Just in case I go through the same process of pondering why annotating non existing screenshots fails again. |
||
| MESSAGE | ||
| UI.user_error!(message) | ||
|
Contributor
Author
There was a problem hiding this comment. Used |
||
| end | ||
|
|
||
| def resolve_text_into_path(text, locale) | ||
|
|
@@ -444,6 +464,7 @@ def resolve_text_into_path(text, locale) | |
| elsif can_resolve_path(localized_file) | ||
| resolve_path(localized_file).realpath.to_s | ||
| else | ||
| UI.important("Could not identify '#{localized_file}' as a file path or the file was not found. Will use its value as a raw string. This may result in undesired annotations.") | ||
|
Contributor
Author
There was a problem hiding this comment. In testing, I was surprised to find annotated screenshots with a path instead of of text as the title. The reason was misconfiguration of the path on my end, but the fact remains that the tool, in being flexible, might hide missing files. This can be dangerous if automation runs unsupervised. We might want to rethink the tool's behavior. But in the meantime, here's a warning in the logs. 
(Not sure yet why it's printed twice)
Contributor
There was a problem hiding this comment.
👍 I agree.
I see that if both paths are misconfigured and can't be resolved, we'll then have two warnings as
Contributor
Author
There was a problem hiding this comment.
Oh, of course! Thanks @iangmaia ! In my tests, I had hardcoded only one language, but forgot Woo is configured to create promo screenshots for iPhone and iPad. Given the copy is the same, we'll obviously attempt two reads of that localization file. All clear now. |
||
| format(text, 'source') | ||
| end | ||
| end | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems more appropriate as an exit call resulting from a
confirmuser input thanuser_error!.I'd actually argue we should not crash and simply terminate the execution (
return,next). But that's out of scope at the moment because it would require restructuring the entire method to bubble up the intention of terminating. Simply callingreturnhere would only result in the code continuing, which is not what we desire.