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

Header part not reflecting Jupyter Notebook Top Area default setting #6833

Closed
yumyumqing opened this issue Apr 5, 2023 · 3 comments
Closed
Labels
Milestone

Comments

@yumyumqing
Copy link
Contributor

yumyumqing commented Apr 5, 2023

Describe the bug
In settings editor, Jupyter Notebook Top Area is only reflecting changes from User Preferences but not Settings Overrides.

When I start the jupyter notebook with default settings overrides, the menu does pick up the visible: false setting, but the notebook is still showing header part.
If I toggle the setting manually by clicking the box, the change does reflect in the notebook header part.
But then if I click Restore to Defaults, the notebook header is not picking up the change again.

To Reproduce
Steps to reproduce the behavior:

  1. Set up dev environment as https://github.com/jupyter/notebook/blob/main/CONTRIBUTING.md#setting-up-a-development-environment
  2. Create a overrides.json file in settings dir in JupyterLab Application Directory.
    If using mamba as step 1, the dir should look something like $HOME/mambaforge/envs/notebook/share/jupyter/lab/settings.
{
  "@jupyter-notebook/application-extension:top": {
    "visible": false
  }
}
  1. Build and launch jupyter notebook in dev mode
  2. Go to the browser page opened by jupyter notebook
  3. Click Settings - Settings Editor - Jupyter Notebook Top Area
  4. See the box is not checked
  5. In a new tab, open a notebook in notebook mode, and the header is still showing
  6. Back to the setting page and check the box and uncheck again
  7. Refresh the notebook page, see the header is not showing now
  8. Back to the setting page and check the box and then click Restore to Defaults
  9. Refresh the notebook page, see the header is still showing

Expected behavior
In my opinion, I think the header part should reflect the default settings (if there is no User Preference) for 2 reasons:

  1. The user might get confused when a unchecked box can lead to 2 different behaviors
  2. For tools like jupyterlite, that uses this plugin but does not support User Preference override, it would be hard for the users to hide the header part by default.
    (I think jupyterlite uses browser cache for User Preference settings and does not allow overrides at build time. Please correct me if I am wrong on this.)

Screenshots
Screenshot 2023-04-05 at 8 11 38 PM
Screenshot 2023-04-05 at 8 13 02 PM

Desktop (please complete the following information):

  • OS: MacOS Ventura 13.2.1
  • Browser: chrome
  • Version: tip of main branch

Additional context
I think the reason for the bug might come from these 2 places:

  1. updateSettings is only changing top visibility when there is User Preference set up. This should probably be outside of the user setting check.
    if (settings.user.visible !== undefined) {
    settingsOverride = true;
    top.setHidden(!visible);
  2. Whenever refreshing a page, if the User Preference isn't set up, the plugin will adjust the header showing based on screen size only.
    const onChanged = (): void => {
    if (settingsOverride) {
    return;
    }
    if (app.format === 'desktop') {
    notebookShell.expandTop();
    } else {
    notebookShell.collapseTop();
    }
    };

If we fix the bug as in Expected behavior, this neat feature of adjusting based on screen size might go away.
If we still want to keep the adjustment, maybe we can have another variable to flag whether we would like to do adjustment like adjustToScreen: true which could overrides the header settings.

Thanks for taking a look!

@yumyumqing yumyumqing added the bug label Apr 5, 2023
@jupyterlab-probot jupyterlab-probot bot added the status:Needs Triage Applied to issues that need triage label Apr 5, 2023
@jtpio
Copy link
Member

jtpio commented Apr 5, 2023

Thanks @yumyumqing for opening this issue and providing all the details 👍

In my opinion, I think the header part should reflect the default settings (if there is no User Preference) for 2 reasons:

Would you like to open a draft PR so we can see how that would work?

  1. updateSettings is only changing top visibility when there is User Preference set up.

I don't recall exactly why it is like this. But this git history seems to be pointing to this commit: 6c96c80

@jtpio jtpio removed the status:Needs Triage Applied to issues that need triage label Apr 5, 2023
@jtpio jtpio added this to the 7.0 milestone Apr 5, 2023
yumyumqing added a commit to yumyumqing/notebook that referenced this issue Apr 6, 2023
  * The header part was only reflecting user preference but not default
    settings overrides.
@yumyumqing
Copy link
Contributor Author

yumyumqing commented Apr 6, 2023

Thanks @jtpio for your help!

I opened this PR draft and listed a couple different expected behavior cases. #6836
All the checks passed except for the Enforce PR label. Can you please help take a look? Thanks a lot!

yumyumqing added a commit to yumyumqing/notebook that referenced this issue Apr 7, 2023
* The header part reflects both user preference and default settings overrides.

* Toggle 'Show Header' turns off the automatic adjustment.
yumyumqing added a commit to yumyumqing/notebook that referenced this issue Apr 14, 2023
* The header part reflects both user preference and default settings overrides.

* Toggle 'Show Header' turns off the automatic adjustment.
yumyumqing added a commit to yumyumqing/notebook that referenced this issue May 2, 2023
* The header part reflects both user preference and default settings overrides.

* Toggle 'Show Header' turns off the automatic adjustment.
yumyumqing added a commit to yumyumqing/notebook that referenced this issue May 2, 2023
    * Make 'visible' enum type and pick up both default and user
      settings.
jtpio pushed a commit that referenced this issue May 3, 2023
* Make 'visible' enum type and pick up both default and user
      settings.
@yumyumqing
Copy link
Contributor Author

Fixed with PR #6836

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

2 participants