Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New vars and cleanup for hover tooltips #12147

Merged
merged 30 commits into from
Jun 8, 2022
Merged

Conversation

bryevdv
Copy link
Member

@bryevdv bryevdv commented May 30, 2022

A recent SO question asked about hover values for the actual tooltip "snap" locations. It seemed like this should be an easy add, but I was quickly reminded what an intolerable mess hover_tool.ts is. This PR adds those special vars and also attempts some minimal cleanup up that file. Not anywhere close to what is really needed, but a start. Goals for this PR:

  • TooltipVars well-defined and typed and consistent for all cases
  • Expose tooltip (x, y) and (sx, xy) as tt_x, tt_sy etc.
  • Improve const-ness throughout
  • Avoid redundant parameters in functions
  • Simplify things where possible
  • Expose the glyph object as a tooltip vac

I've left individual commits in case that is simpler to review. I tested with all the User Guide examples, but I do want/intend to create more examples that exercise a wider range of case. I have made a couple of explicit complexity trade-offs in a couple of places. I will note them with comments inline.

I would be interested in suggestions for:

  • Any other TooltipVars we can add now
  • Better names for tt_x, tt_sy, etc
  • Adding (unit) tests
  • Splitting up _update

For the latter I had thought possibly we could move the code for each case to the relevant glyph view classes, and we could do that. But, it would require passing a lot of parameters to the new methods.

I do plan to expand the relevant docs sections once the implementation work is completed.

Edit: follow-on question: This PR removes these oddball special vars from #11992

        const data_x = glyph._x
        const data_y1 = glyph._y1
        const data_y2 = glyph._y2

The can now be accessed in a CustomJSHover as e.g. special_vars.glyph._x etc. In fact anything on the glyph can now be easily used for a tooltip, which is great! But my question is, should the _x attrs etc . be made public?

Lastly, of course, the value is actually a glyph view, not a glyph. I've stayed consistent to the existing code for now, but should we tighten up the naming here at this point?

cc @mattpap @tcmetzger

cc @xaptronic for changes related to #11992

@bryevdv bryevdv added this to the 3.0 milestone May 30, 2022
@bryevdv bryevdv requested a review from mattpap May 30, 2022 18:00
const ptc = glyph.get_anchor_point("center", i, [sx, sy])
if (ptc != null)
return [ptc.x, ptc.y]
return [sx, sy]
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the "TODO" above, using sx and sy as in other cases. That said, I think glyph.get_anchor_point("center"...) will always return a value so maybe figure out a way to express that instead.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If that's the case, then this information has to be encoded in get_anchor_point()'s type, i.e.:

get_anchor_point(anchor: "center", i: number, [sx, sy]: [number, number]): {x: number, y: number}
get_anchor_point(anchor: Anchor, i: number, [sx, sy]: [number, number]): {x: number, y: number} | null

get_anchor_point(anchor: Anchor, i: number, [sx, sy]: [number, number]): {x: number, y: number} | null {
  // implementation
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was too circumspect. It's not really the fact that it is center, per se. It is that, for the glyphs that happen to land on this codepath ("scatter-ish" more or less), the get_anchor_point always returns something for all values (including "center"). But that's not true for other kinds of glyphs, that don't take this codepath. So, I think it's a little more complicated. I can make an issue regarding cleaning up an making anchor point reporting more consistent everywhere, if you like.

const el = template.cloneNode(true) as HTMLElement

// if we have an image_index, that is what replace_placeholders needs
const i = is_undefined(vars.image_index) ? vars.index : vars.image_index
Copy link
Member Author

@bryevdv bryevdv May 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The trade-off here was to reduce the number of params and simplify the calling in half a dozen other places (as well as avoid passing the same parameter essential twice)

@bryevdv bryevdv changed the title Bryanbv/hover rxry New vars and cleanup for hover tooltips May 30, 2022
@bryevdv
Copy link
Member Author

bryevdv commented Jun 1, 2022

Marking ready for initial technical review

@mattpap
Copy link
Contributor

mattpap commented Jun 5, 2022

I was about to go ahead and merge this, but those imports need to be fixed first.

bryevdv and others added 2 commits June 5, 2022 10:46
Co-authored-by: Mateusz Paprocki <mattpap@gmail.com>
@bryevdv
Copy link
Member Author

bryevdv commented Jun 5, 2022

I was about to go ahead and merge this, but those imports need to be fixed first.

Hold up a bit, I will proceed to add more examples now

@xaptronic
Copy link
Contributor

Sorry for my delayed reply here - I will aim to test my use-cases from #11992 with this branch. If you merge before I report back, I will open an issue/PR if there's an issue with the use cases. It sounds like if I can access the glyph from the tooltip, things should work out.

@bryevdv
Copy link
Member Author

bryevdv commented Jun 8, 2022

@tcmetzger I ended up changing to snap_x, snap_sy, etc on the rationale that it is the position that the tooltip snaps to (which may just be the cursor position, but may also be glyph "anchors" for glyphs that support that). LMK what you think.

@bryevdv
Copy link
Member Author

bryevdv commented Jun 8, 2022

@xaptronic note I also just changed the var name to glyph_view (since it is, in fact, a GlyphView. The inconsistency around glyph/glyphview was barely tolerable when it was internal but letting it escape to the wild is bridge to far)

@bryevdv
Copy link
Member Author

bryevdv commented Jun 8, 2022

@tcmetzger it looks like pydata sphinx theme 0.9 is out and breaks our prev/next layout customizations

@bryevdv
Copy link
Member Author

bryevdv commented Jun 8, 2022

@mattpap @tcmetzger I have fixed the theme breakage in 2c43494 I intend to merge this when green to unblock CI (other comments have been addressed, as noted)

@bryevdv bryevdv merged commit 2a44125 into branch-3.0 Jun 8, 2022
@bryevdv bryevdv deleted the bryanbv/hover_rxry branch June 8, 2022 20:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants