Skip to content

IME Events are now completely variable sized. #1120

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

Closed
wants to merge 10 commits into from

Conversation

StayTalm
Copy link
Contributor

@StayTalm StayTalm commented Apr 7, 2020

IME events now have no cap. This handles a new event that comes from native Unity that has a variable sized IME event. Do not merge until new event is available.

-Updated IME test to still work.

@StayTalm StayTalm requested a review from Rene-Damm April 7, 2020 00:46
Copy link
Contributor

@Rene-Damm Rene-Damm left a comment

Choose a reason for hiding this comment

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

No tests. Some comments RE API docs.

unsafe
{
int sizeInBytes = (InputEvent.kBaseEventSize + sizeof(int) + sizeof(char)) + (sizeof(char) * str.Length);
IntPtr evtPtr = Marshal.AllocHGlobal(sizeInBytes);
Copy link
Contributor

Choose a reason for hiding this comment

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

Use Unity NativeArray (with Allocator.Temp) and Unity Memcpy instead of Marshal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not used NativeArray much, did an attempt, feel I could be missing an easier way to do it, but this passes the tests.

}

/// <summary>
/// Queues up an IME Composition Event.!-- This event is for unit testing and debug purposes, and is slow, due to the variable way that IME Composition events work.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is it slow?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed, I didn't like the AllocHGlobal and copying 1 character at a time. Had no good justification it was slow.

/// <summary>
/// Queues up an IME Composition Event.!-- This event is for unit testing and debug purposes, and is slow, due to the variable way that IME Composition events work.
/// </summary>
/// <param name="deviceId">The Id you'd like to send the composition event at.</param>
Copy link
Contributor

Choose a reason for hiding this comment

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

ID of the device (see <see cref="InputDevice.deviceId") to which the composition event should be sent to. Should be an <see cref="ITextInputReceiver"/> device. Will trigger <see cref="ITextInputReceiver.OnIMECompositionChanged"/> call when processed.

Overall, doc comments that don't really provide information and only restate what's already obvious from the code are really aggravating.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Took that segment and did a pass on some of the other doc elements around. Tried to add a bit of something to any of the docs, or remove the code comments entirely for things like GetEnumerator.

public int Count => size;
int m_Length;
/// <summary>
/// The number of characters in the current IME Composition
Copy link
Contributor

Choose a reason for hiding this comment

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

Punctuation. Sentences in API docs should be properly formed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added



/// <summary>
/// An Indexer into an individual character in the IME Composition. Will throw an out of range exception if the index is greater than the length of the composition.
Copy link
Contributor

Choose a reason for hiding this comment

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

Use <exception> tags to document exceptions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@StayTalm StayTalm requested a review from Rene-Damm May 11, 2020 05:02
@StayTalm
Copy link
Contributor Author

@renedamm should be good for re-review, moved to using Unity's unsafe & native array tools, improved the docs.

Added 1 test, and was already using the existing IME event test to verify that the composition events get received properly.

@unity-cla-assistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@jimon
Copy link
Contributor

jimon commented Jan 11, 2023

I'm recreating this work in #1628

@jimon jimon closed this Jan 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants