grastello / ytel

Youtube "front-end" for Emacs
GNU General Public License v3.0
48 stars 10 forks source link

Search term fails with Args out of Range #12

Open spiderbit opened 4 years ago

spiderbit commented 4 years ago

If I search "React Nightwish" it fails:

seq-subseq: Args out of range: "REACTING to NIGHTWISH (Ghost River) 👻🏞🔥", 0, 40

It probably has something to do with this symbols at the end of the title not sure if that is unicode or what, and why it exactly fails.

Ok found the problem, the function string-width counts the length as 41, if I replace it with length it detects 39.

For some reason it counts the ghost and the fire symbol as 2 length the symbol in the middle it counts as 1, but whatever the reason is that string-width don't counts correctly length does, something about proportional fonts I guess, but something is going wrong.

I set ytel-title-video-reserved-space to 40, otherwise you probably don't get the error.

spiderbit commented 4 years ago

Ok replacing seq-subseq like that:

-    (concat (seq-subseq title 0 ytel-title-video-reserved-space)
+    (concat (truncate-string-to-width title ytel-title-video-reserved-space)

fixes the problem.

grastello commented 4 years ago

I'll have to took into that more seriously (a shame I also have to take my lessons and am already behind on that, too). The problem is that while changing these two lines solves the problem in this particular case it might still break on some other strings (video titles), that happened to me before. To be fair I'm not sure if we can solve this once and for all.

spiderbit commented 4 years ago

I think you need to be consistent, either you expect no proportional fonts then you can use length and seq-subseq or you want to address proportional fonts then you need to use string-width and truncate-string-to-width.

Even if the latter would missbehave that would then be a bug in that implementation, but using 1 function that don't cares about tab-width and one that does, is wrong, and as demonstrated creates errors.

Also this code is buggy, you say that the fix could create other problems, but A logically I don't think it should and B we have evidence that seq-subseq implementation is a bug, we don't have that for the latter.

I mean every change might break something else, that is true for every fix.