-
-
Notifications
You must be signed in to change notification settings - Fork 945
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
Added key listener to allow d-pad navigation of days in month view. S… #220
base: master
Are you sure you want to change the base?
Conversation
…et MonthView to be focusable to allow view to receive key events
…mented in dialog; add calls in MonthView to switch focus when focus reaches left / right edge of date picker
…transition between year and month based on d-pad right event
…clean up year tv compat code and isTv util code comments
Thanks for the contribution. |
No problem, actually have improve it a lot and currently working on the time picker now, you'll get another pull request later today :D |
You can keep pushing into this pull request. |
Ah of course, cool will do |
…f elements in timer picker, especially related to cancel / ok buttons; animation shows focus change back to picker also
…addition or subtraction causes hour or minute to change; fix am / pm bug
@wdullaer okay this should be good to go, added time picker support and just finished today with something I'd missed, support for selectable dates / times |
Cool. |
That's cool, labour of love :D let me know what you think, I had to get creative ;) |
That must be the best single line summary of android development I've seen |
Bump |
I have already taken a quick look, but the code is rather dense. I also need to look for an additional maintainer for this library. I have long stopped working on the project that spawned this, and my day job + newborn are preventing me from putting in the time and effort that this thing needs. |
It's true, there's a lot of conditional logic. In MonthView.java, there is a key handler which mainly processes the up, down, left and right keypresses to support keyboard and D-pad navigation. At about 160 lines, it's a lot, and looks a bit spaghetti. I'd defend it's usage on the grounds of keeping the TV code as a separate block away from the standard handlers, and if you read through the algorithm, there's quite a lot of messing about to moving through dates, especially between months. I felt I had to do this because the code is bound in with the month view. Obviously this is not ideal but due to the existing code structures, the view knows quite a lot about the month workings and so I proceeded with this compromise. Most of the code here deals with the difficulty of moving the day selection in a way that corresponds with how the user would expect visually. The other large clunk is in RadialPickerLayout.java, where there is again a long key handler, about the same length as for the month. It mostly deals with making sure that the hours, minutes and seconds are incremented / decremented correctly, with respect to the various conditions, such as 24 hour view, selectable times, and the fact that a minute or second change can affect the hour, etc. Other than that there are some small enough supporting changes. |
Would you be open to help support this feature? Like I mentioned, I'm pretty low on time, so I'm cautious when adding things, because I'm on the hook to support it indefinitely. If you'd be open to keep looking after this particular feature, that would change things. |
Yes I would be open to it. I think it would be great to have a material time / date picker which works with TV, certainly handy to me. I'm in :D |
BTW newborn on the way for me too in the new year :) |
@digithree congratulations! It can be though at the start, but I guarantee you it's all worth it. I'll merge this in over the weekend and do a release. |
(deleted comment from wrong account) Thank you @wdullaer ! It's exciting times. Let me know how you get on then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've done a first review round.
Depending on how much time I have, I might fix some of it myself later today
if(!mAllowAutoAdvance) return; | ||
if(index == HOUR_INDEX && mEnableMinutes) { | ||
setCurrentItemShowing(MINUTE_INDEX, true, true, false); | ||
public void advancePicker(int index, boolean force) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the purpose of this force flag here?
@@ -158,6 +158,9 @@ protected void setUpListView() { | |||
setFadingEdgeLength(0); | |||
// Make the scrolling behavior nicer | |||
setFriction(ViewConfiguration.getScrollFriction() * mFriction); | |||
// Turn off focus so that MonthView can get key events, for Android TV | |||
setFocusable(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't this be wrapped in Utils.isTv(getContext());
?
@@ -704,6 +735,13 @@ public void onClick(View v) { | |||
|
|||
mTimePicker.setBackgroundColor(mThemeDark? lightGray : circleBackground); | |||
view.findViewById(R.id.time_picker_dialog).setBackgroundColor(mThemeDark ? darkBackgroundColor : backgroundColor); | |||
|
|||
/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this code is not necessary, it should probably be removed
} | ||
} else if (index == SECOND_INDEX) { | ||
if (!mIs24HourMode) { | ||
// TODO : check this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you try the sample app on an Android TV box? This should allow you to try out this test case
(it's what I hope to do this afternoon)
…et MonthView to be focusable to allow view to receive key events