Any issues with this? Seems like the easiest solution.

The ImageHash is for making so we don't download the cover image, if a user needs it for manual matching we should instead give a link to the webpage for the comic.

Also adding url to ImageHash seems silly to me when we have _alternate_images: list[str | ImageHash]. Now if it's changed to _alternate_images: list[ImageHash] then it makes sense and overriding __str__ is also silly we can just do imagehash.URL

ImageHash isn't to prevent the downloading of the image per se, it's to not do it if there is a hash for matching. For the issue window downloading the cover is fine (by Metron at least) as it's going to be far more limited. If the source doesn't wish to allow for the use of image download at all, the talker should leave it empty.

This is confusing when the alternative images will load (as there are no hashes for them and so normal URL strings are used).

It's the same problem with alternative images as they could be Hashes or URLs, so say, pushing the "main" cover URL to alternatives wouldn't work either.

Using the __str__ means not having to do if isinstance(obj, str) so that self.coverWidget.set_issue_details(self.issue_id, [str(issue._cover_image) or "", *alt_images]) from the issueselectinwindow.py for example can remain simple. Maybe it's preferred to break it out?

cover_image_url = issue._cover_image if isinstance(issue._cover_image, str) else issue._cover_image.URL

Then self.coverWidget.set_issue_details(self.issue_id, [cover_image_url or "", *alt_images]) from the issueselectinwindow.py

This is confusing when the alternative images will load (as there are no hashes for them and so normal URL strings are used).

That's on the talker for being inconsistent

cover_image_url = issue._cover_image if isinstance(issue._cover_image, str) else issue._cover_image.URL

Why?
If you're adding URL to ImageHash. Drop the string alternative
This way you never have to do any isinstance checks you just take the url attribute

This is confusing when the alternative images will load (as there are no hashes for them and so normal URL strings are used).

That's on the talker for being inconsistent

The source may not provide hashes for alternatives (Metron being our only example currently) so it should be expected that alternatives may be a URL list and the cover is an ImageHash.

cover_image_url = issue._cover_image if isinstance(issue._cover_image, str) else issue._cover_image.URL

Why? If you're adding URL to ImageHash. Drop the string alternative This way you never have to do any isinstance checks you just take the url attribute

_cover_image can be a string URL or an ImageHash depending on what the source provides. I don't see how the string URL can be dropped as it's required for IssueIdentifier (rather ImageHash is but then it presumes string) to know if it should check hashes or generate them?

Otherwise the talker would have to make all cover image URL into a hash and then I guess have the hash value be 0 or None and then IssueIdentifier would need to be changed to check for that but then that doesn't help with the problem of knowing when the image URL can be used for hashing or not.

Maybe it's something simple I'm missing here?

Why would we download an image to hash it when we already have the hash?

Download the image

ImageHash(0, "", "https://image.jpg")

Use the hash

ImageHash(23456, "ahash", "")

Use the hash

ImageHash(23456, "ahash", "https://image.jpg")

I don't understand how it would be interpreted any other way

Download the image

ImageHash(0, "", "https://image.jpg")

Use the hash

ImageHash(23456, "ahash", "")

Use the hash

ImageHash(23456, "ahash", "https://image.jpg")

I don't understand how it would be interpreted any other way

All talkers would need to be changed. I presumed this was the reason for having the ImageHash | str so as to avoid that.

The edge case of being able to use the cover image URL in the GUI but not for matching would be ignored? It might help to prevent confusion I suppose as if you see the cover in the GUI but then it won't match...

I feel like other than the edge case there's something else but I can't think of it so maybe there isn't...

Shall I do something like:

class GenericMetadata:
    def __setattr__(self, name: str, value: Any):
            if name == "_cover_image" and isinstance(value, str):
                self.__dict__["_cover_image"] = ImageHash(Hash=0, Kind="", URL=value)
            else:
                super().__setattr__(name, value)

And rename ImageHash to ImageInfo?

No, plugins will need to update.
Either leave it ImageHash and put a comment in noting that if the hash/kind is empty then it will need to be calculated from the image at the url. If the url is empty then the hash/kind should not be empty. If both are empty it's invalid

At the moment there is an image hash class for quicktag and genericmetadata. Should I consolidate them?

At the moment there is an image hash class for quicktag and genericmetadata. Should I consolidate them?

No that's going through a bunch of different changes right now

Right now your tests are failing, but otherwise this looks good