RNMT-3515 Listen to 2-finger long press and open ECT #12

EiyuuZack · 2019-11-25T13:55:11Z

Description

Following https://github.com/OutSystems/cordova-outsystems-broadcaster/pull/17, this PR makes the App Feedback subscribe to gesture events and react when a 2-finger long press is broadcast.

This will only hold true when targeting Android 10 (API 29), otherwise the old mechanism is used.

See the commit messages for more information.

Context

References https://outsystemsrd.atlassian.net/browse/RNMT-3515

Type of changes

Feature (change which adds functionality)
- BREAKING CHANGE (existing functionality will not work as expected due to new feature)
Fix (change which fixes an issue)
- BREAKING CHANGE (existing functionality will not work as expected due to fix)
Performance (change which improves performance)
- BREAKING CHANGE (existing functionality will not work as expected due to performance improvement)
Refactor (non-breaking change that is neither feature, fix nor performance)
Style (non-breaking change that only affects formatting and/or white-space)

Components affected

Android
iOS
JavaScript

Tests

Manual tests

Screenshots (if appropriate)

Checklist

Code follows code style of this project
CHANGELOG.md file is correctly updated
Changes require an update to the documentation
- Documentation has been updated accordingly

To avoid a breaking change, this feature will live side-by-side with the old code and only run on applications which were built on MABS 6 and above and thus only target SDK 29 and onwards, which provides the required version of the Broadcaster plugin. The breaking change would manifest due to the lack of proper release segmentation on this asset. Since this plugin always injected at the latest version of the respective O<version> branch that means it is possible to have these changes running on an application built with MABS < 6. References https://outsystemsrd.atlassian.net/browse/RNMT-3515

References https://outsystemsrd.atlassian.net/browse/RNMT-3515

usernuno

Aren't there some preferences that allow the target SDK to change? These preferences can't change every value (hence the patches for MABS 6) but I'm not sure if they can change the target SDK.

If that is the case, we can break builds in every MABS version.

Avoids potential NPE. References https://outsystemsrd.atlassian.net/browse/RNMT-3515

EiyuuZack · 2019-11-25T17:23:29Z

Aren't there some preferences that allow the target SDK to change? These preferences can't change every value (hence the patches for MABS 6) but I'm not sure if they can change the target SDK.

If that is the case, we can break builds in every MABS version.

There are preferences that can change the targetSDK but I'm not sure we should support that -- it is a large reason for why there are different MABS versions.

This check just ensure that this code does not run when the targetSDK is below one that we're sure is compatible. If people try to bump up a targetSDK (e.g. for MABS 5) then there is a high probability that it will not compile on MABS in the first place.

We could try leverage the NATIVE_SHELL_VERSION preference much like Application Info does to ensure we only run this code starting from a specific NativeShell version, but alas it is still a preference. Do you have any suggestions?

usernuno · 2019-11-25T18:16:01Z

Aren't there some preferences that allow the target SDK to change? These preferences can't change every value (hence the patches for MABS 6) but I'm not sure if they can change the target SDK.
If that is the case, we can break builds in every MABS version.

There are preferences that can change the targetSDK but I'm not sure we should support that -- it is a large reason for why there are different MABS versions.

This check just ensure that this code does not run when the targetSDK is below one that we're sure is compatible. If people try to bump up a targetSDK (e.g. for MABS 5) then there is a high probability that it will not compile on MABS in the first place.

We could try leverage the NATIVE_SHELL_VERSION preference much like Application Info does to ensure we only run this code starting from a specific NativeShell version, but alas it is still a preference. Do you have any suggestions?

I was just raising the flag regarding the android-targetSdkVersion preference. From what I understand the validations implemented in the supported plugins for the universal versions took this preference into consideration. I'd prefer if @Chuckytuh could chime in this thread.

Regarding alternatives I'm not seeing much. My only guess would be using reflection to see if one of the new classes exists but it's quite ugly and "depends" on knowledge of another plugin.

luissilvaos

I think we have a breaking change here 😞 check my comments below, please.

luissilvaos · 2019-11-26T09:27:36Z

src/android/OSAppFeedback.java

-                            }
-                            if ((System.currentTimeMillis() - mSecondFingerTimeDown) >= INTERVAL_TO_SHOW_MENU) {
-                                mSecondFingerTimeDown = 0;
+        if(cordovaActivity.getApplicationInfo().targetSdkVersion >= 29) {


Houston, I think we have a breaking change here.
Although this is protected with the targetSdkVersion for the runtime, I'm not sure if this will build with previous MABS Versions 😞

After looking better I believe you are correct. There is a breaking change on the Constants import from the Broadcaster. The constants used here (e.g. Constants.GESTURE_EVENT) do not exist in older MABS versions (more specifically older Broadcaster versions) and will cause a compile-time error when using this App Feedback commit/version.

A way to avoid this breaking change is to drop the Constants import and replace the constants used here with their string literals, although if the respective values change on the Broadcaster Constants class this code can suddenly stop working without visibility.

Regarding the Event import from the Broadcaster there is no breaking change because all MABS versions dating back from 3.3 include the Broadcaster with this Event interface and the getData() method. Any MABS version using this App Feedback commit/version will compile. However this is creating a dependency that may break if the Event interface is changed...

What are your thoughts on this?

The price to pay for not having versioning on this plugin 😞
Go ahead with that changes.

We should aim to have the plugins backwards compatibility with other MABS versions, in this case, it's more justified because we don't have versioning for this plugin. That being said, I agree with @luissilvaos to implement those changes.

Chuckytuh · 2019-11-27T11:36:04Z

@EiyuuZack what are we trying to achieve here? Is the new mechanism (with Broadcast Receiver) just to be used when running on Android 10 or when we compile the application with targetSdkVersion to 29?

If we're just trying to understand whether the application is running on Android 10 than we should be using https://developer.android.com/reference/android/os/Build.VERSION.html#SDK_INT .

usernuno · 2019-11-27T11:45:03Z

@EiyuuZack what are we trying to achieve here? Is the new mechanism (with Broadcast Receiver) just to be used when running on Android 10 or when we compile the application with targetSdkVersion to 29?

If we're just trying to understand whether the application is running on Android 10 than we should be using https://developer.android.com/reference/android/os/Build.VERSION.html#SDK_INT .

@Chuckytuh this should run in all MABS 6 builds. It should not run in MABS 5- builds. The code that supports this is in the broadcaster plugin, but only in MABS 6. Therefore older builds should use the currently existing touch handler while newer builds should use this new code.

EiyuuZack · 2019-11-27T15:32:55Z

@EiyuuZack what are we trying to achieve here? Is the new mechanism (with Broadcast Receiver) just to be used when running on Android 10 or when we compile the application with targetSdkVersion to 29?

If we're just trying to understand whether the application is running on Android 10 than we should be using https://developer.android.com/reference/android/os/Build.VERSION.html#SDK_INT .

We do not want this to run just because an app is running on Android 10. We want to run this new code when the app has been built targeting Android 10 (API 29).

This would cause a breaking change due to the way this plugin is (not) versioned. Bear in mind that these constants still mirror the ones in the Broadcaster plugin and should any of these be modified there such changes must be reflected here, lest this code start failing suddenly. References https://outsystemsrd.atlassian.net/browse/RNMT-3515

This fix is twofold: 1) Unregisters the receiver which was not being done at all 2) Moves the register/unregister calls to the onStart and onStop lifecycle callbacks as per Android best practices References https://outsystemsrd.atlassian.net/browse/RNMT-3515

References https://outsystemsrd.atlassian.net/browse/RNMT-3515

EiyuuZack added 2 commits November 25, 2019 13:34

docs: update CHANGELOG.md

c004b26

References https://outsystemsrd.atlassian.net/browse/RNMT-3515

EiyuuZack requested review from Chuckytuh, cmfsotelo, vmfo22, pmaroco, luissilvaos and usernuno November 25, 2019 13:55

EiyuuZack self-assigned this Nov 25, 2019

usernuno reviewed Nov 25, 2019

View reviewed changes

refactor: test equality on constant

0f00c07

Avoids potential NPE. References https://outsystemsrd.atlassian.net/browse/RNMT-3515

luissilvaos requested changes Nov 26, 2019

View reviewed changes

usernuno previously approved these changes Nov 28, 2019

View reviewed changes

luissilvaos previously approved these changes Nov 28, 2019

View reviewed changes

pmaroco previously approved these changes Nov 29, 2019

View reviewed changes

EiyuuZack added 2 commits November 29, 2019 14:29

docs: update CHANGELOG.md

20ef2ca

References https://outsystemsrd.atlassian.net/browse/RNMT-3515

EiyuuZack dismissed stale reviews from pmaroco, luissilvaos, and usernuno via 20ef2ca November 29, 2019 14:33

usernuno approved these changes Nov 29, 2019

View reviewed changes

Chuckytuh approved these changes Dec 2, 2019

View reviewed changes

luissilvaos approved these changes Dec 2, 2019

View reviewed changes

EiyuuZack merged commit 9c69bd6 into master Dec 2, 2019

EiyuuZack deleted the feat/RNMT-3515/longpress-activation-android branch December 2, 2019 11:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RNMT-3515 Listen to 2-finger long press and open ECT #12

RNMT-3515 Listen to 2-finger long press and open ECT #12

EiyuuZack commented Nov 25, 2019 •

edited

Loading

usernuno left a comment

EiyuuZack commented Nov 25, 2019

usernuno commented Nov 25, 2019

luissilvaos left a comment

luissilvaos Nov 26, 2019

EiyuuZack Nov 27, 2019 •

edited

Loading

luissilvaos Nov 27, 2019

vmfo22 Nov 27, 2019

Chuckytuh commented Nov 27, 2019

usernuno commented Nov 27, 2019

EiyuuZack commented Nov 27, 2019

RNMT-3515 Listen to 2-finger long press and open ECT #12

RNMT-3515 Listen to 2-finger long press and open ECT #12

Conversation

EiyuuZack commented Nov 25, 2019 • edited Loading

Description

Context

Type of changes

Components affected

Tests

Screenshots (if appropriate)

Checklist

usernuno left a comment

Choose a reason for hiding this comment

EiyuuZack commented Nov 25, 2019

usernuno commented Nov 25, 2019

luissilvaos left a comment

Choose a reason for hiding this comment

luissilvaos Nov 26, 2019

Choose a reason for hiding this comment

EiyuuZack Nov 27, 2019 • edited Loading

Choose a reason for hiding this comment

luissilvaos Nov 27, 2019

Choose a reason for hiding this comment

vmfo22 Nov 27, 2019

Choose a reason for hiding this comment

Chuckytuh commented Nov 27, 2019

usernuno commented Nov 27, 2019

EiyuuZack commented Nov 27, 2019

EiyuuZack commented Nov 25, 2019 •

edited

Loading

EiyuuZack Nov 27, 2019 •

edited

Loading