[PVR][guiinfo] Videofullscreen/Musicvisualisation: Direct channel number input #11434

ksooo · 2017-01-12T21:36:39Z

Instead of opening the numeric input dialog when pressing 0 - 9 while in PVR fullscreen or PVR music visualisation, now only the entered number appears on screen, like this works for almost all other tv sets and settop boxes.

2 seconds (existing advancedsettings value!) after the last digit was entered, Kodi will automatically switch to the channel with the entered number.

Entering an invalid number will be ignored and nothing will happen.

Entering more than 4 digits results in left-shifting the previously entered digits, discarding the oldest digit, and appending the just entered number.

Largest supported channel number is 9999.

Entering 4 zeros in a row will immediately end direct channel number input.

This PR was tested on latest Kodi master on Linux and macOS.

@da-anda fyi

@phil65 already reviewed the skin change

@ronie: skins must be adjusted to use the new info label PVR.ChannelNumberInput. Otherwise the feature will also work, but nothing will be seen on the screen while entering the number.

@Jalle19 for review?

ronie · 2017-01-12T21:44:24Z

thanx for the heads-up!

system/settings/settings.xml

@@ -1411,7 +1411,7 @@
        </setting>
        <setting id="pvrplayback.confirmchannelswitch" type="boolean" label="19281" help="36231">
          <level>1</level>
-          <default>false</default>
+          <default>true</default>


Jalle19 · 2017-01-13T13:50:56Z

Isn't there a button in the OSD during full screen playback which used to open the numpad? AFAIK it was only for users with limited remotes.

ksooo · 2017-01-13T14:00:07Z

Isn't there a button in the OSD during full screen playback which used to open the numpad? AFAIK it was only for users with limited remotes.

The button was "1" to "9", which does not help for remotes without these keys.

Jalle19 · 2017-01-13T14:07:51Z

@ksooo I mean a GUI button in the OSD.

ksooo · 2017-01-13T14:28:02Z

@ksooo I mean a GUI button in the OSD.

if there is such a GUI button i don't know it.

xbmc/pvr/PVRManager.h

@@ -603,6 +604,12 @@ namespace PVR
     */
    void PublishEvent(PVREvent state);

+    /*!
+     * @brief Get the nmeric channel input handler.


xbmc/pvr/PVRNumericChannelInputHandler.cpp

+
+  CSingleLock lock(m_critSection);
+
+  if (iDigit == 0 && m_digits.size() == 0)


xbmc/pvr/PVRNumericChannelInputHandler.cpp

+    if (m_digits.size() != m_iDigits || GetChannel() > 0)
+    {
+      for (int digit : m_digits)
+        m_strChannel.append(std::to_string(digit));


ksooo · 2017-01-13T14:39:36Z

jenkins build this please

ksooo · 2017-01-13T19:25:59Z

@Jalle19 all changes you requested are in. good to go?

xbmc/pvr/PVRNumericChannelInputHandler.cpp

+namespace PVR
+{
+
+CPVRNumericChannelInputHandler::CPVRNumericChannelInputHandler(int iDigits, int iSwitchDelay)


xbmc/pvr/PVRNumericChannelInputHandler.cpp

+  return iNumber;
+}
+
+std::string CPVRNumericChannelInputHandler::GetChannelAsString() const


xhaggi · 2017-01-13T21:58:40Z

Looks good and makes sense because the numric input dialog can only be reached by the numpad of a remote and then you will never use the numeric controls of the dialog.

ksooo · 2017-01-13T22:54:31Z

@xhaggi requested changes are in.

ksooo · 2017-01-13T22:55:13Z

jenkins build this please

xhaggi · 2017-01-13T22:57:20Z

Thanks, that was fast :)

Jalle19

Apart from these +1, I don't really have an opinion on the setting default change

xbmc/pvr/PVRChannelNumberInputHandler.cpp

+    if (m_digits.size() != m_iDigits || GetChannelNumber() > 0)
+    {
+      for (int digit : m_digits)
+      m_strChannel.append(StringUtils::Format("%d", digit));


xbmc/pvr/PVRChannelNumberInputHandler.cpp

+{
+
+CPVRChannelNumberInputHandler::CPVRChannelNumberInputHandler(int iDigits, int iSwitchDelay)
+: m_iDigits(iDigits),


xbmc/pvr/PVRManager.cpp

@@ -162,7 +163,8 @@ CPVRManager::CPVRManager(void) :
    m_bEpgsCreated(false),
    m_progressHandle(NULL),
    m_managerState(ManagerStateStopped),
-    m_isChannelPreview(false)
+    m_isChannelPreview(false),
+    m_channelNumberInputHandler(4 /* digits */, g_advancedSettings.m_iPVRNumericChannelSwitchTimeout)


ksooo · 2017-01-14T11:31:53Z

@Jalle19 all of the changes you requested are now in. thanks for the review.

ksooo · 2017-01-14T11:35:55Z

jenkins build this please

da-anda · 2017-01-14T12:38:12Z

ok, I tested this PR a bit with latest Milhouse build and found some issues.

the number input didn't seem to work when playing a radio channel. All I could enter was 1, all other keys where ignored
when in any PVR window (like the channels list) pressing a number still trigger the numpad OSD. We probably should get rid of it and instead jump to the related channel in the list

Then I was wondering about if we could probably use the channel switch delay setting for the delay of the numeric input instead of an advanced setting. IIRC this setting is currently only used when pressing up/down in the OSD and not for channelUp/channelDown actions. Just a thought.

ksooo · 2017-01-14T12:45:40Z

Confirming the radio channel bug. Will have look.

The other pvr windows suggestion is for another PR. This is simply not part of this iteration.

Using the official setting instead of the advanced setting (that we were also using here before for whatever reason, btw) makes sense.

EDIT: On second thought, using the official channel switch delay setting makes no sense at it serves a different purpose, imo. The advanced setting is for adjusting the time between the last key press and the actual switch. This generally will usually never be more than 3 seconds, i guess. Compared to this, the official setting is for delay between up/down press followed by immediate display of next/prev channel epg data and the delayed actual switch. Thus, the value depends on how fast people can read the epg data, which could be easily more than 3 seconds, for slower people. So, I guess people might want really different values for the twi use case and think it is fine as it is now.

EDIT2: Radio channel input (other numbers than 1) works fine on my Mac, but not on my custom LibreELEC build. How I hate platform dependent issues!

da-anda · 2017-01-14T12:51:29Z

btw, thanks sooooo much for this PR. I think I owe you a brewery by now ;)

ksooo · 2017-01-14T13:22:29Z

ok, I tested this PR a bit with latest Milhouse build and found some issues.
the number input didn't seem to work when playing a radio channel. All I could enter was 1, all other keys where ignored

Radio channel input (other numbers than 1) works fine on my Mac, but not on my custom LibreELEC build.

Seems to be a remote vs. keyboard config issue problem. When using a keyboard, numbers other than 0 and 1 also work on LibreELEC.

Strange.

da-anda · 2017-01-15T11:38:40Z

thought about this feature some more. I think it might be good if the "back" action could also abort the number input process. What do you think?

edit: as for the numeric input. I just saw your fix by also listening to JUMPSMS. The correct fix IMO would be to adjust the remote.xml keymap so that the keys send NUMBER[X] instead of JUMPSMS[X] action, because this mapping seems to be missing for PVR windows from a quick look at the keymap file

edit2: I think the mapping has to be added to the visualisation window (to which FullscreenRadio falls back)

ksooo · 2017-01-15T12:51:11Z

thought about this feature some more. I think it might be good if the "back" action could also abort the number input process. What do you think?

maybe later.

edit: as for the numeric input. I just saw your fix by also listening to JUMPSMS. The correct fix IMO would be to adjust the remote.xml keymap

both is needed, to be on the "save" side.

ksooo · 2017-01-15T22:05:16Z

jenkins build this please

…, no longer for direct channel number input. the latter will always auto switch after the value of 'channel switch delay'.

ksooo · 2017-01-16T21:13:10Z

Well, reworked everything and now it is also possible to use direct input in channel and guide window, @da-anda

@Jalle19 could you please re-review?

Jalle19

More complicated than the previous implementation, but hey, if it works...

xbmc/pvr/windows/GUIWindowPVRGuide.cpp

@@ -45,6 +44,7 @@ using namespace EPG;

 CGUIWindowPVRGuide::CGUIWindowPVRGuide(bool bRadio) :
  CGUIWindowPVRBase(bRadio, bRadio ? WINDOW_RADIO_GUIDE : WINDOW_TV_GUIDE, "MyPVRGuide.xml"),
+  CPVRChannelNumberInputHandler(1000),


da-anda · 2017-01-17T08:12:38Z

I think we should drop the JumpSMS stuff now that we found the root cause of it not working on radio channels.
Besides of that, thanks so much again.

addons/skin.estuary/xml/DialogSeekBar.xml

@@ -1,11 +1,23 @@
 <?xml version="1.0" encoding="UTF-8"?>
 <window>
-	<visible>[[Player.Seeking | Player.DisplayAfterSeek | [Player.Paused + !Player.Caching] | Player.Forwarding | Player.Rewinding | Player.ShowInfo | Window.IsActive(fullscreeninfo) | Window.IsActive(videoosd) | Window.IsActive(playerprocessinfo)] + Window.IsActive(fullscreenvideo)] | Window.IsActive(visualisation) | !String.IsEmpty(Player.SeekNumeric)</visible>
+	<visible>[[Player.Seeking | Player.DisplayAfterSeek | [Player.Paused + !Player.Caching] | Player.Forwarding | Player.Rewinding | Player.ShowInfo | Window.IsActive(fullscreeninfo) | Window.IsActive(videoosd) | Window.IsActive(playerprocessinfo)] + Window.IsActive(fullscreenvideo)] | Window.IsActive(visualisation) | !String.IsEmpty(Player.SeekNumeric) | !String.IsEmpty(PVR.ChannelNumberInput)</visible>


ksooo · 2017-01-17T17:10:41Z

I think we should drop the JumpSMS stuff now that we found the root cause of it not working on radio channels.

Nope. Currently not needed, but correct nevertheless.

ksooo · 2017-01-17T17:12:42Z

More complicated than the previous implementation,

Not really; just factored out a base class of what we had before to reuse the code for three different use cases (before it was only one).

da-anda · 2017-01-17T17:52:45Z

I think we should drop the JumpSMS stuff now that we found the root cause of it not working on radio channels.

Nope. Currently not needed, but correct nevertheless.

it's two different things, isn't it? JumpSMS is about jumping by name while numeric input is jumping by number. But up to you.

… pvr core changes, new guiinfo label

… fix remote keymap numeric key definitions

…nges

ksooo · 2017-01-17T22:08:11Z

jenkins build this please

ksooo · 2017-01-17T22:41:22Z

jenkins error is unrelated

da-anda · 2017-01-18T08:06:09Z

thanks a lot, huge improvement

ksooo added Type: Feature non-breaking change which adds functionality Component: PVR Component: Skin v18 Leia labels Jan 12, 2017

ksooo added this to the L 18.0-alpha1 milestone Jan 12, 2017

ksooo requested a review from Jalle19 January 12, 2017 21:36

afedchin reviewed Jan 13, 2017

View reviewed changes

Jalle19 requested changes Jan 13, 2017

View reviewed changes

ksooo force-pushed the pvr-channel-switch-with-okay-cleanup branch from 8d5c1c4 to 35c5b15 Compare January 13, 2017 19:24

xhaggi reviewed Jan 13, 2017

View reviewed changes

xbmc/pvr/PVRNumericChannelInputHandler.cpp

namespace PVR

{

CPVRNumericChannelInputHandler::CPVRNumericChannelInputHandler(int iDigits, int iSwitchDelay)

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

xhaggi reviewed Jan 13, 2017

View reviewed changes

xbmc/pvr/PVRNumericChannelInputHandler.cpp

return iNumber;

}

std::string CPVRNumericChannelInputHandler::GetChannelAsString() const

This comment was marked as spam.

Sign in to view

ksooo force-pushed the pvr-channel-switch-with-okay-cleanup branch from 35c5b15 to ab840a0 Compare January 13, 2017 22:53

Jalle19 reviewed Jan 14, 2017

View reviewed changes

ksooo force-pushed the pvr-channel-switch-with-okay-cleanup branch from ab840a0 to ccaf719 Compare January 14, 2017 11:30

ksooo force-pushed the pvr-channel-switch-with-okay-cleanup branch from ccaf719 to 9e15960 Compare January 14, 2017 11:35

ksooo force-pushed the pvr-channel-switch-with-okay-cleanup branch from 9e15960 to efabcb3 Compare January 15, 2017 10:59

ksooo force-pushed the pvr-channel-switch-with-okay-cleanup branch from efabcb3 to c0d8714 Compare January 15, 2017 12:43

ksooo added 2 commits January 16, 2017 22:10

[PVR] Settings: change default for 'confirm channel switch' to true.

5571af6

[PVR] Settings: use 'confirm channel switch' only for channel surfing…

0ce3406

…, no longer for direct channel number input. the latter will always auto switch after the value of 'channel switch delay'.

ksooo force-pushed the pvr-channel-switch-with-okay-cleanup branch from c0d8714 to 43e86a0 Compare January 16, 2017 21:11

Jalle19 approved these changes Jan 17, 2017

View reviewed changes

da-anda reviewed Jan 17, 2017

View reviewed changes

ksooo force-pushed the pvr-channel-switch-with-okay-cleanup branch 2 times, most recently from e872535 to 3288734 Compare January 17, 2017 22:05

ksooo added 3 commits January 17, 2017 23:05

[PVR][guiinfo] Rework fullscreen window direct channel number input -…

257c44b

… pvr core changes, new guiinfo label

[PVR][keymaps] Rework fullscreen window direct channel number input -…

7041405

… fix remote keymap numeric key definitions

[PVR] Rework fullscreen window direct channel number input - skin cha…

3288734

…nges

ksooo merged commit c255597 into xbmc:master Jan 18, 2017

ksooo deleted the pvr-channel-switch-with-okay-cleanup branch January 18, 2017 06:49


		CSingleLock lock(m_critSection);

		if (iDigit == 0 && m_digits.size() == 0)

[PVR][guiinfo] Videofullscreen/Musicvisualisation: Direct channel number input #11434

[PVR][guiinfo] Videofullscreen/Musicvisualisation: Direct channel number input #11434

Conversation

ksooo commented Jan 12, 2017 • edited

ronie commented Jan 12, 2017

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

Jalle19 commented Jan 13, 2017

ksooo commented Jan 13, 2017

Jalle19 commented Jan 13, 2017

ksooo commented Jan 13, 2017

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

ksooo commented Jan 13, 2017

ksooo commented Jan 13, 2017

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

xhaggi commented Jan 13, 2017

ksooo commented Jan 13, 2017

ksooo commented Jan 13, 2017

xhaggi commented Jan 13, 2017

Jalle19 left a comment

Choose a reason for hiding this comment

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

ksooo commented Jan 14, 2017

ksooo commented Jan 14, 2017

da-anda commented Jan 14, 2017

ksooo commented Jan 14, 2017 • edited

da-anda commented Jan 14, 2017

ksooo commented Jan 14, 2017

da-anda commented Jan 15, 2017 • edited

ksooo commented Jan 15, 2017

ksooo commented Jan 15, 2017

ksooo commented Jan 16, 2017

Jalle19 left a comment

Choose a reason for hiding this comment

This comment was marked as spam.

da-anda commented Jan 17, 2017 • edited

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

ksooo commented Jan 17, 2017

ksooo commented Jan 17, 2017

da-anda commented Jan 17, 2017 • edited

ksooo commented Jan 17, 2017

ksooo commented Jan 17, 2017

da-anda commented Jan 18, 2017

ksooo commented Jan 12, 2017 •

edited

ksooo commented Jan 14, 2017 •

edited

da-anda commented Jan 15, 2017 •

edited

da-anda commented Jan 17, 2017 •

edited

da-anda commented Jan 17, 2017 •

edited