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

Added key listener to allow d-pad navigation of days in month view. S… #220

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

digithree
Copy link

…et MonthView to be focusable to allow view to receive key events

Simon Kenny added 4 commits July 25, 2016 17:30
…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
@wdullaer
Copy link
Owner

Thanks for the contribution.
I recently got an Nvidia Shield. I'll see how this behaves shortly :-)

@digithree
Copy link
Author

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

@wdullaer
Copy link
Owner

You can keep pushing into this pull request.
I'll go through the other PRs and feature requests while you're hacking away at this one.

@digithree
Copy link
Author

Ah of course, cool will do

@digithree
Copy link
Author

@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

@wdullaer
Copy link
Owner

wdullaer commented Aug 2, 2016

Cool.
Give me some time to check this out. I'll hopefully get around to it this weekend.
Real life has been rather hectic the last few months, so this library hasn't always received the TLC it deserves.

@digithree
Copy link
Author

That's cool, labour of love :D let me know what you think, I had to get creative ;)

@wdullaer
Copy link
Owner

wdullaer commented Aug 2, 2016

I had to get creative ;)

That must be the best single line summary of android development I've seen

@digithree
Copy link
Author

Bump

@wdullaer
Copy link
Owner

wdullaer commented Sep 7, 2016

I have already taken a quick look, but the code is rather dense.
There's a whole lot of if/else statements, which on first look feels very brittle, but I haven't grokked it completely, so I can't say how I'd like it done otherwise.

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.

@digithree
Copy link
Author

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.

@wdullaer
Copy link
Owner

Would you be open to help support this feature?
Debug issues that are created against this?

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.

@digithree
Copy link
Author

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

@digithree
Copy link
Author

BTW newborn on the way for me too in the new year :)

@wdullaer
Copy link
Owner

@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.
I'll also see if I can grant you commit rights, so you can more easily handle any necessary bug fixes.

@digithree
Copy link
Author

(deleted comment from wrong account)

Thank you @wdullaer ! It's exciting times.

Let me know how you get on then

Copy link
Owner

@wdullaer wdullaer left a 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) {
Copy link
Owner

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);
Copy link
Owner

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);

/*
Copy link
Owner

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
Copy link
Owner

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)

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.

None yet

2 participants