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
Comments
I'm willing to look into this myself but won't have the bandwidth to do so until November. |
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 |
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. |
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. |
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
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: jupyterlab/packages/fileeditor-extension/src/commands.ts Lines 168 to 191 in 70ffebf
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: jupyterlab/packages/fileeditor-extension/src/commands.ts Lines 777 to 798 in 70ffebf
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: jupyterlab/packages/fileeditor-extension/src/commands.ts Lines 204 to 216 in 70ffebf
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? |
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. |
I think this perhaps is the best way forward. |
@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.
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.
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. |
Actually I believe it just dawned on me why my change in my PR "fixes" the issue for me. The var |
For reference these are the clean and rebuild steps I run for a consistent change in experience between checkouts.
|
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 |
Also as a note on:
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:
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. |
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. |
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:
Am I reproducing what you saw? |
@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 My PR does fix this particular issue though, this is because the value of 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. |
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. |
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! |
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 |
@telamonian I'm currently testing out an idea for a fix for this, if it works I'll open a PR. |
Status update from in-person discussions: |
@telamonian @jasongrout I've open my PR #7611 as discussed last week, I'll make comments around my code decisions in that PR |
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
Expected behavior
Changing editor settings should not change line numbers settings
Context
The text was updated successfully, but these errors were encountered: