search

maforget / ComicRackCE

A Community Edition for the legendary Comic Book Manager ComicRack. ComicRack is back from the dead.
GNU General Public License v2.0
335 stars 33 forks source link

Improved Method SplitIconKeyWithYear to SpliIconKeyWithYearAndMonth. … #111

Closed ucapato closed 1 month ago

ucapato commented 2 months ago

Improved Method SplitIconKeyWithYear to SpliIconKeyWithYearAndMonth. This way Publisher Icons might change at Info Window on a Year-Month basis and not only Year basis

maforget commented 2 months ago

This will not work for a couple of reasons.

  1. You changed the regex making the month part required. So existing files like DC Comics(2011-2016) would not match the regex and has such return directly DC Comics(2011-2016) when it should be returning a list of DC Comics (2011) ... up to DC Comics (2016).
  2. You didn't wire up the part where the date of the book is checked when loading the images. It still just checks only the year.
  3. In the return string at the end you only add the "_month" if there was a startMonth. It is valid, but since you already set the month from 1 to 12 anyway, I would just leave it. Also when we wire it up with the book date we will have to consider what happens when there are no month. I would output both with the year only & year & month.

Also looking at the code the Dark Horse Comics(1990) & Dark Horse Comics(1991) does nothing. So it might be a good opportunity to test it correctly.

I've modified the code but there is still 1 potential problem. If some month are missing on the icons names, because of the fail-safe for the whole year, you could get the whole year with the same icon. Either we have the default icon fill in or have an other icon offset for some month.

github-actions[bot] commented 2 months ago

Build Successful! You can find a link to the downloadable artifact below.

Name Link
Commit 2c3d431990a46bf47888960ae62416043f69b342
Logs https://github.com/maforget/ComicRackCE/actions/runs/10748424610
Download https://nightly.link/maforget/ComicRackCE/suites/28095530284/artifacts/1903904034
maforget commented 1 month ago

Did you test or create icons that use this?

ucapato commented 1 month ago

Did you test or create icons that use this?

Hi @maforget,

I could check the message today. I've been through a lot recently and haven't taken any tests yet. To make things worse my HD had some mechanical internal fault and I lost everything in there (or I can pay a huge amount of money to recover it). So I am considering re-starting my entire collection.

I still have the branch with this change; I can look into it and test in a later time.

maforget commented 1 month ago

I've merged it into master, so you can test it out with the main release.