pannal / plex-for-kodi

Unoffical Plex for Kodi add-on releases.
GNU General Public License v2.0
237 stars 30 forks source link

Adjust position of online ratings to avoid overlap with text on the same line #69

Closed rdswords closed 1 month ago

rdswords commented 7 months ago

https://github.com/pannal/plex-for-kodi/blob/5083f9492ee837dc619ed1893bea6af8e7801a88/resources/skins/Main/1080i/script-plex-pre_play.xml#L327

Recommend changing Y coordinate from 70 to 10 to raise the ratings up to the first line (where the title is) since the second line of text (including duration, genres, year, and video/codec info) has the potential to become long enough to overlap the ratings.

I encountered the issue while using the ZidooPlexMod fork where the same issue exists. This issue becomes more significant if any skin/font/scaling variations are present.

Example of clipping: Clipped Text at posy 70

Example of proposed resolution: Proposed Resolution at posy 10

@bowlingbeeg

pannal commented 7 months ago

Hmm, PM4K doesn't really support any other skins than Estuary and Plextuary.

You're right about the font scaling, though. I wasn't aware font scaling was even a thing with P4K/PM4K? IIRC the code uses its own font sizes.

I can certainly move the rating images. Let's see.

rdswords commented 7 months ago

Hmm, PM4K doesn't really support any other skins than Estuary and Plextuary.

You're right about the font scaling, though. I wasn't aware font scaling was even a thing with P4K/PM4K? IIRC the code uses its own font sizes.

I can certainly move the rating images. Let's see.

I was very confused about why the text scaling wasn't quite right, but @bowlingbeeg identified that PM4K does seem to inherit font/size behavior from a skin even when the skin is configured to use the default font. In my case, I'm using the Arctic Zephyr - Reloaded skin (https://kodi.tv/addons/nexus/skin.arctic.zephyr.mod/). The Arial font preset used by the skin has even more exaggerated results than my example image where I have it set to the Default font instead.

pannal commented 7 months ago

Would the ideal solution here be to try and force PM4Ks font sizes? Not certain.

rdswords commented 7 months ago

I don't have any experience with the type of UI layout that is used in the Kodi add-ons, so it's hard for me to say. The way many elements are hard coded with exact pixel sizes and placements, but other elements have some relative placements and sizing means there are probably a lot of cases where pieces of the interface could overflow, get truncated, etc. Issues of accessibility and localization may also bring surprise interface conflicts into play.

There are probably some movies/shows that exist where the title could be long enough to reach the edge of the screen where the ratings are, but in my library it is the second line of text that is far more likely to have the issue. Some of the text elements in the UI have fixed sizes with auto-scrolling inside the text box, and that might be a way to better constrain some of the text boxes on the pre-play screen. I'm not sure if most users would prefer for the title and info/codec lines at the top to auto-scroll when they are too long, or if they'd rather it stay static and just clip. The movie description box looks like it has room to be expanded into unused space above/below, and could arguably benefit from auto-scrolling.

In any case, having tried it for a day I personally like the feel of the ratings placement at posy=10.

Side question: Is there a visual UI layout editor available for developing these KODI add-ons, or do they always have to be manually edited as XML by trial and error?

bowlingbeeg commented 7 months ago

I don't think moving the ratings up a line is a bad idea regardless of what the decision is for the fonts themselves. I thought when I did an initial search for how to work with fonts in Kodi addons it was mentioned in a couple of places that addons themselves couldn't use their own fonts but maybe those comments were old/wrong.

pannal commented 7 months ago

The way many elements are hard coded with exact pixel sizes and placements

Well yeah, I'm maintaining work from 2016. I have no idea to be honest how XBMC's skinning engine was back then, but I can imagine it was pretty limited. Which is why I'm always baffled about what @ruuk managed to build here. This UI and the network/model layer, under these circumstances, are simply amazing.

Little in-context-sidenote for seekdialog.py and select dialogs (not sure if this is still needed):

    def doKodiSelectDialogHack(self, action):
        command = {
            xbmcgui.ACTION_MOVE_UP: "Up",
            xbmcgui.ACTION_MOVE_DOWN: "Down",
            xbmcgui.ACTION_MOVE_LEFT: "Right",  # Not sure if these are actually reversed or something else is up here
            xbmcgui.ACTION_MOVE_RIGHT: "Left",
            xbmcgui.ACTION_SELECT_ITEM: "Select",
            xbmcgui.ACTION_PREVIOUS_MENU: "Back",
            xbmcgui.ACTION_NAV_BACK: "Back"
        }.get(action.getId())

        if command is not None:
            xbmc.executebuiltin('Action({0},selectdialog)'.format(command))
            return True

        return False

The whole thing is based on the fact that modal dialogs can be fullscreen, which, in the way it's set up, is still a hack.

It took me the better part of two years to understand everything he's built, especially in the quirky cases, where you need to get why something was done, not how.

I'm sure there's a lot to be had with Kodi Skin variables and includes, especially in the case of script.plex/script.plexmod and its derivatives.

Maintaining this is lunacy to a certain degree. But I use a SHIELD and I've had my fair share of "why doesn't this simply work?" or "why do we run out of memory after a couple of days and lag?" with the official app, when I stopped using P4K for a while.

While it has its own issues, Kodi is the best video playback engine on this planet, which is why I maintain this addon. Not because it's beautiful or easy to maintain :)

tl;dr: The whole thing could be rewritten, especially the XML part. I've had plans in my mind for a good while to rewrite parts of it, to conform to a more modern Plex client UX (unified home view over multiple servers etc). Not sure if I can ever do it, or even want to - I've probably got a 70/30 split in PM4K's users with 70% saying "don't change the UI, it fits the purpose" and 30% saying "could this look more modern?".

Implementing watchlist and an expandable sidebar with libraries instead of the sections header, together with dynamically configurable library lists and home views, would suffice I think (btw @bowlingbeeg thank you for the PR regarding the configurable home/libraries idea, I'll take some cues from that, but I'd rather implement it in a plug&play/in-UI way).

Side question: Is there a visual UI layout editor available for developing these KODI add-ons, or do they always have to be manually edited as XML by trial and error?

Yes. Kind of. Partially. I've never used one, as I'm pretty confident in navigating the XML files (and they're pretty limited once you get used to them), but take a look at: https://github.com/jurialmunkey/script.skinvariables for example.

In any case, having tried it for a day I personally like the feel of the ratings placement at posy=10.

Check, will try.

Next up on my list is critics/reviews btw, before I tackle the huge ones (watchlist, home/library customization). 0.6.6/0.6.7 will be pretty massive already (UI handling rework, AC3/passthrough rework, DirectStream finally working properly, ...), maybe I'll push them this weekend.

Edit: Sorry for the unprovoked tangent :D

rdswords commented 7 months ago

I noticed one more related issue today that should be corrected at the same time as the ratings coordinate. There is a misalignment of the remaining time text that appears when applicable on the title line. It needs the PosY value changed from 6 to 13 so it actually aligns properly with the title text. (see my later post below for more issues related to the title font which has its own issue).

https://github.com/pannal/plex-for-kodi/blob/5083f9492ee837dc619ed1893bea6af8e7801a88/resources/skins/Main/1080i/script-plex-pre_play.xml#L245

Time Remaining Misaligned Time Remaining PosY Corrected

bowlingbeeg commented 7 months ago

@rdswords The time remaining field is the same in the ZidooPlexMod and the PM4k. I'm guessing it looks strange on your system because of your non-default skin. Just like how your movie synopsis doesn't fully show. I think it would take a lot of work to try and patch every issue you have using that non-default skin.

Here is what it looks like on my system: PXL_20231208_015833061 MP

And here is what it looks like if I change the PosY from 6 to 13 PXL_20231208_015901080 MP

So I don't think we should be changing that value.

bowlingbeeg commented 7 months ago

@bowlingbeeg Crap, the text scaling issue again. I thought I had the default text style active. I've hidden my previous post to avoid confusion. Which font set is the Estuary skin actually using when set to "Skin default"?

Not sure

pannal commented 7 months ago

@bowlingbeeg Crap, the text scaling issue again. I thought I had the default text style active. I've hidden my previous post to avoid confusion. Which font set is the Estuary skin actually using when set to "Skin default"?

NotoSans-Regular.ttf

Ref: https://github.com/xbmc/xbmc/blob/master/addons/skin.estuary/xml/Font.xml

rdswords commented 7 months ago

@bowlingbeeg Crap, the text scaling issue again. I thought I had the default text style active. I've hidden my previous post to avoid confusion. Which font set is the Estuary skin actually using when set to "Skin default"?

NotoSans-Regular.ttf

Ref: https://github.com/xbmc/xbmc/blob/master/addons/skin.estuary/xml/Font.xml

Thanks! I'm doing some cross referencing of the font/skin relationship to see if I have any ideas. The skin I like to use for Kodi (Arctic Zephyr Reloaded) uses a completely different naming convention for the fonts defined in font.xml, and does not retain any of the font definitions that P4K calls by name (but yet it does include a copy of NotoSans-Regular.ttf in its fonts folder. That may be good news though, if it means the relatively limited set of font definitions P4K uses can be inserted into the font.xml for the skin I use (and conceivably be established as a workaround for others to configure non-default skins for compatibility).

From searching the P4K codebase, it appears to call the following set of fonts by name: font10, font12, font13, font16, font30, and WeatherTemp. It looks like font16 and font30 are actually currently undefined in the Estuary skin font.xml. When the default Estuary skin is active, and P4K calls font16 or font30, Kodi does not appear to be properly resolving them because they do not appear to be defined.

https://github.com/xbmc/xbmc/blob/ff07dd77605fdd30c6e8fe26730b356b2b808a97/addons/skin.estuary/xml/Font.xml#L147

After distilling down the list of font definitions used by P4K and grabbing them from the Estuary font XML, I saved them in a truncated XML file.

`<?xml version="1.0" encoding="UTF-8"?>

font10
        <filename>NotoSans-Regular.ttf</filename>
        <size>23</size>
        <style>lighten</style>
    </font>
    <font>
        <name>font12</name>
        <filename>NotoSans-Regular.ttf</filename>
        <size>25</size>
        <style>lighten</style>
    </font>
    <font>
        <name>font13</name>
        <filename>NotoSans-Regular.ttf</filename>
        <size>30</size>
        <style>lighten</style>
    </font>
    <font>
        <name>font30_title</name>
        <filename>NotoSans-Regular.ttf</filename>
        <size>30</size>
        <style>bold</style>
    </font>
    <font>
        <name>WeatherTemp</name>
        <aspect>0.85</aspect>
        <filename>NotoSans-Regular.ttf</filename>
        <size>120</size>
        <style>bold</style>
    </font>`

I inserted those into the stack of font definitions for the custom skin I use and it works! However, it also appears to indicate that the font30 definition issue is real and not actually working by default when Estuary is active. Since those font definitions are in the XBMC build and may not be easy to correct, you may have to resort to using either font32 or font30_title (which is a bold version of font30) since those are included in the font.xml for Estuary.

Estuary skin with default skin font (appears to actually suffer from the font16/font30 missing definitions in the Estuary font.xml): Default Estuary skin

Arctic Zephyr - Reloaded skin with its default font: Arctic Zephyr Reloaded skin unmodified

Arctic Zephyr - Reloaded skin with additional fonts inserted. The font is now the same as in the default skin (except the title font is bold) and you can see that the plot synopsis box is an exact match for size and amount of text visible: Artic Zephyr Reloaded skin with modified font list

Ironically, now that the font30 definition is present and rendering properly, the alignment of the remaining time text appears to need a PosY value of 13 after all (so that the 34 pixel control containing the remaining time text is aligned to the center line of the 60 pixel control containing the title text -> 13 + 34 + 13 = 60): font30 Title with Misaligned Time at 6px font30 Title with Aligned Time at 13px

bowlingbeeg commented 7 months ago

So in other words it's a hot mess 😄 ... I do like the look of the larger font size for the title

pannal commented 7 months ago

the alignment of the remaining time text appears to need a PosY

I think it's possible to auto-valign in certain group containers, not sure right now. Might be a good fix instead of relying on absolute offsets.

Edit: Ah, small sidenote, @rdswords, when referencing file in this github repository, please to so from the develop-kodi21 branch, not from master. Master is super stale and unused right now.

Edit 2: The title is at 0 btw, not 10.

rdswords commented 7 months ago

the alignment of the remaining time text appears to need a PosY

I think it's possible to auto-valign in certain group containers, not sure right now. Might be a good fix instead of relying on absolute offsets.

Edit: Ah, small sidenote, @rdswords, when referencing file in this github repository, please to so from the develop-kodi21 branch, not from master. Master is super stale and unused right now.

Edit 2: The title is at 0 btw, not 10.

No problem, I went back through this thread and tried to make sure to edit the permalinks so they'd be pointing to the develop-kodi21 branch codebase. Can you please set the develop-kodi21 branch as the repo default so that it won't be something that has to be manually selected each time?

There is a tricky alignment between the title text row and the internet ratings because they are not actually in the same container (this all gets so confusing to track down because the XML elements are not generally in order in the file, so you have to jump around in the list to find elements that are actually positioned next to each other). I'm going to try on my end to

Does your current branch have compatibility with a specific branch of XBMC? Their repo is defaulted to their Master branch so when I cross reference, for example, the font.xml in the stock skins it's coming from that branch of the XMBC repo. That was the source for my code links to the font.xml files in Estouchy and Estuary.

Here is a new screenshot showing the layout when using font30_title instead of font30 since the former is defined in the Estuary skin and could be resolved properly by P4K when the default skin/default font is active. I took this one with both remaining time and edition text in play to see how everything looks together. I need to look into the internet ratings control group because they have an odd separate group with a different approach to the height/pixel values that is harder to align to the title text row. They should probably just be integrated into the same control group with the other items on that line so it will be possible to actively align all of those elements to each other. I'm hoping to try experimenting with that tonight. font30_title for Title Text with Edition and Remaining Time Text

If you like the look of that in terms of the title font, you'd want to replace all instances of font30 in the P4K codebase with font30_title so that they can all resolve a name that is actually defined in the Estuary skin. I haven't investigated how widely used font16 is, but it doesn't appear to have a direct alternate font to name.

pannal commented 7 months ago

Can you please set the develop-kodi21 branch as the repo default so that it won't be something that has to be manually selected each time?

I've thought about that in the past. Not sure if it's feasible, though or what the implications are, I've never moved the main branch in github before.

There is a tricky alignment between the title text row and the internet ratings because they are not actually in the same container

Hmm not really. They're both in their own grouplist, which in turn are part of a group. So their Y alignment is based on the group. The title element's Y offset is inherited from its grouplist, which has a Ypos of 0 and sits at 0 of the surrounding group.

It's probably impossible to include the ratings inside the grouplist that contains the title, as its alignment is left and we want the ratings to sit at a specific X position.

Btw, interesting, I think I just found out why we use font30. Because estuary wasn't always the default, but Confluence was before Kodi 17: https://github.com/xbmc/skin.confluence/blob/master/720p/Font.xml

Does your current branch have compatibility with a specific branch of XBMC? Their repo is defaulted to their Master branch so when I cross reference, for example, the font.xml in the stock skins it's coming from that branch of the XMBC repo. That was the source for my code links to the font.xml files in Estouchy and Estuary.

Not sure what you're asking. PM4K isn't related to the XBMC repo, or do you mean plex-for-kodi? If not, PM4K is cross compatible since a couple of versions and supports Kodi 18 (via the addon_kodi18 branch) and all following versions including 21 with the develop_kodi21 branch.

Edit: Would be interesting to know what the fallback for a nonexistant font definition is, though. I could simply default to that and use it instead of font30, as I'm not super keen to use font30_title, as it would be the only text in the whole addon using bold.

Edit 2: "In the event that Kodi is unable to locate the specified font, it will default to "font13". It is required that every skin has a font13 definition in their Font.xml file. Apart from that, you can modify this file as you like and add/delete/change fonts. The user friendly font name is referenced by the other xml files mentioned below."

Edit 3: Confirmed. It falls back to font13. Would it help you if I changed all font30 references to font13?

Edit 4: Changed the default branch.

rdswords commented 7 months ago

Not sure what you're asking. PM4K isn't related to the XBMC repo, or do you mean plex-for-kodi? If not, PM4K is cross compatible since a couple of versions and supports Kodi 18 (via the addon_kodi18 branch) and all following versions including 21 with the develop_kodi21 branch.

I was trying to be sure, since the font.xml is part of the XBMC repo, whether there were any distinct branches there in the event they have a different set of definitions in the font.xml for the Estuary skin. The font.xml files I was referencing are the ones in their default branch.

Edit 2: "In the event that Kodi is unable to locate the specified font, it will default to "font13". It is required that every skin has a font13 definition in their Font.xml file. Apart from that, you can modify this file as you like and add/delete/change fonts. The user friendly font name is referenced by the other xml files mentioned below."

I'm glad you found that! I was really confused about what text was being rendered when citing an undefined font.

Edit 3: Confirmed. It falls back to font13. Would it help you if I changed all font30 references to font13?

I actually quite like the look of the font30_title on the pre-play page. However, I'm not an expert in all of the various layouts where this could have either a beneficial or detrimental effect. It's too bad the middle option isn't defined in font.xml because font13 is size 30 but is lightened, while font30_title is size 30 but is bolded. The skin is missing a definition for a size 30 font which would split the difference. There is also the issue of calls to the undefined font16 to resolve (font16 appears to be primarily used for music/artist/album titles and TV episode titles on their respective screens).

Now that you solved the mystery of the original definitions, I now realize that the same-named definitions I grabbed from the Estouchy skin to fill in the gaps are actually not likely suitable for direction substitution either (I'm going to update my earlier post to cleanup that extracted font XML I had pasted in). To help us figure out where we stand on the fonts, I did some comparing and pulled some info about the named fonts between Confluence (720p) and Estuary (1080i). Maybe this will help us interpolate on where the broken references should land:

  1. font10: Confluence (size 14, regular), Estuary (size 23, lighten)
  2. font12: Confluence (size 17, regular), Estuary (size 25, lighten)
  3. font13: Confluence (size 20, regular), Estuary (size 30, lighten)
  4. font16: Confluence (size 25, regular), Estuary (MISSING DEF)
  5. font30: Confluence (size 30, regular), Estuary (MISSING DEF)
  6. WeatherTemp: Confluence (size 80, bold), Estuary (size 120, bold)

The fonts in Estuary seem to be designed for a ~1.5x point size vs the old Confluence skin. Having looked at the current font usage for font30 and font16, that size would end up with size ~48 and ~36 in the Noto Sans font. Neither of those would be appropriate sizes for use in the current skin within plex.

TL:DR - It looks like the most simple fix to un-break the undefined font situation would be to change all font16 and all font30 references to font13 instead. That's what the code is using as a fallback this whole time anyway, so it makes since to go ahead and bake that in. I would personally prefer seeing font13_title like I said, but that's just my one vote among you and anyone else that may have a different preference.

rdswords commented 7 months ago

After making commits in my fork (and PR #70 and #71), this is where the layout stands with the fonts and alignments resolved, including the repositioned ratings objects. Fonts and Alignments Resolved

pannal commented 1 month ago

Is this still an issue?

rdswords commented 1 month ago

Is this still an issue?

It's been so long that I can't recall at this point. Everything works fine on my end and I'm not running a special build of anything, but it's possible I tweaked some font definitions in my app or skin install folder to make things work properly.