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

Fix #10786 - Allow QuickView image preview for arbitrary URLs #10788

Merged
merged 3 commits into from May 26, 2015

Conversation

Projects
None yet
4 participants
@humphd
Contributor

humphd commented Mar 24, 2015

This leverages the img element's load and error events to try and load arbitrary images. It also expands the list of known image extensions to include .bmp, which browsers can display.

@humphd

This comment has been minimized.

Show comment
Hide comment
@humphd

humphd Mar 27, 2015

Contributor

I've updated this with the suggestion made to examine extensions.

Contributor

humphd commented Mar 27, 2015

I've updated this with the suggestion made to examine extensions.

@MarcelGerber

This comment has been minimized.

Show comment
Hide comment
@MarcelGerber

MarcelGerber Mar 27, 2015

Contributor

Could you take a look at the data provided by language.json/LanguageManager and see if you can use them? image is particularly interesting for whitelist (please concat "svg" yourself), audio and binary for blacklist (concat the ones you already have, plus maybe also "rss", "php").

Contributor

MarcelGerber commented Mar 27, 2015

Could you take a look at the data provided by language.json/LanguageManager and see if you can use them? image is particularly interesting for whitelist (please concat "svg" yourself), audio and binary for blacklist (concat the ones you already have, plus maybe also "rss", "php").

@humphd

This comment has been minimized.

Show comment
Hide comment
@humphd

humphd Apr 13, 2015

Contributor

I've rewritten this to use LanguageManager, good suggestion.

Contributor

humphd commented Apr 13, 2015

I've rewritten this to use LanguageManager, good suggestion.

@humphd

This comment has been minimized.

Show comment
Hide comment
@humphd

humphd Apr 25, 2015

Contributor

This is ready for review.

Contributor

humphd commented Apr 25, 2015

This is ready for review.

@nethip nethip self-assigned this Apr 28, 2015

@nethip

This comment has been minimized.

Show comment
Hide comment
@nethip

nethip Apr 30, 2015

Contributor

@humphd Can you add some unit tests, if possible? Also run smoke tests with these changes, especially the one relating to editor and Extensions.

Contributor

nethip commented Apr 30, 2015

@humphd Can you add some unit tests, if possible? Also run smoke tests with these changes, especially the one relating to editor and Extensions.

ePos = {line: pos.line, ch: token.end};
}
var imgPreview = "<div class='image-preview'>" +

This comment has been minimized.

@nethip

nethip Apr 30, 2015

Contributor

Could you put in the imgPath check here?

@nethip

nethip Apr 30, 2015

Contributor

Could you put in the imgPath check here?

This comment has been minimized.

@humphd

humphd Apr 30, 2015

Contributor

It's already been dealt with in the if-block above from lines 475-483, returning early if we have no value for imgPath, and therefore not needing to do the check and indent all this code.

@humphd

humphd Apr 30, 2015

Contributor

It's already been dealt with in the if-block above from lines 475-483, returning early if we have no value for imgPath, and therefore not needing to do the check and indent all this code.

@nethip

This comment has been minimized.

Show comment
Hide comment
@nethip

nethip Apr 30, 2015

Contributor

LGTM

Contributor

nethip commented Apr 30, 2015

LGTM

@redmunds

This comment has been minimized.

Show comment
Hide comment
@redmunds

redmunds Apr 30, 2015

Contributor

The original Quick View implementation had support for arbitrary URLs, but it was removed due to concerns about performance (i.e. downloading external files on mousemove!). The code has been optimized since then so it now does not try to parse code in editor until mouse hasn't moved for 350ms, but people with a slow network or no network connection might see problems.

Be sure to test this with no internet connection!

You should consider:

  1. Making this a preference. It's probably OK to default to ON as long as there's a way to turn it OFF.
  2. Caching images. No need to re-download same image over and over. Maybe this already takes advantage of browser cache. Then there's the opposite problem of a stale cache :)
Contributor

redmunds commented Apr 30, 2015

The original Quick View implementation had support for arbitrary URLs, but it was removed due to concerns about performance (i.e. downloading external files on mousemove!). The code has been optimized since then so it now does not try to parse code in editor until mouse hasn't moved for 350ms, but people with a slow network or no network connection might see problems.

Be sure to test this with no internet connection!

You should consider:

  1. Making this a preference. It's probably OK to default to ON as long as there's a way to turn it OFF.
  2. Caching images. No need to re-download same image over and over. Maybe this already takes advantage of browser cache. Then there's the opposite problem of a stale cache :)
@humphd

This comment has been minimized.

Show comment
Hide comment
@humphd

humphd Apr 30, 2015

Contributor

I've updated this again. New commit adds:

  • pref for enabling/disabling extensionless previews for URLs (on by default). Let me know if this is how you want such a pref to be done.
  • tests for everything that seemed obvious to me about what this patch is doing.

Regarding @redmunds' questions. First, with no network, you end up in the error path (net::ERR_INTERNET_DISCONNECTED exception or the like trying to load the resource) and it does the right thing. I think cache is best handled by the browser. Trying to build something more intelligent than what is already there for free seems like a waste of effort to me.

Contributor

humphd commented Apr 30, 2015

I've updated this again. New commit adds:

  • pref for enabling/disabling extensionless previews for URLs (on by default). Let me know if this is how you want such a pref to be done.
  • tests for everything that seemed obvious to me about what this patch is doing.

Regarding @redmunds' questions. First, with no network, you end up in the error path (net::ERR_INTERNET_DISCONNECTED exception or the like trying to load the resource) and it does the right thing. I think cache is best handled by the browser. Trying to build something more intelligent than what is already there for free seems like a waste of effort to me.

Show outdated Hide outdated src/extensions/default/QuickView/main.js
@@ -784,10 +812,15 @@ define(function (require, exports, module) {
// Setup initial UI state
setEnabled(prefs.get("enabled"), true);
setExtensionlessImagePreview(prefs.get("extensionlessImagePreview"));

This comment has been minimized.

@redmunds

redmunds May 5, 2015

Contributor

Should never write to prefs on initial load. This is unnecessary and slows down startup. I can see that this is emulating the other pref here, so remove that also if possible.

@redmunds

redmunds May 5, 2015

Contributor

Should never write to prefs on initial load. This is unnecessary and slows down startup. I can see that this is emulating the other pref here, so remove that also if possible.

This comment has been minimized.

@redmunds

redmunds May 5, 2015

Contributor

Actually, call to setEnabled(prefs.get("enabled"), true); on line above is not writing to prefs, so that should be ok.

@redmunds

redmunds May 5, 2015

Contributor

Actually, call to setEnabled(prefs.get("enabled"), true); on line above is not writing to prefs, so that should be ok.

This comment has been minimized.

@humphd

humphd May 5, 2015

Contributor

So the whole doNotSave dance was done for the initial load case? I'll echo that for this as well and avoid the write.

@humphd

humphd May 5, 2015

Contributor

So the whole doNotSave dance was done for the initial load case? I'll echo that for this as well and avoid the write.

@redmunds

This comment has been minimized.

Show comment
Hide comment
@redmunds

redmunds May 5, 2015

Contributor

In general, this looks good. Be sure to turn on JSLint as I am seeing some JSLint errors in QuickView/main.js. Also, there is a merge conflict that needs to be resolved. Just 1 other comment.

Contributor

redmunds commented May 5, 2015

In general, this looks good. Be sure to turn on JSLint as I am seeing some JSLint errors in QuickView/main.js. Also, there is a merge conflict that needs to be resolved. Just 1 other comment.

@humphd

This comment has been minimized.

Show comment
Hide comment
@humphd

humphd May 5, 2015

Contributor

@redmunds thanks for the review. I've rebased and pushed fixes. See if this approach satisfies what you wanted with the pref saving and startup.

Contributor

humphd commented May 5, 2015

@redmunds thanks for the review. I've rebased and pushed fixes. See if this approach satisfies what you wanted with the pref saving and startup.

@redmunds

This comment has been minimized.

Show comment
Hide comment
@redmunds

redmunds May 6, 2015

Contributor

@humphd

See if this approach satisfies what you wanted with the pref saving and startup.

Looks good.

Contributor

redmunds commented May 6, 2015

@humphd

See if this approach satisfies what you wanted with the pref saving and startup.

Looks good.

@humphd

This comment has been minimized.

Show comment
Hide comment
@humphd

humphd May 8, 2015

Contributor

Great, can someone land this then?

Contributor

humphd commented May 8, 2015

Great, can someone land this then?

nethip added a commit that referenced this pull request May 26, 2015

Merge pull request #10788 from humphd/issue10786
Fix #10786 - Allow QuickView image preview for arbitrary URLs

@nethip nethip merged commit 654e750 into adobe:master May 26, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@nethip

This comment has been minimized.

Show comment
Hide comment
@nethip

nethip May 26, 2015

Contributor

Thanks @humphd for this fix

Contributor

nethip commented May 26, 2015

Thanks @humphd for this fix

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment