search

mkremins / fanciful

Fancy Bukkit message formatting
MIT License
95 stars 82 forks source link

Allow .text additions with first text already set #41

Closed ConnorStone closed 9 years ago

ConnorStone commented 10 years ago

Something was weird when i edited the last one, so here it is revised

MiniDigger commented 9 years ago

Why do you call then?

glen3b commented 9 years ago

Is this the same as the second commit of #37, as that's what it looks like. It looks good to me, but I only checked briefly.

EDIT: Yeah, I'd close this and reopen that one.

glen3b commented 9 years ago

@MiniDigger he calls the then function so that the text is not changed on the current element, but is appended as a new element to the chain.

MiniDigger commented 9 years ago

I think that would be misleading. If you call the method you expect to write the text. If there already is a text, you did something wrong and you should get a notification about that (the exception) If it just calls then, you may wonder why your text appears at the wrong place with wrong formatting etc.

glen3b commented 9 years ago

@MiniDigger I was documenting his code (and I sent a PR to him), and I agree. I found this documentation (which I think I wrote) on the FancyMessage class:

This class follows the builder pattern, allowing for method chaining. It is set up such that invocations of property-setting methods will affect the current editing component, and a call to then() or then(Object) will append a new editing component to the end of the message, optionally initializing it with text. Further property-setting method calls will affect that editing component.

Considering this, and looking through the current code, the text method is the only method that is not consistent with this doc. IMHO the best way to do this would be to not care if the current element has text, but just change the current text. For existing plugins, if they work, nothing would change in terms of usage, as the method currently throws an exception in that case. For new plugins, they would know that every method changes the current element and the current element only. The component would only change when they call then.

MiniDigger commented 9 years ago

That would make sense. I think you just need to remove https://github.com/mkremins/fanciful/blob/master/src/main/java/mkremins/fanciful/FancyMessage.java#L114-L116 and https://github.com/mkremins/fanciful/blob/master/src/main/java/mkremins/fanciful/FancyMessage.java#L124-L126. This would make the class consistent with the doc. I doesn't realy make sense to disallow the user to change the text but allow him to change the color, extras etc after the initial assignment.

glen3b commented 9 years ago

@MiniDigger I agree. I checked, and I wrote the doc after those methods were implemented. I clearly missed that, however I think that a change that does that would be good. I created PR #55 for this change.

MiniDigger commented 9 years ago

So this can be closed, right?

glen3b commented 9 years ago

I think so.

mkremins commented 9 years ago

Closing this in favor of #55.