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

Rename HotCommands and DocComment #5524

Merged
merged 2 commits into from
Aug 25, 2021
Merged

Conversation

JeremyKuhne
Copy link
Member

@JeremyKuhne JeremyKuhne commented Aug 25, 2021

Names were difficult to follow. Rename and match with existing public API documentation. Clean up IComPropertyBrowser (related).

Microsoft Reviewers: Open in CodeFlow

Names were difficult to follow. Rename and match with existing public API documentation. Clean up IComPropertyBrowser (related).
@JeremyKuhne JeremyKuhne requested a review from a team as a code owner August 25, 2021 00:03
@ghost ghost assigned JeremyKuhne Aug 25, 2021
Copy link
Member

@dreddy-work dreddy-work left a comment

Choose a reason for hiding this comment

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

Some methods names still pointing to old names.

RussKie
RussKie previously approved these changes Aug 25, 2021
Copy link
Member

@RussKie RussKie left a comment

Choose a reason for hiding this comment

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

👍 modulo @dreddy-work's comments

@JeremyKuhne
Copy link
Member Author

x86 debug failed with #4736

@JeremyKuhne
Copy link
Member Author

JeremyKuhne commented Aug 25, 2021

This time ToolStripItems_FontScaling failed on x64 debug.

Copy link
Member

@KlausLoeffelmann KlausLoeffelmann left a comment

Choose a reason for hiding this comment

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

Looks good.
I had some discussions with myself though (😄) if I liked DescriptionPane or HelpPane better. Since this is more from the internal Developer point of view and not an external API, I'm leaning to DescriptionPane to be honest.

/// Returns the value of the specified <paramref name="propertyID"/> from the element.
/// </summary>
/// <param name="propertyID">Identifier indicating the property to return</param>
/// <returns>Returns the requested value if supported or null if it is not.</returns>
Copy link
Member

Choose a reason for hiding this comment

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

Do we omit 'Returns'?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, missed a few.

/// Returns the element in the specified <paramref name="direction"/>.
/// </summary>
/// <param name="direction">Indicates the direction in which to navigate.</param>
/// <returns>Returns the element in the specified direction if it exists.</returns>
Copy link
Member

Choose a reason for hiding this comment

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

Do we omit 'Returns'?

@@ -6,30 +6,26 @@

namespace System.Windows.Forms.PropertyGridInternal
{
internal partial class DocComment
internal partial class HelpPane
Copy link
Member

@KlausLoeffelmann KlausLoeffelmann Aug 25, 2021

Choose a reason for hiding this comment

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

So...this is the pane that shows the text which is defined by the DescriptionAttribute of a Property. Wouldn't therefore DescriptionPane a more intuitive name? (Not really sure myself, just thinking out loud.)

Copy link
Member Author

Choose a reason for hiding this comment

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

I waffled. The other comments on the public API refer to this as "help" so I settled on that. PropertyGrid.HelpVisible.

@@ -8,10 +8,18 @@

namespace System.Windows.Forms.PropertyGridInternal
{
internal partial class DocComment : PropertyGrid.SnappableControl
/// <summary>
/// The help (description) pane optionally shown at the bottom of the <see cref="PropertyGrid"/>.
Copy link
Member

Choose a reason for hiding this comment

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

See... 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I put the parenthesis around it as we call it "description" in the UI. It felt better than description (help) given that we call this "help" in the public API. :)

@JeremyKuhne JeremyKuhne merged commit 83ea914 into dotnet:main Aug 25, 2021
@ghost ghost added this to the 7.0 alpha1 milestone Aug 25, 2021
@JeremyKuhne JeremyKuhne deleted the pgrid12 branch August 25, 2021 22:22
@JeremyKuhne
Copy link
Member Author

I'll catch my missed comments in the next PR (up shortly).

@ghost ghost locked as resolved and limited conversation to collaborators Jan 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants