-
Notifications
You must be signed in to change notification settings - Fork 39
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enable Language Fallback support for media (screenshots and icons) #80
Conversation
b971ba4
to
48a9c1d
Compare
@@ -76,6 +76,13 @@ | |||
</xs:restriction> | |||
</xs:simpleType> | |||
</xs:attribute> |
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.
So I'm thinking that, in most cases, if I want to specify a fallback language for a particular image, I probably want to specify the same fallback language for all images in the set. It would make most sense for this attribute to apply on the ScreenshotCaptions element. If an individual Caption element has a different fallback language, then the language specified on the individual element should override the one specified on the ScreenshotCaptions element.
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.
Agreed. Added that as an option.
StoreBroker/PackageTool.ps1
Outdated
|
||
[AllowNull()] | ||
[AllowEmptyString()] | ||
[string] $MediaFallbackLanguage |
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.
No need for the Allow* attributes because the parameter is optional
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.
Agreed
StoreBroker/PackageTool.ps1
Outdated
$mediaFallbackLanguageSourcePath = [System.IO.Path]::Combine($ImagesRootPath, $Release, $MediaFallbackLanguage) | ||
if (-not (Test-Path -Path $mediaFallbackLanguageSourcePath -PathType Container)) | ||
{ | ||
Write-Log "A fallback language was specified [$MediaFallbackLanguage], but a folder for that language does not exist [$mediaFallbackLanguageSourcePath], so media fallback support has been disabled." -Level Verbose |
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.
I think we should error out in this case. The user specified a fallback but the fallback does not exist, which indicates the state of their folders is not what the user intended.
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.
Disagreed. The purpose of fallback is to default to prefer the language-specific one, and only use the fallback if the language-specific one can't be found. Won't fix.
StoreBroker/PackageTool.ps1
Outdated
@@ -864,6 +860,86 @@ function Convert-ListingToObject | |||
} | |||
} | |||
|
|||
function Get-LocalizedMediaFile | |||
{ | |||
[CmdletBinding(SupportsShouldProcess)] |
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.
Needs CBH, especially a description that describes the steps used to resolve fallback image.
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.
Agreed. Oversight
StoreBroker/PackageTool.ps1
Outdated
|
||
if (Test-Path -Path $mediaLanguageSourcePath -PathType Container) | ||
{ | ||
$image = Get-ChildItem -Recurse -File -Path $mediaLanguageSourcePath -Include $Filename | Select-Object -First 1 |
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.
I'm not sure selecting the first one is the right choice here. If there are multiple files with the same name under this path, I would rather error out so we don't accidentally make the wrong decision. Worse yet, the user may not know that there we are selecting between multiple images and so it's harder for them to know that their structure is incorrect in the first place.
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.
This was the pre-existing file selection logic. I agree with your observation though and will improve this to error out if more than one matching file is found.
StoreBroker/PackageTool.ps1
Outdated
{ | ||
New-Item -ItemType directory -Path $packageImagePath | Out-Null | ||
} | ||
|
||
$imageListings = @() |
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.
I'd like if we kept the comment about # ScreenshotCaptions because it tells the reader we're switching to handling the images now.
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.
Done
'ImagesRootPath' = $ImagesRootPath | ||
'Language' = $language | ||
'Release' = $ProductDescriptionNode.Release | ||
'MediaFallbackLanguage' = $requestedFallbackLanguage |
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.
I don't think both Language and MediaFallbackLanguage should be parameters here. If the user specified a MediaFallbackLanguage, then we should overwrite the value of $language and call Get-LocalizedMediaFile using the fallback language as the value of the parameter. I'm not sure why Get-LocalizedMediaFile needs to know that any fallback language is being used at all.
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.
Discussed in person. This is a common helper designed to determine the correct file to use here. As discussed in person, this is a fallback feature, and not an override feature. We need both languages to be provided, because we need to check for the existence of the main language, and only then if not found will we then try finding the file in the fallback language's folder.
StoreBroker/PackageTool.ps1
Outdated
$fileRelativePackagePath = [System.IO.Path]::Combine($script:packageImageFolderName, $Language, $Filename) | ||
} | ||
|
||
if (($null -eq $image) -and ($null -ne $mediaFallbackLanguageSourcePath)) |
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.
So I think the way this is written, it's covering for the case where the user specified a fallback language but there is also a matching image for the original language. In this case, we're choosing the original image and ignoring what was specified for fallback language. Again, I would rather error out here because it indicates that the user has an incorrect folder structure or an incorrect understanding of using fallback images. I'd rather not make these decisions for them, instead just provide a helpful error message so they can fix the problem. If packaging succeeds, it should be because we were clearly able to resolve packaging parameters without ambiguity. This is just my approach so I'm assuming we'll talk more about it in-person.
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.
Won't fix. This is explicitly designed as a fallback option, similar to how we fallback on the default language for localized strings if we can't find a specific version of that string for the current language.
PDP/ProductDescription.xsd
Outdated
@@ -252,6 +252,13 @@ | |||
</xs:complexType> | |||
</xs:element> | |||
</xs:sequence> | |||
<xs:attribute name="FallbackLanguage"> |
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.
Can't you declare the FallbackLanguage attribute once at the top of the file and then reference it both here and in the Caption element?
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.
Indeed.
StoreBroker/PackageTool.ps1
Outdated
@@ -925,6 +978,13 @@ function Get-LocalizedMediaFile | |||
throw $output | |||
} | |||
|
|||
if ($image.Count -gt 1) | |||
{ | |||
$output = "More then one version of [$Filename] has been found for this language. Please ensure only one copy of this image exists within the language's sub-folders: [$($image -join ', ')]" |
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.
The join operation needs $image.FullName
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.
It actually worked fine without it (had verified before the update), but I agree that it's better to be declarative here.
bde6161
to
11c3dfa
Compare
The Store Submission API was recently updated to allow for submissions to reference the same media file across multiple elements. In other words, the same .png file can now be referenced as a screenshot for multiple languages. Some applications choose not to localize their screenshots, only focusing on localizing the written metadata for their PDP's. In these scenarios, they would have a smaller StoreBroker package (and a simpler authoring environment) if they could simply tell StoreBroker to use a different language's screenshots when language-specific ones are not available. With this change, the following end-user features are now enabled: * The StoreBroker config file has been updated to support a `MediaFallbackLanguage` config option that allows users to specify a lang-code (e,g, `en-us`, `es-es`, etc...) * Users can specify a `-MediaFallbackLanguage` parameter to `New-SubmissionPackage` and `New-InAppProductSubmissionPackage`, specifying a lang-code (e,g, `en-us`, `es-es`, etc...) which will override any config file option that has been specified. * Users can specify a `FallbackLanguage` attribute on the `<caption />` and `<icon />` elements in PDP files to optionally choose a different fallback language on a per-element basis. In any of these cases, if the language-specific media file cannot be found, and a fallback language has been specified, StoreBroker will then look in the fallback language's Images/media sub-folder for the same-named media file. Only then, if still not found, will packaging fail. It should be noted that there can only ever be _one_ designated fallback language for any given file. The logic does not cascade. Therefore, if a user specifies a fallback language in both the config file _and_ at the command prompt, the command prompt always wins. If a value has been specified via the config file and/or command prompt, _as well as_ in a PDP file, the fallback language from the PDP file will be the one that is used _for that element_, and then StoreBroker will revert back to using the one from the config file / command prompt until it encounters another one defined specifically in the PDP file. Addresses microsoft#28: Unable to submit common screenshot for all languages
11c3dfa
to
09a2fec
Compare
The Store Submission API was recently updated to allow for submissions to reference the same media file across multiple elements. In other words, the same .png file can now be referenced as a screenshot for multiple languages.
Some applications choose not to localize their screenshots, only focusing on localizing the written metadata for their PDP's. In these scenarios, they would have a smaller StoreBroker package (and a simpler authoring environment) if they could simply tell StoreBroker to use a different language's screenshots when language-specific ones are not available.
With this change, the following end-user features are now enabled:
MediaFallbackLanguage
config optionthat allows users to specify a lang-code (e,g,
en-us
,es-es
, etc...)-MediaFallbackLanguage
parameter toNew-SubmissionPackage
andNew-InAppProductSubmissionPackage
, specifying a lang-code (e,g,en-us
,es-es
, etc...) which willoverride any config file option that has been specified.
FallbackLanguage
attribute on the<caption />
and<icon />
elements inPDP files to optionally choose a different fallback language on a per-element basis.
In any of these cases, if the language-specific media file cannot be found, and a fallback language has
been specified, StoreBroker will then look in the fallback language's Images/media sub-folder for the
same-named media file. Only then, if still not found, will packaging fail.
It should be noted that there can only ever be one designated fallback language for any given file.
The logic does not cascade. Therefore, if a user specifies a fallback language in both the config file
and at the command prompt, the command prompt always wins. If a value has been specified via
the config file and/or command prompt, as well as in a PDP file, the fallback language from the PDP
file will be the one that is used for that element, and then StoreBroker will revert back to using the
one from the config file / command prompt until it encounters another one defined specifically in
the PDP file.
Addresses #28: Unable to submit common screenshot for all languages