Setting to use the advanced setting editor for the settings #12466

echarles · 2022-04-25T14:40:04Z

References

Code changes

Add a setting useJsonEditor in the settingeditor-extension package. If that setting is `true, the advanced json editor is use to edit the settings.

User-facing changes

Backwards-incompatible changes

None.

jupyterlab-probot · 2022-04-25T14:40:05Z

Thanks for making a pull request to jupyterlab!
To try out this branch on binder, follow this link:

fcollonval

I would use an enumeration instead of a boolean as setting; something like

Default settings editor: ['UI', 'json']

jtpio · 2022-04-26T07:50:34Z

Also, is this new option needed? Or could the menu customization via the settings be used to replace the menu entry by "command": "settingeditor:open-json"?

echarles · 2022-04-26T08:25:01Z

I would use an enumeration instead of a boolean as setting; something like Default settings editor: ['UI', 'json']

Thx @fcollonval. Yes, we can do that.

Also, is this new option needed? Or could the menu customization via the settings be used to replace the menu entry by "command": "settingeditor:open-json"?

Thx @jtpio. With option, are you meaning the new setting, or the new command? I guess you mean the latter. I have thought replacing the menu entry (which is the visual menu show here below) based on the user setting ediotr (ui or json), but I wonder if it is possible to achieve the replace without the full browser refresh. Once the menu is built, is it mutable?

fcollonval · 2022-04-26T09:51:21Z

I like a lot the idea of @jtpio - but I think it is nice to still combine it with a boolean settings as menu customization is not easy for the end users.

So to layout the changes needed, you would not add a new command, but extend the open one to receive arguments.

If the argument defines the type of settings editor, use the argument otherwise fallback to the settings value. And I think that for 4.0 it will make sense to drop the command 'settingeditor:open-json'. But when backporting this enhancement we should keep it around.

echarles · 2022-04-28T04:40:39Z

So to layout the changes needed, you would not add a new command, but extend the open one to receive arguments. If the argument defines the type of settings editor, use the argument otherwise fallback to the settings value. And I think that for 4.0 it will make sense to drop the command 'settingeditor:open-json'. But when backporting this enhancement we should keep it around.

@fcollonval I have implemented your suggestion in my latest commit. Maybe you could have a look and confirm this was the intent. If confirmed, I will then implement your suggestion I would use an enumeration instead of a boolean as setting; something like Default settings editor: ['UI', 'json'].

echarles · 2022-04-28T04:49:13Z

and I think that for 4.0 it will make sense to drop the command 'settingeditor:open-json'

I have not been able to achieve that as the execution of the open and open-json commands are defined in separated plugins.

fcollonval

Thanks @echarles yes this is the idea.

packages/settingeditor-extension/src/index.ts

echarles · 2022-04-28T17:13:49Z

check-release fails with subprocess.CalledProcessError: Command 'jupyter-releaser check-links --force' returned non-zero exit status 1. This is not related to this PR

@fcollonval this is ready to be merged.

echarles · 2022-04-29T07:33:05Z

@fcollonval I have merged with latest master and resolved the conflicts.

echarles · 2022-04-29T12:18:45Z

Galata is failing on Open the settings editor with a specific search query

I am not familiar with the recent setting search query. I run openUi({ query: '' }); What should I do?

echarles · 2022-05-03T06:45:14Z

Did a rebase on latest master and UI test is now green. A few other flows are failing mostly on splice feature but should not berelated to this PR.

echarles · 2022-05-03T09:25:44Z

The UI regression test fails on Extension Manager › Search which is not related to this PR.

@fcollonval I think this is ready to be merged.

jtpio · 2022-05-03T11:14:06Z

The UI regression test fails on Extension Manager › Search which is not related to this PR.

Yes also noticed in other places: #12340 (comment)

echarles · 2022-05-03T13:02:09Z

Just solved recent conflicts with master and CI is still green (exception of the not-revelant Extension Manager › Search).

echarles · 2022-05-04T03:14:14Z

@fcollonval @jtpio I have rebased and solved the conflicts. Happy to get your reviews so we can merge and backport.

fcollonval

Thanks @echarles LGTM

CI failure is not related

echarles · 2022-05-04T10:11:50Z

@meeseeksdev please backport to 3.4.x

…tor for the settings

echarles · 2022-05-04T10:12:55Z

Thx @fcollonval

The backport PR is #12529

github-actions · 2022-05-04T12:47:33Z

Benchmark report

The execution time (in milliseconds) are grouped by test file, test type and browser.
For each case, the following values are computed: min <- [1st quartile - median - 3rd quartile] -> max.

The mean relative comparison is computed with 95% confidence.

Results table

Test file	large_code_notebook	large_md_notebook
open
`chromium`
actual	14125 <- [14422 - 14607 - 14913] -> 15941	2648 <- [2717 - 2754 - 2785] -> 2976
expected	13693 <- [14157 - 14370 - 14597] -> 15846	2480 <- [2681 - 2711 - 2742] -> 2983
Mean relative change	1.9% ± 0.8%	1.5% ± 0.6%
switch-from
`chromium`
actual	564 <- [612 - 651 - 731] -> 865	425 <- [449 - 460 - 470] -> 555
expected	568 <- [616 - 634 - 685] -> 862	427 <- [446 - 459 - 469] -> 596
Mean relative change	1.8% ± 3.0%	0.2% ± 1.2%
switch-to
`chromium`
actual	347 <- [420 - 454 - 468] -> 525	272 <- [294 - 302 - 308] -> 367
expected	383 <- [409 - 450 - 470] -> 515	273 <- [293 - 301 - 309] -> 341
Mean relative change	0.6% ± 2.1%	0.1% ± 1.2%
close
`chromium`
actual	1030 <- [1085 - 1102 - 1135] -> 1245	469 <- [488 - 501 - 515] -> 578
expected	1047 <- [1082 - 1098 - 1119] -> 1159	466 <- [486 - 496 - 506] -> 573
Mean relative change	1.2% ± 0.9%	1.5% ± 1.2%

Changes are computed with expected as reference.

…466-on-3.4.x Backport PR #12466 on branch 3.4.x (Setting to use the advanced setting editor for the settings)

github-actions bot assigned echarles Apr 25, 2022

echarles changed the title ~~Feat/settings editor last mode~~ Setting to use the advanced setting editor for the settings Apr 25, 2022

echarles added this to the 4.0 milestone Apr 25, 2022

fcollonval added the enhancement label Apr 26, 2022

fcollonval requested changes Apr 26, 2022

View reviewed changes

fcollonval reviewed Apr 28, 2022

View reviewed changes

packages/settingeditor-extension/src/index.ts Outdated Show resolved Hide resolved

packages/settingeditor-extension/src/index.ts Outdated Show resolved Hide resolved

echarles mentioned this pull request Apr 28, 2022

JupyterLab Advanced (JSON) settings editor sometimes does not launch #12485

Closed

echarles force-pushed the feat/settings-editor-last-mode branch from 1c5ac58 to 43db4a3 Compare May 3, 2022 05:37

echarles referenced this pull request May 3, 2022

Merge branch 'commenting-extension' into master

740f779

echarles added 4 commits May 3, 2022 09:18

Add setting to use the advanced json editor for the settings

7ca31a6

Add setting to use the advanced json editor for the settings

9d24960

integrity

d742f84

use an enum for the setting editor type

6659cb6

echarles force-pushed the feat/settings-editor-last-mode branch from 63278a0 to 6659cb6 Compare May 3, 2022 07:21

echarles added 2 commits May 3, 2022 09:26

Package integrity updates

8107b6f

remove comments package in tsconfigdoc.json

f47d63d

Merge branch 'master' into feat/settings-editor-last-mode

9187fb8

fcollonval approved these changes May 4, 2022

View reviewed changes

echarles merged commit dc6aceb into jupyterlab:master May 4, 2022

meeseeksmachine pushed a commit to meeseeksmachine/jupyterlab that referenced this pull request May 4, 2022

Backport PR jupyterlab#12466: Setting to use the advanced setting edi…

6d68640

…tor for the settings

meeseeksmachine mentioned this pull request May 4, 2022

Backport PR #12466 on branch 3.4.x (Setting to use the advanced setting editor for the settings) #12529

Merged

echarles added a commit that referenced this pull request May 5, 2022

Merge pull request #12529 from meeseeksmachine/auto-backport-of-pr-12…

866d06f

…466-on-3.4.x Backport PR #12466 on branch 3.4.x (Setting to use the advanced setting editor for the settings)

jtpio mentioned this pull request Jan 12, 2023

Wrong Setting Editor opened when executing the "Settings Editor" command #13760

Closed

github-actions bot locked as resolved and limited conversation to collaborators May 5, 2023

Uh oh!

Setting to use the advanced setting editor for the settings #12466

Setting to use the advanced setting editor for the settings #12466

Uh oh!

Conversation

echarles commented Apr 25, 2022

References

Code changes

User-facing changes

Backwards-incompatible changes

Uh oh!

jupyterlab-probot bot commented Apr 25, 2022

Uh oh!

fcollonval left a comment

Choose a reason for hiding this comment

Uh oh!

jtpio commented Apr 26, 2022

Uh oh!

echarles commented Apr 26, 2022

Uh oh!

fcollonval commented Apr 26, 2022

Uh oh!

echarles commented Apr 28, 2022

Uh oh!

echarles commented Apr 28, 2022

Uh oh!

fcollonval left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

echarles commented Apr 28, 2022

Uh oh!

echarles commented Apr 29, 2022

Uh oh!

echarles commented Apr 29, 2022

Uh oh!

echarles commented May 3, 2022

Uh oh!

echarles commented May 3, 2022

Uh oh!

jtpio commented May 3, 2022

Uh oh!

echarles commented May 3, 2022

Uh oh!

echarles commented May 4, 2022

Uh oh!

fcollonval left a comment

Choose a reason for hiding this comment

Uh oh!

echarles commented May 4, 2022

Uh oh!

echarles commented May 4, 2022

Uh oh!

github-actions bot commented May 4, 2022

Benchmark report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants