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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Line numbers in file editor reappear when changing font or tab size #7295

Closed
ajbozarth opened this issue Oct 4, 2019 · 22 comments · Fixed by #7611
Closed

Line numbers in file editor reappear when changing font or tab size #7295

ajbozarth opened this issue Oct 4, 2019 · 22 comments · Fixed by #7611
Assignees
Labels
pkg:fileeditor status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion.
Milestone

Comments

@ajbozarth
Copy link
Contributor

ajbozarth commented Oct 4, 2019

Description

When in a File Editor tab if you use either the menu or the palette to change the tab size or font size it enable the line numbers if you previously disabled them (the same way as enabling them in the menu would).

Reproduce

  1. Open any text file
  2. Open the view menu.
  3. Uncheck the Line Numbers option
  4. Open the palette (or settings menu)
  5. Select a different Tab Size or Increase/Decrease the Font Size
  6. Note the line numbers in the file editor reappear
  7. Note that in the View Menu the Line Numbers option is now enabled.

Expected behavior

Changing editor settings should not change line numbers settings

Context

  • Operating System and version: latest MacOS
  • Browser and version: Safrai, FF, Chrome
  • JupyterLab version: latest master (2.0 alpha)
@ajbozarth
Copy link
Contributor Author

I'm willing to look into this myself but won't have the bandwidth to do so until November.

@telamonian
Copy link
Member

To get you started, the basic problem seems to be that the "Show Line Numbers" item in the "view" menu does not change the "Text Editor" settings. Thus, the editorConfig.lineNumbers setting ends up clobbering the setting in the view menu in a bunch of different cases

@ajbozarth
Copy link
Contributor Author

Thanks, that's was what I figured was happening, but my confusion stems around the fact that it works in v1.1.4 but not master, which means something that changed since that release caused this, which I assume is my update since it touched the section of code which handles it.

@ajbozarth
Copy link
Contributor Author

I've started working on this since I noticed it's going to be in 1.2

As for early findings, I have traced down the commit it first appears in to 62522e8 which is one of my commits as I thought. I've also confirmed that no code was changed in that commit, just which file it's located in, so the cause of the bug must be with where the definition of the commands are made.

ajbozarth added a commit to ajbozarth/jupyterlab that referenced this issue Oct 12, 2019
Due to a still unclear issue, when passing settingRegistry to
functions it changes the beahvior of the commands that use it.

This change moves the six commands that interact with settingRegistry
back to index.ts from commands.ts where they were oved in jupyterlab#6904

This fixes jupyterlab#7295
@jasongrout
Copy link
Contributor

jasongrout commented Oct 12, 2019

It seems like your PR is not working for me.

I think @telamonian is right above. Notice how tabs and font size is changed here:

commands.addCommand(CommandIDs.changeFontSize, {
execute: args => {
const delta = Number(args['delta']);
if (Number.isNaN(delta)) {
console.error(
`${CommandIDs.changeFontSize}: delta arg must be a number`
);
return;
}
const style = window.getComputedStyle(document.documentElement);
const cssSize = parseInt(
style.getPropertyValue('--jp-code-font-size'),
10
);
const currentSize = config.fontSize || cssSize;
config.fontSize = currentSize + delta;
return settingRegistry
.set(id, 'editorConfig', (config as unknown) as JSONObject)
.catch((reason: Error) => {
console.error(`Failed to set ${id}: ${reason.message}`);
});
},
label: args => args['name'] as string
});

For the font size, the global config object is modified, and then the config object is set as a whole, resetting any config the editor might have had to the global config object.

The problem for me is that the line numbers, word wrap, and match brackets settings triggered from the View menu are set here:

menu.viewMenu.editorViewers.add({
tracker,
toggleLineNumbers: widget => {
const lineNumbers = !widget.content.editor.getOption('lineNumbers');
widget.content.editor.setOption('lineNumbers', lineNumbers);
},
toggleWordWrap: widget => {
const oldValue = widget.content.editor.getOption('lineWrap');
const newValue = oldValue === 'off' ? 'on' : 'off';
widget.content.editor.setOption('lineWrap', newValue);
},
toggleMatchBrackets: widget => {
const matchBrackets = !widget.content.editor.getOption('matchBrackets');
widget.content.editor.setOption('matchBrackets', matchBrackets);
},
lineNumbersToggled: widget =>
widget.content.editor.getOption('lineNumbers'),
wordWrapToggled: widget =>
widget.content.editor.getOption('lineWrap') !== 'off',
matchBracketsToggled: widget =>
widget.content.editor.getOption('matchBrackets')
} as IViewMenu.IEditorViewer<IDocumentWidget<FileEditor>>);

Notice that they only change the live editor instance. They don't change the editor config. So when the commands above set the entire config, the line number setting is reset back to its config generated from the settings. For some reason, the View menu items are not calling these commands for me, which do use this global config:

commands.addCommand(CommandIDs.lineNumbers, {
execute: () => {
config.lineNumbers = !config.lineNumbers;
return settingRegistry
.set(id, 'editorConfig', (config as unknown) as JSONObject)
.catch((reason: Error) => {
console.error(`Failed to set ${id}: ${reason.message}`);
});
},
isEnabled,
isToggled: () => config.lineNumbers,
label: 'Line Numbers'
});

I do remember having some conversations at some point about if those View menu items should be a temporary setting, or if their state should be saved to the config (@ian-r-rose, remember this conversation?). Clearly we decided that it should be a temporary thing? Should it instead set the config? Or should our config-setting options only set the config options we are actually changing, rather than resetting the entire config?

@jasongrout
Copy link
Contributor

To be clear, I just tested on JLab 1.1.4, and I see the same problem there, so I don't think this issue stemmed from #6904.

@jasongrout jasongrout added this to the 2.0 milestone Oct 12, 2019
@jasongrout
Copy link
Contributor

Or should our config-setting options only set the config options we are actually changing, rather than resetting the entire config?

I think this perhaps is the best way forward.

@ajbozarth
Copy link
Contributor Author

@jasongrout let me make sure I have a grasp on your above comments.

The bug is where changing tab/font size makes the line numbers disappear in an editor.

  • You see the bug in v1.1.4 and not just on master? Because myself and my 2 teammates only see that bug on master and not v1.1.4 (I myself checked out commit by commit and only saw it starting on the commit 62522e8 on master

  • When running my PR's changes you still see the bug.

On the above two comments I have to mention that I needed to do a complete clean and rebuild on every checkout otherwise I would see the same behavior as on my previous full clean build.

  • The fact that the ViewMenu options are transient is intentional and they don't use their relevant command on purpose.

  • The command that set the settingsRegistry reset any transient commands to default.

On the above two I'm confused as to why this ever worked for me then. The line numbers default value is disabled, why have line numbers always shown up on default then?

I generally feeling like I understand why the code worked previous the more I read it. If you can confirm or deny my above claims and assertions it will help me move forward on fixing this.

@ajbozarth
Copy link
Contributor Author

Actually I believe it just dawned on me why my change in my PR "fixes" the issue for me. The var config holds the current instance of the editor settings, including the transient ones. So when the functions are location in the index.ts file (as I moved them back to in my PR) when the command is called it grabs the current value of config, but by moving the declaration of the addCommand into a function that passes in config, it then uses the value of config in that moment, which will always be the default. I'll see if making config a form of global access var will allow the addCommands to stay in commands.ts. This of course all assumes that my comment about building is the reason for your differences in experience than myself.

@ajbozarth
Copy link
Contributor Author

For reference these are the clean and rebuild steps I run for a consistent change in experience between checkouts.

rm -rf ~/.jupyter/lab
rm -rf $ANACONDA_HOME/share/jupyter/lab

pip install -e .
jlpm install
jlpm run build
jlpm run build:core
jlpm run build:dev:prod
jupyter lab build

@ajbozarth
Copy link
Contributor Author

I opened a new pr with a different method of solving the issue #7350 as explained in my previous comment. I believe it's a better solution that is also more in line with the idea of the separate commands.ts utility file.

@ajbozarth
Copy link
Contributor Author

Also as a note on:

Or should our config-setting options only set the config options we are actually changing, rather than resetting the entire config?

I think this perhaps is the best way forward.

IIUC the settingsRegistry can only update a whole config at a time. So it is necessary to update a single config value then pass in the whole updated config to the settingsRegistry.

@jasongrout
Copy link
Contributor

IIUC the settingsRegistry can only update a whole config at a time. So it is necessary to update a single config value then pass in the whole updated config to the settingsRegistry.

So we have a natural tension here:

  • We have three places state is stored - on the editor instance, in that global config cache, and in the settings system
  • The view menu changes the editor instance, but does not update the settings or the global config cache
  • The font size change changes the settings, which then sets the global config, which is then used to reset all the options for editors. This is where instance-specific settings like line numbers are overridden.

So perhaps we do a diff for options that actually changed when updating the global config, and then only set those particular options. That preserves the instance-specific settings.

@ajbozarth
Copy link
Contributor Author

I agree that the way things are handled now is not the best, but at the moment I'm just focused on the exact bug I introduced. I agree that a refactor of the code would be worthwhile, but this tangled code was working previous to my update. I say we split this Issue in two, one part to fix the immediate bug introduced in my code, and two to refactor how settings are handled in the editor as a whole.

@jasongrout
Copy link
Contributor

Here's how I reproduce it in jlab 1.1.4:

conda create -c conda-forge -n del-jlabeditor jupyterlab=1.1.4 -y
conda activate del-jlabeditor
jupyter lab

Then in jlab:

  1. Open a new Markdown file
  2. Click the View | Show Line Numbers menu item. Verify that the line numbers are showing in this editor.
  3. Click Settings | Increase Text Editor Font Size. Verify that the line numbers disappear.

Am I reproducing what you saw?

@ajbozarth
Copy link
Contributor Author

@jasongrout Ok so I went through your steps and I believe I figured out where the confusion is coming from. The change in behavior (bug) caused by my code change was smaller than I thought it was. The newly introduced issue isn't that the line number setting resets, its what it resets to. In v1.1.4 changing font/tab size resets the line numbers to on, but with my change it resets them to off.

My PR does fix this particular issue though, this is because the value of lineNumbers in CodeEditor.defaultConfig is false but CodeMirrorEditorFactory which FileEditorFactory "extends" set's it to true on creation. My code undid this though by passing config into functions to create each command. This meant that when each command updated it used config = { ...CodeEditor.defaultConfig } rather than its current value. My PR fixes this by making sure the commands use the current value of config which will include lineNumbers = true as it was set by the factory.

So I would argue that my PR addresses fixes the immediate bug for 1.2 and that we should update the title of this Issue for tracking an overall fix for the view menu settings reseting bug in a future release.

@jasongrout
Copy link
Contributor

I just noticed that I have a custom setting for the text editor line numbers, setting the value to off, so that might have affected my experiments.

So if I understand you correctly, with no custom setting, in 1.1.4 line numbers defaults to on, and that is what changing the font size will reset the line numbers to. And now in 1.2a1, it resets to off by default? I'll delete my custom setting and try it out.

@jasongrout
Copy link
Contributor

Ah, now I can reproduce exactly what you are seeing, and I understand your solution. Before, you were passing in the initial value of config, but the config was being reset every time the settings was updated, which of course wouldn't be passed in. Now, it uses the current global value. Makes sense!

@ajbozarth ajbozarth changed the title Line numbers in file editor disappear when changing font or tab size Line numbers in file editor reappear when changing font or tab size Oct 15, 2019
@ajbozarth
Copy link
Contributor Author

Thanks @jasongrout I've updated the title and description in this issue so it can now be used to track the future fix for the issue with the config

@ajbozarth
Copy link
Contributor Author

@telamonian I'm currently testing out an idea for a fix for this, if it works I'll open a PR.

@ajbozarth
Copy link
Contributor Author

Status update from in-person discussions:
We want to keep the transient editor-specific functionality of those three settings, so instead of updating them to be global I'm going to update the updateSettings function to grab and set those to match the current editor's value on update (currently it just sets all non-globally set values to default).
I should have a PR open for this next week once I return from Austin (Tuesday or Wednesday)

@ajbozarth
Copy link
Contributor Author

@telamonian @jasongrout I've open my PR #7611 as discussed last week, I'll make comments around my code decisions in that PR

@lock lock bot added the status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion. label Jan 10, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Jan 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
pkg:fileeditor status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants