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.

Sign up for GitHub

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

Jump to bottom

Update MobileModel parseDescriptor to support iPhone 11 #3277

Merged
merged 2 commits into from
Nov 12, 2019
Merged

Update MobileModel parseDescriptor to support iPhone 11 #3277

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0cdf54a
Update MobileModel parseDescriptor to support iPhone 11
devinbileck Sep 17, 2019
4c27d08
Update to reflect isContentAvailable supports iPhone 6s and newer
devinbileck Oct 30, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 16 additions & 2 deletions core/src/main/java/bisq/core/notifications/MobileModel.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@

import com.google.common.annotations.VisibleForTesting;

import java.util.Arrays;

import lombok.Data;
import lombok.Getter;
import lombok.extern.slf4j.Slf4j;
Expand Down Expand Up @@ -107,6 +109,12 @@ boolean parseDescriptor(String descriptor) {
iPhone 8
iPhone 8 Plus
iPhone X
iPhone XS
iPhone XS Max
iPhone XR
iPhone 11
iPhone 11 Pro
iPhone 11 Pro Max
iPad 2
iPad 3
iPad 4
Expand All @@ -131,9 +139,15 @@ boolean parseDescriptor(String descriptor) {
String model = descriptorTokens[0];
if (model.equals("iPhone")) {
String versionString = descriptorTokens[1];
versionString = versionString.substring(0, 1);
if (versionString.equals("X") || versionString.equals("SE"))
String[] validVersions = {"X", "XS", "XR"};
if (Arrays.asList(validVersions).contains(versionString)) {
return true;
}
if (versionString.matches("\\d[^\\d]")) {
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 •
edited

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.

Copy link

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.

Expand Down
11 changes: 9 additions & 2 deletions core/src/test/java/bisq/core/notifications/MobileModelTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,14 @@ public void testParseDescriptor() {
new Tuple2<>("iPhone 8", true),
new Tuple2<>("iPhone 8 Plus", true),
new Tuple2<>("iPhone X", true),
new Tuple2<>("iPhone XS", true),
new Tuple2<>("iPhone XS Max", true),
new Tuple2<>("iPhone XR", true),
new Tuple2<>("iPhone 11", true),
new Tuple2<>("iPhone 11 Pro", true),
new Tuple2<>("iPhone 11 Pro Max", true),
new Tuple2<>("iPhone 11S", true), // not sure if this model will exist, but based on past versioning it is possible
// need to ensure it will be parsed correctly just in case

new Tuple2<>("iPad 2", false),
new Tuple2<>("iPad 3", false),
Expand All @@ -74,8 +82,7 @@ public void testParseDescriptor() {
);

list.forEach(tuple -> {
log.error(tuple.toString());

log.info(tuple.toString());
assertEquals("tuple: " + tuple, mobileModel.parseDescriptor(tuple.first), tuple.second);
});

Expand Down