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

Update MobileModel parseDescriptor to support iPhone 11 #3277

Merged

Conversation

devinbileck
Copy link
Member

@devinbileck devinbileck commented Sep 17, 2019

This is just something I stumbled upon during a review.

It was parsing only the first digit of the version and using that in a
comparison check to determine if the version is greater than 5.
This meant for the iPhone 11 it was comparing 1 > 5, returning an
incorrect result.

It now supports multi-digit version numbers (i.e. 11), including
support for a suffix in the version (i.e. 11S), just in case...

It was parsing only the first digit of the version and using that in a
comparison check to determine if the version is greater than 5.
This meant for the iPhone 11 it was comparing 1 > 5, returning an
incorrect result.

It now supports multi-digit version numbers (i.e. 11), including
support for a suffix in the version (i.e. 11S), just in case...
versionString = versionString.substring(0, 1);
} else if (versionString.matches("\\d{2}[^\\d]")) {
versionString = versionString.substring(0, 2);
}
try {
int version = Integer.parseInt(versionString);
return version > 5;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing I would like clarification on, if anyone knows...

From the comment on line 134:

    // iPhone 6 does not support isContentAvailable, iPhone 7 does.
    // We don't know for other versions, but lets assume all above iPhone 6 are ok.

If that is the case, I assume this should be changed to:
return version > 6;

Unless the comment is incorrect?

I am not familiar with isContentAvailable, but is this capability really device dependent?
It appears content-available was added to notifications in iOS7: https://stackoverflow.com/questions/20690220/proper-use-of-content-available-in-ios-7-push-notifications
And iOS7 is supported as far back as iPhone 4: http://osxdaily.com/2013/06/10/ios-7-supported-devices/

So then maybe it should be:
return version >= 4;

Copy link
Contributor

@ripcurlx ripcurlx Sep 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it looks like as if the check is wrong. Even if the hardware string is used it must be even higher (iPhone9,1+ == iPhone 7). Maybe @joachimneumann could shed some light on this.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,

you are probably right. I have copied the file MobileModel.java from somewhere on the Internet and also, I have not spend a lot of time to identify the hardware and software requirements. I only remember that the notifications did not work on an iPhone 6, but they did work on an iPhone 6S. Since newer operating systems (still true for iOS 13) run on the iPhone 6S, but not on the iPhone 6 and I assumed that most user will update their OS, I opted for the lazy route.

If you have older hardware and/or iPhones with older OS versions to test this, feel free to correct the requirements.

Joachim

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I have updated this PR to reflect isContentAvailable supports iPhone 6s and newer.

Copy link
Contributor

@ripcurlx ripcurlx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

@ripcurlx ripcurlx merged commit ce20bb4 into bisq-network:master Nov 12, 2019
@devinbileck devinbileck deleted the update-mobile-model-parse-descriptor branch November 12, 2019 21:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants