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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add fontFamily and fontSize to CodeEditor configuration. #4673

Merged
merged 3 commits into from Jun 28, 2018

Conversation

@AlbertHilb
Copy link
Member

@AlbertHilb AlbertHilb commented Jun 1, 2018

This PR intends to fix issue #4139.
I'm new to jupyterlab code, so the way I implemented this feature can be completely wrong.
If that is the case, feel free to move it to trash. 馃槉
Otherwise, I'll be happy to add any change you request.

Cheers

@bollwyvl
Copy link
Contributor

@bollwyvl bollwyvl commented Jun 1, 2018

This is very cool, and definitely answers #4139! I'm really excited about this, and have even started some of the stuff on the repo linked over on jupyterlab-fonts.

I'll do some more thorough review comments, and then swing back around with some more summary stuff...

@@ -602,6 +612,8 @@ namespace CodeEditor {
*/
export
let defaultConfig: IConfig = {
fontFamily: '\'Source Code Pro\', monospace',
Copy link
Contributor

@bollwyvl bollwyvl Jun 1, 2018

Choose a reason for hiding this comment

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

I am -1 on moving this (and almost only this) to having a canonical value in the source code... perhaps leaving these as null such that the theme layer can carry it's preferred font, and these settings are really an end-user override.

@@ -602,6 +612,8 @@ namespace CodeEditor {
*/
export
let defaultConfig: IConfig = {
fontFamily: '\'Source Code Pro\', monospace',
fontSize: 13,
Copy link
Contributor

@bollwyvl bollwyvl Jun 1, 2018

Choose a reason for hiding this comment

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

The other thing that contributes significantly to code readability is line-height... this has a variable, --jp-code-line-height, and is particularly fickle with respect to which font a user might pick.

tabSize,
readOnly,
...otherOptions
} = config;
Copy link
Contributor

@bollwyvl bollwyvl Jun 1, 2018

Choose a reason for hiding this comment

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

Having to enumerate all of these seems like it will be easy to miss when something changes... perhaps explore going back to Partial...

Copy link
Member

@ian-r-rose ian-r-rose Jun 27, 2018

Choose a reason for hiding this comment

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

I agree that this is easy to miss things if something changes. I don't think it's too bad though.

I thought it might be nice to annotate bareConfig as a CodeMirror.EditorConfiguration object so that the typescript compiler could catch some of that stuff. However, when I tried that I found that the CodeMirror typings were not quite up to snuff. That being the case, I think this is a fine solution.

@@ -23,8 +23,6 @@

.CodeMirror {
line-height: var(--jp-code-line-height);
font-size: var(--jp-code-font-size);
Copy link
Contributor

@bollwyvl bollwyvl Jun 1, 2018

Choose a reason for hiding this comment

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

as mentioned, leaving this managed by default by the theme would be nicer for theme implementers...

@@ -9,6 +9,12 @@
"autoClosingBrackets": {
"type": "boolean"
},
"fontFamily": {
Copy link
Contributor

@bollwyvl bollwyvl Jun 1, 2018

Choose a reason for hiding this comment

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

permitting this to be null and handling that explicitly, with some doc suggesting that it will delegate to the CSS theme if left blank might be more robust... just deleting the key might not be sufficient if the style is already set, for example.

@@ -39,6 +45,8 @@
"$ref": "#/definitions/editorConfig",
"default": {
"autoClosingBrackets": true,
"fontFamily": "'Source Code Pro', monospace",
Copy link
Contributor

@bollwyvl bollwyvl Jun 1, 2018

Choose a reason for hiding this comment

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

And here use default null...

@AlbertHilb
Copy link
Member Author

@AlbertHilb AlbertHilb commented Jun 8, 2018

Hi @bollwyvl,
thank you for reviewing my code.

I wiped out the CSS piece of code related to editor font properties because I'm not convinced font settings are just a matter of style for a text editor.
It seems I'm not the only one thinking so. 馃槉
Both Atom and VSCode don't put editor font properties in themes but store them in different config files (Atom: here, and VSCode: here and here).

Anyway, if we keep setting default editor font properties via stylesheets, then we have to make a choice, we should decide what values settingeditor has to show as default for them.
There are two possibilities:

  1. null as you suggested or
  2. actual values as established in the CSS code.

The latter one is my preferred solution. Trying to increase font size, for example, without knowing current size, it's like walking blindly.
However this is not achievable at moment, option default values in settingeditor are read from plugin.json static files. Should we change that, allowing to calculate them dynamically?

If we opt for the first solution, instead, we have to clearly explain to users why font properties are unset. This requires to add description fields to them.
Unfortunately, currently, settingeditor shows description field only for top level schema properties and they are not, they are nested in the editorConfig property.
So, should we give up showing description right above the associated property and move all of them in the top level editorConfig description?
Should we promote editorConfig children to top level properties removing editorConfig altogether?
Or should we modify the way property descriptions are handled by settingeditor?

Let me know your ideas.

@bollwyvl
Copy link
Contributor

@bollwyvl bollwyvl commented Jun 9, 2018

thank you for reviewing my code.

thanks for submitting it!

I'm not convinced font settings are just a matter of style for a text editor.

Agreed, and absolutely needs to be directly accessible to the user. In fact, I would like to see this PR take a stab at adding commands/menu items to modify these values so we can flush out some of the limitations, as "go hack some JSON" isn't going to jump out and show someone they have that kind of agency.

Both Atom and VSCode don't put editor font properties in themes

Jupyter frontends live in a weird place, as they aspire to not just be code editors, but show code in more contexts than standard editors, become the first place people will compute, and be more hackable than a traditional editor.

For giggles, one can already change the two values proposed thus far with, e.g. an IPython console:

%%html
<style>body {--jp-code-font-size: 48px; --jp-code-font-family: "Comic Sans"; }</style>

It will make all code basically unusable... but in a way that it is consistently unusable.

By keeping everything in CSS variables, these values are available to higher-order style concerns like vertical rhythm, coordinated character width and others... though it would be even better to yet more font metrics available for these purposes.

I look forward to the day when we have a letter-perfect, archival-grade, css-powered static representation from nbconvert without $LaTeX$. Whether the existing theme system is powerful enough to also power that remains to be seen, but I wouldn't want to start the precedent of "themes work for everything except..." and require some JS shim to achieve. Looking through the current code base, there are 23 instances of touching .style., of which most could probably be delegated to stylesheets, with the remainder being mouse-related positioning. Every time one of those is added, it increases the arms race between the ability to be themed vs coded, which leads to !important... which leads to violence, suffering, etc.

SHAME 馃敂 Of course, over on jupyterlab-fonts, I'm directly going to some dark places with JSS, and not even generating intermediate CSS, but that's the nice thing about proof-of-concept extensions. :P

Should we change that, allowing to calculate them dynamically?

I think at the data layer (JSON, without comments), null is appropriate. I would 鉂わ笍 more dynamic declarative UI which could look around at things and interpret values. I would not like to see the settings schema system get more complicated/special and introduce interpreted javascript, templates, s-expressions, etc. as these things encode into JSON poorly, can't be schema validated, etc.

Trying to increase font size, for example, without knowing current size, it's like walking blindly.

Again, if you're hacking the scary-sounding Advanced Settings Editor, you're not particularly uninformed that the lights are off. I am thinking a suitable Increase Font Size command would accept a value parameter, which if left blank could go off and pull the default value off the DOM.

Set property default values to `null`.
Use style from theme as default.
@AlbertHilb
Copy link
Member Author

@AlbertHilb AlbertHilb commented Jun 11, 2018

Hi @bollwyvl, please take a look at changes I made.

I think the only thing left over is deciding if we should add descriptions for the new properties in the settingeditor, and in such a case how to do so.

@dhirschfeld
Copy link
Member

@dhirschfeld dhirschfeld commented Jun 11, 2018

xref: #4648

Add `fontFamily`, `fontSize` and `lineHeight` configuration options to
editors used in notebook cells.
@AlbertHilb
Copy link
Member Author

@AlbertHilb AlbertHilb commented Jun 14, 2018

I added some comment on use of new properties to editorConfig description and I enabled fontFamily, fontSize and lineHeightoptions also for editors used in notebook cells.

Cheers

@jasongrout jasongrout added this to the Beta 3 milestone Jun 27, 2018
Copy link
Member

@ian-r-rose ian-r-rose left a comment

Thanks for all the hard work @AlbertHilb (and thanks for the reviews @bollwyvl)! I think this is in really good shape. I had a couple of questions, but overall this is a great contribution, and I think we should move forwards with it.

@bollwyvl, anything to add about how the current implementation will interact with your experiments with juptyerlab-fonts?

@@ -981,7 +990,7 @@ namespace CodeMirrorEditor {
* (it will default to be to the right of all other gutters).
* These class names are the keys passed to setGutterMarker.
*/
gutters?: ReadonlyArray<string>;
gutters?: string[];
Copy link
Member

@ian-r-rose ian-r-rose Jun 27, 2018

Choose a reason for hiding this comment

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

What is the reason for changing this?

Copy link
Member Author

@AlbertHilb AlbertHilb Jun 28, 2018

Choose a reason for hiding this comment

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

Without that change I get:

../codemirror/src/editor.ts(1127,8): error TS2345: Argument of type '{ mode?: string | IMode; theme?: string; smartIndent?: boolean; electricChars?: boolean; keyMap?:...' is not assignable to parameter of type 'EditorConfiguration'.
  Types of property 'gutters' are incompatible.
    Type 'ReadonlyArray<string>' is not assignable to type 'string[]'.
      Property 'push' is missing in type 'ReadonlyArray<string>'.

Copy link
Member

@ian-r-rose ian-r-rose Jun 28, 2018

Choose a reason for hiding this comment

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

Ah, I see. It's another example of the CodeMirror typings being slightly different from ours. I think this is fine, in that case. Thanks!

tabSize,
readOnly,
...otherOptions
} = config;
Copy link
Member

@ian-r-rose ian-r-rose Jun 27, 2018

Choose a reason for hiding this comment

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

I agree that this is easy to miss things if something changes. I don't think it's too bad though.

I thought it might be nice to annotate bareConfig as a CodeMirror.EditorConfiguration object so that the typescript compiler could catch some of that stuff. However, when I tried that I found that the CodeMirror typings were not quite up to snuff. That being the case, I think this is a fine solution.

"fontSize": {
"type": ["integer", "null"],
"minimum": 1,
"maximum": 100
Copy link
Member

@ian-r-rose ian-r-rose Jun 27, 2018

Choose a reason for hiding this comment

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

Is there any particular reason you chose this maximum number?

Copy link
Member Author

@AlbertHilb AlbertHilb Jun 28, 2018

Choose a reason for hiding this comment

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

I think an upper limit to fontSize helps catching typos early.
But no, there is no particular reason to choose 100, except for being a round number, the same chosen by Atom editor and big enough to be confident no one wants larger fonts.

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented Jun 28, 2018

Thanks @AlbertHilb, I think this is great. It would be nice to have some sort of UI to increase and decrease the font size for the file editor, but I think we can do that in a follow-up.

@ian-r-rose ian-r-rose merged commit 05a2089 into jupyterlab:master Jun 28, 2018
2 checks passed
@AlbertHilb AlbertHilb deleted the FontSetting branch Jun 28, 2018
@ian-r-rose ian-r-rose mentioned this pull request Jul 20, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Aug 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants