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

fix(android): fix setting DatePicker's minDate\maxDate #11369

Closed
wants to merge 4 commits into from

Conversation

ypbnv
Copy link
Contributor

@ypbnv ypbnv commented Nov 26, 2019

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

Description:
Fix for setting min\max Date for Date type of Ti.UI.Picker. Some code optimization. Adding accessors
for the properties. There was a scenario where the value was not matching what the Picker was showing, so in order to avoid that now the getter is returning what the native component is showing.
Thus the unit tests that would cover such an attempt.

About DatePickerProxy - would that be a leftover class ? I don't see support for Ti.UI.createDatePicker on Android side and I think we can remove the file.

Test case:
app.js

var maxDate = new Date();
 
var window = Ti.UI.createWindow({
    layout: "vertical",
    backgroundColor: "#454441"
});
var picker = Ti.UI.createPicker({
     type: Ti.UI.PICKER_TYPE_DATE,
     maxDate: maxDate
});
window.add(picker);
var button = Ti.UI.createButton({
    title: "Set Max Date"
});
button.addEventListener("click", function() {
    picker.maxDate = new Date(maxDate.getFullYear() + 1, maxDate.getMonth(), maxDate.getDate());
});
window.add(button);
window.open();

Fix for setting min\max Date for Date type of Ti.UI.Picker. Some code omptimization. Adding getters
for the properties.
@ypbnv ypbnv added this to the 9.0.0 milestone Nov 26, 2019
@build build requested a review from a team November 26, 2019 17:49
@build
Copy link
Contributor

build commented Nov 26, 2019

Fails
🚫 Tests have failed, see below for more information.
Messages
📖 ✊ The commits in this PR match our conventions! Feel free to Rebase and Merge this PR when ready.
📖 ❌ 6 tests have failed There are 6 tests failing and 472 skipped out of 4601 total tests.

Tests:

ClassnameNameTimeError
android.emulator.fs#readFile() returns String when utf-8 encoding set via options object argument (9)2.062
Error: timeout of 2000ms exceeded
at Titanium.<anonymous> (/ti-mocha.js:6535:53723)
android.emulator.fs#readFile() returns String when utf-8 encoding set via second argument (9)2.051
Error: timeout of 2000ms exceeded
at Titanium.<anonymous> (/ti-mocha.js:6535:53723)
android.emulator.fs#readFile() returns Buffer when no encoding set (9)2.106
Error: timeout of 2000ms exceeded
at Titanium.<anonymous> (/ti-mocha.js:6535:53723)
android.emulator.Titanium.Media.AudioPlayer#restart (9)7.936
Error: timeout of 5000ms exceeded
at Titanium.<anonymous> (/ti-mocha.js:6535:53723)
android.emulator.Titanium.Media.AudioPlayer#start, #stop (9)5.43
Error: timeout of 5000ms exceeded
at Titanium.<anonymous> (/ti-mocha.js:6535:53723)
android.emulator.Titanium.UI.WebViewuserAgent (9)30.003
Error: timeout of 30000ms exceeded
at Titanium.<anonymous> (/ti-mocha.js:6535:53723)

Generated by 🚫 dangerJS against 91f30f9

@ypbnv
Copy link
Contributor Author

ypbnv commented Nov 27, 2019

I believe the minDate test fails due to a typo in it. It was undetected so far because minDate did not have a getter providing the value directly from the native picker component.

@Kroll.getProperty
public Date getValue()
{
if (type == UIModule.PICKER_TYPE_DATE) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't account for Time pickers. I know Android doesn't support Date + Time pickers for now, but for a Time picker, I think this should likely return the Date object equivalent to "today" at the selected time of the picker.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, not the point of this PR, but why don't we have a Date + Time picker on Android? I mean we support each individually. Why can't we just create a component that includes both (and merges the date and time value into a single Date object?). Maybe this is a good job to do at the JS level, overriding the createPicker method to handle that specific case and have it silently delegate to both pickers under the hood?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that we should our own custom Date+Time picker.
I've written a feature request for it here: TIMOB-27840


describe('Titanium.UI.Picker', function () {

var win;
Copy link
Contributor

Choose a reason for hiding this comment

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

might want to "clean up" (close) the window after the test like the other UI test suites do.

@sgtcoolguy
Copy link
Contributor

Related, the useSpinner property has been deprecated since 5.2.1, I think we can remove the property and the extra code we have around it here. (TiUITimeSpinner, TiUIDateSpinner, TiUISpinner)

@jquick-axway
Copy link
Contributor

@sgtcoolguy, I don't think we should remove the "useSpinner" feature and perhaps consider un-deprecating it. This is because some app devs like using it.

If you follow the link below, the reason it was deprecated was because there used to be a bug where the spinner's content would not line-up with the other columns correctly. Instead of fixing it, the feature was deprecated and we told devs to use the drop-down picker instead.
https://jira.appcelerator.org/browse/TIMOB-17998

But we fixed the spinner's bug in 7.3.0. So, I think we should keep it now. :)
https://jira.appcelerator.org/browse/TIMOB-19822

@ewanharris ewanharris modified the milestones: 9.0.0, 9.1.0 Feb 12, 2020
@jquick-axway
Copy link
Contributor

I'm closing this PR in favor of PR #11621 which is focused more on the bug itself.
We can refactor things later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants