PawCorp / walltaker-desktop-client

Desktop client for Walltaker powered by golang
13 stars 10 forks source link

[Bug] Large files result in a black wallpaper #29

Closed NameNotFinal closed 2 years ago

NameNotFinal commented 2 years ago

Describe the bug Wallpaper fails to load in when file size is too large

To Reproduce Steps to reproduce the behavior:

  1. Set the wallpaper on Walltaker to a large file size (unsure of exact size, but I've experienced this with 37.5 19.7 MB files) (Here's an example ID that's SFW: 2515016)
  2. Wait for the client to poll for new wallpaper
  3. See it start to set the wallpaper (this takes a while so it seems like it's not failing on download, but rather on actually setting the wallpaper)
  4. Log says "Set!" but no wallpaper is there

Expected behavior That the wallpaper gets set to the given image, or at least gives some error.

Desktop (please complete the following information):

Suggestion for a fix I would've submitted a PR for this, but Go is not my forte so I'll just give my two cents on how this could be done Detect a large file size (might be tricky) and switch to sample version of image by changing the url like this: (adding /sample and set filetype to jpg) Original: https://static1.e621.net/data/a4/18/a41859386beeb5e9ad03c9e98f799578.png Sample: https://static1.e621.net/data/sample/a4/18/a41859386beeb5e9ad03c9e98f799578.jpg

oddpawsx commented 2 years ago

Thank you for reporting this issue! I'll do some testing and follow up here when I have more to report.

NameNotFinal commented 2 years ago

Did some more research and found out that Windows 10 has a hard limit on images 18 MB or over, and just sets the background to a color instead. So if you were to implement some way of checking the file size (maybe check the content-length response header), you could just check if it's under 18 MB, and if not use the sample instead (and pray that it somehow isn't 18+ MB)

LakesideMiners commented 2 years ago

The file size can be checked via the e6 api on the /posts.json endpoint. e.g. if we wanted to get info about post with the ID 3264793. e621.net/posts.json?tags=id:3264793 would return the size value under

posts:
    file:
        size:

It will return the file size in bytes which then can be used to guess if the sample file is needed instead.

oddpawsx commented 2 years ago

@LakesideMiners thank you; we are already working on an implementation for this tracked in a private issue tracker.

LakesideMiners commented 2 years ago

oh alright, thank you!

NameNotFinal commented 2 years ago

Hate to clutter this issue but just want to put in my own thoughts (or make it more clear):

While the solution @LakesideMiners put forward works, it adds an unnecessary request to the API (if you use md5 search, since Walltaker doesn't give you the post id). So instead of requesting the file size from the API, you can just use the content-length header that gets returned by the direct link. It gives the exact same value as the API does, and removes the need to send a request to the API.

So you can start the request -> check content-length -> cancel if too large/or let the request run if under 18MB

oddpawsx commented 2 years ago

Totally valid, though regarding this flow:

So you can start the request -> check content-length -> cancel if too large/or let the request run if under 18MB

I don't think that we'd want to all-out cancel the request if too large, because then the wallpaper that is set becomes out-of-sync with what the Walltaker web app says should be the wallpaper. Instead we should request the sample size image like you suggested in your original report.

oddpawsx commented 2 years ago

I will try to finish up my implementation in the next few days, don't worry, it's in progress. Or if you have something already written, feel free to open a PR against develop

NameNotFinal commented 2 years ago

Instead we should request the sample size image like you suggested in your original report.

Sorry for being unclear about that, but yeah that's what I meant

I will try to finish up my implementation in the next few days, don't worry, it's in progress. Or if you have something already written, feel free to open a PR against develop

Great! Thanks for making this stuff possible :D Haven't written anything since I've never touched Go before, but who knows, maybe this'll be my entrypoint for learning a new language. Probably not for this issue, but if I notice a bug or something that would be a cool feature I might tinker with it in the future.

oddpawsx commented 2 years ago

All good :) Fun fact, this is the first big golang project for me! Been using it as practice haha

oddpawsx commented 2 years ago

@NameNotFinal @LakesideMiners I ended up going with the API call because it was a useful way to enhance the "Open e621" taskbar button anyway. I figured it was worth it to make one small request before pulling down an image no matter the size, rather than pull down the entire large image, check the content-length, and then pull down another whole image because the first was too big (feels a little more efficient to me to do it this way, personally)

I've tagged this and one other change to be the changes in v2.0.3, feel free to test out the build in v2.0.3-rc.1 if you want to try it out, though I'm likely going to release this change by the end of the night as a full v2.0.3 :)

Thanks again for bringing this issue to my attention @NameNotFinal ! :tada:

EDIT: if you downloaded before 1649384021, I totally made the release candidate based on the wrong branch. It's fixed/rebuilt now!

NameNotFinal commented 2 years ago

rather than pull down the entire large image, check the content-length, and then pull down another whole image because the first was too big (feels a little more efficient to me to do it this way, personally)

To be fair (though I may not have been clear enough about it) my idea was to start pulling the image, check the content-length while it still was pulling the image and cancel/continue the download based on what the content-length is. Honestly though, I don't know if that would be feasible in Golang, I just know that in Python something like that could be done by using the requests library with the stream functionality to process headers while the main content is still downloading.

However, considering the fact that you need to call the e6 API regardless for the "Open e621" button makes the content-length way of doing it obsolete and I agree that in this case it's just easier and makes a lot more sense to just handle both the file size and button link at the same time.

Great work. I haven't tested it yet, but I'll close this issue as soon as I've done so :)

NameNotFinal commented 2 years ago

Works like a charm!

Something I noticed while testing though: If you have it set to save posts, it'll download the full quality version, something I think is good, however this also causes the application to hang/not poll Walltaker until that download has finished. Something multithreading could fix maybe? Anyways, closing this issue now.

LakesideMiners commented 2 years ago

@oddpawsx that was my exact thought as for the API call vs content-length, as it avoids downloading an image that might not be set. Glad to see this issue has been closed.