Re: [PATCH 1/3 v2] Limit width of formatted text by characters rather than bytes

--===============3139063736412247433==
Content-Type: multipart/signed; boundary="=-=-=";
	micalg=pgp-sha512; protocol="application/pgp-signature"

--=-=-=
Content-Type: text/plain
Content-Transfer-Encoding: quoted-printable

On Tue, Aug 29 2017, Will Storey <[email protected]> wrote:
> On Mon 2017-08-28 20:50:19 +0200, Jeremie Courreges-Anglas wrote:
>>=20
>> Hi Will,
>
> Hi!
>
> Thank you for looking at this.
>
>> First, thanks for your submission.  You're dealing with a known problem.
>>=20
>> The direction taken so far in ratpoison was: don't deal with wide
>> characters, only handle UTF-8 in a rather dumb but at least simple way.
>>=20
>> Rationale:
>> - the wide characters API has a lot of gotchas.  I won't detail them
>>   here but what to do in case of an invalid sequence often remains an
>>   open question.  Here, I can see that you return a partial length
>>   early.  I'm not sure this is desirable.
>
> I see. I'm not super familiar with the wchar.h API. I was not aware
> ratpoison had functionality for this!
>
> Regarding returning on invalid characters: Another option we could do wou=
ld
> be to replace them with U+FFFD.
>
>> - UTF-8 is easy and looks like the sanest choice for a multibyte locale.
>>   No offense, but other less commonly used locales are just a pain to
>>   handle.  Think state-dependant encodings.
>
> Well, even with UTF-8 it is not so easy to do everything perfectly! I'm n=
ot
> sure how wchar.h deals with there being combining characters in weird spo=
ts
> for instance. That might be something to look at if we ever revisited usi=
ng
> it.
>
>> So while technically speaking the wide characters API looks like the
>> obvious choice, I think its cost is a bit high.  Consistency is good.
>> If we start using the wide chars API somewhere, it should be used in all
>> places where it makes sense.  I'm not sure this is an easy task even in
>> ratpoison. :)
>>
>> Handling only UTF-8 as a multibyte locale, the tentative diff below
>> seems to do the job.  *WARNING*: I have barely tested it with your html
>> testcase.
>>=20
>> Feedback / test reports welcome.
>
> Cool! Thanks for writing that. I've tried it out and it works well.

Actually the behavior was rather incorrect.  After my patch,
concat_width was copying up to 'width' bytes, not up to 'width' UTF-8
characters.  This gotcha was caught by your test case.  I find the
latter behavior more useful.

Note that the function only cares about how many characters we concat,
not about their actual width on screen.  I believe this behavior is
reasonable, though.

> I'm in agreement about only worrying about support for UTF-8.
>
> After looking at the UTF8 macros, one thought I have is we could improve
> this to be more conservative about what we accept. For example, only
> consuming two, three, or four bytes when the first byte indicates that is
> appropriate, rather than having no limit. I suppose it depends how far we
> want to go in writing UTF-8 decoding.

So far the direction taken was not to bother validating anything.
Garbage in, garbage out.

> Anyway, I think it is a big improvement as is.
>
> It also still might be good to have a few unit tests. Would you be okay
> with tests in the form of my third patch?

Yep.  I have applied your 3/3 patch, which introduces a testcase, and
made further tweaks.  Namely:
=2D rename concat_width to something more appropriate
=2D move it so sbuf.c
=2D shuffled some code to make it easier to link against it
=2D move the testcase to its own .c file
=2D simplify src/Makefile.am accordingly

Feedback welcome.

Thanks,
=2D-=20
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE

--=-=-=
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAEBCgAdFiEEUTWSwa02UpMr393MDfp0rhUk5+4FAlm+zXwACgkQDfp0rhUk
5+4d6hAAraoGSjOZAcvNix55Vl2RKsZ2kNoSY1WuLTUDTDDZSy/9vmRriilxlMjk
cf0Zm+88aJCUmBphR0TbinkEoRRzWEpIgv8o+ZvGfmDlVq9SeDcv/sywjfxhOJdS
CPgL7BUKS12z623OKR85bi/KW8LGCX7wAkq3IEQqe05RzxPxpQhQ9MtBCaXWZBuV
nW5E3adeRRjMyG81umyqacq51RMgi+5rbWlgM0oYNXMAim6AADe91SdDkZJu7mO6
x5udWI9VzDfYSSlIUpUso+aLNIlj6wTrBfCs+eEQngpBZIz7SqjfxvWlAQ8VMvd/
8E7yitmphBB3XlXFoINMS7tWw07RyhaIU5Ubm7UXOCWKUwNTINmpm5d44djY8HAY
7IdT5kdLlejPWz9JnG8M6izUN/L230Kpv5bO/amTS49EaPWUI1Hnp5pDKzojvnMR
m1bBAt3aH5uYo6svfWYL4CaVes30W26LMwOejUGtjs7yKtMd3UibWJS/79lT2i8i
r/JozDOd38AyqpvXuecZLvhUrdGGCy/dnQyuzdzFuRR9cTKB/Vk0yZzvH1zusTa9
wuenrmIuB022CV7AKc5U6RDrGrqioNSr9Yh74FzGts3SS3q4vqdNobtuPHFhtL26
gB4nZP7VduAjnU55QD1q3gbPB44uZUNS6pzewjeWI5iEsvqk/mg=
=SMFb
-----END PGP SIGNATURE-----
--=-=-=--


--===============3139063736412247433==
Content-Type: text/plain; charset="us-ascii"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Content-Disposition: inline

_______________________________________________
Ratpoison-devel mailing list
[email protected]
https://lists.nongnu.org/mailman/listinfo/ratpoison-devel

--===============3139063736412247433==--