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

Improve tooltip API greatly. Fixes issues #6 and #29. #30

Merged
merged 18 commits into from
Jun 11, 2014

Conversation

glen3b
Copy link
Contributor

@glen3b glen3b commented Jun 7, 2014

This PR improves the API for dealing with tooltips. The first change is not using items to represent multiple line tooltips, and instead joining a string with newlines. This prevents the ominous colon/comma bug.

The second change added by this PR is accepting fancy message as a tooltip. The code will validate that the fancy message does not contain tooltip or click data, then it will apply it as a formatted JSON tooltip to the message component.

The final change addressed by this PR is allowing multiple lines of formatted messages. This is implemented by cloning each message component, and adding a MessagePart representing \n in between each line of formatted text. As a result, one of the features I noted on #23 (cloneability of core objects) was implemented.

glen3b added 10 commits June 7, 2014 11:06
Works even better than mkremins#13 and mkremins#19.
Disclaimer: Not tested with complex textual values which include
ChatColor.toString()-marked colors.
This now uses JsonString to wrap a string as a Json value in an OOP way.
This commit adds formatted text by accepting a FancyMessage without
click or hover data (enforced upon set). The abstraction made possible
by JsonRepresentedObject allows easily writing formatted or unformatted
text to the stream, which is needed because two distinct types of data
are used here: raw string (not composite object), and formatted message
(composite object).
This commit implements that by iterating through message components of
each message, and after reaching the end of a message, appending a
newline message component.
@glen3b
Copy link
Contributor Author

glen3b commented Jun 8, 2014

Out of curiosity, why were you using items to represent multiline text? My testing shows that the newline character works just fine.

Fixed documentation as such. This is consistent with the API not having
any form of root characteristics, and is probably a good thing for
consistency's sake.
method accepting Iterable<FancyMessage>
This was broken because |Iterable<FancyMessage>| == |Iterable<String>|
(type erasures), so I changed the tooltip methods to be
tooltipUnformatted and tooltipFormatted
This is actually done by invalidating the internal state temporarily to
remove the default MessagePart created by the constructor. Doing so is
necessary to make sure there isn't a (GSON equivalent to a) NPE later
down the line.
This allows for more logical iteration through objects, and although the
method is explicitly marked as not for public API consumption, it is
still publicly visible.
This may not be the best option, in which case I will happily revert the
commit.
@mkremins
Copy link
Owner

mkremins commented Jun 8, 2014

I suspect that the use of item descriptions to represent multiline text is a holdover quirk from a time before the Minecraft client was able to correctly handle newlines in ordinary text tooltips. Fanciful started out as an attempt to canonicalize and modularize some of the patterns emerging around the new chat API, and some of the existing code I looked at as reference during the initial development period handled multiline tooltips the same way.

@MiniDigger
Copy link
Contributor

Does this fix the tooltip issues? If so, please merge it. Would be great.

@mkremins
Copy link
Owner

mkremins commented Jun 8, 2014

Planning to merge as soon as I've finished reviewing all the changes :)

@glen3b
Copy link
Contributor Author

glen3b commented Jun 8, 2014

The one thing I'm not completely sure of is the last commit, as it is somewhat out-of-scope and more of a developer convenience, I'm perfectly fine removing it but it's up for debate. I do, however, think that FancyMessages logically represent an iterable over MessageParts.

@mkremins
Copy link
Owner

mkremins commented Jun 8, 2014

Could we restore the old (strings-only) definition of tooltip, drop tooltipUnformatted, and rename tooltipFormatted to formattedTooltip? It's a little nitpicky, but I think these names read better in client code than the alternatives, while also allowing us to avoid breaking backwards compatibility with regards to the plain tooltip API.

Other than that, everything here looks good.

@glen3b
Copy link
Contributor Author

glen3b commented Jun 8, 2014

Done.

@glen3b
Copy link
Contributor Author

glen3b commented Jun 11, 2014

I merged the changes from my other PR into this one. If all other issues are resolved, we're ready to merge.

mkremins added a commit that referenced this pull request Jun 11, 2014
Improve tooltip API greatly. Fixes issues #6 and #29.
@mkremins mkremins merged commit 5238ab5 into mkremins:master Jun 11, 2014
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.

None yet

3 participants