Skip to content

Addendum to #5849#5882

Merged
RussKie merged 1 commit intodotnet:mainfrom
RussKie:fix_msg_names
Oct 6, 2021
Merged

Addendum to #5849#5882
RussKie merged 1 commit intodotnet:mainfrom
RussKie:fix_msg_names

Conversation

@RussKie
Copy link
Contributor

@RussKie RussKie commented Sep 30, 2021

  • Rename Message fields from '_Xyz' to 'XyzInternal`
  • Rename WINFORMSDEV0001 to WFDEV001 to align with existing diagnostic IDs
Microsoft Reviewers: Open in CodeFlow

@RussKie RussKie requested a review from a team as a code owner September 30, 2021 06:12
@ghost ghost assigned RussKie Sep 30, 2021
* Rename Message fields from '_Xyz' to 'XyzInternal`
* Rename WINFORMSDEV0001 to WFDEV001 to align with existing diagnostic IDs
// The first thing the ime does on a key it cares about is send a VK_PROCESSKEY, so we use
// that to sling focus to the grid.
if (m._WParam == VK_PROCESSKEY)
if (m.WParamInternal == VK_PROCESSKEY)
Copy link
Member

Choose a reason for hiding this comment

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

tbh, i did not like this naming convention either. We use lParam and wParam extensively across VS and .NET repos. why are they not good here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Having LParam and lParam in the same codebase would be very confusing. I proposed LParamInternal as this a) would align with HandleInternal and b) clearly indicate the internal purpose.

Copy link
Member

Choose a reason for hiding this comment

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

Having LParam and lParam in the same codebase

Can you elaborate on what do you mean by same code base here? In case of HandleInternal we have a public property Handle in the same class.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Message type has both public properties (e.g. LParam, etc.) and internal fields (e.g. _LParam, etc.). We are breaking our designer guidelines by exposing fields, which in this case can be justified, by doing so we need to fix the naming convention (LParamInternal instead _LParam), which is the intent of this PR.

Copy link
Member

@JeremyKuhne JeremyKuhne 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.

@dreddy-work dreddy-work added the ready-to-merge PRs that are ready to merge but worth notifying the internal team. label Oct 6, 2021
@RussKie RussKie merged commit 77b1fbc into dotnet:main Oct 6, 2021
@RussKie RussKie deleted the fix_msg_names branch October 6, 2021 23:22
@ghost ghost added this to the 7.0 alpha1 milestone Oct 6, 2021
@RussKie RussKie added code cleanup cleanup code for unused apis/properties/comments - no functional changes. and removed ready-to-merge PRs that are ready to merge but worth notifying the internal team. labels Oct 6, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jan 26, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

code cleanup cleanup code for unused apis/properties/comments - no functional changes.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants