Themes tab #8759

Merged
merged 11 commits into from Aug 20, 2014

Projects

None yet

8 participants

@MiguelCastillo
Member
@MiguelCastillo
Member

@dangoor @peterflynn @larz0 Here we go folks. First pass. Maybe we could use a different icon for the themes tab

@larz0
Member
larz0 commented Aug 14, 2014

@MiguelCastillo you sir, are a legend. BTW I have a feeling you can create branches on the brackets repo from now on since you're a committer now? Looks great!

@larz0
Member
larz0 commented Aug 14, 2014

@MiguelCastillo here's the themes icon: http://cl.ly/372J3W0R423H

@ingorichter ingorichter commented on an outdated diff Aug 14, 2014
src/nls/root/strings.js
@@ -519,6 +519,7 @@ define({
"REGISTRY_SANITY_CHECK_WARNING" : "NOTE: These extensions may come from different authors than {APP_NAME} itself. Extensions are not reviewed and have full local privileges. Be cautious when installing extensions from an unknown source.",
"EXTENSIONS_INSTALLED_TITLE" : "Installed",
"EXTENSIONS_AVAILABLE_TITLE" : "Available",
+ "EXTENSIONS_THEMES_TITLE" : "Themes",
@ingorichter
ingorichter Aug 14, 2014 Contributor

minor nit: there is a tab char used for indentation.

@MiguelCastillo
Member

@larz0 the icon looks sooooo good!!! :D

@larz0
Member
larz0 commented Aug 15, 2014

269281

@dangoor
Contributor
dangoor commented Aug 15, 2014

Hey @MiguelCastillo, this is awesome! Thanks for getting this together so quickly.

A quick comment: there are lots of tests for the Extension Manager, so I don't think we'd want to land this without one. Can you add a test or two for the new functionality?

@MiguelCastillo
Member

@dangoor yeah that makes sense. I will take a look tomorrow.

@TomMalbran
Contributor

This is great. In the past we discussed about adding images that represent a theme to the zip, and a name in the package so that we could show them in the installer, making it easier to find a good theme. Are we planning on adding this now, or later?

@Mark-Simulacrum
Contributor

@TomMalbran If you do add it, I think that a nice way of showing them could be clicking on a scaled down version of the extension's image (maybe about 16x16px) in the top right corner, so as not to interfere with extensions such as https://github.com/dnbard/brackets-extension-rating that add content below the extension's name.

The way I would prefer seeing the image would be in a modal that pops up, as I've never liked in-line images too much, especially when they can be quite large (as in this case). My main reasoning for not liking inline images is that it is often difficult to hide them once they have been opened (or at least takes a fair bit of scrolling).


As well as the above, are there any plans for moving the More Info... project link (for most extensions it leads to GitHub) to the title of the extension, or at least changing its text? I was slightly confused before I first clicked on it as to where it would lead me. May have been just me, though.

@TomMalbran
Contributor

@Mark-Simulacrum I am thinking that the image could just replace the description, since an image is the best way to describe. We would then have enough space to show a reasonable size thumbnail where is easy to see the theme, but not big enough so that it takes too much space. Additionally it could be expanded into a modal dialog. If a theme doesn't provide an image we can then just show the description.

@Mark-Simulacrum
Contributor

@TomMalbran That sounds good, perhaps the image could expand when clicked on? I still worry about the size of the image being an issue.

@pthiess pthiess added the Ready label Aug 18, 2014
@dangoor
Contributor
dangoor commented Aug 18, 2014

@TomMalbran Images are tricky, but I agree that they're ideal. The trickiness comes in two forms:

  1. Extension Manager works completely from the JSON file. It doesn't have the zip files, so changes would be required on the registry if screenshots were going to be pulled from them.
  2. If the screenshot was just a URL, we could theoretically just display the image from wherever it is hosted. However, pulling an image from an external URL into Brackets and displaying it could represent a (small) security threat. (Imagine that there's a buffer overflow in Chromium's image display code that's patched in Chrome but we're not on the very latest...)

Come to think of it, the second problem basically applies to both cases.

I don't think we should hold up this pull request trying to get images in, because this pull request is a good step forward given the number of themes we've got. That said, adding images would be a big win, so it's worth figuring out the right approach. Perhaps that discussion should occur in a separate bug, though, so that this PR can move forward as-is...

@MiguelCastillo
Member

@dangoor I added a simple unit test for this to verify that the themes viewmodel is loading things up correctly. Most other stuff is just tested in the other unit tests...

One thing I changed is that there can be multiple calls to downloadRegistry... If there is a call already pending, then I return the promise that's keeping track of that request. This allows me call downloadRegistry from the themes viewmodel and the registry viewmodel without generating multiple requests.

@TomMalbran
Contributor

@dangoor I didn't thought about the zip issue. I thought it would be easier to implement. Anyway I wasn't expecting this to be added to this PR, I was just starting a discussion about it. It can be implemented later, since I think that we need to change the registry so that it can get the images.

@ingorichter ingorichter self-assigned this Aug 20, 2014
@ingorichter ingorichter and 1 other commented on an outdated diff Aug 20, 2014
src/extensibility/ExtensionManagerViewModel.js
@@ -494,7 +500,81 @@ define(function (require, exports, module) {
}
return entry;
};
+
@ingorichter
ingorichter Aug 20, 2014 Contributor

this is only a minor nit to have one line separation between the functions. Makes is more consistent with the rest of the source.

@MiguelCastillo
MiguelCastillo Aug 20, 2014 Member

Yeah, I realized the rest of the world mostly likes one line between functions. I get really claustrophobic with 1 line. I will change it though :)

@ingorichter
ingorichter Aug 20, 2014 Contributor

Sorry! :-)
It's about consistency. I had this kind of discussions 1000 times in other teams. Style is a very sensitive topic and every team I've worked with had different ideas about the ideal solution in mind.

@MiguelCastillo
MiguelCastillo Aug 20, 2014 Member

Oh yeah you are absolutely correct. Consistency trumps most things. :)

@ingorichter
Contributor

@MiguelCastillo It looks good to me. I think I'm going to merge it soon when you are done. Great to see this land in 0.43.

@TomMalbran TomMalbran commented on an outdated diff Aug 20, 2014
src/extensibility/ExtensionManager.js
@@ -180,7 +188,12 @@ define(function (require, exports, module) {
* or rejected if the server can't be reached.
*/
function downloadRegistry() {
- var result = new $.Deferred();
+ if (pendingDownloadRegistry) {
+ return pendingDownloadRegistry;
@TomMalbran
TomMalbran Aug 20, 2014 Contributor

Shouldn't this be return pendingDownloadRegistry.promise();?

@TomMalbran TomMalbran commented on an outdated diff Aug 20, 2014
src/extensibility/ExtensionManager.js
@@ -53,6 +53,14 @@ define(function (require, exports, module) {
var semver = require("extensibility/node/node_modules/semver/semver.browser");
/**
+ * @private
+ * @type {$.Promise} Keeps track of the current registry download so that if a request is already
@TomMalbran
TomMalbran Aug 20, 2014 Contributor

It looks like pendingDownloadRegistry is $.Deferred

@ingorichter
Contributor

That looks good to me. Thanks @MiguelCastillo.

@ingorichter ingorichter merged commit 8712385 into adobe:master Aug 20, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@MiguelCastillo
Member

No problem dude!

@MiguelCastillo MiguelCastillo referenced this pull request Aug 20, 2014
Closed

Own tab for themes #8821

@MiguelCastillo MiguelCastillo deleted the unknown repository branch Aug 21, 2014
@peterflynn
Member

Wanted to belatedly add a few thoughts to the theme-thumbnails thread here... (post-1.0, it'd be good to start a discussion on brackes-dev to move this along further, though)

  • I don't think we should be concerned about the security issue Kevin mentioned. There's an insane number of websites that assume user-generated content images are safe, and for a very long time now that assumption has held (the last serious libjpeg vulnerability was > 10 years ago). So IMHO, linking to images hosted elsewhere is just fine.
  • We shouldn't limit this to just Themes. There are lots of extensions where a quick screenshot explains way more than a text description could. Some extensions might also want to show a logo for easy recognition.
  • Do we want the ability to display multiple images? Most mobile app stores let you include a few different screenshots. (Could always be added later though).
  • I wouldn't worry too much about working around the 'extension rating' UI extensions. We should evolve the Extension Manager UI in the way we think is best, and then work with extension authors to adapt to that.
  • To avoid the listing from getting too visually noisy, we should probably set some rules such as disallowing animated GIFs.
  • OTOH, animated GIFs are great to illustrate an extension's functionality in more depth - so maybe we'd want to allow the zoomed image to point to a different URL than the thumbnail? There'd be other benefit to doing that -- faster load times (especially if people provide Retina screenshots), and the ability for thumbnails of screenshots to be zoom-cropped for legibility rather than just downsized to a teeny tiny mess.
@larz0
Member
larz0 commented Sep 17, 2014

Ideally we should be using static previews instead of a preview images for themes. I actually feel really strongly about this one and I think I've mentioned this far too many times.

@peterflynn
Member

@larz0 What do you mean by 'static preview'?

@larz0
Member
larz0 commented Sep 18, 2014

@peterflynn by 'static preview' I meant a read-only preview of the theme achieved by applying the theme's stylesheet to a sample code area that resembles Codemirror.

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