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

[confluence] fix music OSD alignment and improve visible conditions on OSD #7789

Merged
merged 1 commit into from
Oct 23, 2015

Conversation

un1versal
Copy link
Contributor

@da-anda @phil65 @ronie

MusicOSD
1) Had the alignment fix (tested) for player controls it was just too far left imo.
2) Also added visible condition that should allow the record button to become active/visible when the player can record stream this change is untested as I dont use PVR or any circumstances where PlayerControl(record) becomes selectable at all. Please advise on this visible condition change does it also need Player.Recording?


Note 1: There are MusicOSD buttons for accessing playlists that is never shown its part of skin textures but no used in skin, any idea why this is @ronie @phil65?

Note 2: Ive adjusted and added visible condition for the gap so that the osd controls aligns on the left hand side irrespective if item is visible or invisible @phil65 please check


Before
screenshot000

After - With conditional gap
screenshot009

VideoOSD
1) had visible condition applied to the DVD button, which is only usable when you are playing a DVD/DIsk which has menus VideoPlayer.HasMenu this was tested and seems to work as desired.

Before - playing a normal video
screenshot001

After - playing a normal video
screenshot005

After Playing normal video (subtitle submenu adjustment)
screenshot006

After - playing a DVD
screenshot004

After - playing a DVD (subtitle submenu normal position)
screenshot007

@un1versal un1versal force-pushed the RomeoMustDie branch 2 times, most recently from 420c5a2 to 3dda0a9 Compare August 12, 2015 16:18
@da-anda
Copy link
Member

da-anda commented Aug 14, 2015

untested, but looks good from the screenshots. Thanks

@un1versal
Copy link
Contributor Author

Note 1: There are MusicOSD buttons for accessing playlists that is never shown its part of skin textures but no used in skin, any idea why this is @ronie @phil65?

PING @ronie also I see the playlists OSD buttons were removed in be35997 So I presume we cant use them at all with some visible conditon?

@un1versal
Copy link
Contributor Author

PING @ronie

@un1versal un1versal force-pushed the RomeoMustDie branch 2 times, most recently from 3892621 to 16c1471 Compare August 24, 2015 15:24
@un1versal
Copy link
Contributor Author

@da-anda, also just fixed (caused by this PR) the subtitles submenu when !VideoPlayer.HasMenucondition without it the subtitles sub menu was not aligned with OSD button while not playing a DVD. So all fixed now.

Now its aligned when normal video and when is also optical media updated screenshots above in OP.

@un1versal
Copy link
Contributor Author

One more PING since ronie seems not to be reading this.

@phil65
Copy link
Contributor

phil65 commented Aug 28, 2015

We have lot to do and this seems far from being urgent. Please dont ping all few days.

@un1versal
Copy link
Contributor Author

Can anyone review this and let me know if its OK, its not urgent but it does fix the alignment shown and it seems to work as intended.

@@ -332,6 +339,7 @@
<onclick>PlayerControl(record)</onclick>
<enable>Player.CanRecord</enable>

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@un1versal un1versal force-pushed the RomeoMustDie branch 2 times, most recently from 2440df9 to e92364c Compare September 21, 2015 19:27
@@ -248,9 +254,9 @@
<onup>1000</onup>
<ondown>1000</ondown>
<onclick>PlayerControl(ShowVideoMenu)</onclick>
<enable>VideoPlayer.HasMenu</enable>

This comment was marked as spam.

This comment was marked as spam.

@un1versal un1versal force-pushed the RomeoMustDie branch 2 times, most recently from c0ac702 to 5a919e5 Compare September 22, 2015 03:55
@un1versal
Copy link
Contributor Author

Done thx, :) ,much cleaner also. Tested this also and all seems well.

@un1versal un1versal force-pushed the RomeoMustDie branch 2 times, most recently from 2d99bd0 to aac1928 Compare September 22, 2015 04:18
@@ -248,9 +253,8 @@
<onup>1000</onup>
<ondown>1000</ondown>
<onclick>PlayerControl(ShowVideoMenu)</onclick>
<enable>VideoPlayer.HasMenu</enable>
<animation effect="fade" start="100" end="50" time="75" condition="!VideoPlayer.HasMenu">Conditional</animation>

This comment was marked as spam.

This comment was marked as spam.

msduketown added a commit to msduketown/skin.xonfluence that referenced this pull request Sep 22, 2015
@un1versal un1versal force-pushed the RomeoMustDie branch 2 times, most recently from 4f4c96e to 412f992 Compare October 9, 2015 13:24
@un1versal
Copy link
Contributor Author

@da-anda @ronie something weird happens if adsp is enabled this PR doesnt account for it as is
(I simply forgot since adsp isnt usable)

Im lost on what to do when that happens + when canrecord and adsp would be visible.

for instance to make this work I added and this what makes me very confused.

            <control type="image" id="3000">
                <width>-55</width>
                <height>55</height>
                <texture>-</texture>
                <visible>!Player.CanRecord + system.getbool(audiooutput.dspaddonsenabled)</visible>
            </control>

width of minus 55?? but it works, also need to account for when adsp + player.canrecord are both visible, please help

@un1versal
Copy link
Contributor Author

@da-anda @ronie for comment above I added un1versal@f345eb8 to try account for DSP, but please let me know if its right or incomplete and what to add/change

local tests seem OK. but I can definetly need expert eyes and guidance and a decision what happens to this PR as its 2 months old now.

@un1versal
Copy link
Contributor Author

Thers already talks of jarvis beta #8239 is this PR being considered at all? @MartijnKaijser

@un1versal un1versal changed the title [confluence] fix alignment and improve visible conditions on OSD [confluence] fix music OSD alignment and improve visible conditions on OSD Oct 21, 2015
@@ -332,8 +343,8 @@
<texturefocus>OSDRecordOffFO.png</texturefocus>
<texturenofocus>OSDRecordOffNF.png</texturenofocus>
<onclick>PlayerControl(record)</onclick>
<enable>Player.CanRecord</enable>
<animation effect="fade" start="100" end="50" time="75" condition="!Player.CanRecord">Conditional</animation>

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@un1versal un1versal force-pushed the RomeoMustDie branch 2 times, most recently from 246e750 to c4726c0 Compare October 23, 2015 10:56
Hitcher added a commit that referenced this pull request Oct 23, 2015
[confluence] fix music OSD alignment and improve visible conditions on OSD

Merged, thanks.
@Hitcher Hitcher merged commit b349446 into xbmc:master Oct 23, 2015
@Hitcher Hitcher added this to the Jarvis 16.0-alpha4 milestone Oct 23, 2015
@Hitcher Hitcher added the Type: Fix non-breaking change which fixes an issue label Oct 23, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Fix non-breaking change which fixes an issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants