Skip to content
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

[deliver] Cannot upload iPad 3rd generation screenshots #14116

Closed
dayjer opened this issue Jan 22, 2019 · 49 comments
Closed

[deliver] Cannot upload iPad 3rd generation screenshots #14116

dayjer opened this issue Jan 22, 2019 · 49 comments

Comments

@dayjer
Copy link

@dayjer dayjer commented Jan 22, 2019

New Issue Checklist

Issue Description

Despite using snapshot to generate screenshots "iPad Pro (12.9-inch) (3rd generation)-01_screenshot_X.png", and after battling with the ongoing sizing issues, such as #13698 and #13610
It still uploads the images for "iPad Pro (2nd Generation) 12.9-inch Display" -- the problem with this is that all my localisations then go and use English (e.g. https://cl.ly/db4d1a265bbb)

Complete output when running fastlane, including the stack trace and command used
[08:19:18]: Uploading 30 screenshots for language zh-Hant
[08:19:18]: Uploading './screenshots/zh-Hant/iPad Pro (12.9-inch) (3rd generation)-01_screenshot_1.png'...
[08:19:43]: Uploading './screenshots/zh-Hant/iPad Pro (12.9-inch) (3rd generation)-02_screenshot_2.png'...
[08:20:08]: Uploading './screenshots/zh-Hant/iPad Pro (12.9-inch) (3rd generation)-03_screenshot_3.png'...
[08:20:35]: Uploading './screenshots/zh-Hant/iPad Pro (12.9-inch) (3rd generation)-04_screenshot_4.png'...
[08:20:52]: Uploading './screenshots/zh-Hant/iPad Pro (12.9-inch) (3rd generation)-05_screenshot_5.png'...
...
[✔] Saving changes 
[08:23:45]: Successfully uploaded screenshots to App Store Connect

Environment

fastlane environment

Stack

Key Value
OS 10.14.2
Ruby 2.3.7
Bundler? false
Git git version 2.17.2 (Apple Git-113)
Installation Source /usr/local/bin/fastlane
Host Mac OS X 10.14.2 (18C54)
Ruby Lib Dir /System/Library/Frameworks/Ruby.framework/Versions/2.3/usr/lib
OpenSSL Version LibreSSL 2.6.4
Is contained false
Is homebrew false
Is installed via Fabric.app false
Xcode Path /Applications/Xcode.app/Contents/Developer/
Xcode Version 10.1

System Locale

Variable Value
LANG en_AU.UTF-8
LC_ALL
LANGUAGE

fastlane files:

No Fastfile found

No Appfile found

fastlane gems

Gem Version Update-Status
fastlane 2.114.0 Up-To-Date

Loaded fastlane plugins:

No plugins Loaded

Loaded gems
Gem Version
did_you_mean 1.0.0
slack-notifier 2.3.2
atomos 0.1.3
CFPropertyList 3.0.0
claide 1.0.2
colored2 3.1.2
nanaimo 0.2.6
xcodeproj 1.7.0
rouge 2.0.7
xcpretty 0.3.0
terminal-notifier 1.8.0
unicode-display_width 1.4.0
terminal-table 1.8.0
plist 3.4.0
public_suffix 2.0.5
addressable 2.5.2
multipart-post 2.0.0
word_wrap 1.0.0
tty-screen 0.6.5
tty-cursor 0.6.0
tty-spinner 0.9.0
babosa 1.0.2
colored 1.2
highline 1.7.10
commander-fastlane 4.4.6
excon 0.62.0
faraday 0.15.4
unf_ext 0.0.7.5
unf 0.1.4
domain_name 0.5.20180417
http-cookie 1.0.3
faraday-cookie_jar 0.0.6
fastimage 2.1.5
gh_inspector 1.1.3
json 1.8.3.1
mini_magick 4.5.1
multi_json 1.13.1
multi_xml 0.6.0
rubyzip 1.2.2
security 0.1.3
xcpretty-travis-formatter 1.0.0
dotenv 2.6.0
bundler 2.0.1
faraday_middleware 0.12.2
naturally 2.2.0
simctl 1.6.5
uber 0.1.0
declarative 0.0.10
declarative-option 0.1.0
representable 3.0.4
retriable 3.1.2
mime-types-data 3.2018.0812
mime-types 3.2.2
jwt 2.1.0
signet 0.11.0
memoist 0.16.0
os 1.0.0
googleauth 0.6.7
httpclient 2.8.3
google-api-client 0.23.9
google-cloud-env 1.0.5
google-cloud-core 1.2.7
digest-crc 0.4.1
google-cloud-storage 1.15.0
emoji_regex 0.1.1
io-console 0.4.5
libxml-ruby 2.9.0
psych 2.1.0.1

generated on: 2019-01-22

@janpio

This comment has been minimized.

Copy link
Member

@janpio janpio commented Jan 22, 2019

(Please report your size issues in #13610 - we need more information there as we couldn't replicate this problem yet.)

What command did you execute?
Can you please share the complete output of the upload process?
Are those screenshots plain from snapshot or did you do any work on them?

Did you debug what screenshots App Store Connect is actually showing for "iPad Pro (2nd generation) 12.9-inch"? How does that match the output you posted? Did it also upload the 2nd generation screenshots?

@dayjer

This comment has been minimized.

Copy link
Author

@dayjer dayjer commented Jan 22, 2019

What command did you execute?

"fastlane deliver"

Can you please share the complete output of the upload process?

It uploads all assets successfully. I don't have the complete output, but it's your typical output for a successful "fastlane deliver" -- when I next update screenshots for an app I can paste it in here.

Are those screenshots plain from snapshot or did you do any work on them?

I had to change them to the correct size, as snapshot is currently generating a really wonky size for ipad Pro 3rd gen. I left the names in tact. I did not provide 2nd gen screenshots, just these ones so they could be used for all ipad's for the language. Otherwise, it uses english for 3rd gen. You bring up a good point though, because they have the same resolution and the 2nd gen ones are required. I wonder if copy pasting the screenshots in so there's 10 in total would solve my problem.

screen shot 2019-01-22 at 8 31 17 pm

screen shot 2019-01-22 at 8 29 14 pm

Presumably deliver see's the size matches the required size of the ipad pro 2nd gen and treats it as this despite the naming convention done by snapshot. I will try duplicating all my ipad screenshots and report back!

@janpio

This comment has been minimized.

Copy link
Member

@janpio janpio commented Jan 22, 2019

Very possible that deliver doesn't handle the screenshot file names correctly, so please report back what works or does not work for you.

Strange that it falls back to English instead of the screenshots of the almost identical size that are right there...

@dayjer

This comment has been minimized.

Copy link
Author

@dayjer dayjer commented Jan 22, 2019

Not almost identical, completely identical. Super weird. Maybe down the road they'll change it so the 3rd gen size is the required one. Will report back tomorrow if doubling up works.

@janpio

This comment has been minimized.

Copy link
Member

@janpio janpio commented Jan 22, 2019

(Researching this sent me down a very not pretty journey: #14121)

@dayjer

This comment has been minimized.

Copy link
Author

@dayjer dayjer commented Jan 23, 2019

Update: no way to deliver iPad 3rd gen screenshots via deliver. Doubling up the ipad screenshots just put 10 in each ipad 2nd gen list.

@hnakamura-fsv

This comment has been minimized.

Copy link

@hnakamura-fsv hnakamura-fsv commented Feb 15, 2019

Hey guys, any updates on this issue? I think this will cause problems once March 1st rolls around and iPad 3rd generation screenshots become required in appstore connect

@marktrobinson

This comment has been minimized.

Copy link

@marktrobinson marktrobinson commented Feb 15, 2019

Yep, doesn't upload the iPad Pro (12.9-inch) (3rd generation) screenshots for me either

@benzman81

This comment has been minimized.

Copy link

@benzman81 benzman81 commented Feb 20, 2019

Same here, no screentshots for iPad Pro (12.9-inch) (3rd generation). What I noticed is, that fastlane complains about too many screenshots, too. I generate screenshots for iPad Pro (12.9-inch) (2rd generation), too. So the cause might be, that 3rd Gen ist not handled correctly. In this file there are some comments about it, maybe those are the reason: https://github.com/fastlane/fastlane/blob/master/deliver/lib/deliver/app_screenshot.rb

@hnakamura-fsv

This comment has been minimized.

Copy link

@hnakamura-fsv hnakamura-fsv commented Feb 22, 2019

So the calculate_screen_size method evaluates the resolution of the image and maps to the various screensizes, which I guess is flawed because all generations of the iPad pro will map to ScreenSize:: IOS_IPAD_PRO which when uploading gets mapped to ipadPro: "MZPFT.SortedJ99ScreenShot" in DUClient (ignoring the entry for ipadPro129 which is the correct Validation-RuleSet for the 3rd gen slot on appstore connect)

Would it be a better approach to use subfolders to each device like supply does so there's no way of uploading to the incorrect place? The downside to that being its might be a bit more work to make sure the file structure is correct etc.

@squallstar

This comment has been minimized.

Copy link

@squallstar squallstar commented Mar 25, 2019

Is there any update on this? Apple will be requiring the new screenshots (for 12.9-inch iPad Pro 3rd generation) on March 27th, which is this Wednesday.

https://developer.apple.com/news/?id=03202019a
Starting March 27, 2019, all new apps and app updates for iPhone or iPad, including universal apps, must be built with the iOS 12.1 SDK or later and support iPhone XS Max or the 12.9-inch iPad Pro (3rd generation). Screenshots for these devices will also be required.

@benzman81

This comment has been minimized.

Copy link

@benzman81 benzman81 commented Mar 26, 2019

Tomorrow is March 27th. Is there anything happening here? Sorry to bother, but due to apple this is quite urgent if we want to keep using fastlane as a solution.

@janpio

This comment has been minimized.

Copy link
Member

@janpio janpio commented Mar 26, 2019

I don't see any Pull Requests that could be merged that promise to solve this problem :/ Fastlane is Open Source so that anyone can jump in and help solve any problems or obstacles they may be facing. The people spending more time on fastlane by being part of the team can't always solve all the problems.

@pingwinator

This comment has been minimized.

Copy link

@pingwinator pingwinator commented Mar 27, 2019

Is it make sense to use the same name convention, as Apple uses? I downloaded screenshots via deliver fastlane deliver download_screenshots and got files in the format:
%numer%_%deviceType%_%numer%.%originalName%.png, where
numer - is screenshot's order
deviceType - values like phone65 for iphone XS max, ipadPro for the ipad pro 12.9 2nd gen and ipadPro129 for the ipad pro 12.9 3rd gen
eg 3_ipadPro129_3.favorites.png

@pingwinator

This comment has been minimized.

Copy link

@pingwinator pingwinator commented Mar 27, 2019

As I understand, currently we have 2 blockers: no frames from a Facebook design team and frameit can't detect screenshots for the iPad pro 3rd gen.
I solved the first problem with Apple's marketing images. if you need asap to do screenshots, you could use my solution. it's not perfect, but better than nothing https://github.com/pingwinator/frameitIpadPro

@janpio

This comment has been minimized.

Copy link
Member

@janpio janpio commented Mar 27, 2019

Is it make sense to use the same name convention, as Apple uses? I downloaded screenshots via deliver fastlane deliver download_screenshots and got files in the format:
%numer%_%deviceType%_%numer%.%originalName%.png, where
numer - is screenshot's order
deviceType - values like phone65 for iphone XS max, ipadPro for the ipad pro 12.9 2nd gen and ipadPro129 for the ipad pro 12.9 3rd gen
eg 3_ipadPro129_3.favorites.png

Possibly, yes. You will have to investigate how this works with our current screenshot creation action and naming convention there.

@tonytlwu

This comment has been minimized.

Copy link

@tonytlwu tonytlwu commented Mar 28, 2019

image

As a reminder, starting March 27, 2019 all new apps and app updates for iPhone or iPad, including universal apps, will need to be built with the iOS 12.1 SDK and support iPhone XS Max or the 12.9-inch iPad Pro (3rd generation). Screenshots for these devices will also be required. All new apps and app updates for Apple Watch will need to be built with the watchOS 5.1 SDK and support Apple Watch Series 4. Learn More

It looks like Apple was ready to enforce screenshots on App Store Connect on March 27, but they haven't actually enforced it yet because when I attempt to create a new submission, screenshots for the iPhone XS Max and 12.9-inch iPad Pro (3rd generation) are still optional (see below).

image

I'm guessing this could happen at any moment? Or never? 🤷‍♂️

@codyrotwein

This comment has been minimized.

Copy link

@codyrotwein codyrotwein commented Apr 2, 2019

Looks like it's being enforced now.

@jnbt

This comment has been minimized.

Copy link
Contributor

@jnbt jnbt commented Apr 3, 2019

I don't see any Pull Requests that could be merged that promise to solve this problem :/ Fastlane is Open Source so that anyone can jump in and help solve any problems or obstacles they may be facing. The people spending more time on fastlane by being part of the team can't always solve all the problems.

This is a good point. For me the biggest problem working on this issue is, that it seems to be much coupled to other parts of the fastlane. What would be the best strategy to approach this issue? Should one first refactor the whole resolution-logic into a single module (see #14121), or try to "only" replace the auto-detection of "image size" -> "device type" with a less "clever" parsing of the file name.

We could use Apple's naming scheme as the base, but this would still be a breaking change.
This would not only require to rewrite parts of deliver, e.g. AppScreenshot::formatted_name or AppScreenshot::devices, but also other parts, e.g. Screenshot#device_name.

@tonytlwu

This comment has been minimized.

Copy link

@tonytlwu tonytlwu commented Apr 3, 2019

Looks like App Store Connect is enforcing it, but the API isn't? 🤔 We've managed to submit apps without all the required screenshots via fastlane, despite App Store Connect states they are all required.

@janpio

This comment has been minimized.

Copy link
Member

@janpio janpio commented Apr 4, 2019

@jnbt Yep, that is what makes it so hard: We have the upload part in deliver, the screenshot taking stuff in snapshot, and also some framing and positioning stuff in frameit.

In theory it would be enough if deliver somehow could upload the correct file to the correct location in a first step though. Maybe this could help to limit the scope of necessary changes and see how we can solve that.

Of course we optimally don't want any breaking changes at all. Making people change their screenshot names might be a valid option though (but that would also apply for downloading existing screenshots then - which is another complication).

I will try to spend some time thinking into this tomorrow... I gave up in January, but maybe now I can get further. Will document here as usual. (Please don't take this as a "You don't need to do anything" - all help or suggestions are welcome. I would love not to have to think about this again 🙈 )

@jnbt

This comment has been minimized.

Copy link
Contributor

@jnbt jnbt commented Apr 5, 2019

@janpio If we use Apple's (current) naming schema, a possible migration path for users with existing screenshots would be to use deliver to download all existing screenshots once. Do you know if there is a "official" document where the internal deviceType of the ITC API are listed?

@janpio

This comment has been minimized.

Copy link
Member

@janpio janpio commented Apr 5, 2019

No, most probably there is none as those APIs we are using in spaceship are all just the ones being used in the web interface (not the new incoming, official API). So your best bet is the spaceship source code or whatever requests the web UI sends.

@jnbt

This comment has been minimized.

Copy link
Contributor

@jnbt jnbt commented Apr 5, 2019

I also looked into the new official API, but it doesn't seem to contain a specification for these. (The new API seems to be also quite limited, regarding the meta information)

@liamnichols

This comment has been minimized.

Copy link
Contributor

@liamnichols liamnichols commented Apr 7, 2019

In theory it would be enough if deliver somehow could upload the correct file to the correct location in a first step though. Maybe this could help to limit the scope of necessary changes and see how we can solve that.

Of course we optimally don't want any breaking changes at all. Making people change their screenshot names might be a valid option though (but that would also apply for downloading existing screenshots then - which is another complication).

I'm aware that there are also wider problems to consider but following on from @pingwinator's suggestion, I had a little play around and maybe something like this might work as a backwards compatible change that fixes the issue in the context of deliver:

Since using deliver's download_screenshots command will save the file with the correct device_type in the format of "{sort_order}_{device_type}_{sort_order}.{original_file_name}", we could introduce an isolated fix that does something like the following inside app_screenshot.rb:

 # The iTC API requires a different notation for the device
 def device_type
-  # This list does not include iPad Pro 12.9-inch (3rd generation)
-  # because it has same resoluation as IOS_IPAD_PRO and will clobber
+
+  basename = File.basename(path)
+  device_type = basename[/^([0-9]+)_(\S+)_(\1)/, 2]
+  if Spaceship::Tunes::DeviceType.exists?(device_type)
+    return device_type
+  end
+
   matching = {
     ScreenSize::IOS_35 => "iphone35",
     # ...
     ScreenSize::APPLE_TV => "appleTV"
   }
   return matching[self.screen_size]
 end

(Please excuse my poor Ruby)

I think that this would "fix" the way that deliver works in the sense that you could use it in isolation to download all of your apps screenshots and then upload them again without anything breaking and as a result, your 3rd generation iPad Pro screenshots will still exist in AppStoreConnect at the end of it all...

There is a bit more to it than just that, but this approach basically means that we could start moving away from relying on the image resolution alone to detect device_type and the only two solutions I see are either adding it into the filename or nesting in directories that are named after the device types

@brightsider

This comment has been minimized.

Copy link

@brightsider brightsider commented Apr 9, 2019

Any news?

@liamnichols

This comment has been minimized.

Copy link
Contributor

@liamnichols liamnichols commented Apr 9, 2019

@janpio - it's just one of the values listed here: https://github.com/fastlane/fastlane/blob/master/spaceship/lib/spaceship/tunes/device_type.rb

Problem with those names is that they're a little verbose (i.e iphone6 literally means 'iPhone 6 (or 7 or 8)' whereas iphone65 means 6.5" iPhone (iPhone XS Max)) .. I was wondering if Fastlane itself should come up with some more user-friendly definitions that we can map between? Alternatively we could just add a table to the documentation...

Also I was thinking a little more about the naming convention of the downloaded images in deliver and was wondering if maybe actually we shouldn't rely on that.. I feel like having directories in each language folder might be a better idea actually because say that you have 10 screenshots for each language.. to support each required device types that will be 40 files in a directory and maybe its a bit too much... it might be nicer to encourage the use of subdirectories instead? Like you said, we could still just support both approaches but maybe it will be nicer to push towards using subdirectories

@janpio

This comment has been minimized.

Copy link
Member

@janpio janpio commented Apr 10, 2019

Using directories would also enable us to implement some "initialize" method that creates the appropriate folder structure for the user, where they just have to drop their files.

The basic problem of the naming is that Apple itself is inconsistent here, mixing different conventions as they see fit. If we can find a different, more consistent naming scheme it would probably be beneficial to fastlane users.

Maybe more aligned to how the App Store Connect UI labels them?

@liamnichols

This comment has been minimized.

Copy link
Contributor

@liamnichols liamnichols commented Apr 10, 2019

Maybe more aligned to how the App Store Connect UI labels them?

This could be a good idea...

device_type App Store Connect Label
iphone65 6.5-inch Display
iphone58 5.8-inch Display
iphone6Plus 5.5-inch Display
iphone6 4.7-inch Display
iphone4 4-inch Display
iphone35 3.5-inch Display
ipadPro129 iPad Pro (3rd Generation) 12.9-inch Display
ipadPro11 11-inch Display
ipadPro iPad Pro (2nd Generation) 12.9-inch Display
ipad105 10.5-inch Display
ipad 9.7-inch Display

The iPad Pro situation even in App Store Connect is a bit horrific 😅 I worry about the future lol

If we go with this approach, and then the UI labels update, do you think that would cause more problems or would we be able to update the label definitions in a new version of deliver? 🤔

@janpio

This comment has been minimized.

Copy link
Member

@janpio janpio commented Apr 10, 2019

If we go with this approach, and then the UI labels update, do you think that would cause more problems or would we be able to update the label definitions in a new version of deliver? 🤔

Nope, that's the one thing that I see as a problem. As soon as we create folder or files on user machines with those, we pretty much should support those. But if we take one more step, and abstract those UI labels to our own system, we could maybe be fine.

(Until Apple releases some new devices that don't match with our abstraction... that's why using the device_type would be "nice" - it's just what their API uses.)

Meh.

@janpio

This comment has been minimized.

Copy link
Member

@janpio janpio commented Apr 10, 2019

Having written this, it is probably our best bet to just go with device_type as our identifier as an evolution of the screen sizes, and use your table above as the base for some proper documentation. Yes, the filenames/folder names by themselves would be confusing to users, but as long as Apple doesn't completely replace/change their API we should be fine.

@janpio

This comment has been minimized.

Copy link
Member

@janpio janpio commented Apr 10, 2019

Some context: supply creates this file structure for Android app's metadata on supply init:

        └───images
            ├───phoneScreenshots
            ├───sevenInchScreenshots
            ├───tenInchScreenshots
            ├───tvScreenshots
            └───wearScreenshots
@dave-perry

This comment has been minimized.

Copy link

@dave-perry dave-perry commented Apr 10, 2019

Looks like this is being enforced on the API now, I'm getting an error when trying to submit an app:

You must add at least one screenshot. There is an error for 1 of your localizations.

@nolanwilson

This comment has been minimized.

Copy link
Contributor

@nolanwilson nolanwilson commented Apr 10, 2019

Looks like this is being enforced on the API now, I'm getting an error when trying to submit an app:

Fwiw, I just got off the phone with apple developer support and they're not even aligned on what's supposed to be enforced right now. Their first response was that the 3rd generation screenshots are required and the 2nd gen are optional. But when I informed them that that's not how Appstore Connect is enforcing things right now (both 2nd and 3rd gen are currently required), they looked up the "official" (read: public-facing) support docs and then stuck with the line that both are required. I tried to point out the redundancy in this requirement but was told to file a bug report through "Bug Reporter" which I've done. Who knows if/when this will change though

If history is any indication of the future, at some point the 2nd generation screenshots will no longer be required and the 3rd gen screenshots will be.

But this is all just to hopefully provide folks some sanity as they are trying to reconcile all of the changes here. As of right now, it looks like fastlane is unable to submit iOS apps through Appstore Connect because of the new requirement of 3rd gen screenshots :(

@janpio

This comment has been minimized.

Copy link
Member

@janpio janpio commented Apr 10, 2019

Still thinking about way to fix that identifier mess, which could give us a proper way forward:

I just had a look at Apple's identifiers that @liamnichols provided the nice summary table for: #14116 (comment)

The odd ones out, that do not have the inch size in their label, are iphone6, iphone6Plus, ipad and ipadPro. For the first 3, we can actually create the "correct" labels: iphone47, iphone55 and ipad97.
That way we could create our own, consistent naming scheme that could be used for e.g. folders or file names to identify the screenshots (that actually would accept both values - the Apple one and our more consistent alternative).
You could even just use the "inch" value as a proper identifier.

Only for the ipadPro, which represents the "iPad Pro (2nd Generation) 12.9-inch Display" this would clash with ipadPro129 that already represents "iPad Pro (3rd Generation)".

device_type possible device type alias inch
iphone35 3.5
iphone4 4
iphone6 iphone47 4.7
iphone6Plus iphone55 5.5
iphone58 5.8
iphone65 6.5
ipad ipad97 9.7
ipad105 10.5
ipadPro ??? 12.9 💣
ipadPro11 11
ipadPro129 12.9 💣

I am not fully knowledgeable about the newer iPad Pro 11 and iPad Pro (3rd Generation) - what is so different about them, that they get their own "screenshot category"?

How are screenshots between these two devices different in appearance?

@aastlind

This comment has been minimized.

Copy link
Contributor

@aastlind aastlind commented Apr 10, 2019

How are screenshots between these two devices different in appearance?

I guess it's because the 3rd gen is without a home button, so if you frame your screenshots it should look like the current device you're browsing App Store on. I had apps rejected previously just because it's framed in the wrong device (i.e. iPhone 8+ on a iPhone X device or vice versa).

@janpio

This comment has been minimized.

Copy link
Member

@janpio janpio commented Apr 11, 2019

Here someone created a super quick hack to work around the problem: #14474 (comment)

@hnakamura-fsv

This comment has been minimized.

Copy link

@hnakamura-fsv hnakamura-fsv commented Apr 11, 2019

something like this could also be an option to map the device id to folders and/or screenshot types - it might be more reliable than going with the device name itself since it can potentially change with renamed simulators etc

@liamnichols

This comment has been minimized.

Copy link
Contributor

@liamnichols liamnichols commented Apr 11, 2019

I had a go at implementing a change that allows you to nest screenshots within folders named after each device_type as suggested previously (I.e /screenshots/en-GB/ipadPro129/image.png)

I uncovered a lot of things that made it hard to achieve the desired change without being sure that I’m not introducing regressions though

A rough version can be found here for reference: liamnichols/fastlane@master...experimental-fix

My plan is to try and introduce the changes in small and testable chunks but I’m not really an expert with ruby so this is going to take a bit of time

The first two parts however are #14533 and #14574

@janpio

This comment has been minimized.

Copy link
Member

@janpio janpio commented Apr 12, 2019

@hnakamura-fsv You mean using the technical device identifier (instead of something the API or App Store Connect uses)? Interesting idea, although I always hated how there are multiple per device and e.g. iPad6,11 is actually an iPad 5... Confusing much :/ But at least consistent and independent from whatever the API uses, so that's a plus.

Oh so complicated.


What do you all think about maybe adding something like #14474 (comment) as a temporary workaround to just enable all fastlane users to upload their apps with full metadata and screenshots again?

@liamnichols

This comment has been minimized.

Copy link
Contributor

@liamnichols liamnichols commented Apr 12, 2019

What do you all think about maybe adding something like #14474 (comment) as a temporary workaround to just enable all fastlane users to upload their apps with full metadata and screenshots again?

I guess that my one question would be about how we migrate support for this workaround going forward? There are already lots of custom edge cases that weren't really documented/tested and I am wondering if checking the filename for "(3rd generation)" forever going forward is going to be something else that has to remain in the codebase? Or will it be possible to make this a breaking change once a more suitable long-term solution is introduced?

I'm not really sure how these things work in the open source world

@janpio

This comment has been minimized.

Copy link
Member

@janpio janpio commented Apr 12, 2019

I'm not really sure how these things work in the open source world

We make it up as we go... 🥇

Seriously: If we can't figure out a backwards compatible solution to solve this whole mess properly, we will have to either a) introduce fastlane 3.x or b) add an option to switch from the old to the new behavior.

As both things are not really what we want (for a) people will make assumptions about a major verison increase that are just not true, like a lot of new functionality, all the old inconsistencies being fixed etc), and for b) needing an option will leave out new users) I am still hoping we can just find a "different" way to do things that works.

For a workaround that works now:
Introducing some code that is triggered by e.g. (3rd generation) in a file name would be fine from a semver view point - it fixes an existing bug with new behaviour. But if Apple ever releases another device with that string in the official name, we will to deal with it then.

Oh so complicated :/

janpio added a commit that referenced this issue Apr 13, 2019
<!-- Thanks for contributing to _fastlane_! Before you submit your pull request, please make sure to check the following boxes by putting an x in the [ ] (don't: [x ], [ x], do: [x]) -->

### Checklist
- [x] I've run `bundle exec rspec` from the root directory to see all new and existing tests pass
- [x] I've followed the _fastlane_ code style and run `bundle exec rubocop -a` to ensure the code style is valid
- [x] I've read the [Contribution Guidelines](https://github.com/fastlane/fastlane/blob/master/CONTRIBUTING.md)
- [x] I've updated the documentation if necessary.

### Motivation and Context

The iPad Pro 12.9" (3rd Generation) has the same resolution as the 2nd and 1st Generation versions however in App Store Connect, they have different slots (and `device_type` values) for screenshots and this breaks a fundamental assumption that `deliver` has made when uploading screenshots.  

#14116 is tracking the actual issue itself and its pretty certain that `app_screenshot.rb` and `upload_screenshots.rb` will need to be updated to account for this however while I was investigating, I struggled undemanding all of the complicated things that these modules are handling so decided to try and write some tests for the existing behaviour.

### Description

These changes simply introduce an additional spec to test the collection of screenshots by `Deliver::UploadScreenshots`. The following cases are tested: 

- An empty directory (no screenshots)
- The most basic case of a screenshot
- Excluding device_type's unsupported by App Store Connect (the iPhone XR)
- Localisation
- Excluding original screenshots if _framed variants exist
  - But preserving Apple Watch screenshots without frames
- The magic appleTV directory
- iMessage screenshots

Since this module relies heavily on the file system, I copied the approach used in loader_spec.rb and used FakeFS to simulate things. Not sure if it's the best approach but it seems to have worked!
@cherpake

This comment has been minimized.

Copy link

@cherpake cherpake commented Apr 14, 2019

I had a go at implementing a change that allows you to nest screenshots within folders named after each device_type as suggested previously (I.e /screenshots/en-GB/ipadPro129/image.png)

I uncovered a lot of things that made it hard to achieve the desired change without being sure that I’m not introducing regressions though

A rough version can be found here for reference: liamnichols/fastlane@master...experimental-fix

My plan is to try and introduce the changes in small and testable chunks but I’m not really an expert with ruby so this is going to take a bit of time

The first two parts however are #14533 and #14574

Still have to sort the screenshots to preserve the order.

@cherpake

This comment has been minimized.

Copy link

@cherpake cherpake commented Apr 14, 2019

Here is my path for supporting 10 screenshots per device + sorting them to preserve the order.

diff --git a/deliver/lib/deliver/upload_screenshots.rb b/deliver/lib/deliver/upload_screenshots.rb
index 2778a7b5d..dde46c2af 100644
--- a/deliver/lib/deliver/upload_screenshots.rb
+++ b/deliver/lib/deliver/upload_screenshots.rb
@@ -50,18 +50,18 @@ module Deliver
       screenshots_per_language.each do |language, screenshots_for_language|
         UI.message("Uploading #{screenshots_for_language.length} screenshots for language #{language}")
         screenshots_for_language.each do |screenshot|
-          indized[screenshot.language] ||= {}
-          indized[screenshot.language][screenshot.formatted_name] ||= 0
-          indized[screenshot.language][screenshot.formatted_name] += 1 # we actually start with 1... wtf iTC
+          indized[screenshot.language+screenshot.device_type] ||= {}
+          indized[screenshot.language+screenshot.device_type][screenshot.formatted_name] ||= 0
+          indized[screenshot.language+screenshot.device_type][screenshot.formatted_name] += 1 # we actually start with 1... wtf iTC
 
-          index = indized[screenshot.language][screenshot.formatted_name]
+          index = indized[screenshot.language+screenshot.device_type][screenshot.formatted_name]
 
           if index > 10
             UI.error("Too many screenshots found for device '#{screenshot.formatted_name}' in '#{screenshot.language}', skipping this one (#{screenshot.path})")
             next
           end
 
-          UI.message("Uploading '#{screenshot.path}'...")
+          UI.message("Uploading '#{screenshot.path}' for '#{screenshot.device_type}'...")
           v.upload_screenshot!(screenshot.path,
                                index,
                                screenshot.language,
@@ -105,6 +105,7 @@ module Deliver
         files = Dir.glob(File.join(lng_folder, "*.#{extensions}"), File::FNM_CASEFOLD)
         files.concat(Dir.glob(File.join(lng_folder, device_types, "*.#{extensions}"), File::FNM_CASEFOLD))
         next if files.count == 0
+        files = files.sort
 
         framed_screenshots_found = files.any? { |file| file_is_framed?(file) }
 
joshdholtz added a commit to bguidolim/fastlane that referenced this issue May 3, 2019
…lane#14533)

<!-- Thanks for contributing to _fastlane_! Before you submit your pull request, please make sure to check the following boxes by putting an x in the [ ] (don't: [x ], [ x], do: [x]) -->

### Checklist
- [x] I've run `bundle exec rspec` from the root directory to see all new and existing tests pass
- [x] I've followed the _fastlane_ code style and run `bundle exec rubocop -a` to ensure the code style is valid
- [x] I've read the [Contribution Guidelines](https://github.com/fastlane/fastlane/blob/master/CONTRIBUTING.md)
- [x] I've updated the documentation if necessary.

### Motivation and Context

The iPad Pro 12.9" (3rd Generation) has the same resolution as the 2nd and 1st Generation versions however in App Store Connect, they have different slots (and `device_type` values) for screenshots and this breaks a fundamental assumption that `deliver` has made when uploading screenshots.  

fastlane#14116 is tracking the actual issue itself and its pretty certain that `app_screenshot.rb` and `upload_screenshots.rb` will need to be updated to account for this however while I was investigating, I struggled undemanding all of the complicated things that these modules are handling so decided to try and write some tests for the existing behaviour.

### Description

These changes simply introduce an additional spec to test the collection of screenshots by `Deliver::UploadScreenshots`. The following cases are tested: 

- An empty directory (no screenshots)
- The most basic case of a screenshot
- Excluding device_type's unsupported by App Store Connect (the iPhone XR)
- Localisation
- Excluding original screenshots if _framed variants exist
  - But preserving Apple Watch screenshots without frames
- The magic appleTV directory
- iMessage screenshots

Since this module relies heavily on the file system, I copied the approach used in loader_spec.rb and used FakeFS to simulate things. Not sure if it's the best approach but it seems to have worked!
@petekanev

This comment has been minimized.

Copy link

@petekanev petekanev commented May 8, 2019

This is also a problem with the iPhone XS Max screenshots - when in the same screenshots/en-US/ directory as iPhone 8 Plus snaps, only the iPhone 8 ones get picked up and uploaded.

@janpio

This comment has been minimized.

Copy link
Member

@janpio janpio commented May 8, 2019

Huh? Could you please create a new issue @petekanev and provide more information? That should be investigated.

@hellkith

This comment has been minimized.

Copy link

@hellkith hellkith commented May 20, 2019

Has there been any progress with a fix for this issue? For now I'm still manually adding the 3rd gen screenshots, but having deliver do so for me makes it alot easier.

@janpio

This comment has been minimized.

Copy link
Member

@janpio janpio commented May 20, 2019

Yes, the original issue of not being able to upload a iPad Pro 12.9 3rd gen screenshot should be fixed: https://docs.fastlane.tools/actions/deliver/#uploading-screenshots-for-ipad-pro-129-inch-3rd-generation

Related problems like size differences, not being able to frame the screenshot etc might be not - but there are individual issues for these problems.

@janpio janpio closed this May 20, 2019
@fastlane fastlane locked and limited conversation to collaborators Jul 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
You can’t perform that action at this time.