Skip to content

Conversation

@mizaki
Copy link
Contributor

@mizaki mizaki commented Feb 11, 2025

Is this the kind of thing you had in mind?

This turned out quite simple.

@mizaki
Copy link
Contributor Author

mizaki commented Feb 11, 2025

Metron talker patch:

Subject: [PATCH] Use image hash
---
Index: metron_talker/metron.py
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/metron_talker/metron.py b/metron_talker/metron.py
--- a/metron_talker/metron.py	(revision 4e0d5c134d65ac4aa908a1cbd45a71cc6846d2c7)
+++ b/metron_talker/metron.py	(date 1739233743602)
@@ -31,7 +31,7 @@
 
 import settngs
 from comicapi import utils
-from comicapi.genericmetadata import ComicSeries, GenericMetadata, MetadataOrigin
+from comicapi.genericmetadata import ComicSeries, GenericMetadata, MetadataOrigin, ImageHash
 from comicapi.issuestring import IssueString
 from comicapi.utils import LocationParseError, parse_url
 from comictalker import talker_utils
@@ -1022,7 +1022,8 @@
             elif series["year_began"]:
                 md.year = utils.xlate_int(series["year_began"])
 
-        md._cover_image = issue["image"]
+        # md._cover_image = issue["image"]
+        md._cover_image = ImageHash(Hash=int(issue["cover_hash"], 16), Kind="phash")
 
         if issue.get("alt_number"):
             md.alternate_number = issue["alt_number"]

Copy link
Member

@lordwelch lordwelch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this looks good

@mizaki
Copy link
Contributor Author

mizaki commented Feb 12, 2025

I don't think it's going to be much more effort to put a cover_hashes and alt_covers_hashes into GenericMetadata and then it's all set up and ready to go into the future? (And also, the issue cover images are broken in the issue window which there isn't a real way around if the url has been replaced.)

Only real question is representation of the GM field: tuple[ImageHash] or {phash: <hash>, ahash: <hash>}?

@lordwelch
Copy link
Member

I don't think it's going to be much more effort to put a cover_hashes and alt_covers_hashes into GenericMetadata and then it's all set up and ready to go into the future?

Lets keep it as is for now. We can look at other ways of doing it later

(And also, the issue cover images are broken in the issue window which there isn't a real way around if the url has been replaced.)

#730 (comment) should take care of any unexpected errors, they should already handle an empty url (or the metron talker shouldn't work there at all).

@mizaki
Copy link
Contributor Author

mizaki commented Feb 12, 2025

I don't think it's going to be much more effort to put a cover_hashes and alt_covers_hashes into GenericMetadata and then it's all set up and ready to go into the future?

Lets keep it as is for now. We can look at other ways of doing it later

I've added URL to ImageHash which seems like the simplest options atm and solves the problems.

If you're happy, the hard part next is making the tests :)

@lordwelch
Copy link
Member

It's simpler to just explicitly ignore the ImageHash outside of the IssueIdentifier

@lordwelch lordwelch closed this Feb 24, 2025
@lordwelch lordwelch reopened this Feb 24, 2025
year=ct_md.year,
publisher=None,
image_url=ct_md._cover_image or "",
image_url=str(ct_md._cover_image) or "",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this okay as is or should it be ct_md._cover_image if isinstance(issue._cover_image, str) else "" as well?

@mizaki
Copy link
Contributor Author

mizaki commented Feb 24, 2025

I thought adding the URL to ImageHash covered all the bases and at a later date it could be used as a fallback if wanted. But I can see that not being wanted anyway.

Is there anything you want me to do with this?

@lordwelch lordwelch merged commit 8837fea into comictagger:develop Feb 27, 2025
6 of 9 checks passed
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