DOCS: Update Keyboard documentation #2069

ritamerkl · 2024-12-03T18:08:28Z

Description

Updates the Keyboard documentation.

Testing status & QA

No testing.

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.

jfreire-unity

The code examples are not compiling if you copy paste them into an empty script.

Also, I'm not sure if the protected methods would need a proper example (like OnAdded).

Unless users could use them as well out of the box. Maybe they can and I would just need to see if the code example compiles.

Packages/com.unity.inputsystem/InputSystem/Controls/InputControl.cs

Packages/com.unity.inputsystem/InputSystem/Devices/InputDevice.cs

jfreire-unity · 2024-12-04T11:18:54Z

Packages/com.unity.inputsystem/InputSystem/Devices/Keyboard.cs

@@ -1006,6 +1034,25 @@ public event Action<IMECompositionString> onIMECompositionChange
        /// See <see cref="Keyboard.SetIMECursorPosition"/>, <see cref="Keyboard.onIMECompositionChange"/>,
        /// <see cref="Keyboard.imeSelected"/> for more IME settings and data.
        /// </remarks>
+        /// <param name="enabled">
+        ///     The new IME composition enabled state. True to enable the IME, false to disable it.


Should "the new" be removed and instead just use "The IME composition..."

Also I believe true and false when referencing keywords or identifiers I believe

fixed in 8ad97b6

jfreire-unity · 2024-12-04T11:24:57Z

Packages/com.unity.inputsystem/InputSystem/Devices/Keyboard.cs

@@ -1971,7 +2038,7 @@ public string keyboardLayout
        /// True when IME composition is enabled.  Requires <see cref="Keyboard.SetIMEEnabled"/> to be called to enable IME, and the user to enable it at the OS level.
        /// </summary>
        /// <remarks>
-        ///
+        /// <see cref="ButtonControl"/> representing a combined left and right alt key.


Do we need this remark? I don't quite understand why this is relevant for imeSelected?

fixed in 8ad97b6

jfreire-unity · 2024-12-04T11:25:10Z

Packages/com.unity.inputsystem/InputSystem/Devices/Keyboard.cs

@@ -1985,6 +2052,7 @@ public string keyboardLayout
        /// <param name="key">Key code of key control to return.</param>
        /// <exception cref="ArgumentOutOfRangeException">The given <paramref cref="key"/> is not valid.</exception>
        /// <remarks>
+        /// <see cref="KeyControl"/> representing a combined left and right alt key.


Same as above comment

fixed in 8ad97b6
I don't know how that slipped in -Thanks

ekcoh · 2024-12-04T14:45:47Z

Packages/com.unity.inputsystem/InputSystem/Devices/Keyboard.cs

@@ -32,9 +32,8 @@ namespace UnityEngine.InputSystem.LowLevel
    public unsafe struct KeyboardState : IInputStateTypeInfo
    {
        /// <summary>
-        /// Memory format tag for KeybboardState.
+        /// Memory format tag for KeybboardState. Returns "KEYS".


fixed in 389cce9

ekcoh · 2024-12-04T14:46:28Z

Packages/com.unity.inputsystem/InputSystem/Devices/Keyboard.cs

    [InputControlLayout(stateType = typeof(KeyboardState), isGenericTypeOfDevice = true)]
    public class Keyboard : InputDevice, ITextInputReceiver
    {
        /// <summary>
        /// Total number of key controls on a keyboard, i.e. the number of controls
        /// in <see cref="allKeys"/>.
        /// </summary>
-        /// <value>Total number of key controls.</value>
+        /// <remarks>The integer value represents the total number of key controls.</remarks>


Remove remarks? Its the same info as in summary it seems

fixed in 8ad97b6

ekcoh · 2024-12-04T14:48:11Z

Packages/com.unity.inputsystem/InputSystem/Devices/Keyboard.cs

@@ -1021,6 +1068,26 @@ public void SetIMEEnabled(bool enabled)
        ///
        /// See <see cref="Keyboard.SetIMEEnabled"/> for turning IME on/off
        /// </remarks>
+        /// <param name="position">


Why new lines in param?

fixed in 8ad97b6

ekcoh · 2024-12-04T14:49:02Z

Packages/com.unity.inputsystem/InputSystem/Devices/Keyboard.cs

+        ///     void Update ()
+        ///     {
+        ///         // Set the IME cursor position to the mouse position
+        ///         var x = Input.GetAxis("Mouse X");


Why is this using Input Manager? Wouldn't it make sense to use input system instead?

Ups, old habit - exactly me should know better now after working on the deprecation for so long :D - fixed in 95c3c3e

ekcoh · 2024-12-04T14:50:10Z

Packages/com.unity.inputsystem/InputSystem/Devices/Keyboard.cs

        /// <remarks>
+        /// <see cref="KeyControl"/> representing the numpad minus key.


needs a paragraph after it?

fixed in 8ad97b6

ekcoh · 2024-12-04T14:50:22Z

Packages/com.unity.inputsystem/InputSystem/Devices/Keyboard.cs

        /// <remarks>
+        /// <see cref="KeyControl"/> representing the numpad plus key.


Needs a paragraph after it?

fixed in 8ad97b6

ekcoh · 2024-12-04T14:51:44Z

Packages/com.unity.inputsystem/InputSystem/Devices/Keyboard.cs

@@ -2009,21 +2077,33 @@ public KeyControl this[Key key]
        public static Keyboard current { get; private set; }

        /// <summary>
-        /// Make the keyboard the current keyboard (i.e. <see cref="current"/>).
+        /// Make a keyboard the current active keyboard.


Could we do inheritDoc on this since defined in base? It should not change meaning in base classes right?

I think it's neat to have a example code for this, but I added the inherited doc above for more information: 58eeef6

ekcoh · 2024-12-04T14:52:25Z

Packages/com.unity.inputsystem/InputSystem/Devices/Keyboard.cs

@@ -2182,11 +2280,23 @@ protected override void RefreshConfiguration()
        /// <summary>
        /// Called when text input on the keyboard is received.
        /// </summary>
-        /// <param name="character">Character that has been entered.</param>
+        /// <param name="character">Char value that represents the character that has been entered.</param>


char value.... ?

fixed in 8ad97b6

ekcoh · 2024-12-04T14:53:22Z

Would recommend fixing issues with CI and comments and then re request review

jfreire-unity

Nice work and nice examples! 🎉
Approving it already but added some comments/suggestions. Feel free to follow my suggestions or not (some are more nitpicks than others).

Packages/com.unity.inputsystem/InputSystem/Devices/InputDevice.cs

Packages/com.unity.inputsystem/InputSystem/Devices/Keyboard.cs

Co-authored-by: João Freire <[email protected]>

ekcoh

LGTM, congrats to getting through the CI errors on this larger doc PR!

Packages/com.unity.inputsystem/InputSystem/Controls/InputControl.cs

Packages/com.unity.inputsystem/InputSystem/Devices/InputDevice.cs

Packages/com.unity.inputsystem/InputSystem/Devices/Keyboard.cs

Co-authored-by: Ben Pitt <[email protected]>

update keyboard documentation

a0ad998

ritamerkl added the DocsQualityWeek2024 Temporary label for docs week label Dec 3, 2024

ritamerkl requested a review from ekcoh December 3, 2024 18:08

ritamerkl changed the title ~~DOSC: Update Keyboard documentation~~ DOCS: Update Keyboard documentation Dec 3, 2024

jfreire-unity requested changes Dec 4, 2024

View reviewed changes

ekcoh reviewed Dec 4, 2024

View reviewed changes

ritamerkl added 15 commits December 5, 2024 12:26

fix xml format for MakeCurrent() example

0bddacb

fix code examples in InputControl and InputDevice

04a3e1b

added example for OnAdded too, uniformed the remarks section

6703c57

fixed doc examples in Keyboard.cs

4cfc8a6

remove input manager references from Keyboard.cs

95c3c3e

fix para section in code example

603a947

fixes on typo and <see.. /> usage

389cce9

inherit documentation for MakeCurrent

58eeef6

formatting small text adjustments

8ad97b6

added type reference for char

ec93f25

use <see> instead of <seealso>

e4cfd6a

doc fixes

229b655

remove code out of remarks section

99ca583

fixes for docs

e2d5fd2

fix CI failures

0edc347

ritamerkl requested a review from jfreire-unity December 6, 2024 10:27

ritamerkl requested a review from ekcoh December 6, 2024 10:27

jfreire-unity requested a review from duckets December 6, 2024 11:07

Merge branch 'develop' into docs-update-keyboard-documentation

fc574d2

jfreire-unity approved these changes Dec 6, 2024

View reviewed changes

ritamerkl and others added 5 commits December 6, 2024 15:04

small text fix

51c9865

Co-authored-by: João Freire <[email protected]>

phrasing fix

69612a3

Co-authored-by: João Freire <[email protected]>

Small phrasing fix

f6755a4

Co-authored-by: João Freire <[email protected]>

small addition on text for tab key

e107217

Co-authored-by: João Freire <[email protected]>

Formatting of text

9ea96b9

Co-authored-by: João Freire <[email protected]>

ekcoh approved these changes Dec 6, 2024

View reviewed changes

duckets requested changes Dec 6, 2024

View reviewed changes

ritamerkl and others added 3 commits December 8, 2024 17:20

Input Control update text

62ca3ba

Co-authored-by: Ben Pitt <[email protected]>

Input Device update text

a2619bc

Co-authored-by: Ben Pitt <[email protected]>

Update text for IME composition event

6342ca8

Co-authored-by: Ben Pitt <[email protected]>

ritamerkl merged commit 790d9cc into develop Dec 9, 2024
77 checks passed

ritamerkl deleted the docs-update-keyboard-documentation branch December 9, 2024 10:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DOCS: Update Keyboard documentation #2069

DOCS: Update Keyboard documentation #2069

ritamerkl commented Dec 3, 2024

jfreire-unity left a comment

jfreire-unity Dec 4, 2024

ekcoh Dec 4, 2024

ekcoh Dec 4, 2024

ritamerkl Dec 5, 2024

jfreire-unity Dec 4, 2024

ritamerkl Dec 5, 2024

jfreire-unity Dec 4, 2024

ritamerkl Dec 5, 2024

ekcoh Dec 4, 2024

ritamerkl Dec 5, 2024

ekcoh Dec 4, 2024

ritamerkl Dec 5, 2024

ekcoh Dec 4, 2024

ritamerkl Dec 5, 2024

ekcoh Dec 4, 2024

ritamerkl Dec 5, 2024

ekcoh Dec 4, 2024

ritamerkl Dec 5, 2024

ekcoh Dec 4, 2024

ritamerkl Dec 5, 2024

ekcoh Dec 4, 2024

ritamerkl Dec 5, 2024

ekcoh Dec 4, 2024

ritamerkl Dec 5, 2024

ekcoh commented Dec 4, 2024

jfreire-unity left a comment •

edited

Loading

ekcoh left a comment

		/// <remarks>
		/// <see cref="KeyControl"/> representing the numpad minus key.

DOCS: Update Keyboard documentation #2069

DOCS: Update Keyboard documentation #2069

Conversation

ritamerkl commented Dec 3, 2024

Description

Testing status & QA

Overall Product Risks

Comments to reviewers

Checklist

jfreire-unity left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekcoh commented Dec 4, 2024

jfreire-unity left a comment • edited Loading

Choose a reason for hiding this comment

ekcoh left a comment

Choose a reason for hiding this comment

jfreire-unity left a comment •

edited

Loading