Toggles for outlining and structured guidelines #4227

cartermp · 2018-01-20T05:40:37Z

Code cleanup
Tests

I think this is just how we handle things in our language service, but you will need to close and re-open documents to see the changes take effect.

Addresses #3681.

There's a global option for structured guidelines as well, and it's controlled at the workspace level. I'm a bit on the fence about having a toggle for them in the F# settings, but I'd think people would look for them there first, then get confused when they're unable to find them. This also allows you to turn it off for F# code, but keep it on for C# code, so I don't think it's horrible to have it here.

cartermp · 2018-01-20T19:06:00Z

Code is cleaned up and there isn't a need to modify tests, as the unit tests for block structure just test that Structure.getOutliningRanges works (which is fine).

So, weird behavior:

With outlining enabled...

Turning off block structure guidelines globally will keep outlining (Good)
Turning off block structure guidelines in F# only will also kill outlining (Bad)

As far as I can tell, there is nothing in BlockStructureService which should control outlining, as all this does is reporting BlockSpans to the workspace. @Pilchie any idea why this may be?

I cannot toggle settings in debug mode - is this due to recent changes in how resources are generated @brettfo? I noticed that I'd need to build vs and install the VSIX to see the new settings page appear.

I also cannot debug, period. No breakpoints can be hit. 😕

Pilchie · 2018-01-20T22:18:01Z

@olegtk or @CyrusNajmabadi - do you remember how this works?

CyrusNajmabadi · 2018-01-20T22:39:31Z

Outlining/BlockStructure are all the same thing nowadays. It's all done through the StructureTaggerProvider, BlockSpans and IBlockTags.

As far as the language (i.e. F#) is concerned, it should just make BlockSpans with the appropriate bits on it saying what span it is for, and if it is collapsible or not. Then, these are converted to IBlockTags (by Roslyn), basically passing along the data without doing anything interesting with it (except making a parent/child relationship between IBlockTags). It's then up to the Editor (IIRC) to make the decisions about what to do with with all of this.

CyrusNajmabadi · 2018-01-20T22:45:54Z

I'm probably missing something. But i don't see anything in your code that actually hooks up your user facing options to actually changing these settings. Can you point out where you do it? Thanks!

cartermp

@CyrusNajmabadi See below for explanations

cartermp · 2018-01-20T23:03:01Z

vsintegration/src/FSharp.Editor/Options/EditorOptions.fs

@@ -42,6 +41,11 @@ type LanguageServicePerformanceOptions =
    { EnableInMemoryCrossProjectReferences: bool
      ProjectCheckCacheSize: int }

+[<CLIMutable>]
+type AdvancedOptions =


This is the backing data for the new page I added

cartermp · 2018-01-20T23:03:48Z

vsintegration/src/FSharp.Editor/Options/EditorOptions.fs

+type AdvancedOptions =
+    { IsBlockStructureEnabled: bool 
+      IsOutliningEnabled: bool }
+
 [<Export(typeof<ISettings>)>]
 type internal Settings [<ImportingConstructor>](store: SettingsStore) =


This is where you access the data in our settings store, with default values listed below

cartermp · 2018-01-20T23:04:03Z

vsintegration/src/FSharp.Editor/Options/EditorOptions.fs

+
+    [<Guid(Guids.advancedSettingsPageIdSring)>]
+    type internal AdvancedSettingsOptionPage() =


Here's the wireup

cartermp · 2018-01-20T23:04:37Z

vsintegration/src/FSharp.Editor/Structure/BlockStructureService.fs

@@ -148,6 +148,8 @@ type internal FSharpBlockStructureService(checker: FSharpChecker, projectInfoMan

    override __.GetBlockStructureAsync(document, cancellationToken) : Task<BlockStructure> =
        asyncMaybe {
+            do! Option.guard Settings.Advanced.IsBlockStructureEnabled


This code basically means "only proceed to get the info for block structure if that setting has a value of true"

cartermp · 2018-01-20T23:18:58Z

And w.r.t this:

As far as the language (i.e. F#) is concerned, it should just make BlockSpans with the appropriate bits on it saying what span it is for, and if it is collapsible or not. Then, these are converted to IBlockTags (by Roslyn), basically passing along the data without doing anything interesting with it (except making a parent/child relationship between IBlockTags). It's then up to the Editor (IIRC) to make the decisions about what to do with with all of this.

That's what we do: https://github.com/Microsoft/visualfsharp/blob/master/vsintegration/src/FSharp.Editor/Structure/BlockStructureService.fs#L136

cartermp · 2018-01-20T23:21:59Z

Basically, what I'm looking for here is the ability to make the guidelines disappear but keep the collapsing nodes, like you can do in C# (just with one less degree of configurability), from the F# settings

cartermp · 2018-01-20T23:39:14Z

I figured it out! Just needed to set the block type to nonstructural based on the setting.

Guidelines off/Outlining on:

Guidelines on/Outling off:

You still need to reload the file for this to take effect, but that's due to how our language service works right now.

Now to bikeshed on the naming...

CyrusNajmabadi · 2018-01-21T07:03:10Z

Hey Phillip! Is there a reason you're not using these built in Roslyn options for outlining/structure-guides?

BlockStructureOptions.ShowOutliningForDeclarationLevelConstructs
BlockStructureOptions.ShowOutliningForCodeLevelConstructs
BlockStructureOptions.ShowOutliningForCommentsAndPreprocessorRegions
BlockStructureOptions.CollapseRegionsWhenCollapsingToDefinitions
BlockStructureOptions.ShowBlockStructureGuidesForDeclarationLevelConstructs
BlockStructureOptions.ShowBlockStructureGuidesForCodeLevelConstructs

If you use these then making any changes will automatically cause the file to refresh if you change them. Also, it will give you parity with how VB and C# expose these options:

CyrusNajmabadi · 2018-01-21T07:11:55Z

Note: if you use these options, you'll still have to have the following code somewhere (unless Roslyn moved it to a core location):

https://github.com/dotnet/roslyn/blob/823d9730aed86f9bba80e18b9888ca103dbee9ab/src/Features/Core/Portable/Structure/Syntax/AbstractBlockStructureProvider.cs#L77-L87

majocha · 2018-01-21T10:22:19Z

@CyrusNajmabadi IIRC there was a problem that we coulnd't easily provide UI for built-in Roslyn options. All of the persistence infrastructure was internal.

cartermp · 2018-01-21T16:57:30Z

@majocha is correct; all of the options defined in Roslyn are unavailable to us (as are the providers). That would all have to move for us to take advantage of them.

I see your PR on Roslyn would allow us to basically add our own duplicate options and then get the same functionality. Would we have to change anything w.r.t how we construct a BlockSpan?

CyrusNajmabadi · 2018-01-21T20:19:02Z

@majocha is correct; all of the options defined in Roslyn are unavailable to us (as are the providers). That would all have to move for us to take advantage of them.

Sorry, can you explain this to me? It's unclear to me why the options would be unavailable. They're at the Feature layer, and don't seem to have any C# or VB specific code AFAICT.

All of the persistence infrastructure was internal.

I don't quite understand this. To persist, all you'd have to do is:

workspace.Options = workspace.Options.WithChangedOption(TheOption, theNewValue);

CyrusNajmabadi · 2018-01-21T20:19:59Z

Would we have to change anything w.r.t how we construct a BlockSpan?

If you used our options, you wouldn't have to contruct blockspans differently. If you didn't use our options, you'd have to duplicate that logic, setting NonStructural and IsCollapsible in a similar manner to how my code does it.

cartermp · 2018-01-21T21:54:21Z

@CyrusNajmabadi I don't quite grasp how options are done in Roslyn. To do it "right" and get access to everything we need from Roslyn, would we need to export a provider like this? If so, we can't do that - IOptionProvider and ExportOptionProvider are inaccessible.

CyrusNajmabadi · 2018-01-21T22:44:25Z

I don't quite grasp how options are done in Roslyn.

Understood. They can be a bit difficult to understand. And we haven't made them fully functional for use outside of C#/VB. That said, they are still very usable for some purposes, including this one (i think).

To do it "right" and get access to everything we need from Roslyn, would we need to export a provider like this? If so, we can't do that - IOptionProvider and ExportOptionProvider are inaccessible.

Nope. That's only if you're defining new options. I'm not asking you to do that. Instead, i'm just saying: Use the options we've already defined in Roslyn for this feature. i.e. the 6 options i listed above.

You can expose these options in your UI however you want (for example, having 6 fine-grained checkboxes, or maybe just 2 coarse grained checkboxes). You can populate your checkboxes by reading these options. When the user interacts with your UI, just set these options on the workspace accordingly.

To read the value of the option all you need to do is:

var optionVal = workspace.Options.GetOption(BlockStructureOptions.TheOption, FSharpLanguageName);

To set the value, all you need to do is:

workspace.Options = workspace.Options.WithChangedOption(BlockStructureOptions.TheOption, FSharpLanguageName, newVal);

Once you do that (and #24362 goes in), you can just make BlockSpans simply for all your language constructs. Roslyn will then check the options against those BlockSpans and will do the right thing vis-as-vis collapsing and the structure type.

Roslyn should also take care of persisting these options for you (including working with roaming profiles).

CyrusNajmabadi · 2018-01-21T22:47:34Z

Ok. So this should def work (once my PR goes in). Here's an example of where you're interacting with Roslyn options already today:

https://github.com/Microsoft/visualfsharp/blob/5a12cde14c18646380a05371297e38e74e96bc18/vsintegration/src/FSharp.Editor/Navigation/GoToDefinitionService.fs#L257-L261

And:

https://github.com/Microsoft/visualfsharp/blob/5a12cde14c18646380a05371297e38e74e96bc18/vsintegration/src/FSharp.Editor/LanguageService/LanguageService.fs#L374-L377

As you can see, you can read and change existing Roslyn options without any problems :)

CyrusNajmabadi · 2018-01-21T23:01:47Z

If so, we can't do that - IOptionProvider and ExportOptionProvider are inaccessible.

BTW: i would def open an issue (if it isn't already there) for options to be fully usable by all roslyn-languages. They're great. An easy way to represent user configuration, abstracting away from pesky things like "is the option in VS, the registry, .editorconfig, or somewhere else?" They're tied tightly into the workspace model, so they're easy to check from any service that has access to a Document.

They're also normally very easy to create and export. ideally they could be how you do all your options yourself.

majocha · 2018-01-21T23:12:44Z

@cartermp I forgot and @CyrusNajmabadi is absolutely right. It were the custom defined options that were the problem.

@CyrusNajmabadi that would be so awesome to have.

cartermp · 2018-01-22T00:20:14Z

@CyrusNajmabadi Thanks! That makes sense. I'll create a tracking issue here to address Structure options once your PR is on and remove the code I wrote here to set the BlockType.

I'll also open an issue on Roslyn (if it's not already there) for opening up those APIs. There are a few things we do that are F#-specific. Say we were to use this Roslyn mechanism, and they don't have a corresponding PerLanguageOption. Would the course of action be to create one on the Roslyn side?

CyrusNajmabadi · 2018-01-22T00:28:13Z

There are a few things we do that are F#-specific. Say we were to use this Roslyn mechanism, and they don't have a corresponding PerLanguageOption. Would the course of action be to create one on the Roslyn side?

Depends on the option. If it's something you want the core rolsyn code to do something differently for (i.e. you want to customize core completion behavior), then you'd have to put inside Roslyn. But if it's about an option that only you F# wants to set and only F# code will read, then you can just define and export that option from your own dlls.

If you have questions, it might be helpful to have specific examples you're thinking about.

CyrusNajmabadi · 2018-01-22T00:29:50Z

For example:

F# wanted to turn off 'blocking for completion items'. That affected core roslyn behavior, so we had to add that option in Roslyn (including the code in the core completion service to respect it).
C# added a bunch of options around using => members. But these are totally specific to C#, so those options are defined in the C# layer, and only checked in the C# layer.

Does that make sense?

cartermp · 2018-01-22T01:48:56Z

Makes sense!

cartermp · 2018-03-01T17:48:12Z

@KevinRansom rebased and tested in VS again; still works so this is good to merge for now.

@CyrusNajmabadi Once this is merged, I'll file a tracking issue to replace code here with what you've opened up here. Since 15.7 is starting to get close, it's unclear if we could do this with your work in time for the release, hence the strategy to merge now.

cartermp · 2018-03-01T17:48:33Z

Arg FCS build failed: WARNING: Unable to find version '14.3.0' of package 'Microsoft.Build.Framework'.

cartermp · 2018-03-01T19:50:38Z

@dotnet-bot test Ubuntu16.04 Release_fcs Build

* Toggles for outlining and structured guidelines * Cleanup * Don't remove outlining alongside block structure lines and better names * fix up rebase;

cartermp commented Jan 20, 2018

View reviewed changes

CyrusNajmabadi mentioned this pull request Jan 21, 2018

Move checking of structure-options to a common location (so it will be picked up by all roslyn languages). dotnet/roslyn#24362

Merged

cartermp mentioned this pull request Jan 25, 2018

Odd random vertical line in Visual Studio #4266

Closed

cartermp added 4 commits March 1, 2018 09:26

Toggles for outlining and structured guidelines

b2d80fa

Cleanup

3072f28

Don't remove outlining alongside block structure lines and better names

80f21dd

fix up rebase;

3cdb7cf

cartermp added this to the 15.7 milestone Mar 6, 2018

Merge branch 'master' into outlining-structure-toggle

a0eef59

KevinRansom merged commit 47e74f5 into dotnet:master Mar 9, 2018

cartermp deleted the outlining-structure-toggle branch March 9, 2018 22:00

cartermp mentioned this pull request Mar 9, 2018

Add a user setting to disable code folding #3681

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Toggles for outlining and structured guidelines #4227

Toggles for outlining and structured guidelines #4227

cartermp commented Jan 20, 2018 •

edited

Loading

cartermp commented Jan 20, 2018 •

edited

Loading

Pilchie commented Jan 20, 2018

CyrusNajmabadi commented Jan 20, 2018 •

edited

Loading

CyrusNajmabadi commented Jan 20, 2018

cartermp left a comment

cartermp Jan 20, 2018

cartermp Jan 20, 2018

cartermp Jan 20, 2018

cartermp Jan 20, 2018

cartermp commented Jan 20, 2018

cartermp commented Jan 20, 2018 •

edited

Loading

cartermp commented Jan 20, 2018 •

edited

Loading

CyrusNajmabadi commented Jan 21, 2018 •

edited

Loading

CyrusNajmabadi commented Jan 21, 2018

majocha commented Jan 21, 2018

cartermp commented Jan 21, 2018

CyrusNajmabadi commented Jan 21, 2018

CyrusNajmabadi commented Jan 21, 2018

cartermp commented Jan 21, 2018 •

edited

Loading

CyrusNajmabadi commented Jan 21, 2018 •

edited

Loading

CyrusNajmabadi commented Jan 21, 2018 •

edited

Loading

CyrusNajmabadi commented Jan 21, 2018

majocha commented Jan 21, 2018 •

edited

Loading

cartermp commented Jan 22, 2018

CyrusNajmabadi commented Jan 22, 2018

CyrusNajmabadi commented Jan 22, 2018

cartermp commented Jan 22, 2018

cartermp commented Mar 1, 2018

cartermp commented Mar 1, 2018

cartermp commented Mar 1, 2018


		[<Guid(Guids.advancedSettingsPageIdSring)>]
		type internal AdvancedSettingsOptionPage() =

Toggles for outlining and structured guidelines #4227

Toggles for outlining and structured guidelines #4227

Conversation

cartermp commented Jan 20, 2018 • edited Loading

cartermp commented Jan 20, 2018 • edited Loading

Pilchie commented Jan 20, 2018

CyrusNajmabadi commented Jan 20, 2018 • edited Loading

CyrusNajmabadi commented Jan 20, 2018

cartermp left a comment

Choose a reason for hiding this comment

cartermp Jan 20, 2018

Choose a reason for hiding this comment

cartermp Jan 20, 2018

Choose a reason for hiding this comment

cartermp Jan 20, 2018

Choose a reason for hiding this comment

cartermp Jan 20, 2018

Choose a reason for hiding this comment

cartermp commented Jan 20, 2018

cartermp commented Jan 20, 2018 • edited Loading

cartermp commented Jan 20, 2018 • edited Loading

CyrusNajmabadi commented Jan 21, 2018 • edited Loading

CyrusNajmabadi commented Jan 21, 2018

majocha commented Jan 21, 2018

cartermp commented Jan 21, 2018

CyrusNajmabadi commented Jan 21, 2018

CyrusNajmabadi commented Jan 21, 2018

cartermp commented Jan 21, 2018 • edited Loading

CyrusNajmabadi commented Jan 21, 2018 • edited Loading

CyrusNajmabadi commented Jan 21, 2018 • edited Loading

CyrusNajmabadi commented Jan 21, 2018

majocha commented Jan 21, 2018 • edited Loading

cartermp commented Jan 22, 2018

CyrusNajmabadi commented Jan 22, 2018

CyrusNajmabadi commented Jan 22, 2018

cartermp commented Jan 22, 2018

cartermp commented Mar 1, 2018

cartermp commented Mar 1, 2018

cartermp commented Mar 1, 2018

cartermp commented Jan 20, 2018 •

edited

Loading

cartermp commented Jan 20, 2018 •

edited

Loading

CyrusNajmabadi commented Jan 20, 2018 •

edited

Loading

cartermp commented Jan 20, 2018 •

edited

Loading

cartermp commented Jan 20, 2018 •

edited

Loading

CyrusNajmabadi commented Jan 21, 2018 •

edited

Loading

cartermp commented Jan 21, 2018 •

edited

Loading

CyrusNajmabadi commented Jan 21, 2018 •

edited

Loading

CyrusNajmabadi commented Jan 21, 2018 •

edited

Loading

majocha commented Jan 21, 2018 •

edited

Loading