stable color rendering

[CreativeTechGuy]

Fixes #500

There's a ton of variance in how users may configure their chunk names. To ensure stable colors, we need to dynamically identify which (if any) part of the file name is the unique "name" part and use that to determine the colors. If that cannot be identified or if a file doesn't follow the pattern of the rest, the entire file name will be used.

Given that the behavior before was random rainbow colors all the time (colors would even change when zooming/hiding) this is better even in the case where nothing can be uniquely identified. So I'd argue that while this is an imperfect solution, it's better in all cases than what was there by default.

---

This is best visualized with photos of the different cases:

Before this PR:

After this PR (when unique name can be identified):

After this PR (when unique name can be identified - with chunk contents changed but colors are still stable with above):

After this PR (with no identifiable parts of the filename - deterministic but effectively random colors):

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: CreativeTechGuy / name: Jason O'Neill (66841ea)

[valscion]

Thanks for opening a PR! This is indeed an interesting approach.

The loops look a tad scary to me, as I don't know how they would perform with large graphs. I also am not sure if there is no way an infinite loop could happen there.

Is there a way to get some data to the graph data object itself that could be used for this purpose with less likelihood for a performance hit?

[CreativeTechGuy]

Thanks for taking the time to review @valscion!

Quick time complexity analysis just to make sure we are on the same page. I assume you are talking about findChunkNamePartIndex.

It is almost entirely bounded by the number of chunks, secondarily the number of parts of the most complex file name.

So if there are 100 chunks and the most complex file name is a.b.c.d.e.f.g.h.i.j.k.l.m.n.o.p.q.r.s.t.u.v.w.x.y.z.js (27 parts) then it'd be:

O(100 * 27) = O(2700)

The only thing I really see as performance impacting is if a user has a ton of chunks, realistically that's the only variable that affects time complexity in a realistic way here. But if someone has a few hundred chunks, the canvas rendering will also be impacted by having to compute the layout of all of those chunks too. I'd suspect this function's additional performance hit wouldn't be noticeable in comparison.

---

The only place I can see an infinite loop being possible is in getGroupRoot, but that tree traversal is very similar to the one just a few lines below in zoomToGroup which seems to be working fine. Assuming that an infinite loop isn't possible here, I believe the number of nested groups in the graph is based on the folder structure, so I'd be surprised if there's any node that is more than a dozen folders deep in the project.

---

Let me know if we are on the same page here and if you want me to make any edits or run any tests to make sure this is working correctly at scale.

[valscion]

Thanks for the additional information 😊 good point about the count of chunks impacting the layout calculation anyway.

I have been able to only glance at the code on mobile so far so I haven't yet been able to do a more thorough review.

[CreativeTechGuy]

@valscion have you been able to take a look? This seems like a valuable improvement that would be great to get merged! :)

[valscion]

Sorry, this is still pending. I'm currently on paternal leave so I have little time to spend on computer 😊

[valscion]

Hi @CreativeTechGuy! I'm still on parental leave, so I haven't yet had a chance on checking this out. I haven't forgotten about this PR, though, and will get back to this once I have more time ☺️. Thank you for your patience

[valscion]

OK I now took a look here. This indeed makes figuring out which chunks changed in size much easier than before! I ran a simple test with our application where I added a lot of extra strings to a single module to make it grow in size and then looked at the differences before and after:

Without this PR

Before

After

Notes

It's quite difficult to see that the chunk containing packages/edit/index.js shifted and grew in size. Rather, it's quite easy to miss that as the chunks indeed shift around the treemap a lot.

With this PR

Before

After

Notes

I can now spot which chunks changed place and can spot that the packages/edit/index.js shifted, allowing me to see that the chunk sizes differ. Other colors remained stable and allows me to see that the chunk 7928 also changed place.

[valscion]

The only consideration I have in here is that the choice of colors now is quite bright and hides some details in some places that the FoamTree built-in color algorithm was able to discover:

Compare that to the original version (the chunk contents are the same) and you can see that there are more details immediately visible before this PR:

So I wonder is there a way to tweak the color deciding algorithm to get a bit more muted colors so that they'd be easier on the eye and also be able to see the count of sub-modules more easily?

[CreativeTechGuy]

I'd argue that the colors in the original version are just as similar (see the huge cluster of similar teals in the bottom right) but it is just more noticeable now that the colors are more vibrant. I'm not sure there's simply enough colors which are visually distinct enough to avoid this problem.

I'll work on making the colors more muted. Although I think that the more muted the colors are, the more that will start to look visually similar.

In my testing, as we reduce the saturation on the colors, it starts to look "disabled" so I don't want to mute them too much that it starts to communicate some functional state which would be misleading.

[CreativeTechGuy]

I toned down the saturation a bit. I think this is a good middle ground. It looks like this

Hopefully you like it!

[valscion]

I toned down the saturation a bit. I think this is a good middle ground. It looks like this

Oh wow this looks much nicer!

I see no reason why we would not be able to ship this soon. Do you think this feature warrants a small mention in the README.md or is it OK to leave undocumented?

Adding a changelog entry at least would be nice ☺️. Make sure to update your branch (merge commit is OK to me) with the master branch in this repository before tweaking the changelog as otherwise there's likely gonna be conflicts.

[CreativeTechGuy]

Got it! Rebased with master and added a line to the Changelog!

I'm not sure about documenting this. It's not configurable and shouldn't break anything. Also, if someone doesn't understand it and just thinks they are random, then there's no harm. Worst case it's the same as it was previously, best case it's an improvement. :)

[valscion]

Thanks! This looks great to me! 👍

[valscion]

This has now been released in v4.6.0 ☺️