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

Improved Method SplitIconKeyWithYear to SpliIconKeyWithYearAndMonth. … #111

Closed
wants to merge 2 commits into from

Conversation

ucapato
Copy link
Contributor

@ucapato ucapato commented Sep 6, 2024

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

…This way Publisher Icons might change at Info Window on Year-Month basis and not only Year basis
@maforget
Copy link
Owner

maforget commented Sep 7, 2024

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.

Copy link

github-actions bot commented Sep 7, 2024

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

Name Link
Commit 2c3d431
Logs https://github.com/maforget/ComicRackCE/actions/runs/10748424610
Download https://nightly.link/maforget/ComicRackCE/suites/28095530284/artifacts/1903904034

@maforget
Copy link
Owner

Did you test or create icons that use this?

maforget added a commit that referenced this pull request Oct 2, 2024
Publisher Icons Year in filename can be now be Year-Month

---------

Co-authored-by: Ulisses Capato <125318439+ucapato@users.noreply.github.com>
Co-authored-by: maforget <11904426+maforget@users.noreply.github.com>
@maforget maforget closed this Oct 2, 2024
@ucapato
Copy link
Contributor Author

ucapato commented Oct 3, 2024

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
Copy link
Owner

maforget commented Oct 3, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants