search

nkahoang / es-theme-art-book-next-arkos

Art Book Next theme ported from JelOS to ArkOS
35 stars 8 forks source link

Image fix #1

Closed davetansley closed 10 months ago

davetansley commented 10 months ago

Removed height constraint on md_image SIZE declaration to preserve image aspect ratio.

nkahoang commented 10 months ago

Thanks @davetansley you reckon we should remove it from the md_video too? (As image transitions to video if you stay on a single record for a while)

davetansley commented 10 months ago

Thanks @davetansley you reckon we should remove it from the md_video too? (As image transitions to video if you stay on a single record for a while)

Possibly... I didn't change it because I didn't have any videos downloaded to test with.

davetansley commented 10 months ago

Just tested with some videos. It actually doesn't make a difference to the aspect ratio here, but I've updated it anyway for consistency.

anthonycaccese commented 10 months ago

hmm, does maxsize not work in this case?

davetansley commented 10 months ago

hmm, does maxsize not work in this case?

Looking at the docs, it might work better actually. Specifying a horizontal width with SIZE would presumably potentially cut off tall images at the top and bottom. I'll have a play around tonight and update the PR.

anthonycaccese commented 10 months ago

awesome yes, maxsize is what I would recommend for this use case and its what I am using in the original to do the same action. Essentially it creates a bounding box that the image will resize inside (no cropping).

davetansley commented 10 months ago

I ended up going down a bit of a rabbit hole with the layout to try to get it to fit most box art without black borders, and ended up tweaking lots of numbers. It looks good on mine now, but I appreciate this might be beyond the scope of the PR, so feel free to ignore it @nkahoang or just pick out what you want.

@anthonycaccese Awesome work on the original theme... it's really lovely!

davetansley commented 10 months ago

Also, I can't seem to get the metadata-off/on settings to be recognised at all... not sure if this is an ArkOS limitation or a Dave limitation :)

nkahoang commented 10 months ago

Thanks @davetansley brilliant work! I will merge that now

nkahoang commented 10 months ago

@davetansley @anthonycaccese thanks for your work. Btw I just figured out the problem regarding menu / item selection in ArkOS.

So as you know options like Color Schemes do not work. I verified that ifSubset="blah" does not work properly in ArkOS. The following colors selection code block is then not working:


   <!-- Color Scheme -->
   <subset name="color-scheme" displayName="Color Scheme">
      <include name="art-book-next" displayName="Art Book Next" />
      <include name="art-book" displayName="Art Book" />
      <include name="steam-os" displayName="Steam OS" />
      <include name="snes" displayName="SNES" />
      <include name="famicom" displayName="Famicom" />
      <include name="black" displayName="Black" />
      <include name="grayscale" displayName="Grayscale" />
      <include name="custom" displayName="Custom" />
   </subset>
   <include>./colors.xml</include>

Instead I have to split the content of ./colors.xml out into individual file then remove the ifSubset="color-scheme:[something]" out of each file, then the following structure works:


   <subset name="color-scheme" displayName="Color Scheme">
      <include name="art-book-next" displayName="Art Book Next">./_inc/colors/art-book-next.xml</include>
      <include name="art-book" displayName="Art Book">./_inc/colors/art-book.xml</include>
      <include name="steam-os" displayName="Steam OS">./_inc/colors/steam-os.xml</include>
      <include name="snes" displayName="SNES">./_inc/colors/snes.xml</include>
      <include name="famicom" displayName="Famicom">./_inc/colors/famicom.xml</include>
      <include name="black" displayName="Black">./_inc/colors/black.xml</include>
      <include name="grayscale" displayName="Grayscale">./_inc/colors/grayscale.xml</include>
   </subset>

My latest change reflects this. Now if I got time i can do it for the other options too...