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

Separate global options and solution options #55728

Closed
1 task done
tmat opened this issue Aug 19, 2021 · 9 comments
Closed
1 task done

Separate global options and solution options #55728

tmat opened this issue Aug 19, 2021 · 9 comments
Assignees
Labels
Area-IDE Concept-OOP Related to out-of-proc
Milestone

Comments

@tmat
Copy link
Member

tmat commented Aug 19, 2021

Global options should only be accessed (get/set) via IGlobalOptionService. They are only available in-proc and in EditorFeatures layer and above. Global options should be used for client configuration (UI).

Solution.Options is obsolete. This property will remain for API compat and will only be populated with public options in-proc. It will be empty OOP. All currently used solution options are converted to global options.
Workspace.Options is obsolete and equivalent to Workspace.CurrentSolution.Options.
IOptionProvider and ExportOptionProvider are removed.

Instead of reading solution-scoped options from Solution.Options remote services should receive them via a parameter. The parameter should be a readonly record struct XyzOptions or record class XyzOptions following pattern:

Workspace or Features (server) layer:

[DataContract]
internal readonly record struct XyzOptions(
   [property: DataMember(Order = 0)] bool OptionA = true,
   [property: DataMember(Order = 1)] bool OptionB = false)
{
    public XyzOptions() : this(OptionA: true) {}
    public static readonly XyzOptions Default = new();
}

EditorFeatures (client) layer:

internal static class XyzOptionsStorage
{
        public static XyzOptions GetXyzOptions(this IGlobalOptionService options, string language)
          => new(
              OptionA: options.GetOption(OptionA, language),
              OptionB: options.GetOption(OptionB, language)));

        public static readonly Option2<bool> OptionA = new("XyzOptions", "OptionA", XyzOptions.Default.OptionA, storageLocation: ...);
        public static readonly Option2<bool> OptionB = new("XyzOptions", "OptionB", XyzOptions.Default.OptionB, storageLocation: ...);
}

This approach makes it clear which options the service depends on and potentially allows us to reuse the results when unrelated options change.

Option metadata/storage (IOption) is a client concept - it specifies where and how the option is stored. A feature implementation (server code) shouldn't concern itself with such information and thus should not refer to IOption objects. Using the above pattern a feature implementation would operate on XyzOptions type. XyzOptionsStorage is client-only.

TODO:

  • Defaults for document specific (editorconfig) options

Related:

@tmat tmat added the Concept-OOP Related to out-of-proc label Aug 19, 2021
@tmat tmat added this to the Backlog milestone Aug 19, 2021
@tmat tmat self-assigned this Aug 19, 2021
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Aug 19, 2021
@tmat
Copy link
Member Author

tmat commented Aug 19, 2021

@CyrusNajmabadi RFC

@jinujoseph jinujoseph removed the untriaged Issues and PRs which have not yet been triaged by a lead label Sep 14, 2021
@jmarolf
Copy link
Contributor

jmarolf commented Sep 30, 2021

Workspace.Options is obsolete and equivalent to Workspace.CurrentSolution.Options.

I like this. I think this makes things more clear.

My question is how options layer. There are some options that if not set at the solution level will default to whatever the user has set in visual studio (i.e. all editorconifg options). This seems to break this snap-shot friendly model. Should we also do as @sharwell suggests here and change this behavior so solution options do not depend on the current VS options?

@tmat
Copy link
Member Author

tmat commented Sep 30, 2021

Yes, layering is something I'm looking into now. Document options vs. solution options vs. global options. Also some options should flow from TextView, but do not currently.
We definitely don't want document/solution option lookup to fall back to globally mutable options. That's bad for snapshot as you say.
Thus we need to change how the OptionSet operates. It should imo be a plain dictionary with no fallback logic. The client would decide how to set the options (where to find defaults).
Still figuring put the best way to do that...

@jmarolf
Copy link
Contributor

jmarolf commented Oct 1, 2021

It should imo be a plain dictionary with no fallback logic.

Fine with this as a plan.


today if someone changes and editorconfig file the file watcher fires and event, we read the new file contents, and finally fire a workspace event. If, in between all that, someone where to read options that came from that editorconfig file they would be out-of-date. I think this same model can be applied to VS options in some respects. If the user changes an option and that means the solution options have changed, then the client needs to push an update to the server. Until that happens things are out of date.

@sharwell
Copy link
Member

sharwell commented Oct 7, 2021

Fallback is fine as long as the fallback target is also a snapshot.

As an aside, any use of Workspace.CurrentSolution in OOP is a conceptual error. There is no "current solution" there, as each operation has its own representation of the effective solution for that operation.

@tmat
Copy link
Member Author

tmat commented Oct 7, 2021

Agreed on both points.

@jmarolf
Copy link
Contributor

jmarolf commented Oct 7, 2021

Fallback is fine as long as the fallback target is also a snapshot.

This is a good way to phrase it. We have a snapshot of the global vs options. once those change the VS client needs to push the changes to us and a new snapshot is created. anything operating off the old snapshot will have old options but that seems fine to me.

As an aside, any use of Workspace.CurrentSolution in OOP is a conceptual error.

This is a good point @sharwell. It really should be SolutionSnapShot.

@tmat
Copy link
Member Author

tmat commented Oct 7, 2021

As an aside, any use of Workspace.CurrentSolution in OOP is a conceptual error.

More generally, any use of Workspace in feature implementations other than Workspace.Services in OOP is a conceptual error.

@tmat
Copy link
Member Author

tmat commented May 19, 2022

Completed by #60888

@tmat tmat closed this as completed May 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-IDE Concept-OOP Related to out-of-proc
Projects
None yet
Development

No branches or pull requests

4 participants