Conversation
|
Still working on this? @80avin |
|
Waiting for opinions on the architecture. |
|
If you're using AI for the PR, you need to signal it somewhere, and ideally link to the thread.
Also, please use the [SVG](https://lib.rs/crates/svg) crate rather than brittle raw strings.
Just starting advice.
▰▰▰▰▰
Miles Wirth 🙃
… From: Avinash Thakur ***@***.***>
Sent: 17 March 2026 12:33
To: plotters-rs/plotters ***@***.***>
Cc: Miles Wirht ***@***.***>, Comment ***@***.***>
Subject: Re: [plotters-rs/plotters] feat: add tooltip support (PR #721)
80avin left a comment (plotters-rs/plotters#721)
Waiting for opinions on the architecture.
The PR works for most cases already.
--
Reply to this email directly or view it on GitHub:
#721 (comment)
You are receiving this because you commented.
Message ID: ***@***.***>
|
|
Thanks Miles. Also, phrased a clearer question to maintainers in the PR description. Completing the PR is not an issue, but I am myself unconvinced by the current API. I went with this API design while trying to keep the breaking changes to a minimum; otherwise, the backend system would have needed major breaking changes |
|
Breaking changes like what?
(Kind of a psuedo-maintainer)
▰▰▰▰▰
Miles Wirth 🙃
… From: Avinash Thakur ***@***.***>
Sent: 18 March 2026 11:34
To: plotters-rs/plotters ***@***.***>
Cc: Miles Wirht ***@***.***>, Comment ***@***.***>
Subject: Re: [plotters-rs/plotters] feat: add tooltip support (PR #721)
80avin left a comment (plotters-rs/plotters#721)
Thanks Miles.
I added the note about AI usage in the PR description.
Also, phrased a clearer question to maintainers in the PR description. Completing the PR is not an issue, but I am myself unconvinced by the current API. I went with this API design while trying to keep the breaking changes to a minimum; otherwise, the backend system would have needed major breaking changes
--
Reply to this email directly or view it on GitHub:
#721 (comment)
You are receiving this because you commented.
Message ID: ***@***.***>
|
|
I've tried to explain that in the PR description. If something is difficult to understand, I can take specific questions. In abstract terms, breaking changes like
Let me know your thoughts on these, or if I'm thinking too much. If the current design looks good, I can finish the PR and move out of draft. |
|
I think that a Context object, where start and end are fields, and further information could be added to would be the best step forward. It should have a default implementation, and notably not optional (Particular backends should make it explicit when they're skipping out on the object).
▰▰▰▰▰
Miles Wirth 🙃
… From: Avinash Thakur ***@***.***>
Sent: 19 March 2026 23:30
To: plotters-rs/plotters ***@***.***>
Cc: Miles Wirht ***@***.***>, Comment ***@***.***>
Subject: Re: [plotters-rs/plotters] feat: add tooltip support (PR #721)
80avin left a comment (plotters-rs/plotters#721)
I've tried to explain that in the PR description. If something is difficult to understand, I can take specific questions.
In abstract terms, breaking changes like
* Changing what data is shared with backends.
Currently, backends only get low level information of what figure to draw and where. But, for a backend to draw tooltips or other decorations, it needs to have some semantic information about the chart.
The `begin_context`/`end_context` that I added work but are after-thoughts, designed to add tooltip support without changing much in existing design.
* Changing the user facing API.
As I mentioned in the PR description, the backend.with_tooltip is not ideal. Ideally, we would want to enable/disable tooltip per chart ( or element of chart ) than entire backend.
Let me know your thoughts on these, or if I'm thinking too much. If the current design looks good, I can finish the PR and move out of draft.
--
Reply to this email directly or view it on GitHub:
#721 (comment)
You are receiving this because you commented.
Message ID: ***@***.***>
|
fixes #13
It works, but I don't like the presence of
tooltip should be enabled per chart, not backend.
draw_series_with_tooltipsapi.Ideally, it should work with draw_series API and tooltip should be a configuration parameter.
Challenge:
Alternative API:
chart.tooltip_mode(TooltipMode)shall enable tooltips for current chart.mode can be "Auto", "Always", "Disable". If Always and the backend doesn't support, there should be error.
Also, SVG backend shall add classes wherever required to uniquely identify elements, if tooltip mode is enabled.
Question to the maintainers:
AI use:
The initial approach, including the API was designed after manual experiments. AI is used to scale the approach into a wider & cleaner solution.