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

Remove android.text.format.Time usage. #467 #646

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

lemonboston
Copy link
Contributor

@lemonboston lemonboston commented Jan 23, 2018

@dmfs I am submitting this just to start discussing the changes, what to extract, etc. I haven't reviewed the changeset yet.

Notes, questions, comments:

  • 1. New classes, types to for jems: Conditional, MapFlattened (flatmap), Procedure
    I am going to add tickets and pull requests in jems, we can discuss the names, details there.
    -> submitted and listed them for next jems release ticket: Release version 1.16 jems#147
    -> they may not be needed after all, just Procedure is used currently (that doesn't required a new jems necessarily)

  • 2. Unused classes that used Time: NotAfter, NotBefore, ShiftIfAfter, ShiftTime - okay to remove them? -> I've removed them, let me know if they should be kept instead

  • 3. Remove the if start or due then timezone != null validation in Validating allowing floating times? Remove android.text.format.Time usage. #467 #646 (comment)

  • 4. General (not provider related) Single<DateTime> classes to new datetime module? And separate ticket, once they are settled. Remove android.text.format.Time usage. #467 #646 (comment)

.. there are many more questions, general comments, I will put them down later.
I also plan to change some of the new classes, for example to introduce Evaluated decorator where Cursor is passed in. Also not use that new ContainerArgument but the Single<> decoration as you did in the provider.

@lemonboston lemonboston self-assigned this Jan 23, 2018
@lemonboston
Copy link
Contributor Author

lemonboston commented Jan 23, 2018

The changed TimeFieldEditor can produce DateTimes with null timezones currently - not sure if that's correct or not, I am yet to review that. But this is a general question as well, since Time couldn't represent floating times 'UTC' or default time zone was used everywhere. When I did the changes, I tried to keep everything the same (apparently I couldn't at this stage with TimeFieldEditor).

A concrete issue now is that now there is a crash with the current state of TimeFieldEditor because of this check:

// if one of DTSTART or DUE is given, TZ must not be null unless it's an all-day task
if ((dtStart != null || due != null) && !task.valueOf(TaskAdapter.IS_ALLDAY) && task.valueOf(TaskAdapter.TIMEZONE_RAW) == null)
{
throw new IllegalArgumentException("TIMEZONE must be supplied if one of DTSTART or DUE is not null and not all-day");
}

Is this check still relevant or shall we remove it start to allow floating times? (Besides reviewing TimeFieldEditor.)

@lemonboston
Copy link
Contributor Author

There are several new Single<DateTime> classes created here, some of which are totally general, not related to the provider. Like these:

StartOfHour
StartOfNextDay
StartOfNextHour
StartOfNextMonth
StartOfNextYear
MovedToDate
MovedToTimeOfDay
MillisDuration

They are yet to be reviewed together with their place of usage, so they may change.
I'd like to suggest that we create a small datetime module in the project for them. This would also be the only module that imports org.dmfs:rfc5545-datetime.

As far as I know gradle builds multi-module projects faster now compared to having the same code in single-module because of incremental builds (doesn't recompile untouched modules). So from this point of view it is beneficial to modularize the project more.

What do you think, are you okay with creating that datetime module? Once these classes seemed to be settled here, meaning we are sure they are needed, this could be taken into a separate ticket, where tests can be added, too.

@lemonboston
Copy link
Contributor Author

Note that as per current state the new jems types are not needed, only Procedure but that doesn't require a jems release necessarily.

@lemonboston
Copy link
Contributor Author

In order to break this up to smaller pull requests meaningfully, I am going to create a ticket for the new DateTimeFieldAdapters (Combined and Simple). I will continue on a new branch with the stripped down changeset for that pull request and put this branch on hold.

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.

1 participant