Histogram Factors Visualizer: enhancement: mouseover and y-axis

[katestange]

By submitting this PR, I am indicating to the Numberscope maintainers that I have read and understood the contributing guidelines and that this PR follows those guidelines to the best of my knowledge. I have also read the pull request checklist and followed the instructions therein.

---

This PR is an improved version of #228 which will be closed in favour of this one. This is a rebased version of that PR (i.e. cherry-picked the commits from that PR), with improvements to code and removal of bugs (at the level expected if I had reviewed the PR myself). Besides some cleaning up of documentation, and some slight tweaking of visual spacing to allow for text, the scope is no larger than that PR. Namely, it resolves #220 by adding a mouseover feature which shows the stats for each histogram bar, and adds tickmarks to the y-axis. It touches only the visualizer file itself. The original PR has a fair quota of magic numbers and I haven't sorted through it all, although it is a little improved. Feedback appreciated.

[gwhitney]

Nice! Haven't done a code review yet, but in terms of behavior there is a concern that I would say need to be addressed: when the mouse is over the rightmost histogram column, the values of the "factors" and "height" in the mouseover popup are lost because the right edge of the popup would extend to the right of the canvas, and so it is clipped and not displayed. I guess the popup should be kept fully within the canvas even when the mouse pointer is near the right edge of the canvas. Thanks!

[katestange]

Hi @gwhitney I've addressed that issue and improved the code somewhat. I added a little shading the bar you are mousing-over because if the box stops moving as you approach the right, it can be harder to tell which bar you are on.

[gwhitney]

Nice! The highlighting looks good and the popup is always readable. Here's another possible behavior issue, let me know if you want to address it in this PR or leave it as an issue for the future: try doing factor histogram for A321580 with 10000 terms (it has fewer than that, but that way you get them all). The bars for 0 and 1 factors are very short (I think just 1 value each). On the other hand, other bars have hundreds of values. This makes it very picky to try to mouse over those first two short bars. It's hit-or-miss for me whether I can get the popup to stay up long enough to read it. So you might want to add some vertical "fuzz" or minimum for short bars for when the mouseover gets activated, to avoid it being annoying to try to get the info for a tiny bar. Please let us know if you're going to implement that in this PR, or if we will just file an issue to do that at some unspecified future point (could be an introductory issue for one of the team members, I suppose).

[katestange]

Thanks, @gwhitney that is an excellent suggestion and not hard to implement. Added.

[gwhitney]

The grey highlighting occurs even when mouseOver is false. Is that intentional?

[gwhitney]

Passes tests, works for me, docs generate OK. Code looks fine. Once those two questions are settled, I think this is good to merge.

[katestange]

If you are happy now @gwhitney please merge.

[gwhitney]

I am happy and willing to merge. There are two entirely optional things at this point that I wanted to give you a chance to consider before I merge. The first is that conventional coding wisdom, which I'm not rabid about, is that the draw() function in histogram has gotten so long that for the sake of code readability/organization, some self-contained chunks (like maybe drawAxes, drawBar, whatever) should be taken out into their own shorter functions and called from the draw() function.

The second is that if your mouse pointer happens to be over the spot where the "bundlecard" appears when you click "create bundle", and mouseover was true, the fixed snapshot in the bundlecard comes up with one gray bar and a tiny popup. Not sure this is a problem at all; in fact, it's kind of cute (see snapshot). Just wanted to alert you, because possibly you might not want it to be that way (since if your mouse pointer is anywhere else on screen, the histogram is just plain in the bundlecard snapshot, so it's maybe a bit odd that the snapshot depends on where the mouse pointer happens to be when it comes up), or possibly you might want to ignore the mouse pointer when drawing the bundlecard and if mouseover is true, always highlight the middle bar and show its popup (because it is cute, and gives a visual indication that there will be an interaction when the whole visualizer is drawn.

These are both quite minor points, especially as we are under some time pressure, and the second one is a bit UI-dependent (although I imagine even with the refactor we are going to want visualizers to be able to draw thumbnails of themselves). So as I said I am totally happy to merge as is, just wanted to give you the option of contemplating these things briefly.

[katestange]

Thanks @gwhitney I will refactor the code slightly for readability (later today), I think that's a good call. But I think the thumbnail is cute, I don't have any problem with that behaviour. Sound ok?

[gwhitney]

Yes, both of these are completely up to you. I will wait for the draw refactor to merge.

[katestange]

I moved out the mouseover code into separate functions. Happy to merge now if you are. No functionality should have changed.