-
-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
Fix user preferences not being considered for Text Editor #10868
Conversation
Thanks for making a pull request to JupyterLab! To try out this branch on binder, follow this link: |
Thanks for submitting your first pull request! You are awesome! 🤗 |
Thanks for the analysis @Mithil467 and @krassowski The absence of composite is probably related to the transformation introduced with #10469. But the other part directly related to the issue #9623 is linked to the transient configuration introduced in #7611. So the question is should what should we do when settings are changed for editors that are not using the default value? pinging @ajbozarth who implemented the transient feature. |
I'll try to find time to take a look at this later in the week. From reading discussions #10880 may have solved the problem that prevented me from making them global (rather than transient) in my initial work, which was my preference at the time. In which case I would support this solution. But I will need to test the use cases that caused me to find and fix the issue (easily surfaced in Elyra's script editor) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I've tested this locally and I believe my previous statement holds, this LGTM
Thanks for testing @ajbozarth |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Mithil467 could you please rebase this PR, apply the suggestions and check that it has the expected behavior?
packages/codeeditor/src/editor.ts
Outdated
@@ -806,7 +806,7 @@ export namespace CodeEditor { | |||
fontFamily: null, | |||
fontSize: null, | |||
lineHeight: null, | |||
lineNumbers: false, | |||
lineNumbers: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do not change the default setting.
lineNumbers: true, | |
lineNumbers: false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right - this was an inconsistency. And now the UI test are failing because your fix is removing the inconsistency by changing the default.
Sorry for the wrong comment, could you revert to lineNumbers: true
so the default behavior stays the same (aka the editor displays line numbers by default).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, alright!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot @Mithil467
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh maybe you are talking about the predefined configuration for User Preferences
? I actually had overwritten the User Preferences. I deleted the ~/.jupyter
folder to reset the user settings and turns out that it is empty by default with just {}
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @Mithil467 sorry for the late reply. To clean your user settings, you need to remove the settings in %USERPROFILE%.jupyter\lab\user-settings (if on Windows), $HOME/.jupyter/lab/user-settings (otherwise).
So yes we need to figure out why the default configuration for all editors (in the notebook and in the text file) are taken instead of the default settings that should allow to have line numbers in text file by default but not in notebook cells.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apparently there is no file called lab\user-settings
inside the .jupyter
folder
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The user preferences are not set by default on a fresh installation as per the gitter conversation with @jasongrout. I guess for moving ahead we can either
- Set this to
false
-
"lineNumbers": true, - Or update the galata tests to reflect this change.
I guess the second option will help resolving this https://discourse.jupyter.org/t/permanently-activate-line-numbers-in-editor/3864/3 too.
I have rebased master into this |
This pull request has been mentioned on Jupyter Community Forum. There might be relevant details there: https://discourse.jupyter.org/t/permanently-activate-line-numbers-in-editor/3864/3 |
Should I update the UI regression test with the new images (except for the above regression)? |
Ok I figure out what was the trouble. If the user settings define partially the editor configuration or the cell configuration, the default values for the other properties won't be used (as the top configuration exists even if it is empty like in that comment). So I revert the default value for the line numbers in the code. And I set default values for all properties of the configuration objects. |
Oh I see, that makes sense |
Benchmark reportThe execution time (in milliseconds) are grouped by test file, test type and browser. Results table
❗ Test metadata have changed--- /dev/fd/63 2021-09-13 15:06:48.199601801 +0000
+++ /dev/fd/62 2021-09-13 15:06:48.199601801 +0000
@@ -8,7 +8,7 @@
},
"systemInformation": {
"cpu": {
- "brand": "Xeon® Platinum 8272CL",
+ "brand": "Xeon® Platinum 8171M",
"cache": {
"l1d": 65536,
"l1i": 65536,
@@ -28,13 +28,13 @@
"speed": 2.6,
"speedMax": null,
"speedMin": null,
- "stepping": "7",
+ "stepping": "4",
"vendor": "GenuineIntel",
"virtualization": false,
"voltage": ""
},
"mem": {
- "total": 7291699200
+ "total": 7291695104
},
"osInfo": {
"arch": "x64",
@@ -46,7 +46,7 @@
"logofile": "ubuntu",
"platform": "linux",
"release": "20.04.3 LTS",
- "serial": "cfc067bfcb844f35865e279a1b0e66c5",
+ "serial": "0f5fd491ff264b5f9d56e03599b4fae0",
"servicepack": "",
"uefi": false
} |
@meeseeksdev please backport to 3.1.x |
Owee, I'm MrMeeseeks, Look at me. There seem to be a conflict, please backport manually. Here are approximate instructions:
And apply the correct labels and milestones. Congratulation you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon! Remember to remove If these instruction are inaccurate, feel free to suggest an improvement. |
Thank you! I'll try to backport this. |
…ed for Text Editor
References
With reference to issue #9623
Code changes
After this change, all the features like
tabSize
,fontSize
, etc are working. But the featureslineNumbers
,lineWrap
andmatchBrackets
are not working.I removed this if condition and everything works fine now.
jupyterlab/packages/fileeditor-extension/src/commands.ts
Line 208 in bac24a1
Another issue I noticed is that the system defaults says
"lineNumbers": true
but the value used is actuallyfalse
.Hence I've changed the default value to
true
here.jupyterlab/packages/codeeditor/src/editor.ts
Line 809 in bac24a1
Fixed by #10880
I tried printing the
settings
object used in the following line -jupyterlab/packages/fileeditor-extension/src/commands.ts
Line 181 in bac24a1
and got the following output -
There's no
editorConfig
attribute and it is not adding anything toconfig
. However,settings.raw
containts the JSON we need.So I replaced the above line of code to -