Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

[MAC] Enabling back sub pixel antialiasing for code view #11235

Merged
merged 9 commits into from
Jul 10, 2015
Merged

[MAC] Enabling back sub pixel antialiasing for code view #11235

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
1015223
Fix the rendering issue on Mac. Enabling back sub pixel anti-aliasing…
nethip Jun 4, 2015
698f52b
Defaulting the antialiasing to auto
nethip Jun 8, 2015
c7cafae
Merge branch 'master' into prashant/mac-anti-alias-fix
nethip Jun 8, 2015
7a95c0a
Add a preference to turn on/off subpixel-antialiasing
Jun 8, 2015
74e7ff9
Default the code view antialiasing to subpixel which can be overridde…
nethip Jun 9, 2015
7e769b4
Changed the way AA type is getting set. Added a new class by name sub…
nethip Jul 2, 2015
0582f1c
Incorporating some code review comments from @MarcelGerber. Mainly re…
nethip Jul 3, 2015
5efe7a8
Some string changes got from code review comments. Also updated some …
nethip Jul 8, 2015
faad138
Merge branch 'master' into prashant/mac-anti-alias-fix
nethip Jul 10, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/brackets.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ define(function (require, exports, module) {

// read URL params
params.parse();

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you undo these changes to the whitespace lines? Brackets by default does not trim whitespaces. There's no reason to introduce a change to this. If you have a trim extension, it should probably be disabled for brackets project.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@busykai There was some piece of code here in the earlier commits and i just deleted that piece of code based on the code review comments and that is why it is showing as blank space removal change. I could revert this, but I think it is a good idea to remove the extra spaces wherever possible.


/**
* Setup test object
Expand Down Expand Up @@ -358,8 +358,8 @@ define(function (require, exports, module) {
}

// Localize MainViewHTML and inject into <BODY> tag
$("body").html(Mustache.render(MainViewHTML, Strings));
$("body").html(Mustache.render(MainViewHTML, { shouldAddAA: (brackets.platform === "mac"), Strings: Strings }));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't work right now, you need to add the Strings. namespace in main-view.html to the template vars there.
But read my other comment before you do this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an real good catch Marcel 👏


// Update title
$("title").text(brackets.config.app_title);

Expand Down
2 changes: 1 addition & 1 deletion src/htmlContent/main-view.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@
</div>
</div>

<div id="editor-holder">
<div id="editor-holder" {{#shouldAddAA}}class="subpixel-aa"{{/shouldAddAA}}>
<!-- View Panes are programatically created here -->
</div>

Expand Down
3 changes: 2 additions & 1 deletion src/nls/root/strings.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -743,5 +743,6 @@ define({
"DESCRIPTION_USE_THEME_SCROLLBARS" : "True to allow custom scroll bars",
"DESCRIPTION_LINTING_COLLAPSED" : "True to collapse linting panel",
"DESCRIPTION_FONT_FAMILY" : "Change font family",
"DESCRIPTION_FONT_SIZE" : "Change font size; e.g, 13px"
"DESCRIPTION_FONT_SIZE" : "Change font size; e.g, 13px",
"DESCRIPTION_FONT_SMOOTHING" : "\"subpixel-antialiased\" to enable sub-pixel antialiasing and \"antialiased\" for gray scale antialiasing on Mac"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Mac-only: " should probably be the first words of the description ("on Mac" at the end could be removed then).

Also use of and conjunction is confusing. It should be "or" instead.

});
22 changes: 10 additions & 12 deletions src/styles/brackets.less
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,18 +53,14 @@ html, body {

/* And make sure we get a pointer cursor even over text */
cursor: default;

/* Turn off subpixel antialiasing on Mac since it flickers during animations. */
-webkit-font-smoothing: antialiased;

// This is a hack to avoid flicker when animations (like inline editors) that use the GPU complete.
// It seems that we have to put it here rather than on the animated element in order to prevent the
// entire window from flashing.
// See: http://stackoverflow.com/questions/3461441/prevent-flicker-on-webkit-transition-of-webkit-transform
// Mac-only though, since this would turn off subpixel antialiasing (ClearType) on Windows

&.platform-mac {
-webkit-backface-visibility: hidden;
backface-visibility: hidden;
// Use gray scale antialiasing for UI. Code view editor-holder
// overrides this to use subpixel antialiasing, which then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add: to use subpixel antialiasing on Mac

// can be overridden by setting "fontSmoothing" preference to
// "antialiased". Gray scale AA is used for UI has SourceSansPro
// font does not look good with subpixel AA well on low res monitors.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gray scale AA is used for the UI parts which use SourceSansPro font, which doesn't look good with subpixel AA

-webkit-font-smoothing: antialiased;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This setting applies to html, body while the pref applies to #editor-holder. Is this intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that is correct. The entire UI would have gray scale anti aliasing except for the code editor. Subpixel AA for the UI was not looking good. It exposed some artifacts which was making the entire editor look out of place. I will update the PR with my findings and PR, that will help understand the scenario. Thanks!

}

.dark,
Expand All @@ -73,7 +69,9 @@ html, body {
}
}


.subpixel-aa{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why don't we just make it body.platform-mac #editor-holder?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I see now. Disregard that.

-webkit-font-smoothing: subpixel-antialiased;
}

.resizing-container {
position: absolute;
Expand Down
34 changes: 33 additions & 1 deletion src/view/ViewCommandHandlers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
*/

/*jslint vars: true, plusplus: true, devel: true, nomen: true, indent: 4, maxerr: 50 */
/*global define, $ */
/*global define, brackets: true, $ */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

brackets is not writable here, so it should be brackets: false. additionally, it would probably be better if either it was just brackets or other globals should be explicitly marked as read-only as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure! I will make the change.


/**
* The ViewCommandHandlers object dispatches the following event(s):
Expand Down Expand Up @@ -266,6 +266,24 @@ define(function (require, exports, module) {
}
}


/**
* Font smoothing setter to set the anti-aliasing type for the code area on Mac.
* @param {string} aaType The antialiasing type to be set.It can take either "subpixel-antialiased" or "antialiased"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Add a space: set. It

*/
function setMacFontSmoothingType(aaType) {
var $editor_holder = $("#editor-holder"),
hasClass = $editor_holder.hasClass("subpixel-aa");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't really need this hasClass. add/removeClass will no-op in case the class is not present.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure! I will change this.


// Add/Remove the class based on the preference. Also
// default to subpixel AA in case of invalid entries.
if (aaType === "antialiased" && hasClass) {
$editor_holder.removeClass("subpixel-aa");
} else if ((aaType === "subpixel-antialiased" || aaType !== "antialiased") && !hasClass) {
$editor_holder.addClass("subpixel-aa");
}
}

/**
* Font family getter to get the currently configured font family for the document editor
* @return {string} The font family for the document editor
Expand Down Expand Up @@ -515,6 +533,20 @@ define(function (require, exports, module) {
setFontFamily(prefs.get("fontFamily"));
});

// Define a preference for font smoothing mode on Mac.
// By default fontSmoothing is set to "subpixel-antialiased"
// for the text inside code editor. It can be overridden
// to "antialiased", that would set text rendering AA to use
// gray scale antialiasing.
if (brackets.platform === "mac") {
prefs.definePreference("fontSmoothing", "string", "subpixel_antialiased", {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still, make it default to subpixel-antialiased (undercore -> hyphen)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad. I had changed it at one place but forgot to change it here. I will make the changes here. Thanks!

description: Strings.DESCRIPTION_FONT_SMOOTHING,
values: ["subpixel-antialiased", "antialiased"]
}).on("change", function () {
setMacFontSmoothingType(prefs.get("fontSmoothing"));
});
}

// Update UI when opening or closing a document
MainViewManager.on("currentFileChange", _updateUI);

Expand Down