simov / screenshot-capture

Screenshot Capture / Browser Extension
Other
250 stars 86 forks source link

Image result not correctly in Mac retina display #1

Closed iamkwan closed 7 years ago

iamkwan commented 7 years ago

Hello, When i try to crop some area using Macbook pro retina the result of image not match with my cropped target maybe in retina display, the pixel is very high, so the result of image cannot display correctly

simov commented 7 years ago

I don't own a monitor with such high resolution/dpi, nor do I have a Mac, so I probably won't be able to reproduce it on my end.

You can at least do the following:

  1. Switch to Crop and Wait mode
  2. Select some area for cropping and take a screenshot of your entire window with external app
  3. Save the selected area for cropping with the extension
  4. Post both screenshots here
iamkwan commented 7 years ago

entire window of cropping some area https://upload.cc/i3/oAuYaw.png

result of image https://upload.cc/i/C4Qzb6.png

simov commented 7 years ago

It looks like there is a way to detect the retina display, which is good. I'll try to reproduce it on my end, and I'll let you know when I have something ready to test.

iamkwan commented 7 years ago

also when save the selected area, the file of image missing the extension here is my solution

./content/content.js

function save (image) {
    document.location.href = image.replace('image/png', 'image/octet-stream')
}

replace to

function save (image) {
    var link = document.createElement('a');
    link.download = "download.png";
    link.href = image.replace('image/png', 'image/octet-stream');
    link.click();
}
simov commented 7 years ago

I'll try that out, this is on my todo list too :+1:

simov commented 7 years ago

BTW the code responsible for cropping the image is located in background.js - drawImage. My best bet is that the crop area passed from the web page should be scaled there. Then the only thing left would be to detect the higher dpi inside the web page and pass it to the background page along with the cropping information.

iamkwan commented 7 years ago

agree, i find someone got this problem too in stackoverflow http://stackoverflow.com/questions/32013884/chrome-extension-screenshot-partial-image-cropping-for-retina-display

simov commented 7 years ago

@iamkwan can you try something for me?

I am assuming that you already know how to clone this repo and the extensions as unpacked one.

Can you modify this code in background.js:

context.drawImage(img,
  left * 2, top * 2,
  width * 2, height * 2,
  0, 0,
  width * 2, height * 2
)

And then reload the extension and crop something.

2 here should be the same as executing window.devicePixelRatio in your browser console, but it's most probably 2. I'm yet to find a way to reliably reproduce the dpi stuff on my end, so if that works for you I'll push a fix and ask you to test again.

If anything fails post screenshots here. Thanks!

iamkwan commented 7 years ago

still not match

entire window of cropping some area https://upload.cc/i3/Es1hqY.png

result of image https://upload.cc/i/Kh8Esi.png

simov commented 7 years ago

The top left corner is in the correct place, but it looks like the width and the height should be multiplied by 4 Definitely weird, I'll let you know if I come up with something else.

iamkwan commented 7 years ago

also when I try to crop in jcrop demo page the position of crop is wrong, maybe the jcrop demo page have some class or id same with this extension?

I have a idea, when click the extension, capture the current screen and send the data image to new tab to let user crop

update: i think this idea not good, cropping in current page is better

simov commented 7 years ago

@iamkwan can you pull the fix-dpi branch and test, I think I finally got it.

iamkwan commented 7 years ago

@simov the problem was solve, but i think i will change the resolution of the cropped image from retina to normal I have one question, do you know how to insert some text center in the screen? for example when user click the extension icon to trigger the jcrop, then insert the text in the screen without draw canvas (eg. Select Area), when user onmousedown, the text will hide.

simov commented 7 years ago

but i think i will change the resolution of the cropped image from retina to normal

I think in this case the image will look smaller on your high dpi monitor. I should probably add this as an option in the options page.

For your second question: It should be pretty easy to add such message to the web page.

The best way to indicate that the extension is ready for cropping is to dim the page a little bit, like it's done in the jcrop demo, but the problem is that this approach doesn't play well with the extension. Simply because there is no background image to set opacity on. Trust me I've spent a good amount of time on that and there is no apparent way to do it.

iamkwan commented 7 years ago

Not smaller, is the quality lower in my retina display when I change from retina to normal In the fix-pixel extension, when I crop the image Eg. 300x300 But the result is retina image, so the width and high of result image is more than 300x300 On Nov 11, 2016 9:12 PM, "simo" notifications@github.com wrote:

but i think i will change the resolution of the cropped image from retina to normal

I think in this case the image will look smaller on your high dpi monitor. I should probably add this as an option in the options page.

For your second question: It should be pretty easy to add such message to the web page.

The best way to indicate that the extension is ready for cropping is to dim the page a little bit, like it's done in the jcrop demo, but the problem is that this approach doesn't play well with the extension. Simply because there is no background image to set opacity on. Trust me I've spent a good amount of time on that and there is no apparent way to do it.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/simov/screenshot-capture/issues/1#issuecomment-259954386, or mute the thread https://github.com/notifications/unsubscribe-auth/AIfvVvIAlmssMXKtcDNFaM7cceboWnD1ks5q9GnBgaJpZM4KrMRA .

simov commented 7 years ago

Yep, I'm going to add this as an option to this branch. I'll let you know when it's ready to test it.

simov commented 7 years ago

I also like your idea about using some other sort of indication when the extension is ready to crop. I'll think about it but this will most probably be implemented in the next version, after I publish the DPI fix and the option to preserve the size of the cropped area.

iamkwan commented 7 years ago

also when i try to insert element (eg. button) into screen when jcrop onSelect, but when i try to click the element, it trigger jcrop, cannot trigger the element onclick, even i set this element to z-index: 999999999

simov commented 7 years ago

It depends on where you insert the element :)

iamkwan commented 7 years ago

@simov LOL my mistake, it can trigger my function now

iamkwan commented 7 years ago

@simov do you know how to detect when user move the crop box without using onChange?

simov commented 7 years ago

I don't think there is a default event for that (changing the top/left position of a DOM element).

iamkwan commented 7 years ago

finally i use this if condition inside onChange even to detect user is cropping image not just click the area of the screen, because when user just click the crop box it will trigger onChange too if(oldx != e.x || oldx2 != e.x2 || oldy != e.y || oldy2 != e.y2)

simov commented 7 years ago

I just pushed the new DPI option to the fix-dpi branch. Let me know what do you think.

iamkwan commented 7 years ago

i think the options is good, but you need to let user know they can edit this in option (let they know the extension have option to change some config

simov commented 7 years ago

Well, they have to find the options page, there is no other way :) I have outlined how to get to the options page in the extension's description in the chrome store.

What's left is to implement the same option for the Capture Viewport option and then I'm publishing the fix.

Open up another issue for the capture indication overlay and the file name issue if you want.

iamkwan commented 7 years ago

The file name is fixed using my solution On Nov 11, 2016 11:06 PM, "simo" notifications@github.com wrote:

Well, they have to find the options page, there is no other way :) I have outlined how to get to the options page in the extension's description in the chrome store.

What's left is to implement the same option for the Capture Viewport option and then I'm publishing the fix.

Open up another issue for the capture indication overlay and the file name issue if you want.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/simov/screenshot-capture/issues/1#issuecomment-259976927, or mute the thread https://github.com/notifications/unsubscribe-auth/AIfvVr1VBPcBI6FhlUz3k8SgpagA3-Ugks5q9ISRgaJpZM4KrMRA .

simov commented 7 years ago

Ok, I'll take a look at it later on.

simov commented 7 years ago

Just pushed the fix for the Capture Viewport method to be aware of the new DPI option.

iamkwan commented 7 years ago

@simov do you know chrome extension is start to only for chrome OS? I'm just furious. https://blog.chromium.org/2016/08/from-chrome-apps-to-web.html

simov commented 7 years ago

Chrome Apps not Chrome Extensions

iamkwan commented 7 years ago

oh, thank you @simov , you saved my life

simov commented 7 years ago

btw I just merged the DPI fix to master and added the file name fix, take a look at it if you want

iamkwan commented 7 years ago

it works, but i found some problem, i will open a new issue for this problem

simov commented 7 years ago

Ok, I'm closing this one. Thanks for the feedback :+1: I'll think about the overlay too.

simov commented 7 years ago

The fixes are published with verison 1.4