[mikeSiLVO]

I feel the need to preface this with a definitive...
I, absolutely, love this new feature! Thank you very much for working on it!

My only experience with this current iteration is from a skinning standpoint, and while I am ecstatic that it was added, it has been an extremely negative experience for me. Lots of things I expect to work, just do not...

I am a bit late to the party here but it was originally suggested to use dialog select and, honestly, that should have been the way to go.

Even though this was dismissed a few times in this thread, does this actually need to be a separate dialog?

This is coming from someone actively monitoring this addition that has already modified and provided this dialog for my Omega version... just cause I have to. However, I am literally hacking a modified DialogSelect to make this same versions play/manage dialog...

It has to be possible to merge this with DialogSelect. It appears to be similar to what I want and what has been already merged as an update. All that is needed is HasVideoVersions to be separate from a missing HasVideoExtras and for moving off the list to be the same as a select/enter/ok. The current implementation is just plain weird. I cannot find a reasonable skinning engine equivalent to a few issues and a select onunfocus is just blorf and does not work anyway...

[jjd-uk]

As it happens we've been discussing this on the Teams internal Slack channel.

Both myself and Hitcher agree that from a design POV that the long term aim should be to use Dialog Select for selecting a playback item, and the new Dialog Video Version be used only for management.

For consistancy perhaps something similar to the Blu-Ray simple menu could be done



I'm not a C++ coder but I can sometimes follow what a code section is doing, even I don't understand the details of the specific code used, and this would suggest to me that this uses Dialog Select https://github.com/xbmc/xbmc/blob/master...u.cpp#L108

You could have something similar for Versions where the Versions are displayed (like those Main Titles on a Bluray are shown) then have a Show Extras at the bottom which would another Dialog Select showing just the Extras. Having a Extras in their own Dialog Select would also allow a Extras button to be added to Dialog Video Info.

[CrystalP]

That's a great feature, I'm also late to the party, sorry for not providing feedback earlier. I noticed a few things while looking at a fix for the movie list speed.

- concur with @jjd-uk's post about the selected text color in the manage dialog. It is not consistent and difficult to see depending on your screen settings.
Settings > system > display > whitelist has a special text color for the selected resolutions, it could be reused here. Alternatively, using a background highlight color may be better than coloring the text?

- when the default version is not the first in the list, the Play and Manage versions dialogs highlight the first item of the list, not the default version (https://github.com/xbmc/xbmc/issues/24188)

- using the manage dialog with a mouse can be difficult (https://github.com/xbmc/xbmc/issues/24189) with Versions and Extras on same screen.
using the play version from information is impossible, reported in same github issue.
Not a high priority because remotes/keyboards are a more common way to control Kodi, but to be taken care of at some point

- unexpected interactions between the context menu and external players (ie plays the version with default player) - is being looked at (https://github.com/xbmc/xbmc/issues/24184)

Agree with jjd-uk's and Hitcher's ideas in comment 257 just above. Actually they would fix the mouse navigation.

[XODIDOX]

@mikeSiLVO As I'm not a skinner, I don't have much knowledge for skinning design except adding the VideoVersion dialog to Estuary. I can't see what's the pain for you in skin designing for this dialog. But please list all issues in detail (better with screenshots), instead of a generic description, I don't even know how I can check the issue you mentioned

The original idea was just using a single VideoVersion dialog, both management and playing use it. A new dialog is needed for management, and it can also show additional information (artwork, path, etc.) instead of just a plain list. There is a screenshot for this in OP of this thread.

With the feedback for a dedicated dialog for playing, thus the current design, but underlying both management and playing are using the same dialog and only a single variable to control this. I'm not sure if/how about redesigning the playing dialog as the current design already shifted quite a bit from my original idea.

Please do keep in mind each one has their own idea for UI design. It's not possible to make everyone happy for a UI design. I wish Kodi team members can discuss this and make a final decision. The implementation part should be the easiest thing I can see.

For the additional infoLabels, I think people working on this feature in Kodi team are busy and don't even have time for the review for my pending PRs. I can work on these later when my current PRs merged, or someone else can work on this, it's not hard. Please do give a description of the purpose for each infoLabel you want.

[jjd-uk]

- concur with @jjd-uk's post about the selected text color in the manage dialog. It is not consistent and difficult to see depending on your screen settings.
Settings > system > display > whitelist has a special text color for the selected resolutions, it could be reused here. Alternatively, using a background highlight color may be better than coloring the text?

As I put in my post, the dialog should report to the skin engine that the item is selected and then we can adjust the colour on the skin side using the skin colour definition for selected. So something is missing from the core C++ side as changing the colour definition for selected in the skin currently has no effect on dialogvideoversion.

[jjd-uk]

@XODIDOX Regarding the colour of the selected item, first of all I'm not a C++ coder so all I can do is try and interpret the code. For Dialog Select I see mention of strSelectedLabel and SetSelected(label) see https://github.com/xbmc/xbmc/blob/49ba27...t.cpp#L286 so I'm wondering if it's this that allows skins to the set the colour, as I don't see anything like this in https://github.com/xbmc/xbmc/blob/master...ersion.cpp

Looks like a SetSelected needs to be done somewhere for the bool selected to be used, then with that the skin can set the colour with https://github.com/xbmc/xbmc/blob/49ba27...el.cpp#L34

I could be completely and utterly wrong as I'm trying to piece things together without any idea of exactly what the code does.

[mikeSiLVO]

As it happens we've been discussing this on the Teams internal Slack channel.

Both myself and Hitcher agree that from a design POV that the long term aim should be to use Dialog Select for selecting a playback item, and the new Dialog Video Version be used only for management.

For consistancy perhaps something similar to the Blu-Ray simple menu could be done



I'm not a C++ coder but I can sometimes follow what a code section is doing, even I don't understand the details of the specific code used, and this would suggest to me that this uses Dialog Select https://github.com/xbmc/xbmc/blob/master...u.cpp#L108

You could have something similar for Versions where the Versions are displayed (like those Main Titles on a Bluray are shown) then have a Show Extras at the bottom which would another Dialog Select showing just the Extras. Having a Extras in their own Dialog Select would also allow a Extras button to be added to Dialog Video Info.

Simple menu like that is great just not sure if icons are better than a poster or thumb image. DialogSelect has list id="6" visible for when an image is available. I agree and think removing the videoversionplay window and using select dialog is the best way to go. We are just selecting something after all. DialogVideoVersion.xml for version and extras management makes sense to me instead of the one for both management and play

As I put in my post, the dialog should report to the skin engine that the item is selected and then we can adjust the colour on the skin side using the skin colour definition for selected. So something is missing from the core C++ side as changing the colour definition for selected in the skin currently has no effect on dialogvideoversion.

Also agree, this does not currently work and would be great if it did!

@mikeSiLVO As I'm not a skinner, I don't have much knowledge for skinning design except adding the VideoVersion dialog to Estuary. I can't see what's the pain for you in skin designing for this dialog. But please list all issues in detail (better with screenshots), instead of a generic description, I don't even know how I can check the issue you mentioned

The original idea was just using a single VideoVersion dialog, both management and playing use it. A new dialog is needed for management, and it can also show additional information (artwork, path, etc.) instead of just a plain list. There is a screenshot for this in OP of this thread.

With the feedback for a dedicated dialog for playing, thus the current design, but underlying both management and playing are using the same dialog and only a single variable to control this. I'm not sure if/how about redesigning the playing dialog as the current design already shifted quite a bit from my original idea.

Please do keep in mind each one has their own idea for UI design. It's not possible to make everyone happy for a UI design. I wish Kodi team members can discuss this and make a final decision. The implementation part should be the easiest thing I can see.

For the additional infoLabels, I think people working on this feature in Kodi team are busy and don't even have time for the review for my pending PRs. I can work on these later when my current PRs merged, or someone else can work on this, it's not hard. Please do give a description of the purpose for each infoLabel you want.

Not sure how anyone else does skinning but I experiment using whatever is available in the Wikis and Doxygen to get my desired result. The pain I was mentioning is from a few different aspects, for example, the image for the movie selected is not saved anywhere so I currently use window properties and variables to achieve what already happens in DialogSelect. Focus should select the item and store the appropriate image.

As for the INFO labels, I mentioned previously that I cannot get IsVideoExtras to be true. I think it would be more beneficial to have a HasVideoExtras bool instead of HasVideoVersions be true for both. Separating the Extras and Versions just a smidge more makes things easier IMHO. There are ways to do the same by using Integer.IsGreater(Container(51).NumItems,0) or other skinamagics but it just seems simpler to have those bools be true/false and use conditions based on them. No harm in having the ListItem for video extras be true as well as that likely opens up more flexibility but as mentioned, it is never true for me no matter what I have tried.

A new dialog for management does make sense but not for selecting a version to play. I think that can be eliminated entirely for DialogSelect as I see no benefit in having any special handling for a list of choices with an image.

Thanks

[XODIDOX]

@jjd-uk @mikeSiLVO

I've solved the color issue in video version dialog, now the "selected" color will be applied to the selected version (it's the default version when opening the dialog and will be changed to the version you clicked), where the focused item is still applying the current highlight color. Is this expected behavior? If not, can you describe in detail for expected behavior (better with screenshot)? I can create a PR for this after my current pending PRs got merged.

[jjd-uk]

Yes that's the expected behaviour I believe. If you have the code already pushed to your repo I can test.

So with Estuary having default theme:

1. Unselected items in the itemlayout for the list are grey defined by https://github.com/xbmc/xbmc/blob/master...s.xml#L214

2. Unselected items in focus in focusedlayout for the list are defined by default label colour
of white defined by https://github.com/xbmc/xbmc/blob/master...ts.xml#L17

3. Then with your change selected items should use the colour default given by https://github.com/xbmc/xbmc/blob/master...ts.xml#L19 with colour defined by https://github.com/xbmc/xbmc/blob/master...ts.xml#L19

So those 3 types of label should appear the same as they do in Sets for example:



The key is that the skinner should have full control of the label colour for each state.

[XODIDOX]

@jjd-uk you can try this branch https://github.com/xodidox/xbmc/tree/vid...iondefault (it depends on PR 24148 (PR))

[jjd-uk]

@XODIDOX I can use our Jenkins to build a branch from any fork without a PR, so as long as the branch has everything required to allow testing. So in future if you want any Team/User testing done before submitting a PR, then just create a new branch and ask for a Team member to create the builds.

[XODIDOX]

@XODIDOX I can use our Jenkins to build a branch from any fork without a PR, so as long as the branch has everything required to allow testing. So in future if you want any Team/User testing done before submitting a PR, then just create a new branch and ask for a Team member to create the builds.

Thanks! That would be cool!

[jjd-uk]

@XODIDOX something odd is happening with that branch. Initially I thought great it was now working as I got the correct Estuary defined colour for selected however after restarting Kodi it appears to have stopped working, and I'm back to white text for the selected item. Need to investigate further.

[XODIDOX]

@XODIDOX something odd is happening with that branch. Initially I thought great it was now working as I got the correct Estuary defined colour for selected however after restarting Kodi it appears to have stopped working, and I'm back to white text for the selected item. Need to investigate further.

I tested it and it worked after restarting Kodi. May be your color modification lost after restarting. Are you sure the expected defaults.xml file is used after restarting Kodi?

[jjd-uk]

I will start completely from scratch with a blank Kodi, I didn't have a lot of time today to see if I could have done anything wrong.