caleb531 / jcanvas

A jQuery plugin that makes the HTML5 canvas easy to work with.
https://projects.calebevans.me/jcanvas/
MIT License
626 stars 192 forks source link

Wrapping text fails when last word doesn't fit in maxWidth #35

Closed ialiendeg closed 12 years ago

ialiendeg commented 12 years ago

I know is in beta but in case you didn't notice...

When wrapped text ends in a word that doesn't fit in maxWidth, wrapText function keeps looping forever.

I don't know what would be the best solution in this case, maybe we have to start cutting words.

caleb531 commented 12 years ago

Hmm, I don't notice the issue. Could you please provide a code example?

ialiendeg commented 12 years ago

I'm trying with "Texto 3 por defecto" (spanish for "default text 3") with 76.875 for maxWidth.

ialiendeg commented 12 years ago

This is a quick fix to avoid forever loop. For now I'm dropping last word in this case. This is the patch for jcanvas.js file (I don't send you a pull request because I don't think that this is the right solution)

diff --git a/jcanvas.js b/jcanvas.js
index 7601e0b..e924d29 100755
--- a/jcanvas.js
+++ b/jcanvas.js
@@ -1570,8 +1570,13 @@ function wrapText(ctx, params) {
                                line += words.shift() + " ";
                        } else {
                                // If line is too long, break and start a new line
-                               lines.push(line);
-                               line = "";
+                                if (lines[lines.length-1] == "") {
+                                    // we drop the last word if doesn't fit in maxWidth
+                                    words.shift();
+                                } else {
+                                    lines.push(line);
+                                    line = "";
+                                }
                        }
                        if (words.length === 0) {
                                // If we reach the last word, break and add new line
smatthews1999 commented 12 years ago

Hi,

In regards to the above issue. I also experienced that having a word that is larger than the maxWidth will send the code into an infinite loop. Here is a jfiddle that demonstrates the issue.

http://jsfiddle.net/smatthews1999/xMD2L/

caleb531 commented 12 years ago

If my code is correct, the above issue is fixed in the lastest commit (d0fba05872281517e99509d96730315e6b2b2f71). Please test this issue, as well as the other issues above, to see if those issues are also fixed.

Thanks. -Caleb

smatthews1999 commented 12 years ago

The fix works great.

Thanks Sam

ialiendeg commented 12 years ago

I think I've found another problem. This is the way I reproduced it.

Consider a line with two words. If you start adding letters to the last word, the line is wrapped correctly but if you keep adding more letters and that word ends being longer than maxWidth, then the two separated lines are joined again and wrapping is disabled.

caleb531 commented 12 years ago

Hmm, yes, I can see why that would happen based on my fix. I was actually thinking about that this morning. I'll see what I can do later today. ;)

-Caleb

On Oct 29, 2012, at 11:08 AM, ialiendeg notifications@github.com wrote:

I think I've found another problem. This is the way I reproduced it.

Consider a line with two words. If you start adding letters to the last word, the line is wrapped correctly but if you keep adding more letters and that word ends being longer than maxWidth, then the two separated lines are joined again and wrapping is disabled.

— Reply to this email directly or view it on GitHub.

smatthews1999 commented 12 years ago

I see this behavior only on the last line. If you have several lines of wrapped text.. only the last one will join back the words.

ialiendeg commented 12 years ago

It's true, this whole issue is about last text line, I think because the problem is, in fact, the loop closing condition.

Like I said before, maybe we have to start thinking of word hyphening (too much trouble?), or maybe the conclusion is simply defining text limits acordingly to desired text. In our project, by now, we are rejecting last word in that situation because the person who defines the text zone can make it bigger if it is necessary, but in some cases it will be problems, sure of that.

caleb531 commented 12 years ago

I could add support for word hyphening, but that would require a more complicated algorithm. Right now, I am satisfied with the current algorithm, since it seems to prevent the most common errors (which includes the original issue reported here).

I don't see the two-word bug as a huge issue. Remember, this is all being drawn (and redrawn) on a 2D canvas, so I would prefer to sacrifice a little formality for more simplicity and performance.

dksmiffs commented 12 years ago

ialiendeg - is it possible that a non-native (to jCanvas) library like Hyphenator.js might meet your needs?

ialiendeg commented 12 years ago

Oh, no, it was only a suggestion, I agree with you, adding hyphenation is probably unnecessary (I'll take a look to that library, just in case, thx dksmiffs).

Talking about the last line problem, we have two possible solutions: Caleb's one that joins two lines, and mine that drops last word.

Anyway, thx for the good work Caleb, this is a very useful code.