Skip to content
This repository was archived by the owner on Feb 22, 2023. It is now read-only.

Conversation

@diesersamat
Copy link
Contributor

@diesersamat diesersamat commented Jul 4, 2019

Description

This PR addresses this issue. The issue is that Quick actions crash when the app is killed.

The problem for Android is located here, in QuickActionsPlugin.java.
When the user clicks on the quick actions item, the plugin opens ShortcutHandlerActivity, and it is a simple one:


    @Override
    protected void onCreate(Bundle savedInstanceState) {
      super.onCreate(savedInstanceState);
      // Get the Intent that started this activity and extract the string
      Intent intent = getIntent();
      String type = intent.getStringExtra("type");
      if (channel != null) {
        channel.invokeMethod("launch", type);
      }
      finish();
    }
  }
  1. So, as you can see, it just opens the activity, sends the message to the channel, and finishes the activity. If this approach is fine if the app is already launched, — you will just see the activity behind ShortcutHandlerActivity, — it doesn't work if the app is killed. Just because after closing the helper ShortcutHandlerActivity you don't have any open activities left. One thing which could help is to open the main launcher activity if it is not open yet.

  2. And one more thing to notice is in quick_actions.dart. As you can see, the call channel.invokeMethod is executed onCreate() of the very first activity, ShortcutHandlerActivity, and the method handler in quick_actions.dart is set in initialize(), so, in this case with opening the killed app, handler is not set at the time when invokeMethod is executed. You have to save the intention to call the quick action somewhere, for example, in shared preferences and check later.

That's exactly what was implemented in this PR.

Related Issues

issue 31235

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • My PR includes unit or integration tests for all changed/updated/fixed behaviors (See [Contributor Guide]).
  • All existing and new tests are passing.
  • I updated/added relevant documentation (doc comments with ///).
  • The analyzer (flutter analyze) does not report any problems on my PR.
  • I read and followed the [Flutter Style Guide].
  • The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
  • I updated pubspec.yaml with an appropriate new version according to the [pub versioning philosophy].
  • I updated CHANGELOG.md to add a description of the change.
  • I signed the [CLA].
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

  • Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
  • No, this is not a breaking change.

# Conflicts:
#	packages/quick_actions/CHANGELOG.md
#	packages/quick_actions/lib/quick_actions.dart
#	packages/quick_actions/pubspec.yaml
@collinjackson collinjackson self-requested a review July 11, 2019 18:20
@juliocbcotta
Copy link
Contributor

Why not just open the default flutter app and set it to singleTask in the manifest? I bet you could use some deeplink routing from there.

Copy link
Contributor

@collinjackson collinjackson left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

I have a strong preference that we don't have this plugin depend on the shared_preferences plugin. If necessary, we can do things in the Java side of the plugin that uses Android SharedPreferences but it feels like it shouldn't be necessary here.

I'm wondering if it would be possible add some extras in the intent returned by getLaunchIntentForPackage(context.getApplicationContext().getPackageName()) and use that to pass the action info to main activity, where it could be retrieved from the registrar.activity() the call to registerWith.

@diesersamat
Copy link
Contributor Author

diesersamat commented Jul 13, 2019

@collinjackson Thank you for your review and especially for your suggestion!
I was also thinking that adding an extra dependency isn't the best option, but unfortunately, I didn't know that it is possible to retrieve the activity from registrar.activity().
So, I replaced it with the option you suggested, tested and it works perfectly fine as it was with SharedPreferences.
Please, review the updated version.

@juliocbcotta
Copy link
Contributor

@collinjackson Can't we just launch the launcher Activity and forward the bundle arguments to the router in the dart side? No need to shared preferences.

@diesersamat
Copy link
Contributor Author

@BugsBunnyBR one issue I can see with your approach is that it's breaking the current behavior of the plugin on the Android side. Right now, when you click on a quick actions shortcut, and the app was already open, it just reopens it and delivers the action to the app. But with your routes approach, it doesn't matter if the app was open or not — when you click on the shortcut, it will recreate the page completely, even if it was already open. Notice the black screen each time you click on the shortcut, even if the app is already running.
I don't know if this is acceptable or not, but in my other project, this approach would not work.
Also, as you said, it is a breaking change, and I doubt if it is Okay to use routes for this, as it was designed mainly for navigation.
And by the way, I got rid of SharedPreferences dependency, thanks to @collinjackson 's hint.

@juliocbcotta
Copy link
Contributor

@diesersamat , Yeah, it is a breaking change, since the current behavior is not the intent behavior, at least in the Android platform.

  1. App shortcuts are used for navigation, aka, routing. That can be been in the API by the need to give an Intent to be executed when the shortcut is used. From what I understand the current handler for quick actions will do the same thing as route matching and flutter already provide a named routing system.
  2. The app is not supposed to preserve the current stack when using app shortcuts.

I am not saying not to merge your PR, it seems to fix a problem that exist in the current implementation, but I felt I should point that there is a design problem in the plugin.
I opened an issue flutter/flutter#36148 so we can talk about it there if you want.

// Get the Intent that started this activity and extract the string
Intent intent = getIntent();
String type = intent.getStringExtra("type");
if (channel != null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I ran your code and this channel check will make the opening fail when channel != null.
Steps to reproduce:

  1. Install and open the app normally.
  2. Minimize the app by tapping in the home button.
  3. Use the app shortcut.
  4. Exit the app using the back button. ## Not the home button.
  5. Use the app shortcut. ## "nothing happens".

Possible fix that is not a breaking change:
In ShortcutHandlerActivity class :

      // Get the Intent that started this activity and extract the string
      Intent intent = getIntent();
      String type = intent.getStringExtra("type");
      startActivity(getIntentToOpenMainActivity(this, type));

      finish();
        launchIntentForPackage.addFlags(Intent.FLAG_ACTIVITY_NEW_TASK);
        launchIntentForPackage.addFlags(Intent.FLAG_ACTIVITY_CLEAR_TASK);

Copy link
Contributor Author

@diesersamat diesersamat Jul 14, 2019

Choose a reason for hiding this comment

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

Yes, I know about this issue and it isn't something I wanted to fix in this PR. If you can, please, check the current version of the plugin and make sure that the bug was there before my fix. I don't know if I should fix it in this PR, maybe you can open an issue or even submit a PR? Or do you think that it's better to fix the bug in this PR? Anyway, I will check your solution, because I also faced this issue and was thinking about the ways to fix it.

Copy link
Contributor

Choose a reason for hiding this comment

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

@collinjackson ☝️ ? Another PR? It seems small enough to land together with this version.

Copy link
Contributor Author

@diesersamat diesersamat Jul 15, 2019

Choose a reason for hiding this comment

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

@BugsBunnyBR I know why does this happen. When you click the "Back" button, the activity is being killed. So, the activity is killed now, but channel reference and QuickActionsPlugin class instance is still alive. So, later, when you click on the shortcut, it tries to send a message to the channel, because it is not null — here is the difference with the bug fixed in this PR. But as you know, the view is already detached, and you'll get only this message in logcat FlutterView.send called on a detached view, channel=. Unfortunately, you cannot put a callback there to react on this error case and, maybe, start the activity again, because of API design. I've opened an issue a week ago flutter/flutter#35628 but maybe it is possible to fix without changes in FlutterNativeView API design...

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah. I know how to solve the problem. Just remove the static for the channel. In general, having static fields that are associated with context are a bad ideia. But removing the static makes that we need some other few changes, like listening to onNewIntent, that is why I think another pr will be needed to have a proper handling.

Copy link
Contributor

@collinjackson collinjackson left a comment

Choose a reason for hiding this comment

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

@diesersamat Can you please add a unit test for the added MethodChannel call?

It seems like there should be some sort of iOS implementation for getPassedIntent, even if it doesn't do anything.

The name getPassedIntent seems a bit confusing, since really what you're getting is the name of the action. Maybe calling it getLaunchAction or something like that would be clearer.

@BugsBunnyBR, if you're willing to share code for your suggested handling of the back button case, I think including that fix in this PR would be a great addition, but it won't block merging it.

android:name=".QuickActionsPlugin$ShortcutHandlerActivity"
android:exported="false"
android:relinquishTaskIdentity="true"
tools:targetApi="lollipop" />
Copy link
Contributor

Choose a reason for hiding this comment

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

just curious, what is this for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need tools:targetApi to hide a warning in android studio regarding android:relinquishTaskIdentity — the previous one. We can use targetApi — lollipop as the plugin anyways works on devices from API at least 25. Also, in case, if the app is killed and we open it using the shortcut, it will work fine but only the first time. If you click the Home button after that and use the same shortcut again it will not work the second and following times without android:relinquishTaskIdentity.

Copy link
Contributor

Choose a reason for hiding this comment

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

Have you tried android:noHistory="true" ? As android:relinquishTaskIdentity="true" works only on API +25, all users in APIs below that will not have the same behavior.

Copy link
Contributor Author

@diesersamat diesersamat Jul 14, 2019

Choose a reason for hiding this comment

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

Yes, exactly the same API version Android shortcuts and its classes were added ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh shortcuts are API 25+. So there is no problem.

Copy link
Contributor

Choose a reason for hiding this comment

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

I will wait the merge to make another PR. I found that some more changes are needed. Sorry for the trouble.

Method renaming
Added ios placeholder for getLaunchAction method
@collinjackson collinjackson merged commit 4407ad6 into flutter:master Jul 15, 2019
@collinjackson
Copy link
Contributor

Thanks for the contribution. @BugsBunnyBR looking forward to your follow-on PR for the back button issue.

@juliocbcotta
Copy link
Contributor

@diesersamat, if you may, please try this #1856 .

mithun-mondal pushed a commit to bKash-developer/archived_plugins that referenced this pull request Aug 6, 2019
…ps (flutter#1800)

* Implementing "intent extras" approach instead of shared preferences as suggested

* Added some tests
Method renaming
Added ios placeholder for getLaunchAction method
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants