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

Couple of native code fixes #722

Closed
shankari opened this issue Apr 26, 2022 · 14 comments · Fixed by e-mission/e-mission-phone#821
Closed

Couple of native code fixes #722

shankari opened this issue Apr 26, 2022 · 14 comments · Fixed by e-mission/e-mission-phone#821

Comments

@shankari
Copy link
Contributor

shankari commented Apr 26, 2022

Crash reported in the app store

Screen Shot 2022-04-26 at 10 02 28 AM

Related code is:

    NSAssert([lastLocArray count] > 0, @"Found no locations while ending trip!");

UI overlaps the cutout for the camera on newer iPhone versions

Force-kill message is too long to view but doesn't show details when app is opened

Looks like I fixed this anyway....

overlap force kill
Screen Shot 2022-04-26 at 10 26 47 AM Screen Shot 2022-04-26 at 10 41 17 AM
@shankari
Copy link
Contributor Author

cutout overlap seems to require viewport-fit=cover
Not sure if that works with ionic 1

@shankari
Copy link
Contributor Author

For the first issue, let's replace the assert by

    if ([lastLocArray count] == 0) {
        StatsEvent* se = [[StatsEvent alloc] initForEvent:@"no_locations_while_ending_trip"];
        [[BuiltinUserCache database] putMessage:@"key.usercache.client_error" value:se];
    }

@shankari
Copy link
Contributor Author

shankari commented Apr 26, 2022

There are a bunch of additional asserts in the transition notifier code.
Let's replace all of them for now (already removed one in 5a765547f58a8e73668189ffc8d1a0040bfdb3c5; let's replace the others with client stats as well).

We may also consider removing the plugin completely since nobody seems to want to use it.

@shankari
Copy link
Contributor Author

for the second issue:
https://developer.mozilla.org/en-US/docs/Web/HTML/Viewport_meta_tag

https://udn.realityripple.com/docs/Web/CSS/@viewport/viewport-fit

The viewport is scaled to fill the device display. It is highly recommended to make use of the safe area inset variables to ensure that important content doesn't end up outside the display.

@shankari
Copy link
Contributor Author

So ionic1 does not support the safe area inset variables

$ grep -r safe-area-inset-top css/ js/
$

Tried to add them manually per instructions:
https://developer.mozilla.org/en-US/docs/Web/CSS/env
https://udn.realityripple.com/docs/Web/CSS/env

body {
  padding:
    env(safe-area-inset-top, 20px)
    env(safe-area-inset-right, 20px)
    env(safe-area-inset-bottom, 20px)
    env(safe-area-inset-left, 20px);
}

But they are overridden by app.css from www/build/app.css.

Screen Shot 2022-04-26 at 12 48 00 PM

All the stuff in www/build/ is related to habitica.
https://github.com/e-mission/e-mission-phone/commits/master/www/build/app.css

Can we just remove it, or will it break anything else?

@shankari
Copy link
Contributor Author

shankari commented Apr 26, 2022

removed, next overridden by entry is styles.css
removed that as well.

It now works but looks ugly
Screen Shot 2022-04-26 at 1 04 41 PM

and we can't even see the tab bar at the bottom

Screen Shot 2022-04-26 at 1 05 29 PM

@shankari
Copy link
Contributor Author

shankari commented Apr 26, 2022

Turned out that this was because the ion-view was set to a height of 100%, which caused it to go off the screen if we changed the padding of the body.

Further changes:

@shankari
Copy link
Contributor Author

shankari commented Apr 26, 2022

No body Body but no scroll content height Body but with a limited offset Everything + scroll height
165403638-7badcf66-aa2a-4a83-9ecc-3fb6da939294 165403035-6a83ace0-90c5-45e5-a15d-2d8a46565c0e 165404209-7e36d081-49bf-4027-a200-a17ccf7832e5 165405253-f5aad4a6-f749-4564-87b0-a2d95e451db0

@shankari
Copy link
Contributor Author

Apparently, the right way to select by class is:

body.platform-ios {
  padding-top: calc(env(safe-area-inset-top) / 2);
  margin: 0 10px 0 0;
}

.platform-ios .view-container {
  height: calc(100% - (env(safe-area-inset-top) / 2));
}

.platform-ios .scroll-content {
    height: calc(100% - 50px);
}

@shankari
Copy link
Contributor Author

Looks fine on both android and iOS

android iOS
Screen Shot 2022-04-26 at 4 26 28 PM Screen Shot 2022-04-26 at 4 25 10 PM

shankari added a commit to shankari/e-mission-transition-notify that referenced this issue Apr 27, 2022
This is a fix for the first issue in
e-mission/e-mission-docs#722
notably, converting all the asserts to stats so that the app won't crash
e-mission/e-mission-docs#722 (comment)
and returning NULL from those conditions

The new stats names are stored in the `cordova-usercache` plugin
@shankari
Copy link
Contributor Author

One final issue: Giving additional notification privileges causes the notification check on iOS to fail.
We currently request

    return [UIUserNotificationSettings
            settingsForTypes:UIUserNotificationTypeAlert|UIUserNotificationTypeBadge
            categories:nil];

which maps to a types of 5
https://developer.apple.com/documentation/uikit/uiusernotificationtype?language=objc

If the user allows sound as well, the requested types are 5, but the provided types are 7, which causes this check to fail

return requestedSettings.types == currSettings.types;

Couple of potential fixes:

  • request everything including sound
  • check for provided > requested
    • this could be incorrect because UIUserNotificationTypeSound|UIUserNotificationTypeAlert = 6 > 5 but it means that we don't have the badge permission
  • check for the actual permissions using bitwise operators
    • this should be actual settings & requested settings = requested settings

shankari added a commit to shankari/e-mission-data-collection that referenced this issue Apr 27, 2022
To ensure that we don't fail if we have more permissions than requested
e-mission/e-mission-docs#722 (comment)

Without this fix, having sound enabled for app notifications would cause the
checks to fail to fail, since the requested flags = 5 and the actual flags = 7

With this fix, we bitwise and the two values to remove the excess permissions.
7 (111) & 5 (101) = 101 = 5 = requested permissions
shankari added a commit to shankari/e-mission-phone that referenced this issue Apr 27, 2022
This fixes the second issue in
e-mission/e-mission-docs#722 (comment)

In particular, we add the `viewport-fit=cover` tag.
Unfortunately, since we are using ionic version 1, it doesn't automatically set
all the safe areas correctly.

So we had to figure out how to send them manually.
Finally got it to work, applied it only to iOS
e-mission/e-mission-docs#722 (comment)

Both iOS and android work well now, at least in profile mode
e-mission/e-mission-docs#722 (comment)
shankari added a commit to shankari/e-mission-phone that referenced this issue Apr 27, 2022
- notification check: e-mission-data-collection
    e-mission/e-mission-docs#722 (comment)
- crashes e-mission-transition-notify, cordova-usercache
    e-mission/e-mission-docs#722 (comment)
@shankari
Copy link
Contributor Author

shankari commented Apr 27, 2022

There seems to be a minor difference between CanBikeCO and master - CanBikeCO does not need the scroll-content fix. And in fact, if it has the scroll-context fix, it breaks because we cannot see the lowest card fully. Fortunately, android is not affected.

with scroll-content override without scroll-content override android
Screen Shot 2022-04-26 at 10 31 05 PM Screen Shot 2022-04-26 at 10 28 49 PM Screen Shot 2022-04-26 at 10 33 32 PM

@shankari
Copy link
Contributor Author

@asiripanich here are some fit and finish fixes for iOS, including a crash and the "notch" UI on recent model phones.
LMK if you notice any of them!

@asiripanich
Copy link
Member

@shankari thanks! I will get our team to test this.

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 a pull request may close this issue.

2 participants