Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add web.archive.org support #3

Closed
wants to merge 7 commits into from
Closed

Add web.archive.org support #3

wants to merge 7 commits into from

Conversation

belldandu
Copy link
Collaborator

This pull request is to add the enhancement at #2

@belldandu
Copy link
Collaborator Author

Yeah i forgot about that e.e its fixed now.

@belldandu
Copy link
Collaborator Author

As far as i know by design your supposed to get the height and width While the image loads. I also tested this and all the images dimensions are set properly. However i can also parse the data listed on the html page to get the size if you still wan't to avoid taking any chances.

@dteviot
Copy link
Owner

dteviot commented Jun 20, 2016

I think there's a mis-communication here.
I have trouble seeing how you can get the dimensions while loading is in
progress, Until you have the file you don't know how big it is.

Here's my thoughts.

  1. I'm happy with the WebArchive and the image removal bug fixes.
  2. Fetching the large scale image. Not so much. I personally prefer the
    scaled down images. (Although I'm now suspecting most people want the full
    scale images.)

So, I'd accept 1, and 2 I want' to think about a bit.

On Mon, Jun 20, 2016 at 8:27 PM, Belldandu notifications@github.com wrote:

As far as i know by design your supposed to get the height and width While
the image loads. I also tested this and all the images dimensions are set
properly. However i can also parse the data listed on the html page to get
the size if you still wan't to avoid taking any chances.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#3 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AE6w2eGP4EKE0pSpGZFpJ75AsiE8-d0Dks5qNk7kgaJpZM4I5bbo
.

@belldandu
Copy link
Collaborator Author

belldandu commented Jun 20, 2016

@dteviot
I could do this but even this could have potential issues.

    let span = util.getElement(div, "span");
    var img = span.innerHTML.split("(")[1];
    if(img){
        img = img.split(" pixels,")[0].split(" x ");
        imageInfo.height = img[0].replace(",", "")
        imageInfo.width = img[1].replace(",", "");
    }

Also as for your one question https://davidwalsh.name/get-image-dimensions
This has been around for a long time. Everything in onload is called imediatly after the image is loaded.

Also as for your issue with larger images, the problem is that the scaled down images with words in them that were translated are impossible to see (i opened one of the books and dear god the blurryness).

Grabbing the full images ensures visablity.

@dteviot
Copy link
Owner

dteviot commented Jun 20, 2016

Everything in onload is called imediatly after the image is loaded.

That's what I understood.
However, onLoad is only called when the image is loaded. Which occurs some time in the future. Until onLoad is called the height and width values of imageInfo are not valid. So if the main thread tries to read imageInfo before onLoad is called, the values will be wrong. I.e. need to wait until onLoad is called. See http://www.html5rocks.com/en/tutorials/es6/promises/, then look at function BakaTsukiImageCollector.prototype.fetchImages.

@dteviot
Copy link
Owner

dteviot commented Jun 20, 2016

i.e. I think updateImageInfoFromImagePage needs to return a promise that is resolved when onLoad finishes.

Please fogive me, I need to go away and do some thinking.

@belldandu
Copy link
Collaborator Author

Thats why i put the return inside of onload.

In your promise, then cannot be called until after that.updateImageInfoFromImagePage(rawDom, imageInfo); has returned something.

By placing the return inside of onload you keep the integrity of the values as nothing is returned until after the image has been fully loaded.

At least thats how it should do and has done so far. I can even try limiting the connection in the browser and test to make sure its still working.

@belldandu
Copy link
Collaborator Author

And ok

@belldandu
Copy link
Collaborator Author

Ok yeah just tested definetly needs a promise.

@belldandu
Copy link
Collaborator Author

belldandu commented Jun 20, 2016

There we go it's now promised and i tested it to. Works perfectly even when throttled to 50kbps with 500ms ping. @dteviot

@dteviot
Copy link
Owner

dteviot commented Jun 20, 2016

Thanks.
On quick glance, looks OK.
I'm afraid it's getting late for me, and I need to go to sleep.
Will probably merge tomorrow.

@belldandu
Copy link
Collaborator Author

Awesome. and night

@belldandu
Copy link
Collaborator Author

@dteviot o.o

@dteviot
Copy link
Owner

dteviot commented Jun 21, 2016

I've merged it into the current dev branch "Sonako" and will send updated build 0.0.0.4 to Chrome Webstore. If I hear no complaints in a few days, will merge to trunk.

@belldandu
Copy link
Collaborator Author

belldandu commented Jun 21, 2016

Ok

Confused as to how de40496 managed to be slightly different from 09dbe2c Did you edit it afterwards?

Don't have a problem with it. Since it works either way. I've just never seen that happen before.

@belldandu
Copy link
Collaborator Author

Ignore those last 2

@belldandu belldandu closed this Jun 21, 2016
@dteviot
Copy link
Owner

dteviot commented Jun 21, 2016

Confused as to how de40496 managed to be slightly different from 09dbe2c Did you edit it afterwards?

Yes. Got merge conflicts and had to fix.
Took liberty to make some minor tweaks.

@belldandu
Copy link
Collaborator Author

Awesome.

@belldandu
Copy link
Collaborator Author

belldandu commented Jun 21, 2016

I'm moving onto the next pull request to add #4
Feel free to make tweaks.

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.

None yet

2 participants