Load and save variable filters

[kentarolim10]

Description

• Creates settings that stores filters for variable name and type
• Loads settings on startup
• Saves filters onto settings

Improvement for #165

[github-actions]

👈 Launch a Binder on branch kentarolim10/jupyterlab-variableInspector/add-persistent-variable-filtering

[fcollonval]

Thanks @kentarolim10

I have some comments to improve the structure but the current logic is correct.

[fcollonval]

Good practice wants that asynchronous code should not block the activation of the plugin as it may delay the full application start up.

A better pattern is therefore to use a Promise approach:

Suggested change

	let settings: ISettingRegistry.ISettings;
	try {
	settings = await settingRegistry.load(variableinspector.id);
	} catch (error) {
	console.error('Failed to load settings for the Git Extension', error);
	}
	Promise.all([settingRegistry.load(variableinspector.id), app.restored])
	.then(settings => {
	// ... here use the settings
	})
	.catch(error => {
	console.error('Failed to load settings for the Git Extension', error);
	});

[fcollonval]

This is not the best approach as it add an opinionated interface on how the filter state is stored. To ease code maintenance, extensibility and testing, the best is to not passed the settings object but rather to add filter setters on the panel.

Now the trouble raises from how to pass information from the settings only available within the promise to the panel.

The best solution would be to refactor substantially the code as it is a bad idea to embed the view panel in the manager.

But let's go for a simpler fix for now. You could use a PromiseDelegate like this:

const settings = new PromiseDelegate<ISettingRegistry.ISettings>();

Promise.all([settingRegistry.load(variableinspector.id), app.restored])
.then(settings_ => {
  settings.resolve(settings_);
})
.catch(error => {
  console.error('Failed to load settings for the Git Extension', error);
  settings.reject(error);
});

Then change the command execute to be async:

      execute: async () => {
        if (!manager.panel || manager.panel.isDisposed) {
          await settings; // Wait for the settings to be loaded
          manager.panel = newPanel();
          // Best would be to use a single array of filter object: {filter: '', type: ''}[]
          // to clarify the binding between the type and the filter
          manager.panel.filters = settings.filters;
        }
        if (!manager.panel.isAttached) {
          labShell.add(manager.panel, 'main');
        }
        if (manager.source) {
          manager.source.performInspection();
        }
        labShell.activateById(manager.panel.id);
      }

[fcollonval]

Thanks @kentarolim10 for working on this.

Here are some additional suggestions. Do you have time to finish this PR?

[fcollonval]

To make the code more readable, you could use arg expansion:

Suggested change

	.then(returnArray => {
	return settings.resolve(returnArray[0]);
	.then(([settings_]) => {
	return settings.resolve(settings_);

[fcollonval]

This is not a great pattern as we should avoid making this class aware of the settings registry.

To do this, this class need to have public setter for the filters and a signal that is emitted when they change. This signal will be used to store new filters to the settings.

The pseudo code would look like that:

// index.ts

panel.filters = settings.composite['filters];
panel.filtersChanged.connect((_, newFilters) => { settings.set('filters', newFilters); });

// variableinspector.ts

class VariableInspectorPanel {
  private _filtersChanged = new Signal<this, string[]>(this);
  private _filters = new Array<string>();

  set filters(value: string[]) {
    this._filters = value;
    this._filtersChanged.emit(this._filters);
  }

  get filterChanged(): ISignal<this, string[]> {
    return this._filtersChanged;
  }
}

}

[kentarolim10]

@fcollonval Sorry for the delay. I was just wondering how we are able to access the settings in the index for the two lines.

panel.filters = settings.composite['filters];
panel.filtersChanged.connect((_, newFilters) => { settings.set('filters', newFilters); });

Currently, the settings are a promise in which we load the settings.

const settings = new PromiseDelegate<ISettingRegistry.ISettings>();

Promise.all([settingRegistry.load(variableinspector.id), app.restored])
      .then(([_settings]) => {
        return settings.resolve(_settings);
      })
      .catch(error => {
        console.error('Failed to load settings for the Git Extension', error);
        settings.reject(error);
      });

However, when we try to access the settings with the filter, the promise has not resolved yet. How do we go about this?

[fcollonval]

Hey @kentarolim10 - nice to read you.

---

A common pattern is to call a onSettingsChanged method in the plugin activation function that propagate the settings to the appropriate objects when they changed; see e.g. https://github.com/jupyterlab-contrib/search-replace/blob/8f4bdff704339cb189e4d54beb230a1e510000f7/src/index.ts#L75

In this case to set the settings on all variable-inspector panels, you could make use of the tracker.forEach method.

[kentarolim10]

Thanks @kentarolim10 for working on this.

Here are some additional suggestions. Do you have time to finish this PR?

Yes I do, I'm just studying for finals right now so I will try and get it done next week.