Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
- Loading branch information
Showing
6 changed files
with
29 additions
and
15 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
19a98c2
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.
@danielgindi Please review this. I have verified that the issue actually exists and this fixes it. I'm just not sure if it induces any other problems.
I have tested rotated labels on barchart, that seems to work fine.
The issue basically was that the x-axis values had different line-heights, which caused them to be not vertically aligned. I generalized the line height to fix the issue.
19a98c2
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.
I'll look into it. I remember fixing a vertical alignment problem already. But maybe something happened there along the way...
19a98c2
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.
I was using a version at least week old and didn't know there's a fix for that already. The way I fixed it is similar but instead of just calculating text height of "Q" I use "0123456789ABCDEFGHIJKLMNOPRSTUWXYZabcdefghijklmnoprstuwxyz". My thinking was to make it ready for strange fonts where "7" or "S" may be higher than "Q". Just an idea. I'm glad you fixed it anyway.
19a98c2
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.
I've struggled a lot with Android's font system. I can safely say it sucks. There's no way to determine the real line height - so we have to do such tricks...
Anyway - If you have a "7" that's way above the normal character height - that shouldn't be calculated. We need to establish a normalized line height so we have a center point to rotate around. For vertical aligning of horizontal text - that's less important because in the worst case there's more offset and that could be adjusted using other properties.
19a98c2
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.
The "7" was just an example - I see you got it covered :-) I'm happy with my simple fix for the moment.
Cheers! Great library btw, made my life easier.
19a98c2
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.
This seems to have fixed the problem for my project also. Should a pull request be made to merge it into the source?
19a98c2
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.
This solution will fail if you happen use glyphs that reach lower or higher than Q. On some typefaces character Q actually does not reach below generic baseline.
Paint.getTextBounds(String) a is wrong thing to use when aligning text - it simply looks at the rendering bounds of the text. There already is method Utils.getLineHeight(Paint) that uses the correct way of calculating line heights (looking at the difference of typeface's ascent and descent). Replacing the normalizedLineHeight with call to this method should do the trick, and get rid of Utils.calcTextHeight("thisShouldCoverRelevantCharactersInMyTypefaceRight")
http://stackoverflow.com/a/27631737/3432809 has a great write-up on Android font metrics.
19a98c2
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 is an actual fix on 2.2.5 release