Skip to content

Conversation

ekcoh
Copy link
Collaborator

@ekcoh ekcoh commented Dec 9, 2024

Description

Improvements to UnityEngine.InputSystem.EnhancedTouch.Touch based on phase 2 analysis.

-1 for having related links (not recommended for this API type) each for activeFingers, activeTouches, began, delta, displayIndex, ended, finger, fingers, inProgress, isTap, phase, pressure, radius, screenPosition, startScreenPosition, startTime, tapCount, touchId
-1 for insufficient summary length (threshold is 35) each for history, phase, radius
-1 for using the value tag (not recommended; use the remarks tag instead) each for activeFingers, activeTouches, began, delta, displayIndex, ended, finger, fingers, history, inProgress, isInProgress, isTap, maxHistoryLengthPerFinger, phase, pressure, radius, screen, screenPosition, screens, startScreenPosition, startTime, tapCount, time, touchId, valid
-2 for no code samples (required for this API type) each for onFingerDown, onFingerMove, onFingerUp
-2 for no remarks (required for this API type) each for onFingerDown, onFingerMove, onFingerUp
-3 for no related links (required for this API type) on Touch
-4 for missing parameter description for the 'obj' parameter on Equals(object)
-4 for missing parameter description for the 'other' parameter on Equals(Touch)
-4 for no code samples (required for this API type) each for Equals(object), Equals(Touch), GetHashCode(), ToString()
-4 for no remarks (required for this API type) each for Equals(object), Equals(Touch), GetHashCode(), ToString()
-4 for no return values info each for Equals(object), Equals(Touch), GetHashCode(), ToString()
-4 for no summary (required for all apitypes) each for Equals(object), Equals(Touch), GetHashCode(), ToString()
-6 for no code samples (required for this API type) on Touch

Testing status & QA

Please describe the testing already done by you and what testing you request/recommend QA to execute. If you used or created any testing project please link them here too for QA.

Overall Product Risks

Please rate the potential complexity and halo effect from low to high for the reviewers. Note down potential risks to specific Editor branches if any.

  • Complexity:
  • Halo Effect:

Comments to reviewers

Please describe any additional information such as what to focus on, or historical info for the reviewers.

Checklist

Before review:

  • Changelog entry added.
    • Explains the change in Changed, Fixed, Added sections.
    • For API change contains an example snippet and/or migration example.
    • JIRA ticket linked, example (case %%). If it is a private issue, just add the case ID without a link.
    • Jira port for the next release set as "Resolved".
  • Tests added/changed, if applicable.
    • Functional tests Area_CanDoX, Area_CanDoX_EvenIfYIsTheCase, Area_WhenIDoX_AndYHappens_ThisIsTheResult.
    • Performance tests.
    • Integration tests.
  • Docs for new/changed API's.
    • Xmldoc cross references are set correctly.
    • Added explanation how the API works.
    • Usage code examples added.
    • The manual is updated, if needed.

During merge:

  • Commit message for squash-merge is prefixed with one of the list:
    • NEW: ___.
    • FIX: ___.
    • DOCS: ___.
    • CHANGE: ___.
    • RELEASE: 1.1.0-preview.3.

After merge:

  • Create forward/backward port if needed. If you are blocked from creating a forward port now please add a task to ISX-1444.

@ekcoh ekcoh added the DocsQualityWeek2024 Temporary label for docs week label Dec 9, 2024
@ekcoh ekcoh requested review from duckets and stefanunity December 11, 2024 13:35
@ekcoh ekcoh marked this pull request as ready for review December 11, 2024 13:35
@ekcoh ekcoh requested a review from lyndon-unity December 11, 2024 15:02
Copy link
Collaborator

@lyndon-unity lyndon-unity left a comment

Choose a reason for hiding this comment

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

Overall pretty good change. Added some optional feedback.

///
/// void Update()
/// {
/// foreach (var touch in Touch.activeTouches)
Copy link
Collaborator

Choose a reason for hiding this comment

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

personally I would have put braces around the content in the foreach loop

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Same here and I see the main example wasn't replaced either (must have accidentally reverted, so I replaced it in coming commit)

/// the buffer runs out of capacity, older touch entries will get reused. When this happens,
/// existing <c>Touch</c> instances referring to the record become invalid.
///
/// </para>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this need/benefit from the separate para sections. I've generally put it into a single one.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have gotten xmldoc error any time I mix non-para sections with para sections hence why I have used them since the error message say do not mix para with inline content. Maybe there is something I do not understand about them?

public bool began => phase == TouchPhase.Began;

/// <summary>
/// Whether the touch is currently in progress, i.e. whether <see cref="phase"/> is either
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd be tempted to split some of this into 'remarks' so summary is a bit shorter.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think it's fine, it's pretty short when rendered.

/// </para>
/// </remarks>
/// <seealso cref="tapCount"/>
/// <seealso cref="InputSettings.defaultTapTime"/>
Copy link
Collaborator

Choose a reason for hiding this comment

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

might be worth including defaultTapTime in the remarks too

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It already is? See first line in remarks.

@ekcoh ekcoh merged commit 788d2cc into develop Dec 12, 2024
77 checks passed
@ekcoh ekcoh deleted the docs-quality-week-2024-touch branch December 12, 2024 14:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

DocsQualityWeek2024 Temporary label for docs week

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants