Scryfall small images download #12282
Conversation
Add the Scryfall Small Images option
Use pseudo inheritance from the Scryfall Image Source Object
No longer accessing properties of CardImageUrls directly, but using getDownloadList to retrieve them. Also added commented out section to include if ScryfallRework gets merged.
|
rebased because PR 12283 was merged |
| public enum ScryfallImageSourceSmall implements CardImageSource { | ||
| instance; | ||
|
|
||
| private CardImageSource baseSource = ScryfallImageSource.instance; |
There was a problem hiding this comment.
@JayDi85 what do you think of this style? I think it could make more sense to have ScryfallImageSource.LARGE and ScryfallImageSource.SMALL rather than this wrapper class. But it's not a big deal and this way should work.
There was a problem hiding this comment.
I’m surprised why it’s not class with overrided methods or constructor param. You can use it here cause already merged in other PR, but I will refactor and remove that “base source” anyway.
There was a problem hiding this comment.
Make original enum as class and use constructor here. It’s ok to combine enum and class.
There was a problem hiding this comment.
Or getInstance to simulate enum logic:
There was a problem hiding this comment.
Example:
ScryfallImageSource - class with static “instance” field and “getInstance” method
ScryfallImageSourceSmall extends ScryfallImageSource with overrided modify and size methods
Mage.Client/src/main/java/org/mage/plugins/card/dl/sources/ScryfallImageSourceSmall.java
Outdated
Show resolved
Hide resolved
| @@ -32,6 +33,14 @@ public enum ScryfallImageSource implements CardImageSource { | |||
| private final Map<CardDownloadData, String> preparedUrls = new HashMap<>(); | |||
| private final int DOWNLOAD_TIMEOUT_MS = 100; | |||
|
|
|||
| private Object readResolve() throws ObjectStreamException { | |||
There was a problem hiding this comment.
Not for PR -- I don't see any usages of readResolve(). Looks like some outdated code. So it can be removed from all "instance style" classes.
There was a problem hiding this comment.
It's a weird java thing - see https://docs.oracle.com/javase/8/docs/api/java/io/Serializable.html for details. I don't know for sure if it's actually needed, but take care before removing.
edit: some discussion here https://stackoverflow.com/questions/1168348/java-serialization-readobject-vs-readresolve
the purpose is to enforce singleton pattern even when serialized
There was a problem hiding this comment.
Okay, this class doesn't extend Serializable so must not be needed here.
There was a problem hiding this comment.
Hmm. It required to support serialization of singleton classes (without that method each singleton object/instance will be duplicated with new object). Xmage don't use serialization of server side objects, so it's safe from such errors (miss hidden fields/methods). But it can be important for potentially game replay or restore feature (old implementation used serialization to save and load game states from a file).
So it's better to keep such method in all enum style classes (singleton).
Mage.Client/src/main/java/org/mage/plugins/card/dl/sources/ScryfallImageSourceSmall.java
Outdated
Show resolved
Hide resolved
|
Beware of copying example code - you have to check that it works as intended. In this case a few other adjustments were needed (and I went ahead and simplified some other things while I was looking at it). |
…on't throw exception
Sorry I messed up the last version of this pull request by accidentally pushing a commit that didn't belong to it - and when I tried to delete the commit, I deleted the branch instead. So I have recreated here.
This addresses #11577 .