ThemeService registers configuration late triggering change event

[bpasero]

Is there a reason that this call is made from the constructor and not statically outside:

vscode/src/vs/workbench/services/themes/common/themeConfiguration.ts

Line 246 in b03f792

configurationRegistry.registerConfiguration(themeSettingsConfiguration);

This triggers a onDefaultConfigurationChange event that clocks at 16ms on startup.

//cc @jrieken @sandy081

[aeschli]

The default value of the configuration is defined in the constructor (it uses the environment service). Modifying the default value of an existing configuration are problematic/not allowed.

@sandy081 Any advice?

Can the configuration service wait with sending out the event until all services have been created?

[jrieken]

Can the configuration service wait with sending out the event until all services have been created?

💯 for that. Maybe risky but I am also for not sending change events while starting up and then send one combined event once things are initialised

[sandy081]

Configuration service is created in the very beginning with no dependencies on other services. So it is tricky deferring firing events until some lifecycle phase.

@bpasero Is it possible to have lifecycle service along with initial services?

[bpasero]

Is it possible to have lifecycle service along with initial services?

You mean to depend on lifecycle service? Yes, why not?

[sandy081]

yeah, configuration service depending on lifecycle service.

[bpasero]

Yeah I think that might work, in the end not a lot of dependencies:

vscode/src/vs/workbench/services/lifecycle/electron-sandbox/lifecycleService.ts

Lines 27 to 32 in 0420349

	constructor(
	@INotificationService private readonly notificationService: INotificationService,
	@IElectronService private readonly electronService: IElectronService,
	@IStorageService readonly storageService: IStorageService,
	@ILogService readonly logService: ILogService
	) {

This would mean we need to move lifecycle creation into the list of services we create eagerly. And probably need to move notification service as well.

In general I am not a big fan of adding more services to the list of services we create manually and not via registerSingleton.

[sandy081]

I can think of two options

1. Move necessary lifecycle services to create manually during start up as initial services and add dependency (Ideal)
2. Expose an API on configuration service to start triggering events which workbench can call when all services are created.

Let me know which is the better option.

[bpasero]

Actually, 1. is not straight forward as the storage service is used inside notification service but both init in parallel with the config service...

[bpasero]

However, maybe we could simply introduce a new ILifecyclePhaseService which does not have any dependencies, should be no problem to extract this and adopt it.