-
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
AdvancedListings support (gaming options and trailers) + additional screenshots #100
Conversation
f831e89
to
ff56510
Compare
Documentation/PDP.md
Outdated
|
||
> Only relevant for Application submissions | ||
|
||
There are two different types of 3 different types of images that can be used for a listing: |
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.
"There are two different types of 3 different types of images..."
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.
fixed
Documentation/PDP.md
Outdated
3. Trailer screenshots (which also don't have captions, but are tied to trailers) | ||
|
||
In the [previous section](#screenshots-and-captions), we talked about screenshots | ||
(and their captions). Now we will talkabout #2 (Additional Assets). |
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.
talk about
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.
fixed
Documentation/PDP.md
Outdated
(and their captions). Now we will talkabout #2 (Additional Assets). | ||
|
||
You can learn more about the specifics of these different images and how they're used by referring to the | ||
[dev portal's online documentation](https://docs.microsoft.com/en-us/windows/uwp/publish/app-screenshots-and-images). |
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.
Because we're talking about API values, it might be worth also including the API documentation: https://docs.microsoft.com/en-us/windows/uwp/monetize/manage-app-submissions#image-object
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
Documentation/PDP.md
Outdated
* `BackgroundImage1000X800` | ||
* `PromotionalArtwork414X180` | ||
|
||
Those elements do not have any InnerText/content -- they only have a single 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.
These elements?
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.
"Those" worked too, but changed to "These".
Documentation/USAGE.md
Outdated
(the localized content from the PDP's), deleting any existing data along the way. | ||
Please note that your application must have | ||
["Advanced Listing Permission"](https://docs.microsoft.com/en-us/windows/uwp/monetize/manage-app-submissions#advanced-listings) | ||
enabled for this to work. For more info on trailers, review [PDP.md](./PDP.md). |
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.
Don't want to link directly to the Trailers section?
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.
improved link. thanks,
StoreBroker/PackageTool.ps1
Outdated
{ | ||
$assetTypeName = $assetType.LocalName | ||
$imageFileName = $assetType.FileName | ||
if (-not [System.String]::IsNullOrWhiteSpace($imageFileName)) |
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.
According to the schema, this shouldn't ever be false, am I misunderstanding?
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.
Nope, you're right. Removed the check.
|
||
[Parameter(Mandatory)] | ||
[AllowNull()] | ||
[AllowEmptyString()] |
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.
[AllowEmptyCollection()]?
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.
Wait, why is this mandatory?
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.
Same feedback for elsewhere in this script, including PDPInclude/PDPExclude parameters
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 had the same questions around the other places. I was simply duplicating how we handled the other methods with this new one. I was reticent to make the change to remove mandatory from everywhere because it would increase the testing cost / regression potential.
StoreBroker/PackageTool.ps1
Outdated
|
||
Write-Log -Message "Converting application trailers metadata." -Level Verbose | ||
|
||
$global:dictionaries = (Get-ChildItem -File $PDPRootPath -Recurse -Include $PDPInclude -Exclude $PDPExclude).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.
global?
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.
Thanks for catching that. It was $global
temporarily while I was debugging it (so that it left the artifact around), but I neglected to revert it.
StoreBroker/PackageTool.ps1
Outdated
# PowerShell will convert an array of a single object back to a single object. | ||
# We need to force it to stay as an array, since the Trailers node in the JSON | ||
# is an array of trailers. | ||
return (, $trailersArray) |
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.
Typically the decision for array or not should be determined by the caller. Why force it here?
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. Happy to move this logic down to the caller. Thanks for pointing it out.
StoreBroker/PackageTool.ps1
Outdated
@@ -1957,7 +2344,7 @@ function Get-FormattedFilename | |||
# Simplify 'Windows.Universal' to 'Universal' | |||
$deviceFamilyCollection = $Metadata.targetDeviceFamiliesEx.Name | ForEach-Object { $_ -replace '^Windows\.', '' } | |||
|
|||
$formattedBundleTags = @($Metadata.name, $version, $architectureTag) | |||
$formattedBundleTags = @($Metadata.name, $fversion, $architectureTag) |
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.
typo?
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.
yep. thanks.
30ae4a0
to
ad348a6
Compare
0b595b2
to
4654327
Compare
$output += "$(" " * $indentLength)isBroadcastingPrivilegeGranted : $($listings.gamingOptions.isBroadcastingPrivilegeGranted)" | ||
$output += "$(" " * $indentLength)isCrossPlayEnabled : $($listings.gamingOptions.isCrossPlayEnabled)" | ||
$output += "$(" " * $indentLength)kinectDataForExternal : $($listings.gamingOptions.kinectDataForExternal)" | ||
$output += "$(" " * $indentLength)Genres : $($ApplicationSubmissionData.gamingOptions.genres -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.
Why don't we create a format string instead of repeating this pattern?
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's an interesting code-cleanup idea for a future patch release. Wouldn't block this submission on it though
} | ||
else | ||
{ | ||
$global:trailerByLang = [ordered]@{} |
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.
global!
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.
debug data strikes again. good catch, thanks
{ | ||
$global:trailerByLang = [ordered]@{} | ||
$ApplicationSubmissionData.trailers | | ||
ForEach-Object { |
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 see much benefit from pipelining here but do see improved readability from
foreach ($trailer in $ApplicationSubmissionData.trailers)
{
...
}
$trailerFileName = $trailer.videoFileName | ||
$trailer.trailerAssets | | ||
Get-Member -type NoteProperty | | ||
ForEach-Object { |
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.
nit: should be aligned with Get-Member above, correct?
59ef535
to
aa21820
Compare
…creenshots + Adds gamingOptions support * Updated the config file with `gamingOptions` * `New-StoreBrokerConfigFile` now populates those values * `Update-ApplicationSubmission` / `Patch-ApplicationSubmission` updated to support `-UpdateGamingOptions` * `Format-Application` updated to print out hasAdvancedListingSupport * `Format-ApplicationSubmission` updated to print out gaming options when available + Adds trailers support * Updated the PDP schema to support trailers * `ConvertFrom-ExistingSubmission` now exports trailer info to the PDP's. * `New-SubmissionPackage` will coalesce trailer information into the generated json * `Update-ApplicationSubmission` / `Patch-ApplicationSubmission` updated to support `-UpdateTrailers` * `Format-ApplicationSubmission` updated to print out trailers when available + Adds support for additional image/screenshot types * Adds support for all remaining "AdditonalAssets" (all of the other image types that aren't explicitly screenshots with captions), like "hero images".
aa21820
to
4bd928c
Compare
This adds the following core features to Application Listing updates:
The PDP schema has been updated to support this new localizable data. There is a new
AdditionalAssets
node that stores specific-named elements representing all image types that aren't caption-able. There's a newTrailers
node that lets you define the trailers (and associated title and screenshot) that should be available for reach language.Convert-FromExistingSubmission
has been updated to include these values in the generated PDP files.Gaming Options are stored in the config file (and
New-StoreBrokerConfigFile
was updated to capture those values)Now that we have so many different nodes that might want fallback language support, I've also added
FallbackLanguage
attribute support directly on theProductDescription
andInAppProductDescription
elements as well.Adds gamingOptions support
gamingOptions
New-StoreBrokerConfigFile
now populates those valuesUpdate-ApplicationSubmission
/Patch-ApplicationSubmission
updated tosupport
-UpdateGamingOptions
Format-Application
updated to print out hasAdvancedListingSupportFormat-ApplicationSubmission
updated to print out gaming options when availableAdds trailers support
ConvertFrom-ExistingSubmission
now exports trailer info to the PDP's.New-SubmissionPackage
will coalesce trailer information into thegenerated json
Update-ApplicationSubmission
/Patch-ApplicationSubmission
updated tosupport
-UpdateTrailers
Format-ApplicationSubmission
updated to print out trailers when availableAdds support for additional image/screenshot types
image types that aren't explicitly screenshots with captions), like
"hero images".
Resolves #58 - Add Advanced Listing Support
Resolves #85 - StoreBroker cannot upload the full set of possible image types for a listing