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

Add timezone support to Clock component #1966

Open
wants to merge 19 commits into
base: ucr
Choose a base branch
from

Conversation

barreeeiroo
Copy link
Member

@barreeeiroo barreeeiroo commented Dec 28, 2019

This PR adds the ability to play with timezones in which components are stored at

@AppInventorWorkerBee
Copy link
Collaborator

Can one of the admins verify this patch?

@barreeeiroo
Copy link
Member Author

@ewpatton totally forgot about this PR in the meetup. The only missing block I think that this PR need is a "getTimezones" that returns a List of all available timezones.
Is there any other block you may want to add?

@barreeeiroo barreeeiroo marked this pull request as ready for review January 17, 2020 11:58
*/
@SimpleFunction(description = "Updates the timezone in which the instant is saved at")
public static Calendar ChangeTimezone(Calendar instant, String timezone) {
if (timezones.contains(timezone))
Copy link
Contributor

@kkashi01 kkashi01 Jan 17, 2020

Choose a reason for hiding this comment

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

Suggestion: trim spaces and ignore case in comparison. If found, then use the actual value that is in the list (not the passed-in parameter) and set the timezone to the found-value

Copy link
Member Author

Choose a reason for hiding this comment

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

image

TimeZone is actually case sensitive, so "america/detroit" will not be a valid timezone. We should not allow users to input it lowercase even though we know it is "America/Detroit", as it's not officially supported by Java.

Copy link
Contributor

Choose a reason for hiding this comment

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

It is case-sensitive, that's why I mentioned:

If found, then use the actual value that is in the list (not the passed-in parameter) and set the timezone to the found-value

@barreeeiroo
Copy link
Member Author

barreeeiroo commented Jan 17, 2020

If a user wants, for example, america/detroit (and not America/Detroit), they should use the following procedure:

  1. Get all available timezones using the Timezones property
  2. Look on each element and compare both strings using the lowercase text block
  3. If they match, get the index of the element or the value of the element
  4. Use that index to get the value on the Timezones list, or use the fetched value (not the user-given one)

That is how a normal developer would do, as the block is just calling the native method. We're not making conversions or any other work inside/with it, as we would be making suppositions of what a user might want.
The more direct control we give users, the better. Because we are giving them a higher range of opportunities and chances to learn.

If a user sees a procedure of "getTimezoneCaseInsensitive" made with blocks, they will gain the logic of how it can be done. Instead, if we give them already done, we are restricting the possibilities (and such procedure can be done quite easily with blocks, without needing us to make assumptions).

That's my opinion, but I think the block should call the method without applying conversions or anything.

@ewpatton
Copy link
Member

The more direct control we give users, the better. Because we are giving them a higher range of opportunities and chances to learn.
If a user sees a procedure of "getTimezoneCaseInsensitive" made with blocks, they will gain the logic of how it can be done. Instead, if we give them already done, we are restricting the possibilities (and such procedure can be done quite easily with blocks, without needing us to make assumptions).

Well, they could always learn Java. That gives them much more opportunity than App Inventor. 😄

One of the goals with App Inventor is to make hard stuff easier. I think allowing one to set the timezone using a case insensitive string fits within that directive. I expect that in other apps students will have plenty of opportunity to learn how to use the upper/lower case blocks. I'm not convinced there isn't any reason to make a component purposefully "dumber" about something. Timezones are already a challenging enough problem without also introducing things like case sensitivity into it.

@barreeeiroo
Copy link
Member Author

Well, they could always learn Java. That gives them much more opportunity than App Inventor. 😄

Yeah, that's totally right. But what I mean is that, if we make assumptions, we will have to make sure that those assumptions will never cause conflicts with what the user really wants. That was my concern.

Just an example of what worries me. JavaScript is a very weird language, it behaves in really strange manners, like 12 + "1" = "121" but 12 - "1" = 11. I know Java is not like that, with ambiguous cases, but I was worried that, somehow, AI could suffer an awkward case.


One of the goals with App Inventor is to make hard stuff easier. I think allowing one to set the timezone using a case insensitive string fits within that directive. I expect that in other apps students will have plenty of opportunity to learn how to use the upper/lower case blocks.

Well, you are right, I haven't thought about it like that. 😓

I'm not convinced there isn't any reason to make a component purposefully "dumber" about something. Timezones are already a challenging enough problem without also introducing things like case sensitivity into it.

Then I will tweak it so it becomes case-insensitive. However, it would be easier if Java just accepted the lower case one (america/detroit instead of America/Detroit)... I just hope that never two timezones would differ in just having a few letters with capitals.
I will have something by tomorrow.

@barreeeiroo
Copy link
Member Author

Should be now fixed. It now first checks if it matches the equal timezone, and if not it checks if it matches the case-insensitive one. If it does, it re-runs the method but with the correct timezone.

@ewpatton
Copy link
Member

ewpatton commented Mar 9, 2020

@barreeeiroo It looks like you missed an import when you did the merge.

@barreeeiroo
Copy link
Member Author

barreeeiroo commented Mar 10, 2020

@barreeeiroo It looks like you missed an import when you did the merge.

My bad. It should be now fixed. 🤦‍♂
However, I want to test it again.. I'm not totally sure something works properly...

@ewpatton ewpatton added this to the September Release milestone Aug 10, 2020
@ewpatton ewpatton modified the milestones: September Release, nb187 May 14, 2021
@ewpatton ewpatton removed this from the nb187 milestone Aug 4, 2021
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

5 participants