group-pm: fix colorHashFromString #3991

Maskedman99 · 2020-03-21T06:44:53Z

colorHashFromString() now considers all charecters in the string it can also now return colors with red component in it. Also fixed the cases where the function returned #NaN for very long or empty strings.

Fixes: #3985

Maskedman99 · 2020-03-21T06:47:02Z

@ray-kraesig since this function is only used in GroupAvatar.js should I move it into that.

jainkuniya

hi @Maskedman99 thanks for the PR.

Can you please link before and after screenshots here? It will help a lot in reviewing the PR.

Maskedman99 · 2020-03-22T04:27:15Z

@jainkuniya the difference isn't exactly visible in the UI,

apparently colorHashFromString() before would break when dealing with empty strings or strings with just 1 character, but in reality we cannot create a group with no members in it or a username with just 1 character. However this is fixed in the PR by considering every character in the string as opposed to only taking the 2nd character of the string.
In case of strings with the length being a certain large value the hash value would become very large and JS returned Infinity, apparently there's no crash in the App, and a color is being returned, but the same color is being returned for every string whose length is greater than this value. This was fixed by doing a modulos operation on hash and assigning the result onto the hash.
Before the color was decided based on the second character of the string and it's length, if another string had the same second character and length it would also return the same color, this is fixed in the PR by making use of every charector in the string to determine the color.
Before the function couldn't return #ffffff since the final hash was modulosed with #ffffff this is fixed by doing the modulose operator with #1000000 instead.
Before the function adds 0x100000 for every hash value which was less than 0x100000, this caused the function to miss out the color range #000000 to #0fffff, this was done to prevent the colorHash value from returning a string with length less than 6, This has been fixed in the PR

Before:

After:

rk-for-zulip · 2020-03-23T18:33:25Z

src/utils/color.js

-  for (let i = 0; i < name.length; i++) {
-    hash = hash * 31 + name.charCodeAt(1);
+  for (let i = name.length - 1; i >= 0; i--) {
+    hash = hash * 31 + name.codePointAt(i);


codePointAt is normally what you want, for UTF-8 safety – but here charCodeAt is fine. In fact, with this loop, it's required for correctness: name.length gives the number of 16-byte code units, not the number of code points.

Also, why is the string now being iterated in reverse?

rk-for-zulip · 2020-03-23T18:34:50Z

src/utils/color.js

+  while (colorHash.length < 6) {
+    colorHash += '0';


This allows #000000, but still can't generate any other color with no red component.

Also, you may be interested in the padStart and padEnd functions.

rk-for-zulip

The commit message seems to have been accidentally doubled.

This commit will also need some tests demonstrating that it succeeds where the previous code failed. (See src/utils/__tests__/color-test.js for the existing tests.)

While it's unreasonable to find a string which produces exactly #ffffff (and even less reasonable to require that any possible future implementation also produce #ffffff for that string), it shouldn't be difficult to ensure that its other desirable properties are tested – e.g., that similar strings have different hashes, or that short and long strings generate valid values.

(I'd thought I'd mentioned this in a previous review, but apparently not. Apologies.)

rk-for-zulip · 2020-03-26T00:51:41Z

src/utils/color.js

+  let colorHash = hash.toString(16);
+  colorHash = colorHash.padStart(6, '0');
+
+  return `#${colorHash}`;


This could all be a single line. (The first two lines in particular should be a single line.)

Maskedman99 · 2020-03-26T19:21:28Z

since this function is only used in GroupAvatar.js should I move it into that?
Also,

desirable properties are tested – e.g., that similar strings have different hashes,or that short and long strings generate valid values.

you meant the opposite right similar strings have similar hashes

rk-for-zulip · 2020-03-27T02:25:22Z

since this function is only used in GroupAvatar.js should I move it into that?

While that's the only place it's used now, it's conceptually generic and could reasonably be used elsewhere if there were any call for it. Unless that's going to change, it might as well stay where it is.

(It probably wouldn't be worth the churn to move out, if it were already there. But it's not.)

Also,

desirable properties are tested – e.g., that similar strings have different hashes,or that short and long strings generate valid values.

you meant the opposite right similar strings have similar hashes

Decidedly not! That's often exactly the property you want a hash function not to have; it makes finding collisions easier.

This application doesn't exactly call for a cryptography-grade hash, but it's still desirable that (e.g.) "John Doe, Juan Pérez" and "John Doe, Jean Dupont" have different colors, even though under the current function they wouldn't. Obviously the pigeonhole principle guarantees that this won't be true in the general case, but matches should generally be uncommon.

Maskedman99 · 2020-03-27T05:51:03Z

@ray-kraesig thanks for clarifying, It seems I interpreted 'similar strings' as "John Doe" and
"John Doe". I've added the suggested modifications.

chrisbobbe · 2020-04-13T19:22:21Z

Bumping in priority because it's apparent that the bug described in the issue is causing a P0 crash, #4038. I'll try for a simple fix that can ignore the #NaN output, unless @ray-kraesig if you happen to know if this is very close to mergeable (or you can merge with a few tweaks).

rk-for-zulip · 2020-04-13T19:32:20Z

Bumping in priority because it's apparent that the bug described in the issue is causing a P0 crash, #4038. I'll try for a simple fix that can ignore the #NaN output, unless @ray-kraesig if you happen to know if this is very close to mergeable (or you can merge with a few tweaks).

As of the last pass, I think all issues have been resolved. I'll run a couple of additional tests, but it should be mergeable.

colorHashFromString() now considers all characters in the string. It can also now return colors with no red component in it. Also fixed the cases where the function returned #NaN for very long or very short strings. [ray: Minor copyedits to commit message.] Fixes: zulip#3985

chrisbobbe · 2020-04-13T20:21:56Z

Thanks @Maskedman99 and @ray-kraesig!

jainkuniya reviewed Mar 21, 2020

View reviewed changes

rk-for-zulip reviewed Mar 24, 2020

View reviewed changes

Maskedman99 force-pushed the colorHashFromString branch from adef1b3 to c135d02 Compare March 25, 2020 05:29

Maskedman99 requested a review from rk-for-zulip March 25, 2020 05:43

Maskedman99 force-pushed the colorHashFromString branch 2 times, most recently from eb07b9c to 63bc582 Compare March 25, 2020 06:00

rk-for-zulip reviewed Mar 26, 2020

View reviewed changes

Maskedman99 force-pushed the colorHashFromString branch 2 times, most recently from 4276aa1 to 5f89eb4 Compare March 27, 2020 05:40

Maskedman99 requested a review from rk-for-zulip March 27, 2020 05:51

chrisbobbe added the P0 critical Highest priority label Apr 13, 2020

rk-for-zulip force-pushed the colorHashFromString branch from 5f89eb4 to fc00fab Compare April 13, 2020 19:53

rk-for-zulip force-pushed the colorHashFromString branch from fc00fab to b08a014 Compare April 13, 2020 19:57

rk-for-zulip merged commit 9161e47 into zulip:master Apr 13, 2020

chrisbobbe mentioned this pull request Apr 13, 2020

Exception: Unable to parse color from string: #NaN #4038

Closed

Maskedman99 deleted the colorHashFromString branch April 15, 2020 18:59

chrisbobbe mentioned this pull request Apr 18, 2020

Long, sometimes endless loading in message list #4033

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

group-pm: fix colorHashFromString #3991

group-pm: fix colorHashFromString #3991

Maskedman99 commented Mar 21, 2020

Maskedman99 commented Mar 21, 2020

jainkuniya left a comment •

edited

Maskedman99 commented Mar 22, 2020 •

edited

rk-for-zulip Mar 23, 2020 •

edited

rk-for-zulip Mar 23, 2020

rk-for-zulip left a comment

rk-for-zulip Mar 26, 2020

Maskedman99 commented Mar 26, 2020

rk-for-zulip commented Mar 27, 2020

Maskedman99 commented Mar 27, 2020

chrisbobbe commented Apr 13, 2020 •

edited

rk-for-zulip commented Apr 13, 2020

chrisbobbe commented Apr 13, 2020

group-pm: fix colorHashFromString #3991

group-pm: fix colorHashFromString #3991

Conversation

Maskedman99 commented Mar 21, 2020

Maskedman99 commented Mar 21, 2020

jainkuniya left a comment • edited

Choose a reason for hiding this comment

Maskedman99 commented Mar 22, 2020 • edited

Before:

After:

rk-for-zulip Mar 23, 2020 • edited

Choose a reason for hiding this comment

rk-for-zulip Mar 23, 2020

Choose a reason for hiding this comment

rk-for-zulip left a comment

Choose a reason for hiding this comment

rk-for-zulip Mar 26, 2020

Choose a reason for hiding this comment

Maskedman99 commented Mar 26, 2020

rk-for-zulip commented Mar 27, 2020

Maskedman99 commented Mar 27, 2020

chrisbobbe commented Apr 13, 2020 • edited

rk-for-zulip commented Apr 13, 2020

chrisbobbe commented Apr 13, 2020

jainkuniya left a comment •

edited

Maskedman99 commented Mar 22, 2020 •

edited

rk-for-zulip Mar 23, 2020 •

edited

chrisbobbe commented Apr 13, 2020 •

edited