added "hand", "offhand" and "pos1" to the pattern suggestion #2869
Conversation
me4502
left a comment
me4502
left a comment
There was a problem hiding this comment.
Thanks for this, I'm just a little confused about a few parts of this so I've left a few comments :)
|
|
||
| var actor = context.getActor(); | ||
| if (actor instanceof Player player) { | ||
| try { |
There was a problem hiding this comment.
I'm a little confused by these try statements wrapping it, and why you're calling the getBlockInHand methods here? if it's a player, you should be able to always add hand and offhand to this list.
For pos1, it's realistically better to just always provide it as well. It's not only players who can have selections, so pos1 is always a valid selection. It's better to suggest it and have it give the "need to select a region" message upon running it, as that informs the user that it's actually possible.
There was a problem hiding this comment.
You're right I wanted to remove it if the player is holding an item rather than a block, but letting it every time informs the user that it's actually possible as you said, so we could let it being suggested every time
...edit-core/src/main/java/com/sk89q/worldedit/extension/factory/parser/DefaultBlockParser.java
Outdated
Show resolved
Hide resolved
|
just like so then ? |
me4502
left a comment
me4502
left a comment
There was a problem hiding this comment.
Thanks, this looks good to me. I'll just leave it overnight before merging in case others want to review also :)
| if (idx < 0) { | ||
| return SuggestionHelper.getNamespacedRegistrySuggestions(BlockType.REGISTRY, input); | ||
| return Stream.concat( | ||
| Stream.of("hand", "offhand", "pos1"), |
There was a problem hiding this comment.
Shouldn't hand/offhand only be suggested if the actor is a player? This can be called in a console context so I don't think you can assume it's a player.
There was a problem hiding this comment.
you're right I did check if the actor is a player but then removed it in 2157820 I shouldn't have done so
|
Should I also check if 
|
| import java.util.HashMap; | ||
| import java.util.Locale; | ||
| import java.util.Map; | ||
| import java.util.*; |
There was a problem hiding this comment.
This is currently breaking the build, can you please revert the change to the imports?
| import java.util.Locale; | ||
| import java.util.Map; | ||
| import java.util.stream.Stream; | ||
| import java.util.Locale; |
There was a problem hiding this comment.
It looks like these imports are still out of order- Locale shouldn't have moved; it's probably worth ensuring that it builds locally and making the fixes until it actually builds first
3 participants
resolves #2868