search

paulrehkugler / xkcd

xkcd iPhone app
MIT License
44 stars 9 forks source link

High-resolution images #80

Closed azsn closed 4 years ago

azsn commented 5 years ago

Fix for #68. I did my best to add high-resolution image support in a way that a) didn't require huge changes to how the app works and b) minimizes requests to xkcd servers.

Unfortunately there is no way (that I could find) to determine the high-resolution image URL to use. Some comics use the default img URL, some use the img but with _large at the end (those are usually the ones that the xkcd.com comic image is a link to a larger version), and almost every comic >= 1084 has a _2x version (the only ones without _2x are some of the ones that use _large).

So my change will, on loading the image, attempt _large, then attempt _2x, then fall back to the default image URL. However, to minimize requests, I have hardcoded known comics that are _large up to current date, so it doesn't have to check for the vast majority that aren't. This doesn't need to be updated -- future comics not included in the hardcoded part will just always check for _large first. Sorry it's so ugly, but I'm not sure there's a better way to do it, since the xkcd API is so useless for high-res images.

Also comics 256 and 273 are special -- their default image URL has _small at the end, but the link is to one with no suffix. I just hard-coded these URLs. It will still fall back to the default URL if my hard-coded URL fails.

Additionally, since the 2x comics are bigger, I modified the view controller to zoom out to 0.5 when loading comics >= 1084. That way, they don't show up super big on screen by default. (While doing this, I also fixed the "Open Zoomed Out" feature, which did not seem to be working originally.)

paulrehkugler commented 4 years ago

@zelbrium 👋 Thanks for the PR! Unfortunately... I completely missed it and solved a lot of your issues in #81, #82, and #84. Sorry! Bad look for an open-source project 😓

With that said, I really like a few of the ideas in here and I overlooked them in my implementation. I didn't modify the default zoom level. And I don't have any special handling for 256, 273, or 980.

I'm going to cut a release before too long, so let me know if you'd like to contribute. If not, I'll work on the issues I mentioned above. I filed them as #85 and #86. Sorry again for stepping on your toes!

azsn commented 4 years ago

No problem! I don't have time to do any work on this right now, but I think, if I remember correctly, my code has fixes for #85 and #86. So if you want to try to rip those fixes out of my code and merge them into your new changes, feel free!