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

refined text wordwrap #269

Open
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@eagle0wl

eagle0wl commented Oct 24, 2017

refined single-multibyte text wordwrap.
You can now properly wrap Japanese character strings.

You can see some screenshot. If necessary, I can present more screenshots.
http://eagle0wl.hatenadiary.jp/entry/2017/10/24/003606

  • This entry is Japanese Lang Text.

eagle0wl added some commits Oct 23, 2017

@joolswills

This comment has been minimized.

Member

joolswills commented Nov 5, 2017

Thanks for the changes. Does anyone have any objections to me merging this ?

@zigurana

This comment has been minimized.

zigurana commented Nov 5, 2017

I've no idea how to test this, so let's just try it out.

@joolswills

This comment has been minimized.

Member

joolswills commented Nov 5, 2017

Well, assuming it works correctly with Japanese text, so long as it doesn't break display for English etc. I will merge though if you think it looks ok codewise.

@zigurana

This comment has been minimized.

zigurana commented Nov 5, 2017

Well, the readability of the existing code is already abominable, and these additions only add a bit of complexity. I can do a review now, for @eagle0wl to pick up some improvements, or I could do a PR on it later (might take a while).

@joolswills

This comment has been minimized.

Member

joolswills commented Nov 5, 2017

Happy if you have time to review. thanks. no pressure :-)

@altiereslima

This comment has been minimized.

altiereslima commented Nov 5, 2017

Now EmulationStation crashes with Japanese texts in the game descriptions. Tested with this version on Windows 10: https://github.com/jrassa/EmulationStation/releases/tag/continuous

@eagle0wl

This comment has been minimized.

eagle0wl commented Nov 6, 2017

Thank you for comments. I think that Japanese tests will be difficult.

altiereslima
gamelist.xml is UTF-8? UTF-8 works under RPi2B. I will check the Windows version.

@altiereslima

This comment has been minimized.

altiereslima commented Nov 6, 2017

Yes, gamelist.xml is UTF-8

@zigurana

Hi, thanks for picking this up. I'm afraid I got a bit carried away, and went ahead and re-implemented this. You have a PR, can you test and see if it A: works and B: suits you?

@@ -610,42 +610,69 @@ float Font::getLetterHeight()
//the worst algorithm ever written
//breaks up a normal string with newlines to make it fit xLen
// supported UTF-8 text (wordwrap and cheap hyphenation)
std::string Font::wrapText(std::string text, float xLen)

This comment has been minimized.

@zigurana

zigurana Nov 6, 2017

I know you did not name this, but xLen really is just a cryptic name for what should be called 'width' or maxWidth.

std::string Font::wrapText(std::string text, float xLen)
{
std::string out;
std::string line, word;

This comment has been minimized.

@zigurana

zigurana Nov 6, 2017

Both line and word are no longer used.

std::string Font::wrapText(std::string text, float xLen)
{
std::string out;
std::string line, word;
size_t pos = 0, pos2 = 0;

This comment has been minimized.

@zigurana

zigurana Nov 6, 2017

pos2 is not used.

break;
}
char c = text[n];
if ('0' <= c && c <= '9') continue;

This comment has been minimized.

@zigurana

zigurana Nov 6, 2017

So let me try to paraphrase this new strategy;

  1. in stead of searching for a white-space character (space, \t, \n), we now try until we arrive at an out-of-range character,
  2. which might happen to be a whitespace, but might also be a non-ascii character.
  3. If so, check if we were able to make -any- progress since pos
  4. if we made some progress, split at this point.
  5. if not (pos == n), get next characterlocation, this time taking into account utf-8 encoding.
  6. if we find a \n next, cut here.
  7. otherwise, continue until it no longer fits, then cut.

To get the actual next-character-in-utf-8 you use getNextCursor(), which returns the stepsize needed to progress to the next character location. Note that this also works fine for ascii texts, (it will return 1).
Actually, what you should (i.m.o.) use is readUnicodeChar(), to get the next character from the next location, and auto-advance the cursor.
I think I have a working prototype, but I'd like you to take a look at it as well.
I've send a PR your way for you to test this.

}
}
// whatever's left should fit
out += line;
out += text;

This comment has been minimized.

@zigurana

zigurana Nov 6, 2017

Why do we still need this? (We are running until text.length == 0)

@pjft

This comment has been minimized.

pjft commented Nov 6, 2017

Wow. Well done good sir.

Merge pull request #1 from zigurana/textSplitting
UTF-8 support for word-wrap method.
@eagle0wl

This comment has been minimized.

eagle0wl commented Nov 7, 2017

@zigurana Thank you so much. The code that looks useless is my mistake.
I have confirmed your pull request. It worked well. (under RPi2B)
1
2
3

@altiereslima

This comment has been minimized.

altiereslima commented Nov 7, 2017

Still crashes in Windows, but thanks.

@pjft

This comment has been minimized.

pjft commented Nov 7, 2017

@altiereslima At this stage, it might be best if you share the gamelist in question, and/or the logs from ES.

It might be that one of your games or relevant metadata has a differently encoded character there?

@altiereslima

This comment has been minimized.

altiereslima commented Nov 7, 2017

I found that the theme I was crashes with the Japanese texts, many themes crashes

@altiereslima

This comment has been minimized.

altiereslima commented Nov 7, 2017

Gamelist Link:
https://mega.nz/#!fI5g1KYL!P-FpVGZTgXdLWIQPzKzJI8uwCPqEVCcESHKwkXqKR3k

Doom and VR Virtua Racing crashes

@pjft

This comment has been minimized.

pjft commented Nov 7, 2017

Oh. When you select them, is that it?

I don't see anything obvious - I was actually going to suggest something related to the "Ã" characters whenever you have "Japão" or "Ação", but if those work well then I'm really out of ideas, alas.

@altiereslima

This comment has been minimized.

altiereslima commented Nov 7, 2017

When I select them

@zigurana

This comment has been minimized.

zigurana commented Nov 7, 2017

@altiereslima
How are you testing this PR?

@altiereslima

This comment has been minimized.

altiereslima commented Nov 7, 2017

In Doom description when I replace 15–20 by 15-20 work

@pjft

This comment has been minimized.

pjft commented Nov 7, 2017

@altiereslima well, that's a good discovery.

That being said, was this working correctly prior to this PR? Did that particular character render as expected - can you check?

Still, @zigurana 's question stands: I'm not sure that jrassa's continuous build is in any way related to this PR? Are you actually testing this PR?

@altiereslima

This comment has been minimized.

altiereslima commented Nov 7, 2017

Yes, work correctly prior to this PR. Jrassa's continuous build no crashes with the Carbon theme, but neither displays the Japanese texts. With other themes such as Pixel and Nes Mini crashes

@zigurana

This comment has been minimized.

zigurana commented Nov 7, 2017

@altiereslima
I've tried to replicate your issue by copy-pasting the description of the DOOM entry into my gamelist. There was no crash. The only issue was that the description is so long it did not scroll all the way to the end. I can look into why that is later, but it' s minor.

So I ask you again, how exactly did you test the changes proposed in this PR? Did you build the software yourself for Windows? Using the VS compiler?

@altiereslima

This comment has been minimized.

altiereslima commented Nov 8, 2017

I compile via appveyor.
https://ci.appveyor.com/api/buildjobs/3dqbr01m4bss7oc4/artifacts/EmulationStation-Win32.zip

I just tested and it really wasn't that PR problem, it was added to the jrassa fork and I tested it along with this PR. Sorry. Here I compiled with your changes and continue crashes, but it's not the problem of your PR. Sorry, your PR is not the problem, but the Windows version

@joolswills

This comment has been minimized.

Member

joolswills commented Nov 9, 2017

If everyone is happy, I can merge - but it needs squashing first please. Thanks.

@altiereslima

This comment has been minimized.

altiereslima commented Nov 9, 2017

I discovered the error that crashes the EmulationStation in Windows. It's this Commit d3966da

When reversed everything returns to normal.

@pjft

This comment has been minimized.

pjft commented Nov 9, 2017

Wow. Thanks for tracking that one down, must have been a bit of work to get to it.

Maybe you could open an issue and tagging that commit and @tomaz82 separately so we don't affect this PR's thread?

Out of curiosity, does this PR still work for you if you revert that particular commit?

@tomaz82

This comment has been minimized.

Collaborator

tomaz82 commented Nov 9, 2017

I'm on it already

@altiereslima

This comment has been minimized.

altiereslima commented Nov 9, 2017

@pjft
This PR does not work in Windows, it only shows ........ or squares depending of theme in place of japanese texts, but it does not cause any problems.

@zigurana

This comment has been minimized.

zigurana commented Nov 18, 2017

@altiereslima do you have the "meiryo.ttc" (sic) available? (jk, I think there is a spelling error for this (hard-coded) font under windows.
Hmm, it actually is called meiryo.ttc

TrueType Collection. TrueType Collection (TTC) is an extension of TrueType format that allows combining multiple fonts into a single file, creating substantial space savings for a collection of fonts with many glyphs in common.

Ok, so the question remains, do you have it available?

I will submit my branch, as another PR in any case, as this one seems to be stagnant.

@altiereslima

This comment has been minimized.

altiereslima commented Nov 18, 2017

@zigurana You're right, now it works. Thank you.

@zigurana

This comment has been minimized.

zigurana commented Nov 18, 2017

#314 is for the exact same change, but now only has my commit.

@altiereslima

This comment has been minimized.

altiereslima commented Nov 18, 2017

@zigurana, meiryo.ttc is already sufficient for Japanese texts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment