[TIMOB-8430]Android: fix google issue with am/pm #8799

frankieandone · 2017-01-27T21:09:36Z

Test Case
label should show the same time, in 24 hour format, as the time picker when toggling "AM" or "PM".

var win = Titanium.UI.createWindow();
var lbl = Titanium.UI.createLabel({
	color: '#999',
	text: 'Set the time below',
	font: {fontSize: 20, fontFamily: 'Helvetica Neue'},
	textAlign: 'center',
	width: 'auto',
	top: '0'
});
var pkr = Titanium.UI.createPicker({
	type: Ti.UI.PICKER_TYPE_TIME,
	top: '40',
	backgroundColor: "red",
	format24: false
});
pkr.addEventListener('change', function (e) {
	lbl.text = pkr.value;
	console.log("test msg picker value: " + pkr.value);
});
win.add(lbl);
win.add(pkr);
win.open();

JIRA: https://jira.appcelerator.org/browse/TIMOB-8430

lokeshchdhry · 2017-05-03T18:42:02Z

@fmerzadyan,
On Android 5.1.1. & 6.0.1 the change event does not fire when I change From AM to PM & vice versa. But, if you change from e.g AMtoPM, I have to tap on the hourorminute` for the change event to fire.

Is this expected ?

hansemannn · 2017-05-05T18:39:18Z

android/modules/ui/src/java/ti/modules/titanium/ui/widget/picker/TiUITimePicker.java

 import android.os.Build;
 import android.widget.TimePicker;
 import android.widget.TimePicker.OnTimeChangedListener;
+import com.frankify.f;


maggieaxway

In TiUITimePicker.java

@Override
public void onTimeChanged(TimePicker view, int hourOfDay, int minute)
{
	Calendar calendar = Calendar.getInstance();
	calendar.set(Calendar.HOUR_OF_DAY, hourOfDay);
	calendar.set(Calendar.MINUTE, minute);

	// Make sure .value is readable by user
	proxy.setProperty("value", calendar.getTime());

	if (!suppressChangeEvent) {
		KrollDict data = new KrollDict();
		data.put("value", calendar.getTime());
		fireEvent("change", data);
	}
}

jquick-axway · 2017-05-08T16:29:04Z

android/modules/ui/src/java/ti/modules/titanium/ui/widget/picker/TiUITimePicker.java

+			public void run() {
+				detectTimeChange();
+			}
+		}, delay, rate);


Don't use a Java "Timer". It executes the TimerTask on a separate thread, which will cause race conditions in our code. Use the Android "Handler" class instead which runs on the main UI thread (if created on the main thread). You'll want to use its postDelayed() method, but you'll have to call postDelayed() within the Runnable so that it'll schedule itself again.

Also, your current timer will run forever. It'll continue to run after the view has been removed (this is bad). You should start posting with the handler when the View.onAttachedToWindow() method gets called and stop the handler when the View.onDetachedFromWindow() method gets called.
https://developer.android.com/reference/android/view/View.html#onAttachedToWindow()
https://developer.android.com/reference/android/view/View.html#onDetachedFromWindow()

jquick-axway · 2017-05-08T16:38:37Z

android/modules/ui/src/java/ti/modules/titanium/ui/widget/picker/TiUITimePicker.java

+		TimePicker picker = (TimePicker) getNativeView();
+		boolean hasChanged;
+		if (Build.VERSION.SDK_INT >= 23) {
+			hasChanged = lastSelectedHour != picker.getHour() || lastSelectMin != picker.getMinute();


This will cause an "opcode" warning in the log when ran on an API Level 22 or lower device. Please change it to the following...

private void detectTimeChange() { TimePicker picker = (TimePicker) getNativeView(); if (picker == null) { return; } int currentSelectedHour; int currentSelectedMinute; if (Build.VERSION.SDK_INT >= 23) { currentSelectedHour = ApiLevel23.getHourFrom(picker); currentSelectedMinute = ApiLevel23.getMinuteFrom(picker); } else { currentSelectedHour = picker.getCurrentHour(); currentSelectedMinute = picker.getCurrentMinute(); } // ... } private static class ApiLevel23 { private ApiLevel23() {} public static int getHourFrom(TimePicker picker) { return picker.getHour(); } public static int getMinuteFrom(TimePicker picker) { return picker.getMinute(); } }

I don't see any warnings running this on my Android 5.1 emulator. I don't think this is necessary, the if guard should prevent lower devices from executing unsupported code.

Any why do we need 2 if-checks for each API level? Should do that more compact.

@garymathews, the "opcode" warnings are definitely happening. They can be seen via "adb logcat". Our existing codebase has this issue everywhere and there is a desire to fix it per the following JIRA case...
https://jira.appcelerator.org/browse/TIMOB-20258

The solution I posted above resolves it by taking advantage of Java's lazy class loading, which is the recommended solution by Google (link below) and is the same solution Google uses in their Android "support" libraries.
https://android-developers.googleblog.com/2010/07/how-to-have-your-cupcake-and-eat-it-too.html

@hansemannn, yeah, I wrote the code within the github inline commenter and didn't notice I did that (d'oh). I just updated the above code snippet.

Totally fine :-)

These warnings are only thrown from Dalvik which is why I didn't see them (and won't be seen on Android 5.0+ due to moving to ART). These warnings are expected behaviour, but ugly. Since the references are properly guarded no issue will actually come from this.

If we do want to address this we should do it with a class in the appcompat module instead of a nested class. Also note that it doesn't provide any additional safety as it still relies on the

if (Build.VERSION.SDK_INT ...

statement. It's just preventing Dalvik from seeing the broken reference.

I don't agree. For one-off API calls like this, we should use an inner class as I've shown above. It keeps it simple and it keeps the code together in one place... making it easier to maintain and understand. And the ApiLevel23 class name makes it clear to the developer that it should only be accessed by that API level or higher. Also, by making it an inner class, it effectively becomes a private API that module developers cannot use (this is intentional; we're hding the complexity from a public API standpoint).

Moving higher API level calls to another class is only useful if it's needed by multiple classes. Let's avoid this unless needed, because once we create a public class in an open source project, it's set in stone. We'll have to maintain it and provide tech-support for it.

@jquick-axway I agree if it were just a couple of instances, but if we plan on suppressing all of the warnings from our code base like you mentioned with TIMOB-20258 then it would be more maintainable to create a single class for each API level needed (23, 21, 16) in an appropriate place and reference to that; than having nested classes in each class with an unsupported API reference.

Here's a list of references we need to "hide" from Dalvik:

AndroidModule.java API-23 - L365: checkSelfPermission - L389: checkSelfPermission - L396: requestPermissions CalendarProxy.java API-23 - L105: checkSelfPermission - L106: checkSelfPermission CommonContactsApi.java API-23 - L72: checkSelfPermission FilesystemModule.java API-23 - L94: checkSelfPermission - L110: requestPermissions GeolocationModule.java API-23 - L623: checkSelfPermission - L645: requestPermissions MediaModule.java API-23 - L359: checkSelfPermission - L360: checkSelfPermission - L371: checkSelfPermission - L382: checkSelfPermission TiUIWebView.java - API-16 - L331: setAllowUniversalAccessFromFileURLs TiWebChromeClient.java API-23 - L229: checkSelfPermission TiWindowProxy.java API-21 - L531: makeSceneTransitionAnimation WindowProxy.java API-16 - L162: startActivity API-21 - L176: finishAfterTransition - L493: setEnterTransition, createTransition - L497: setExitTransition, createTransition - L501: setReturnTransition, createTransition - L505: setReenterTransition, createTransition - L509: setSharedElementEnterTransition, createTransition - L513: setSharedElementExitTransition, createTransition - L517: setSharedElementReenterTransition, createTransition - L521: setSharedElementReturnTransition, createTransition TiViewProxy.java API-21 - L802: createCircularReveal - L836: createCircularReveal TiBaseActivity.java API-23 - L619: finishAndRemoveTask - L881: finishAffinity TiActivitySupportHelper.java API-16 - startIntentSenderForResult TiFileHelper2.java API-23 - L94: checkSelfPermission TiBorderWrapperView.java API-21 - L128: ViewOutlineProvider TiUIView.java API-16 - L545: removeOnGlobalLayoutListener - L628: removeOnGlobalLayoutListener

I don't like the idea of creating a nested class for each of those instances. We could do something like this:

org.appcelerator.titanium.compat Api23.java Api21.java Api16.java

These classes would be for internal use only, undocumented and not to be used by module developers.

However, I did notice even the Google support libraries do not suppress all of their warnings:

Could not find method android.content.res.Resources.getDrawable, referenced from method android.support.v7.widget.ResourcesWrapper.getDrawable VFY: unable to resolve virtual method 565: Landroid/content/res/Resources;.getDrawable (ILandroid/content/res/Resources$Theme;)Landroid/graphics/drawable/Drawable; VFY: replacing opcode 0x6e at 0x0002 Could not find method android.content.res.Resources.getDrawableForDensity, referenced from method android.support.v7.widget.ResourcesWrapper.getDrawableForDensity VFY: unable to resolve virtual method 567: Landroid/content/res/Resources;.getDrawableForDensity (IILandroid/content/res/Resources$Theme;)Landroid/graphics/drawable/Drawable; VFY: replacing opcode 0x6e at 0x0002 Could not find class 'android.support.graphics.drawable.VectorDrawableCompat', referenced from method android.support.v7.widget.AppCompatDrawableManager.isVectorDrawable VFY: unable to resolve instanceof 365 (Landroid/support/graphics/drawable/VectorDrawableCompat;) in Landroid/support/v7/widget/AppCompatDrawableManager; VFY: replacing opcode 0x20 at 0x0000 Could not find method android.support.graphics.drawable.VectorDrawableCompat.createFromXmlInner, referenced from method android.support.v7.widget.AppCompatDrawableManager$VdcInflateDelegate.createFromXmlInner VFY: unable to resolve static method 1631: Landroid/support/graphics/drawable/VectorDrawableCompat;.createFromXmlInner (Landroid/content/res/Resources;Lorg/xmlpull/v1/XmlPullParser;Landroid/util/AttributeSet;Landroid/content/res/Resources$Theme;)Landroid/support/graphics/drawable/VectorDrawableCompat; VFY: replacing opcode 0x71 at 0x0004 Could not find method android.support.graphics.drawable.AnimatedVectorDrawableCompat.createFromXmlInner, referenced from method android.support.v7.widget.AppCompatDrawableManager$AvdcInflateDelegate.createFromXmlInner VFY: unable to resolve static method 1630: Landroid/support/graphics/drawable/AnimatedVectorDrawableCompat;.createFromXmlInner (Landroid/content/Context;Landroid/content/res/Resources;Lorg/xmlpull/v1/XmlPullParser;Landroid/util/AttributeSet;Landroid/content/res/Resources$Theme;)Landroid/support/graphics/drawable/AnimatedVectorDrawableCompat; VFY: replacing opcode 0x71 at 0x0004

What are your thoughts for this approach?

Is it worthwhile to suppress these warnings?

I wouldn't create public "ApiLevelX" classes because that shifts the responsibility to the user of our classes to implement the correct version fallback behavior. If we were to implement something like this, I think Google's "DrawableCompat" class (link below) would be a better way to do it, because it buries/hides the complexity from the users of our library/classes. With the one difference is that I wouldn't use dessert names like Google. I would name the classes based on API level number instead like I did above, which is less error prone. Especially since Google's API docs displays an API level number next to every API. (I've seen devs make bad assumptions about which dessert maps to which API level number, causing a crash.)
https://github.com/guardianproject/android-support-library/blob/master/v4/java/android/support/v4/graphics/drawable/DrawableCompat.java

But that said, my preference is to not create public bags of static functions under the AppCompat package/namespace. I think it makes more sense to group higher API levels calls by feature under the same package that uses them like "network", "ui", "view", etc. For example, a developer would normally expect all network related classes to be under the network namespace/package. Also, I prefer to keep the higher API level accessing classes as private inner-classes because this makes it more future proof. So, when we do raise the min API level we support in the future, we can refactor the obsolete API level accessing private inner-classes without it being a breaking change. This also means that I think wrapping Android features in our own classes is the way to go so that users of our classes don't have to worry about API levels, like how it works with many of Google's support library classes.

Regarding the TimePicker case above, since no other class in our code needs to call those 2 methods, it's simpler from a productivity standpoint to bury the higher level API accessor as an inner class like I've done it. This reduces the spaghetti code-ness of our library (it's only 14 lines of code). But if we ever need to rip it out in the future, then we can easily do so without it being a breaking change since it's a private inner class.

I don't expect us to fix all of these opcodes issues in one shot. These issues are everywhere and is too big of a job. Fixing these issues a bit at a time as we make changes is probably the way to go and we should strive to avoid adding new opcode warnings. I think this is a low priority issue since I've never heard of Google Play app reviewers reject apps due to opcode warnings (apps to be featured on Google Play actually go through an app review process). This is more of a tech-support issue on our end.

Oh and if you think the solution to solving these opcode warning is a hack, well, you'd be right. :-) But it's a Google sanctioned hack that Google's own support libraries 100% depend on.

jquick-axway · 2017-05-08T16:42:04Z

android/modules/ui/src/java/ti/modules/titanium/ui/widget/picker/TiUITimePicker.java

@@ -33,6 +35,9 @@
 	protected Date minDate, maxDate;


Remove the "suppressChangeEvent" member variable.

Modify the setValue() method to update the "lastSelectedHour" and "lastSelectedMin" member variables before updating the TimePicker object. This way, when TimePicker.onTimeChanged() method gets invoked, your detectTimeChange() method will not fire the time changed event because the picker's selected time will already match your member variables.

jquick-axway

Can you put the "Override" back on the ontTimeChanged() method please?

Other than that, the changes look good!

jquick-axway

CR: Pass

lokeshchdhry · 2017-05-18T00:52:33Z

FR Failed.

On Android 5.1.1. & 6.0.1 the change event does not fire when I change From AM to PM & vice versa. But, if you change from e.g AMtoPM, I have to tap on the hour or minute for the change event to fire.

sgtcoolguy · 2017-05-19T14:27:26Z

I strongly prefer #9026's simpler approach to the one used here. I think if/when that PR passes FR, we should merge it and cherry-pick to master.

However, I do think we should incorporate some of the code cleanup from this PR on master branch where the string literals were replaced with proper constant references for property and event names (such as the changes to android/modules/ui/src/java/ti/modules/titanium/ui/PickerProxy.java there)

lokeshchdhry · 2017-05-19T18:46:57Z

FR Passed.

Switching between am and pm fires the change event successfully every time.

Studio Ver: 4.9.0.201705170123
SDK Ver: 6.2.0.v20170519091937
OS Ver: 10.12.3
Xcode Ver: Xcode 8.3.2
Appc NPM: 4.2.9
Appc CLI: 6.2.1
Ti CLI Ver: 5.0.13
Alloy Ver: 1.9.11
Node Ver: 6.10.1
Java Ver: 1.8.0_101
Devices: ⇨ google Nexus 6 --- Android 6.0.1
⇨ google Pixel XL --- Android 7.1.1

Dismissing outdated review.

frankieandone added android bug labels Jan 27, 2017

frankieandone assigned maggieaxway Jan 27, 2017

frankieandone force-pushed the timob-8430 branch from fa76338 to 2e2dd96 Compare May 5, 2017 17:48

frankieandone requested a review from jquick-axway May 5, 2017 17:50

frankieandone added work in progress 🚧 and removed work in progress 🚧 labels May 5, 2017

frankieandone force-pushed the timob-8430 branch from 4c8f1ba to dcc509b Compare May 5, 2017 17:55

hansemannn reviewed May 5, 2017

View reviewed changes

frankieandone force-pushed the timob-8430 branch 3 times, most recently from 89b2df1 to a36153b Compare May 5, 2017 22:00

frankieandone changed the title ~~[TIMOB-8430] Android fix incorrect meridiem~~ [TIMOB-8430]Android: fix google issue with am/pm May 5, 2017

maggieaxway self-requested a review May 8, 2017 14:53

maggieaxway previously requested changes May 8, 2017

View reviewed changes

jquick-axway suggested changes May 8, 2017

View reviewed changes

frankieandone force-pushed the timob-8430 branch 3 times, most recently from 5a12e47 to 2d49b7e Compare May 8, 2017 22:57

frankieandone force-pushed the timob-8430 branch from 2d49b7e to cff38ae Compare May 16, 2017 21:12

jquick-axway suggested changes May 16, 2017

View reviewed changes

frankieandone force-pushed the timob-8430 branch from eed6db7 to b05101e Compare May 17, 2017 23:32

jquick-axway approved these changes May 17, 2017

View reviewed changes

jquick-axway added the in-qe-testing 🕵 label May 17, 2017

frankieandone force-pushed the timob-8430 branch 2 times, most recently from 7ed1361 to 793fc27 Compare May 18, 2017 17:46

sgtcoolguy mentioned this pull request May 19, 2017

[6_1_0][TIMOB-8430]Android: fix google issue with am/pm #9026

Merged

fmerzadyan and others added 2 commits May 19, 2017 17:17

6_1_0 timob-8430 Android: fix am/pm

838db8e

[TIMOB-8430] Fix TimePicker AM/PM

b8856e7

garymathews force-pushed the timob-8430 branch from 793fc27 to b8856e7 Compare May 19, 2017 16:18

lokeshchdhry merged commit 0029877 into tidev:master May 19, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TIMOB-8430]Android: fix google issue with am/pm #8799

[TIMOB-8430]Android: fix google issue with am/pm #8799

frankieandone commented Jan 27, 2017 •

edited

lokeshchdhry commented May 3, 2017

hansemannn May 5, 2017

maggieaxway left a comment

jquick-axway May 8, 2017

jquick-axway May 8, 2017 •

edited

garymathews May 8, 2017 •

edited

hansemannn May 8, 2017

jquick-axway May 8, 2017 •

edited

jquick-axway May 8, 2017

hansemannn May 8, 2017

garymathews May 9, 2017

jquick-axway May 9, 2017

garymathews May 9, 2017

jquick-axway May 10, 2017

jquick-axway May 8, 2017

jquick-axway left a comment

jquick-axway left a comment

lokeshchdhry commented May 18, 2017 •

edited

sgtcoolguy commented May 19, 2017

lokeshchdhry commented May 19, 2017

[TIMOB-8430]Android: fix google issue with am/pm #8799

[TIMOB-8430]Android: fix google issue with am/pm #8799

Conversation

frankieandone commented Jan 27, 2017 • edited

lokeshchdhry commented May 3, 2017

Choose a reason for hiding this comment

maggieaxway left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jquick-axway May 8, 2017 • edited

Choose a reason for hiding this comment

garymathews May 8, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jquick-axway May 8, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jquick-axway left a comment

Choose a reason for hiding this comment

jquick-axway left a comment

Choose a reason for hiding this comment

lokeshchdhry commented May 18, 2017 • edited

sgtcoolguy commented May 19, 2017

lokeshchdhry commented May 19, 2017

frankieandone commented Jan 27, 2017 •

edited

jquick-axway May 8, 2017 •

edited

garymathews May 8, 2017 •

edited

jquick-axway May 8, 2017 •

edited

lokeshchdhry commented May 18, 2017 •

edited