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

Add the extension settings dialog #6138

Merged
merged 6 commits into from
Aug 2, 2023

Conversation

kenzieschmoll
Copy link
Member

@kenzieschmoll kenzieschmoll commented Aug 1, 2023

Add extension settings dialog that is accessible from an "Extensions" toolbar action:
Screenshot 2023-08-01 at 4 59 53 PM

Screenshot 2023-08-01 at 4 54 30 PM

Each extension will also be possible to enable / disable from its own tab, but this extensions menu will always show all available extensions, so that if a user disables one that they later want to enable, they have a way to do that.

Work towards #1632

rootPath: appRootPath,
extensionName: extension.name,
);
final stateNotifier = _extensionEnabledStates.putIfAbsent(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider making the following changes to enabledStateListenable:

  ValueListenable<ExtensionEnabledState> enabledStateListenable(
    String extensionName,
    {ExtensionEnabledState initialState = ExtensionEnabledState.none,}
  ) {
    return _extensionEnabledStates.putIfAbsent(
      extensionName.toLowerCase(),
      () => ValueNotifier<ExtensionEnabledState>(initialState),
    );
  }

and then invoke it here:

final stateNotifier = enabledStateListenable(
  extension.name,
  initialState: stateFromOptionsFile,
);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, totally forgot this is returning a ValueListenable and you need a ValueNotifier...

Another option is to create _enabledStateNotifier(...) and change enabledStateListenable to ValueListenable<ExtensionEnabledState> enabledStateListenable(String extensionName) => _enabledStateNotifier(extensionName);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This wouldn't really save us much complexity since we would still need to call stateNotifier.value = stateFromOptionsFile; below in _refreshExtensionEnabledStates because the ifAbsent function wouldn't always be called.

onPressed: (index) => states[index].$3(),
selectedStates:
states.map((option) => option.$2(enabledState)).toList(),
children: states
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: something like this would be cleaner

selectedStates: [
  for (final (_, state, __) in states)
    state(enabledState),
],
children: [
  for (final (name, _, __) in states)
    Padding(
      padding: const EdgeInsets.symmetric(
        vertical: densePadding,
        horizontal: denseSpacing,
      ),
      child: Text(name),
    ),
]

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was making the analysis server unhappy:
Screenshot 2023-08-01 at 2 44 10 PM

For some reason the order of the items in the record were not in the order that the analyzer expected. Could have been coincidental but it expected them in alphabetical order, not the order in which they were defined.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, this would have worked with the positional records.

This should work now that we're using named properties:

selectedStates: [
  for (final state in states)
    state.isSelected(enabledState),
],
children: [
  for (final state in states)
    Padding(
      padding: const EdgeInsets.symmetric(
        vertical: densePadding,
        horizontal: denseSpacing,
      ),
      child: Text(state.name),
    ),
]

onPressed: (index) => states[index].$3(),
selectedStates:
states.map((option) => option.$2(enabledState)).toList(),
children: states
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, this would have worked with the positional records.

This should work now that we're using named properties:

selectedStates: [
  for (final state in states)
    state.isSelected(enabledState),
],
children: [
  for (final state in states)
    Padding(
      padding: const EdgeInsets.symmetric(
        vertical: densePadding,
        horizontal: denseSpacing,
      ),
      child: Text(state.name),
    ),
]

@kenzieschmoll
Copy link
Member Author

Will clean up the last comment as part of will clean up as part of #6140

@kenzieschmoll kenzieschmoll merged commit d549e80 into flutter:master Aug 2, 2023
15 checks passed
@kenzieschmoll kenzieschmoll deleted the extension-settings branch August 2, 2023 16:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants