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

[infomanager] add player.dbid infolabel #10567

Merged
merged 1 commit into from Oct 15, 2016

Conversation

@BigNoid
Copy link
Member

commented Sep 26, 2016

Add player.dbid as infolabel

Description

Motivation and Context

We have listitem.dbid so it makes sense to have player.dbid. It may be useful for starting scripts from the video osd.

How Has This Been Tested?

Runtime tested. Infolabel returns the same info as listitem.dbid

Screenshots (if appropriate):

Types of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the Code guidelines of this project
  • My change requires a change to the documentation, either Doxygen or wiki
  • I have updated the documentation accordingly
  • I have read the CONTRIBUTING document
  • I have added tests to cover my change
  • All new and existing tests passed
{ "tempoenabled", PLAYER_SUPPORTS_TEMPO},
{ "istempo", PLAYER_IS_TEMPO},
{ "playspeed", PLAYER_PLAYSPEED}};
{ "tempoenabled", PLAYER_SUPPORTS_TEMPO},

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Sep 26, 2016

Member

please keep as it was. we agreed on no stupid vertical alignment anymore

@BigNoid BigNoid force-pushed the BigNoid:player_dbid branch 3 times, most recently from 6c08141 to 901146c Sep 26, 2016
mikesilvo164 referenced this pull request in BigNoid/Aeon-Nox Sep 29, 2016
Wallpaper by SamFisher
@MartijnKaijser

This comment has been minimized.

Copy link
Member

commented Oct 13, 2016

jenkins build this please

@@ -508,6 +508,7 @@ const infomap player_labels[] = {{ "hasmedia", PLAYER_HAS_MEDIA },
{ "channelpreviewactive", PLAYER_IS_CHANNEL_PREVIEW_ACTIVE},
{ "tempoenabled", PLAYER_SUPPORTS_TEMPO},
{ "istempo", PLAYER_IS_TEMPO},
{ "dbid", PLAYER_DBID },

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Oct 15, 2016

Member

this is wrong section. dbid is no player state

@BigNoid BigNoid force-pushed the BigNoid:player_dbid branch from 901146c to 478d18c Oct 15, 2016
@BigNoid

This comment has been minimized.

Copy link
Member Author

commented Oct 15, 2016

@FernetMenta moved it to underneath title now

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Oct 15, 2016

I don't see any change. dbid is not a player state. dbid an attribute of the videotag. Label needs to be something like XXX_DBID, but not PLAYER_DBID

EDIT: you don't even access player object but m_currentFile

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Oct 15, 2016

I would expect this in MUSICPLAYER_xx or VIDEOPLAYER_xx
There is also a section PLAYER_ITEM_XX, maybe this one fits but not sure why this was introduced.

@BigNoid

This comment has been minimized.

Copy link
Member Author

commented Oct 15, 2016

dbid an attribute of the videotag. Label needs to be something like XXX_DBID, but not PLAYER_DBID

This is true from a dev point of view, but for the ones who this is exposed too it is an attribute of the player as it is for both video and music.
I have no problems changing it to musicplayer.dbid and videoplayer.dbid as it returns empty for everything but database items though

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Oct 15, 2016

but for the ones who this is exposed too

Those are also devs who need to understand what they are want to show on the UI. Otherwise we get something like the DisplayAfterSeek messed up states we had a couple of weeks ago.

@BigNoid

This comment has been minimized.

Copy link
Member Author

commented Oct 15, 2016

Those are also devs who need to understand what they are want to show on the UI. Otherwise we get something like the DisplayAfterSeek messed up states we had a couple of weeks ago.

I see your point. For me player label was always if it was generic for both audio and video, like player.title and player.art(xx)

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Oct 15, 2016

compare the code of PLAYER_TITLE with the code of VIDEO/MUSICPLAYER_TITLE
the first is a mess compared to the latter where the tag is already accessed any new attributes can be easily added.

@BigNoid BigNoid force-pushed the BigNoid:player_dbid branch 2 times, most recently from 7c19f19 to 6ce4188 Oct 15, 2016
@BigNoid BigNoid force-pushed the BigNoid:player_dbid branch from 6ce4188 to 710784c Oct 15, 2016
@BigNoid

This comment has been minimized.

Copy link
Member Author

commented Oct 15, 2016

@FernetMenta updated now to musicplayer.dbid and videoplayer.dbid. Thx for the review, this looks much cleaner.

@ronie

This comment has been minimized.

Copy link
Member

commented Oct 15, 2016

iirc the long term goal was to drop the separate musicplayer.xxx / videoplayer.xxx labels completely and only use player.xxx ones instead.
http://forum.kodi.tv/showthread.php?tid=106772&pid=853324#pid853324

no strong opinion either way, just as a fyi :-)

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Oct 15, 2016

@BigNoid looks good now

@ronie this is a rather old post. Considering how the app has evolved, I don't think this makes sense anymore. Player itself exposes more and more info that separated views on player and items are more appropriate.

@BigNoid

This comment has been minimized.

Copy link
Member Author

commented Oct 15, 2016

jenkins build this please

@MartijnKaijser MartijnKaijser added v17 Krypton and removed v18 Leia labels Oct 15, 2016
@MartijnKaijser MartijnKaijser added this to the Krypton 17.0-beta4 milestone Oct 15, 2016
@MartijnKaijser MartijnKaijser merged commit f9e8847 into xbmc:master Oct 15, 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
@BigNoid BigNoid deleted the BigNoid:player_dbid branch Oct 15, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.