-
Notifications
You must be signed in to change notification settings - Fork 118
Rewrote whitelist code for PlayerProfile #533
base: master
Are you sure you want to change the base?
Conversation
|
The fix extends to a larger problem: The player profile is incomplete. What would ideally occur is instead of working off of name, the UUID is still used but an OfflinePlayer is spun up using that player profile as it's backing object (not sure if this can be done right now though). This would make Glowkit unchanged and make this so it's not all names. |
|
Mind if I change this to WIP and start looking at that? I was looking at that initially but it's a bit of a mess. |
|
WIP is generally only used if you are about to radically change the scope of your PR. "Needs changes" is arguably okay for now. |
ae8bf9d to
0ec5f95
Compare
0ec5f95 to
8917e2e
Compare
8917e2e to
ed0c7fb
Compare
|
@m3rcuriel The large amount of changes is not quite what I meant when talking to you on IRC about this. Instead of rewriting OfflinePlayer to comply with a PlayerProfile, write it to support the PlayerProfile. This means a new constructor and other applicable changes need to be made to avoid breaking everything on the planet. The new constructor would be used where applicable leaving the other two to be used where they are needed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be private
b81e452 to
a124a08
Compare
|
@m3rcuriel this is no longer mergeable, please resolve the issue as soon as possible, thanks! |
|
@m3rcuriel Once again unmergeable :( |
|
@m3rcuriel This pull request is considered unmergeable as of yesterday. Please correct the issue for further review (also please ping me once it is mergeable so that I can continue to review it). Thanks. |
|
Yeah. Sorry, I'll get it done Wednesday. Busy week. |
|
@m3rcuriel Wednesday has past :( |
|
@m3rcuriel This pull request is not mergeable. Please resolve the issue for further review. |
|
@m3rcuriel This is a final warning: Your pull request is not mergable. Please resolve the issue for further review. |
|
@turt2live So sorry - jumped right back in. I lost my IntelliJ liscense and this PR slipped through the cracks. I found a slight problem with it, but I should have it done tomorrow crosses fingers |
|
@turt2live trying to keep you updated - there's a weird bug I haven't resolved but I'll work more tomorrow. |
|
@turt2live I'm not going to develop this any more until even a single PR is pulled into master. I hit a snag and couldn't fix it and I'm not willing to invest into a project not being actively updated. Feel free to close/remove this if you need to. I'll keep an eye though and come back if this project starts getting commits again. A good possibility for something to pull is my other PR, #521, which has been ready to pull for a good 8 months. I know you're waiting on SpaceManiac, and I understand that. Wish you luck. |
|
@m3rcuriel I've merged your PR into Glowstone++, can you elaborate on the weird bug? Glowstone++ might have it |
This PR fixes approximately half of issue #523. Players no longer show up as null in whitelist list, and non-existent players no longer print huge errors.