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

SeekHandler: handle the seeking with numeric input. #10769

Merged
merged 4 commits into from Oct 27, 2016

Conversation

@afedchin
Copy link
Member

commented Oct 25, 2016

this returns the seeking with numpad after #10706

@FernetMenta ping

@afedchin afedchin added this to the Krypton 17.0-beta5 milestone Oct 25, 2016
@afedchin afedchin changed the title Numeric seek SeekHandler: handle the seeking with numeric input. Oct 25, 2016
return 0;
}

bool CSeekHandler::GetTimeCodeStamp(std::string *strDispTime /*= nullptr*/) const

This comment has been minimized.

Copy link
@xhaggi

xhaggi Oct 25, 2016

Member

instead of return true/false in case we have a valid time code, should we introduce a new method and use that instead?

bool CSeekHandler::HasTimeCode()
{
  return m_timeCodePosition > 0;
}

This comment has been minimized.

Copy link
@xhaggi

xhaggi Oct 25, 2016

Member

and instead of doing all formatting magic here, should we use CDateTime instead?

CDateTime time = CDateTime(static_cast<time_t>(GetTimeCodeSeconds()));
return time->GetAsLocalizedTime("HH:MM:SS");

This comment has been minimized.

Copy link
@afedchin

afedchin Oct 25, 2016

Author Member

about CDateTime: I've tried to format seconds in time format, but there is a case then it produces unexpected behavior. if user types a few 9 it should display something like this 00:99:99.

This comment has been minimized.

Copy link
@xhaggi

xhaggi Oct 25, 2016

Member

Should we really do that? IMO we should show the user the correct time label 99 will show up 01:39:00 or shouldn't we?

This comment has been minimized.

Copy link
@afedchin

afedchin Oct 25, 2016

Author Member

i'm not sure, because as an user I'm expecting to see what I'm typing.

}
}

int CSeekHandler::GetTimeCodeAmount() const

This comment has been minimized.

Copy link
@xhaggi

xhaggi Oct 25, 2016

Member

IMO we should rename this method to GetTimeCodeSeconds() to be clear what it does return.

@xhaggi

This comment has been minimized.

Copy link
Member

commented Oct 25, 2016

@afedchin thank you for jumping on the train.

@afedchin afedchin force-pushed the afedchin:numeric_seek branch from c8459e6 to 4eaca52 Oct 25, 2016
@afedchin

This comment has been minimized.

Copy link
Member Author

commented Oct 25, 2016

@ronie ping. new info label added.

@@ -104,6 +105,18 @@ CGUIWindowFullScreen::~CGUIWindowFullScreen(void)

bool CGUIWindowFullScreen::OnAction(const CAction &action)
{
if (CSeekHandler::GetInstance().HasTimeCode() && action.GetButtonCode())

This comment has been minimized.

Copy link
@afedchin

afedchin Oct 25, 2016

Author Member

still don't find a solution for this hack.

This comment has been minimized.

Copy link
@xhaggi

xhaggi Oct 25, 2016

Member

don't care about that IMO. The reason for it is to have a different button mapped to select. IMO we should load the default instead of that virtual window.

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Oct 25, 2016

Member

we do care about this because this code has to leave CGUIWindowFullScreen

This comment has been minimized.

Copy link
@afedchin

afedchin Oct 25, 2016

Author Member

fixed

return 0;
}

std::string CSeekHandler::GetTimeCodeStamp() const

This comment has been minimized.

Copy link
@xhaggi

xhaggi Oct 25, 2016

Member

sorry for being too picky but what do you think about renaming it to GetTimeCodeAsString() like we do in CDateTime.

This comment has been minimized.

Copy link
@xhaggi

xhaggi Oct 25, 2016

Member

if we drop that method ignore my comment 😄

This comment has been minimized.

Copy link
@afedchin

afedchin Oct 25, 2016

Author Member

it's dropped

@@ -8001,6 +8002,10 @@ std::string CGUIInfoManager::GetMultiInfoLabel(const GUIInfo &info, int contextW
if (seekSize > 0)
return "+" + strSeekSize;
}
else if (info.m_info == PLAYER_SEEKNUMERIC)
{
return CSeekHandler::GetInstance().GetTimeCodeStamp();

This comment has been minimized.

Copy link
@xhaggi

xhaggi Oct 25, 2016

Member

it should be possible for the skinner to specify a different time format.

int seekTimeCode = CSeekHandler::GetInstance().GetTimeCodeSeconds();
TIME_FORMAT format = (TIME_FORMAT)info.GetData1();
if (format == TIME_FORMAT_GUESS && seekTimeCode >= 3600)
    format = TIME_FORMAT_HH_MM_SS;
return StringUtils::SecondsToTimeString(seekTimeCode, format);

This comment has been minimized.

Copy link
@xhaggi

xhaggi Oct 25, 2016

Member

if GetTimeCodeStamp is only used here, we should drop it.

This comment has been minimized.

Copy link
@afedchin

afedchin Oct 25, 2016

Author Member

done

@@ -632,7 +632,8 @@ const infomap player_times[] = {{ "seektime", PLAYER_SEEKTIME },
{ "time", PLAYER_TIME },
{ "duration", PLAYER_DURATION },
{ "finishtime", PLAYER_FINISH_TIME },
{ "starttime", PLAYER_START_TIME}};
{ "starttime", PLAYER_START_TIME },
{ "seeknumeric", PLAYER_SEEKNUMERIC } };

This comment has been minimized.

Copy link
@xhaggi

xhaggi Oct 25, 2016

Member

should we use seektimecode instead.

This comment has been minimized.

Copy link
@afedchin

afedchin Oct 25, 2016

Author Member

I think seeknumeric is saying for itself

This comment has been minimized.

Copy link
@xhaggi

xhaggi Oct 25, 2016

Member

Okay, the rest looks good. thanks 👍

@afedchin afedchin force-pushed the afedchin:numeric_seek branch from 4eaca52 to 1583df9 Oct 25, 2016
@@ -1397,13 +1397,19 @@ int CGUIWindowManager::GetActiveWindowID()
// check for LiveTV and switch to it's virtual window
else if (g_PVRManager.IsStarted() && g_application.CurrentFileItem().HasPVRChannelInfoTag())
iWin = WINDOW_FULLSCREEN_LIVETV;
// special casting for numeric seek

This comment has been minimized.

Copy link
@garbear

garbear Oct 25, 2016

Member

casting or casing?

This comment has been minimized.

Copy link
@afedchin

afedchin Oct 25, 2016

Author Member

casing ofc.

@afedchin afedchin force-pushed the afedchin:numeric_seek branch from 6bf46bc to 32312fd Oct 25, 2016
@afedchin

This comment has been minimized.

Copy link
Member Author

commented Oct 26, 2016

@ronie @phil65 @HitcherUK , guys, please someone adapt estuary for these changes please.

@afedchin afedchin force-pushed the afedchin:numeric_seek branch from 32312fd to 1d3ede2 Oct 26, 2016
case REMOTE_8:
case REMOTE_9:
{
if (!g_application.CurrentFileItem().IsLiveTV())

This comment has been minimized.

Copy link
@da-anda

da-anda Oct 26, 2016

Member

I know it's been like that before, but this IMO looks wrong in two ways:
a) when having a timeshift buffer, seeking to a time code within the buffer might still make sense
b) g_application.m_pPlayer->CanSeek() (which is checked in "onAction" method) should already take care of this

This comment has been minimized.

Copy link
@da-anda

da-anda Oct 26, 2016

Member

ah wait, it's conflicting with channel number input, so it might make sense to keep it, unless channel# input isn't processed upfront

This comment has been minimized.

Copy link
@ksooo

ksooo Oct 26, 2016

Member

... i'm getting scared. Do we actually have a clue what we're doing here? ;-)

This comment has been minimized.

Copy link
@da-anda

da-anda Oct 26, 2016

Member

well, you pvr guys should know as likely one of you added this special case handling back then ;) I'm not at all into the codebase anymore, just spotted this and thought this is looking weird

This comment has been minimized.

Copy link
@ksooo

ksooo Oct 26, 2016

Member

this was all long before my time, sorry. but hey, don't take my comment to serious. it was in no way meant to be offensive.

This comment has been minimized.

Copy link
@afedchin

afedchin Oct 26, 2016

Author Member

If I'm not wrong a window handle this upfront, so we can remove this to do seeking for recordings. I can't check this right now, but will do asap.

This comment has been minimized.

Copy link
@afedchin

afedchin Oct 26, 2016

Author Member

just tested. it works as it should.

@ronie

This comment has been minimized.

Copy link
Member

commented Oct 26, 2016

@afedchin skin part in #10786

@afedchin

This comment has been minimized.

Copy link
Member Author

commented Oct 26, 2016

@ronie thank you!

if there are no objections, I will merge this tomorrow morning.

jenkins build this please

@xhaggi
xhaggi approved these changes Oct 26, 2016
Copy link
Member

left a comment

+1

@afedchin afedchin merged commit d94de6b into xbmc:master Oct 27, 2016
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
jenkins4kodi You did a great job. Have a cookie.
Details
@afedchin afedchin deleted the afedchin:numeric_seek branch Oct 27, 2016
@xhaggi

This comment has been minimized.

Copy link
Member

commented Oct 30, 2016

@afedchin I've done some tests while adding the new info label to Xperience1080. The info label works great but it won't jump to the given time. Could someone else confirm if it works?

@xhaggi

This comment has been minimized.

Copy link
Member

commented Oct 30, 2016

@afedchin sorry for the noise i missed that I've to press Enter/OK/Select 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.