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

Cleanup chat boxing #3

Open
wants to merge 1 commit into
base: 0.2.1
Choose a base branch
from
Open

Cleanup chat boxing #3

wants to merge 1 commit into from

Conversation

Twister915
Copy link
Owner

@Twister915 Twister915 commented Jan 12, 2021

The Chat type is defined to have a Vec<BoxedChat> as a member (indirectly, through BaseChatComponent type) because originally I thought the indirection was necessary in Vec type. As it turns out, I didn't know Rust all that well when I wrote this code, and I realize now that we don't need the indirection in this case.

I left the BoxedChat alias and the .boxed() method on the Chat type because indirection is required in one case (the only associated value for ChatHoverEvent::ShowText must be a Chat type).

Review Notes:

  • Are we OK to make this sort of change without bumping to 0.3.0? I don't think this'll break any usages of the library.

@Twister915 Twister915 added this to the 0.2.1 milestone Jan 12, 2021
@Twister915 Twister915 mentioned this pull request Jan 12, 2021
2 tasks
Copy link
Collaborator

@Ichbinjoe Ichbinjoe left a comment

Choose a reason for hiding this comment

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

I think you are right in asserting that this shouldn't break most occurrences of this library (due to auto-dereferencing), but this will break anyone trying to unwrap the box (because spoiler, no box to unwrap).

Approving because I'm going to assume you are the only real user of this library, and if you aren't, sorry to the person who is using this as well as doing weird boxy things with their chat.

@jaecam jaecam self-requested a review January 16, 2021 11:59
Copy link
Collaborator

@jaecam jaecam left a comment

Choose a reason for hiding this comment

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

Yeah, the indirection is required for that hover event enum variant.
Not sure the BoxedChat alias is adding much value, but I can mostly get behind the boxed method from a code style POV. May want to also consider moving the point of indirection (which field is boxed), to increase cache locality (though there's currently no use of these types in craftio as far as I can see, so such an optimization is likely premature.)

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.

3 participants