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

[android] Remove "share" and "save to launcher" actions from the persistent notification #10333

Merged

Conversation

sjchmiela
Copy link
Contributor

@sjchmiela sjchmiela commented Sep 23, 2020

Why

Kind of fixes #10301 (well, removing the buggy feature is kind of a fix).

How

First I found out where the layout for the notification is held (notification{.xml,_shell_app.xml}). I noticed that notification_shell_app.xml defines exactly the layout we would like to keep, so I replaced notification.xml (used in Expo client) with notification_shell_app.xml (used in shell apps if androidShowExponentNotificationInShellApp manifest flag is true), removing the original one (now there's only one layout).

After removing buttons from the layout I had to also remove the code that bound actions to the buttons. I then went down the rabbit hole to code that was called and removed it too (see ExponentIntentService, Kernel#installShortcut).

Note: I left code responsible for handling shortcut opens so this is still supported.

Test Plan

I have run Expo client with these changes and verified that there are no share or pin icons anymore.

@sjchmiela sjchmiela force-pushed the @sjchmiela/remove-extra-buttons-exponent-notification branch from c310a19 to 602c9d7 Compare September 23, 2020 17:07
@sjchmiela sjchmiela changed the title [android] Remove info, share and save to launcher actions from the persistent "Exponent notification" [android] Remove "share" and "save to launcher" actions from the persistent notification Sep 23, 2020
@sjchmiela sjchmiela marked this pull request as ready for review September 23, 2020 17:07
Copy link
Member

@brentvatne brentvatne left a comment

Choose a reason for hiding this comment

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

hurray! i totally forgot about androidShowExponentNotificationInShellApp - perhaps we should remove that at some point too.

sjchmiela added a commit that referenced this pull request Sep 24, 2020
…pp` (#10335)

# Why

Let's go with the flow! #10333 (review). A prerequisite for this PR (I guess? Maybe the other way around?) is expo/universe#5878.

# How

Looked into all the places where `androidShowExponentNotificationInShellApp` and its constant was used, there was one — whether to display the "Exponent persistent notification" in standalone app or not. Since the default is not to show the notification, I've added `mIsShellApp ||` to an if that fast-exits from the `addNotification` method so that the notification is not added to standalone apps.

# Test Plan

I have confirmed the notification is still displayed when an experience is run in Expo client.
sjchmiela added a commit that referenced this pull request Sep 24, 2020
# Why

Follow up to #10333. Removes `InfoActivity` we replaced with sophisticated dev menu.

# How

Removed `InfoActivity`, its layout, assets (`dev_menu` icon) and the code that showed the activity.

# Test Plan

I have confirmed Android Expo client compiled and tapping on the app name in the persistent notification does not open the `InfoActivity`.
@sjchmiela sjchmiela force-pushed the @sjchmiela/remove-extra-buttons-exponent-notification branch from 602c9d7 to 0998ada Compare September 24, 2020 13:22
@sjchmiela sjchmiela merged commit 6268795 into master Sep 24, 2020
@sjchmiela sjchmiela deleted the @sjchmiela/remove-extra-buttons-exponent-notification branch September 24, 2020 13:25
brentvatne pushed a commit that referenced this pull request Oct 1, 2020
…pp` (#10335)

# Why

Let's go with the flow! #10333 (review). A prerequisite for this PR (I guess? Maybe the other way around?) is expo/universe#5878.

# How

Looked into all the places where `androidShowExponentNotificationInShellApp` and its constant was used, there was one — whether to display the "Exponent persistent notification" in standalone app or not. Since the default is not to show the notification, I've added `mIsShellApp ||` to an if that fast-exits from the `addNotification` method so that the notification is not added to standalone apps.

# Test Plan

I have confirmed the notification is still displayed when an experience is run in Expo client.
brentvatne pushed a commit that referenced this pull request Oct 1, 2020
# Why

Follow up to #10333. Removes `InfoActivity` we replaced with sophisticated dev menu.

# How

Removed `InfoActivity`, its layout, assets (`dev_menu` icon) and the code that showed the activity.

# Test Plan

I have confirmed Android Expo client compiled and tapping on the app name in the persistent notification does not open the `InfoActivity`.
brentvatne pushed a commit that referenced this pull request Oct 1, 2020
…istent notification (#10333)

# Why

Kind of fixes #10301 (well, removing the buggy feature is kind of a fix).

# How

First I found out where the layout for the notification is held (`notification{.xml,_shell_app.xml}`). I noticed that `notification_shell_app.xml` defines exactly the layout we would like to keep, so I replaced `notification.xml` (used in Expo client) with `notification_shell_app.xml` (used in shell apps if `androidShowExponentNotificationInShellApp` manifest flag is `true`), removing the original one (now there's only one layout).

After removing buttons from the layout I had to also remove the code that bound actions to the buttons. I then went down the rabbit hole to code that was called and removed it too (see `ExponentIntentService`, `Kernel#installShortcut`).

**Note:** I left code responsible for handling shortcut opens so this is still supported.

# Test Plan

I have run Expo client with these changes and verified that there are no share or pin icons anymore.
@zacnelson
Copy link

@sjchmiela This was a feature that I had used consistently and was surprised today when I updated the Expo client on Android and the feature was gone. Suddenly I'm scrambling to address this. Could you share the suggested path forward? Why remove something that was working?

@brentvatne
Copy link
Member

we looked at analytics for these features and you may have been one of the three people using them.

we didn't want to continue to maintain largely unused features, and "save to launcher" had presented some new issues recently, so we decided that it wasn't worth the engineering time for us to fix it and continue to ensure that it works in the future.

i imagine you were using save to launcher, yeah? if so, i recommend building an apk and installing it on your device instead. alternatively, i think you can also create a shortcut (in the same way as you can for a website) to a specific deep link in userspace (eg: exp://exp.host/@zacnelson/your-app) that would deep link into expo client.

sorry for the hassle! i'm sure you can appreciate the need to prioritize engineering efforts on features that are used.

@zacnelson
Copy link

Hi Brent, appreciate the response. We've started down the APK route and the functionality is great - you install an APK and it auto updates! I should have known about that sooner... So while the initial snag was pretty frustrating, the current state is certainly an improvement. Thanks.

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.

(Expo Development Client, Android app) - Pinning an app from the Expo notification crashes Expo
4 participants