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

Enable new inline rename by default #62992

Merged
merged 12 commits into from
Aug 25, 2022
Merged

Conversation

ryzngard
Copy link
Contributor

@ryzngard ryzngard commented Jul 27, 2022

No description provided.

@ryzngard ryzngard requested review from a team as code owners July 27, 2022 20:42
@JoeRobich
Copy link
Member

Should there also be a VB.NET setting for this?

@ryzngard
Copy link
Contributor Author

Should there also be a VB.NET setting for this?

This is where we get a little tricky. I can duplicate in VB, but it only applies globally not per language. I guess we could do per language...

Unfortunately we don't have a ".NET" node to put stuff like this in :(

@@ -83,6 +89,16 @@
<StackPanel>
<CheckBox x:Name="Rename_asynchronously_exerimental"
Content="{x:Static local:AdvancedOptionPageStrings.Option_Rename_asynchronously_experimental}" />

<Label x:Name="Rename_UI_setting_label" Content="{x:Static local:AdvancedOptionPageStrings.Where_should_the_rename_ui_be_shown}" />
Copy link
Member

Choose a reason for hiding this comment

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

a bit odd that ui is lowercased.

Copy link
Member

Choose a reason for hiding this comment

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

maybe just Rename textbox location: inline/in-top-right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's not completely accurate though. It's a bit more nuanced than that. At most, inline rename options or dashboard them?

Copy link
Contributor

@davidwengier davidwengier left a comment

Choose a reason for hiding this comment

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

Can you put a screenshot in of the options, with the dropdown dropped? Seems like the options are a bit too wordy, and repeat "UI" a lot, but hard to tell for sure from just resource strings

Copy link
Member

@Youssef1313 Youssef1313 left a comment

Choose a reason for hiding this comment

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

Should this be added to AutomationObject?

I'm concerned by this being added both to C#-specific and VB-specific options page but have a single value for both (ie, if user modified it in one language, it gets automatically the same value in the other).

Are there other similar options that are single-valued for both languages and exposed in Tools → Options?

Ideally there should be some sort of a "common" page. @mavasani What do you think?

@davidwengier
Copy link
Contributor

Are there other similar options that are single-valued for both languages and exposed in Tools → Options?

Allow navigation to decompiled sources is for both languages, and only appears on the C# page, presumably because there is nowhere else to put it.

@ryzngard
Copy link
Contributor Author

Should this be added to AutomationObject?

I'm concerned by this being added both to C#-specific and VB-specific options page but have a single value for both (ie, if user modified it in one language, it gets automatically the same value in the other).

Are there other similar options that are single-valued for both languages and exposed in Tools → Options?

Ideally there should be some sort of a "common" page. @mavasani What do you think?

I haven't kept up with AutomationObject. Is there a doc I can look at to what needs to go there and why? Or just a short description

@Youssef1313
Copy link
Member

@ryzngard I don't know of docs about it, but basically you can observe the impact by:

  1. Tools → Import and Export Settings
  2. Follow with the dialog to export settings into a file
  3. Change the value of the option of interest to test
  4. Import the file back

When importing the file back, the old value should be seen.

@ryzngard ryzngard enabled auto-merge (squash) August 1, 2022 22:18
@JoeRobich
Copy link
Member

Are there other similar options that are single-valued for both languages and exposed in Tools → Options?

Editor Color Scheme falls in this bucket. Both pages bind to the same option (not perlanguage) and the OptionStore class ensures that changes are synced.

@mavasani
Copy link
Contributor

mavasani commented Aug 2, 2022

Should this be added to AutomationObject?

I'm concerned by this being added both to C#-specific and VB-specific options page but have a single value for both (ie, if user modified it in one language, it gets automatically the same value in the other).

Are there other similar options that are single-valued for both languages and exposed in Tools → Options?

Ideally there should be some sort of a "common" page. @mavasani What do you think?

I don't think there should be a common option page for these cases where the same option applies to both C# and VB and its value is kept in sync by the OptionStore infrastructure. That would be confusing to users who only work with a single language and want to look at all applicable options for the language. IMO common option pages should only be defined for language-agnostic options and options that apply to all languages supported in VS, and that is the core reason why Roslyn repo has no such common option page infrastructure, we only deal with C# and VB options. Common option page infrastructure is present in the VS platform layer which supports such language-agnostic options.

@ryzngard ryzngard merged commit 24d6e01 into dotnet:main Aug 25, 2022
@ghost ghost added this to the Next milestone Aug 25, 2022
@ryzngard ryzngard deleted the rename/enable_default branch August 25, 2022 00:54
@dibarbet dibarbet modified the milestones: Next, 17.4 P2 Sep 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants