[pvr] fix: missing action mapping for ACTION_RECORD #4171

Merged
merged 1 commit into from Feb 10, 2014

Conversation

Projects
None yet
4 participants
@xhaggi
Member

xhaggi commented Feb 7, 2014

This adds the missing button translator action "record" poiting to ACTION_RECORD which is currently used in several PVR related windows, but could not be used in key mappings.

It also adds a default implementation to GUIWindow which calls the application player record functionality, so there is no need to use XBMC.PlayerControls(record) in key mapping anymore.

Furthermore the default mapping for the record button in remote.xml is changed to this new action.

This is discussed at http://forum.xbmc.org/showthread.php?tid=185107.

@opdenkamp it's up to you if this is a fix or feature for G+1.

@jmarshallnz

View changes

xbmc/guilib/GUIWindow.cpp
+ case ACTION_RECORD:
+ if(g_application.m_pPlayer->IsPlaying() && g_application.m_pPlayer->CanRecord())
+ g_application.m_pPlayer->Record(!g_application.m_pPlayer->IsRecording());
+ break;

This comment has been minimized.

@jmarshallnz

jmarshallnz Feb 7, 2014

Member

CGUIWindow should not need to know about the player. Surely the record action belongs in the player action handling at the app level?

@jmarshallnz

jmarshallnz Feb 7, 2014

Member

CGUIWindow should not need to know about the player. Surely the record action belongs in the player action handling at the app level?

This comment has been minimized.

@xhaggi

xhaggi Feb 7, 2014

Member

should I execute a bulidin instead?

@xhaggi

xhaggi Feb 7, 2014

Member

should I execute a bulidin instead?

This comment has been minimized.

@jmarshallnz

jmarshallnz Feb 7, 2014

Member

Nope, you should move the handling out of here to somewhere else (I'd do it at the application level).

@jmarshallnz

jmarshallnz Feb 7, 2014

Member

Nope, you should move the handling out of here to somewhere else (I'd do it at the application level).

@xhaggi

This comment has been minimized.

Show comment
Hide comment
@xhaggi

xhaggi Feb 8, 2014

Member

@jmarshallnz refactored the player stuff in fixup commit

Member

xhaggi commented Feb 8, 2014

@jmarshallnz refactored the player stuff in fixup commit

@opdenkamp

View changes

xbmc/guilib/GUIWindow.cpp
+ return OnBack(action.GetID());
+
+ case ACTION_RECORD:
+ g_application.Record();

This comment has been minimized.

@opdenkamp

opdenkamp Feb 8, 2014

Member

shouldn't we use app messenger for this instead since this could block the ui thread?

@opdenkamp

opdenkamp Feb 8, 2014

Member

shouldn't we use app messenger for this instead since this could block the ui thread?

This comment has been minimized.

@xhaggi

xhaggi Feb 8, 2014

Member

i don't know enough about this .. @jmarshallnz please help ;)

@xhaggi

xhaggi Feb 8, 2014

Member

i don't know enough about this .. @jmarshallnz please help ;)

This comment has been minimized.

@jmarshallnz

jmarshallnz Feb 9, 2014

Member

I obviously haven't been clear enough :)

CGUIWindow should not need to know or care about ACTION_RECORD. It makes no sense for the handling thereof to be in this class. After all, why should the volume dialog for example need to care about the record action? Why should a settings screen need to care?

This block shouldn't be here, period.

If you need to handle ACTION_RECORD, then the code for doing so should be in CApplication::OnAction() or CApplicationPlayer::OnAction() or whatever it is.

@jmarshallnz

jmarshallnz Feb 9, 2014

Member

I obviously haven't been clear enough :)

CGUIWindow should not need to know or care about ACTION_RECORD. It makes no sense for the handling thereof to be in this class. After all, why should the volume dialog for example need to care about the record action? Why should a settings screen need to care?

This block shouldn't be here, period.

If you need to handle ACTION_RECORD, then the code for doing so should be in CApplication::OnAction() or CApplicationPlayer::OnAction() or whatever it is.

This comment has been minimized.

@xhaggi

xhaggi Feb 9, 2014

Member

Thanks for clarifying this ;)

@xhaggi

xhaggi Feb 9, 2014

Member

Thanks for clarifying this ;)

@xhaggi

This comment has been minimized.

Show comment
Hide comment
@xhaggi

xhaggi Feb 9, 2014

Member

@jmarshallnz done so far

Member

xhaggi commented Feb 9, 2014

@jmarshallnz done so far

@MartijnKaijser MartijnKaijser added this to the Pending for inclusion milestone Feb 9, 2014

+ if (m_pPlayer->CanRecord())
+ m_pPlayer->Record(!m_pPlayer->IsRecording());
+ }
+

This comment has been minimized.

@jmarshallnz

jmarshallnz Feb 9, 2014

Member

I wonder if we should drop it down into CPlayerController so that it's handled in the next block. I don't really mind too much, but it makes sense IMO to move all the above block into there, so adding new code there instead of here seems a bit better?

(Purely cosmetic at this stage!)

@jmarshallnz

jmarshallnz Feb 9, 2014

Member

I wonder if we should drop it down into CPlayerController so that it's handled in the next block. I don't really mind too much, but it makes sense IMO to move all the above block into there, so adding new code there instead of here seems a bit better?

(Purely cosmetic at this stage!)

This comment has been minimized.

@xhaggi

xhaggi Feb 9, 2014

Member

Agree, could be done in a separate cosmetic commit. Do you mean the whole if (m_pPlayer->IsPlaying()) block?

@xhaggi

xhaggi Feb 9, 2014

Member

Agree, could be done in a separate cosmetic commit. Do you mean the whole if (m_pPlayer->IsPlaying()) block?

This comment has been minimized.

@jmarshallnz

jmarshallnz Feb 9, 2014

Member

Yeah - I think it's safer to leave that alone at present. A useful cleanup in G+1 maybe?

@jmarshallnz

jmarshallnz Feb 9, 2014

Member

Yeah - I think it's safer to leave that alone at present. A useful cleanup in G+1 maybe?

This comment has been minimized.

@xhaggi

xhaggi Feb 9, 2014

Member

Okay I'll add a separate PR so it is not lost

@xhaggi

xhaggi Feb 9, 2014

Member

Okay I'll add a separate PR so it is not lost

@jmarshallnz

This comment has been minimized.

Show comment
Hide comment
@jmarshallnz

jmarshallnz Feb 9, 2014

Member

Looks much better.

Member

jmarshallnz commented Feb 9, 2014

Looks much better.

@jmarshallnz

This comment has been minimized.

Show comment
Hide comment
@jmarshallnz

jmarshallnz Feb 9, 2014

Member

jenkins build this please

Member

jmarshallnz commented Feb 9, 2014

jenkins build this please

@xhaggi

This comment has been minimized.

Show comment
Hide comment
@xhaggi

xhaggi Feb 10, 2014

Member

something goes wrong in build 176 while fetching from github.

Receiving objects:  99% (111685/112277), 308.43 MiB | 586 KiB/s   
remote: fatal: unable to read fafbcc65570226aeae31637004fa84e480075c4a�[K
remote: aborting due to possible repository corruption on the remote side.
Member

xhaggi commented Feb 10, 2014

something goes wrong in build 176 while fetching from github.

Receiving objects:  99% (111685/112277), 308.43 MiB | 586 KiB/s   
remote: fatal: unable to read fafbcc65570226aeae31637004fa84e480075c4a�[K
remote: aborting due to possible repository corruption on the remote side.
[pvr] fix: missing action mapping for ACTION_RECORD
This adds the missing button translator action "record" poiting to
ACTION_RECORD which is currently used in several PVR related windows,
but could not be used in key mappings.

It also adds a default implementation to GUIWindow which calls the
application player record functionality, so there is no need to use
XBMC.PlayerControls(record) in key mapping anymore.

Furthermore the default mapping for the record button in remote.xml is
changed to this new action.
@xhaggi

This comment has been minimized.

Show comment
Hide comment
@xhaggi

xhaggi Feb 10, 2014

Member

rebased fixups.

jenkins build this please

Member

xhaggi commented Feb 10, 2014

rebased fixups.

jenkins build this please

jmarshallnz added a commit that referenced this pull request Feb 10, 2014

Merge pull request #4171 from xhaggi/record-action-mapping
[pvr] fix: missing action mapping for ACTION_RECORD

@jmarshallnz jmarshallnz merged commit 733f36b into xbmc:master Feb 10, 2014

1 check failed

default Merged build #177 failed in 26 min
Details

@MartijnKaijser MartijnKaijser modified the milestones: Gotham13.0-alpha12 January 2014, Pending for inclusion Feb 12, 2014

@xhaggi xhaggi deleted the xhaggi:record-action-mapping branch Feb 19, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment