Refactor tooltip handling in TmfCommonXLineChart#383
Refactor tooltip handling in TmfCommonXLineChart#383TranHoangThanhDuy wants to merge 1 commit intoeclipse-tracecompass:masterfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactors tooltip generation in TmfCommonXLineChartTooltipProvider: replaces the internal XYToolTipHandler with CommonToolTipHandler, extracts protected helper methods for hovered-index calculation, series validation, label/HTML formatting, and modularizes tooltip item construction; public API signatures remain unchanged. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tmf/org.eclipse.tracecompass.tmf.ui/src/org/eclipse/tracecompass/tmf/ui/viewers/xychart/linechart/TmfCommonXLineChartTooltipProvider.java (1)
138-177: Add focused regression tests for default parity and hook behavior.Given the internal flow rewrite, tests should assert: (1) default tooltip content parity, and (2) subclass overrides (
addAdditionalTooltipItems,formatSeriesLabel,isValidSeriesIndex) are invoked as intended.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tmf/org.eclipse.tracecompass.tmf.ui/src/org/eclipse/tracecompass/tmf/ui/viewers/xychart/linechart/TmfCommonXLineChartTooltipProvider.java` around lines 138 - 177, Add focused regression tests for TmfCommonXLineChartTooltipProvider to assert default tooltip content parity and to verify subclass hook invocation: create tests that exercise fill(...) by simulating mouse events and compare tooltip content produced by TmfCommonXLineChartTooltipProvider against the previous/default expected output (default parity), and create a test subclass/mock of TmfCommonXLineChartTooltipProvider that overrides addAdditionalTooltipItems, formatSeriesLabel, and isValidSeriesIndex to record invocations; then assert those overrides are called with the expected parameters when fill(...) runs (use getXAxis(), getSeries(), and getHoveredIndex behavior to drive the hovered index). Ensure tests cover the case where xCoordinate < 0 and index < 0 as well to confirm no hooks are invoked.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@tmf/org.eclipse.tracecompass.tmf.ui/src/org/eclipse/tracecompass/tmf/ui/viewers/xychart/linechart/TmfCommonXLineChartTooltipProvider.java`:
- Around line 124-134: The series label `key` is embedded into HTML unescaped in
formatSeriesLabel (class TmfCommonXLineChartTooltipProvider), which can break
tooltip rendering or allow markup injection; fix by HTML-escaping `key` before
building the label (e.g., compute an escapedKey from `key` that replaces & < > "
' or use a standard utility such as StringEscapeUtils.escapeHtml4) and then use
escapedKey (not raw key) when calling String.format(HTML_COLOR_TOOLTIP, ... ,
label) so the tooltip HTML is safe.
---
Nitpick comments:
In
`@tmf/org.eclipse.tracecompass.tmf.ui/src/org/eclipse/tracecompass/tmf/ui/viewers/xychart/linechart/TmfCommonXLineChartTooltipProvider.java`:
- Around line 138-177: Add focused regression tests for
TmfCommonXLineChartTooltipProvider to assert default tooltip content parity and
to verify subclass hook invocation: create tests that exercise fill(...) by
simulating mouse events and compare tooltip content produced by
TmfCommonXLineChartTooltipProvider against the previous/default expected output
(default parity), and create a test subclass/mock of
TmfCommonXLineChartTooltipProvider that overrides addAdditionalTooltipItems,
formatSeriesLabel, and isValidSeriesIndex to record invocations; then assert
those overrides are called with the expected parameters when fill(...) runs (use
getXAxis(), getSeries(), and getHoveredIndex behavior to drive the hovered
index). Ensure tests cover the case where xCoordinate < 0 and index < 0 as well
to confirm no hooks are invoked.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a71cc108-a85e-4bb3-a590-55efb4311739
📒 Files selected for processing (1)
tmf/org.eclipse.tracecompass.tmf.ui/src/org/eclipse/tracecompass/tmf/ui/viewers/xychart/linechart/TmfCommonXLineChartTooltipProvider.java
| protected String formatSeriesLabel( | ||
| IXYSeries series, | ||
| String key) { | ||
|
|
||
| String label = (key == null) ? "" : key; //$NON-NLS-1$ | ||
| Color color = series.getColor(); | ||
| if (color != null) { | ||
| RGBA rgba = color.getRGBA(); | ||
| RGBAColor rgbaColor = new RGBAColor(rgba.rgb.red, rgba.rgb.green, rgba.rgb.blue, rgba.alpha); | ||
| label = String.format(HTML_COLOR_TOOLTIP, rgbaColor, label); | ||
| } |
There was a problem hiding this comment.
Escape series labels before embedding into HTML tooltip content.
key is inserted into HTML directly. If a series ID contains <, &, or quotes, tooltip rendering can break and allow unintended markup injection.
Proposed fix
protected String formatSeriesLabel(
IXYSeries series,
String key) {
- String label = (key == null) ? "" : key; //$NON-NLS-1$
+ String label = escapeHtml((key == null) ? "" : key); //$NON-NLS-1$
Color color = series.getColor();
if (color != null) {
RGBA rgba = color.getRGBA();
RGBAColor rgbaColor = new RGBAColor(rgba.rgb.red, rgba.rgb.green, rgba.rgb.blue, rgba.alpha);
label = String.format(HTML_COLOR_TOOLTIP, rgbaColor, label);
}
return label;
}
+
+private static String escapeHtml(String value) {
+ return value
+ .replace("&", "&") //$NON-NLS-1$ //$NON-NLS-2$
+ .replace("<", "<") //$NON-NLS-1$ //$NON-NLS-2$
+ .replace(">", ">") //$NON-NLS-1$ //$NON-NLS-2$
+ .replace("\"", """) //$NON-NLS-1$ //$NON-NLS-2$
+ .replace("'", "'"); //$NON-NLS-1$ //$NON-NLS-2$
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| protected String formatSeriesLabel( | |
| IXYSeries series, | |
| String key) { | |
| String label = (key == null) ? "" : key; //$NON-NLS-1$ | |
| Color color = series.getColor(); | |
| if (color != null) { | |
| RGBA rgba = color.getRGBA(); | |
| RGBAColor rgbaColor = new RGBAColor(rgba.rgb.red, rgba.rgb.green, rgba.rgb.blue, rgba.alpha); | |
| label = String.format(HTML_COLOR_TOOLTIP, rgbaColor, label); | |
| } | |
| protected String formatSeriesLabel( | |
| IXYSeries series, | |
| String key) { | |
| String label = escapeHtml((key == null) ? "" : key); //$NON-NLS-1$ | |
| Color color = series.getColor(); | |
| if (color != null) { | |
| RGBA rgba = color.getRGBA(); | |
| RGBAColor rgbaColor = new RGBAColor(rgba.rgb.red, rgba.rgb.green, rgba.rgb.blue, rgba.alpha); | |
| label = String.format(HTML_COLOR_TOOLTIP, rgbaColor, label); | |
| } | |
| return label; | |
| } | |
| private static String escapeHtml(String value) { | |
| return value | |
| .replace("&", "&") //$NON-NLS-1$ //$NON-NLS-2$ | |
| .replace("<", "<") //$NON-NLS-1$ //$NON-NLS-2$ | |
| .replace(">", ">") //$NON-NLS-1$ //$NON-NLS-2$ | |
| .replace("\"", """) //$NON-NLS-1$ //$NON-NLS-2$ | |
| .replace("'", "'"); //$NON-NLS-1$ //$NON-NLS-2$ | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@tmf/org.eclipse.tracecompass.tmf.ui/src/org/eclipse/tracecompass/tmf/ui/viewers/xychart/linechart/TmfCommonXLineChartTooltipProvider.java`
around lines 124 - 134, The series label `key` is embedded into HTML unescaped
in formatSeriesLabel (class TmfCommonXLineChartTooltipProvider), which can break
tooltip rendering or allow markup injection; fix by HTML-escaping `key` before
building the label (e.g., compute an escapedKey from `key` that replaces & < > "
' or use a standard utility such as StringEscapeUtils.escapeHtml4) and then use
escapedKey (not raw key) when calling String.format(HTML_COLOR_TOOLTIP, ... ,
label) so the tooltip HTML is safe.
TranHoangThanhDuy
force-pushed
the
patch-1
branch
from
c39f91e to
f7a2611
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tmf/org.eclipse.tracecompass.tmf.ui/src/org/eclipse/tracecompass/tmf/ui/viewers/xychart/linechart/TmfCommonXLineChartTooltipProvider.java (1)
145-148: Consider adding defensive null/bounds check for Y-axis array access.The access
chartViewer.getSwtChart().getAxisSet().getYAxes()[0]assumes at least one Y-axis exists and that the array is non-null. While this pattern appears consistent with other code in the codebase (e.g.,TmfCommonXAxisChartVieweruses similar access), a defensive check would prevent potentialArrayIndexOutOfBoundsExceptionin edge cases.♻️ Optional defensive check
Format format = null; if (getChartViewer() instanceof TmfCommonXAxisChartViewer chartViewer) { - format = chartViewer.getSwtChart().getAxisSet().getYAxes()[0].getTick().getFormat(); + IAxis[] yAxes = chartViewer.getSwtChart().getAxisSet().getYAxes(); + if (yAxes != null && yAxes.length > 0) { + format = yAxes[0].getTick().getFormat(); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tmf/org.eclipse.tracecompass.tmf.ui/src/org/eclipse/tracecompass/tmf/ui/viewers/xychart/linechart/TmfCommonXLineChartTooltipProvider.java` around lines 145 - 148, The code in TmfCommonXLineChartTooltipProvider assumes a non-null, non-empty Y-axes array when calling chartViewer.getSwtChart().getAxisSet().getYAxes()[0]; add defensive checks to ensure getSwtChart(), getAxisSet(), and getYAxes() are non-null and that getYAxes().length > 0 before indexing, and fall back to a safe default (e.g., leave format null or use a sensible fallback format) so the code in the block that uses `format` cannot throw ArrayIndexOutOfBoundsException or NPE; locate this logic around the getChartViewer() instanceof TmfCommonXAxisChartViewer branch and update there.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@tmf/org.eclipse.tracecompass.tmf.ui/src/org/eclipse/tracecompass/tmf/ui/viewers/xychart/linechart/TmfCommonXLineChartTooltipProvider.java`:
- Around line 99-112: The compilation fails because addSeriesTooltipItem() in
TmfCommonXLineChartTooltipProvider calls addItem() which is defined on
TmfAbstractToolTipHandler, not the outer class; update addSeriesTooltipItem(...)
to use the tooltip handler instance (fToolTipHandler) to add items instead of
calling addItem() directly: replace the direct addItem(...) calls with
fToolTipHandler.addItem(...) (or change the method signature to accept a
TmfAbstractToolTipHandler parameter and call handler.addItem(...)); ensure
formatSeriesLabel(...) remains accessible (move into CommonToolTipHandler or
make it package-private/static if needed) and update any callers accordingly
(e.g., CommonToolTipHandler.fill() should call
addSeriesTooltipItem(fToolTipHandler, ... ) or the modified method that accepts
the handler).
---
Nitpick comments:
In
`@tmf/org.eclipse.tracecompass.tmf.ui/src/org/eclipse/tracecompass/tmf/ui/viewers/xychart/linechart/TmfCommonXLineChartTooltipProvider.java`:
- Around line 145-148: The code in TmfCommonXLineChartTooltipProvider assumes a
non-null, non-empty Y-axes array when calling
chartViewer.getSwtChart().getAxisSet().getYAxes()[0]; add defensive checks to
ensure getSwtChart(), getAxisSet(), and getYAxes() are non-null and that
getYAxes().length > 0 before indexing, and fall back to a safe default (e.g.,
leave format null or use a sensible fallback format) so the code in the block
that uses `format` cannot throw ArrayIndexOutOfBoundsException or NPE; locate
this logic around the getChartViewer() instanceof TmfCommonXAxisChartViewer
branch and update there.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ca319e3b-06f6-4599-b24c-c1d18902f2af
📒 Files selected for processing (1)
tmf/org.eclipse.tracecompass.tmf.ui/src/org/eclipse/tracecompass/tmf/ui/viewers/xychart/linechart/TmfCommonXLineChartTooltipProvider.java
| protected void addSeriesTooltipItem(IXYSeries series, String key, int index, Format format) { | ||
| double[] ySeries = series.getYSeries(); | ||
| if (ySeries == null || index < 0 || index >= ySeries.length) { | ||
| return; | ||
| } | ||
|
|
||
| String label = formatSeriesLabel(series, key); | ||
| double yValue = ySeries[index]; | ||
| if (format == null) { | ||
| addItem(null, ToolTipString.fromHtml(label), ToolTipString.fromDecimal(yValue)); | ||
| } else { | ||
| addItem(null, ToolTipString.fromHtml(label), ToolTipString.fromString(format.format(yValue))); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify if addItem method is defined in TmfBaseProvider or inherited from elsewhere
ast-grep --pattern $'class TmfBaseProvider {
$$$
addItem($$$) {
$$$
}
$$$
}'
# Also check TmfAbstractToolTipHandler for addItem signature
ast-grep --pattern $'class TmfAbstractToolTipHandler {
$$$
addItem($$$) {
$$$
}
$$$
}'
# Search for addItem method definition more broadly
rg -n "protected.*void.*addItem\(" --type java -C2Repository: eclipse-tracecompass/org.eclipse.tracecompass
Length of output: 2646
🏁 Script executed:
# Find and examine the TmfCommonXLineChartTooltipProvider class definition
fd -t f "TmfCommonXLineChartTooltipProvider.java" --exec cat -n {}Repository: eclipse-tracecompass/org.eclipse.tracecompass
Length of output: 7363
🏁 Script executed:
# Find TmfBaseProvider and examine its class definition
fd -t f "TmfBaseProvider.java" --exec wc -l {} \;Repository: eclipse-tracecompass/org.eclipse.tracecompass
Length of output: 203
🏁 Script executed:
# Read TmfBaseProvider to understand class hierarchy and addItem delegation
cat -n ./tmf/org.eclipse.tracecompass.tmf.ui/src/org/eclipse/tracecompass/tmf/ui/viewers/xychart/TmfBaseProvider.javaRepository: eclipse-tracecompass/org.eclipse.tracecompass
Length of output: 10970
🏁 Script executed:
# Search for addItem calls in the codebase to see the pattern
rg -n "addItem\(" tmf/org.eclipse.tracecompass.tmf.ui/src/org/eclipse/tracecompass/tmf/ui/viewers/xychart/ --type java -B2 -A1
# Also check if TmfCommonXLineChartTooltipProvider has any other addItem delegation
rg -n "addItem" tmf/org.eclipse.tracecompass.tmf.ui/src/org/eclipse/tracecompass/tmf/ui/viewers/xychart/linechart/TmfCommonXLineChartTooltipProvider.javaRepository: eclipse-tracecompass/org.eclipse.tracecompass
Length of output: 4141
🏁 Script executed:
# Check TmfSimpleTooltipProvider structure to understand the pattern
fd -t f "TmfSimpleTooltipProvider.java" --exec cat -n {}Repository: eclipse-tracecompass/org.eclipse.tracecompass
Length of output: 4135
🏁 Script executed:
# Check TmfClosestDataPointTooltipProvider structure
fd -t f "TmfClosestDataPointTooltipProvider.java" --exec head -150 {} \;Repository: eclipse-tracecompass/org.eclipse.tracecompass
Length of output: 6622
🏁 Script executed:
# Check if TmfCommonXLineChartTooltipProvider might override addItem or delegate
rg -n "protected.*void.*addItem|private.*void.*addItem|public.*void.*addItem" tmf/org.eclipse.tracecompass.tmf.ui/src/org/eclipse/tracecompass/tmf/ui/viewers/xychart/linechart/ --type java
# Check complete inheritance chain for any delegation mechanism
fd -t f "TmfCommonXLineChartTooltipProvider.java" -exec grep -n "extends\|implements" {}Repository: eclipse-tracecompass/org.eclipse.tracecompass
Length of output: 319
🏁 Script executed:
# Check if TmfCommonXLineChartTooltipProvider might override addItem
rg "protected.*void.*addItem|private.*void.*addItem|public.*void.*addItem" tmf/org.eclipse.tracecompass.tmf.ui/src/org/eclipse/tracecompass/tmf/ui/viewers/xychart/linechart/TmfCommonXLineChartTooltipProvider.java --type java
# Check complete inheritance chain
cat -n tmf/org.eclipse.tracecompass.tmf.ui/src/org/eclipse/tracecompass/tmf/ui/viewers/xychart/linechart/TmfCommonXLineChartTooltipProvider.java | head -50Repository: eclipse-tracecompass/org.eclipse.tracecompass
Length of output: 2575
🏁 Script executed:
# Verify if this code actually compiles - check for build artifacts or compilation results
find . -name "TmfCommonXLineChartTooltipProvider.class" 2>/dev/null
# Check git history to see if this code was recently changed
cd tmf/org.eclipse.tracecompass.tmf.ui/src/org/eclipse/tracecompass/tmf/ui/viewers/xychart/linechart/ && git log --oneline -n 20 TmfCommonXLineChartTooltipProvider.java 2>/dev/null || echo "Git history not available"Repository: eclipse-tracecompass/org.eclipse.tracecompass
Length of output: 146
🏁 Script executed:
# Check the recent refactoring commit to understand the change
cd tmf/org.eclipse.tracecompass.tmf.ui/src/org/eclipse/tracecompass/tmf/ui/viewers/xychart/linechart/ && git show f7a2611d --statRepository: eclipse-tracecompass/org.eclipse.tracecompass
Length of output: 50402
🏁 Script executed:
# Get just the diff for the specific file in the commit
cd tmf/org.eclipse.tracecompass.tmf.ui/src/org/eclipse/tracecompass/tmf/ui/viewers/xychart/linechart/ && git show f7a2611d -- TmfCommonXLineChartTooltipProvider.javaRepository: eclipse-tracecompass/org.eclipse.tracecompass
Length of output: 7502
🏁 Script executed:
# Verify if this file might have been changed or if there's an override of addItem
rg -B5 -A5 "protected.*void.*addItem\|private.*void.*addItem\|public.*void.*addItem" tmf/org.eclipse.tracecompass.tmf.ui/src/org/eclipse/tracecompass/tmf/ui/viewers/xychart/TmfBaseProvider.javaRepository: eclipse-tracecompass/org.eclipse.tracecompass
Length of output: 71
🏁 Script executed:
# Final check: search for any delegation pattern that might make addItem accessible
rg "addItem" tmf/org.eclipse.tracecompass.tmf.ui/src/org/eclipse/tracecompass/tmf/ui/viewers/xychart/TmfBaseProvider.javaRepository: eclipse-tracecompass/org.eclipse.tracecompass
Length of output: 71
Fix method accessibility issue in addSeriesTooltipItem()
The addSeriesTooltipItem() method on the outer class calls addItem() at lines 108 and 110, but addItem() is defined in TmfAbstractToolTipHandler, which only the inner class CommonToolTipHandler extends. The outer class TmfCommonXLineChartTooltipProvider extends TmfBaseProvider, which does not provide access to addItem(). This results in a compilation error.
To resolve, either:
- Move
addSeriesTooltipItem()andformatSeriesLabel()logic into theCommonToolTipHandler.fill()method - Pass the handler instance as a parameter:
addSeriesTooltipItem(fToolTipHandler, ...) - Delegate calls through the handler:
fToolTipHandler.addItem(...)instead of directaddItem()calls
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@tmf/org.eclipse.tracecompass.tmf.ui/src/org/eclipse/tracecompass/tmf/ui/viewers/xychart/linechart/TmfCommonXLineChartTooltipProvider.java`
around lines 99 - 112, The compilation fails because addSeriesTooltipItem() in
TmfCommonXLineChartTooltipProvider calls addItem() which is defined on
TmfAbstractToolTipHandler, not the outer class; update addSeriesTooltipItem(...)
to use the tooltip handler instance (fToolTipHandler) to add items instead of
calling addItem() directly: replace the direct addItem(...) calls with
fToolTipHandler.addItem(...) (or change the method signature to accept a
TmfAbstractToolTipHandler parameter and call handler.addItem(...)); ensure
formatSeriesLabel(...) remains accessible (move into CommonToolTipHandler or
make it package-private/static if needed) and update any callers accordingly
(e.g., CommonToolTipHandler.fill() should call
addSeriesTooltipItem(fToolTipHandler, ... ) or the modified method that accepts
the handler).
f7a2611 to
38d85da
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tmf/org.eclipse.tracecompass.tmf.ui/src/org/eclipse/tracecompass/tmf/ui/viewers/xychart/linechart/TmfCommonXLineChartTooltipProvider.java (1)
145-148: Add defensive check before accessinggetYAxes()[0].The code accesses
getYAxes()[0]without verifying the array is non-empty. While line charts typically have at least one Y axis, a defensive check would prevent a potentialArrayIndexOutOfBoundsExceptionin edge cases.🛡️ Proposed fix
Format format = null; if (getChartViewer() instanceof TmfCommonXAxisChartViewer chartViewer) { - format = chartViewer.getSwtChart().getAxisSet().getYAxes()[0].getTick().getFormat(); + org.eclipse.swtchart.IAxis[] yAxes = chartViewer.getSwtChart().getAxisSet().getYAxes(); + if (yAxes.length > 0) { + format = yAxes[0].getTick().getFormat(); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tmf/org.eclipse.tracecompass.tmf.ui/src/org/eclipse/tracecompass/tmf/ui/viewers/xychart/linechart/TmfCommonXLineChartTooltipProvider.java` around lines 145 - 148, The code in TmfCommonXLineChartTooltipProvider accesses chartViewer.getSwtChart().getAxisSet().getYAxes()[0] without verifying the Y axes array is present or non-empty; change the logic to fetch the YAxes array into a local variable (e.g., Axis[] yAxes = chartViewer.getSwtChart().getAxisSet().getYAxes()), check that yAxes != null && yAxes.length > 0 before calling yAxes[0].getTick().getFormat(), and leave format as null or use a safe fallback if the check fails so no ArrayIndexOutOfBoundsException occurs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@tmf/org.eclipse.tracecompass.tmf.ui/src/org/eclipse/tracecompass/tmf/ui/viewers/xychart/linechart/TmfCommonXLineChartTooltipProvider.java`:
- Around line 145-148: The code in TmfCommonXLineChartTooltipProvider accesses
chartViewer.getSwtChart().getAxisSet().getYAxes()[0] without verifying the Y
axes array is present or non-empty; change the logic to fetch the YAxes array
into a local variable (e.g., Axis[] yAxes =
chartViewer.getSwtChart().getAxisSet().getYAxes()), check that yAxes != null &&
yAxes.length > 0 before calling yAxes[0].getTick().getFormat(), and leave format
as null or use a safe fallback if the check fails so no
ArrayIndexOutOfBoundsException occurs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8b222e55-2bb7-4f10-8689-e68af601c08c
📒 Files selected for processing (1)
tmf/org.eclipse.tracecompass.tmf.ui/src/org/eclipse/tracecompass/tmf/ui/viewers/xychart/linechart/TmfCommonXLineChartTooltipProvider.java
TranHoangThanhDuy
force-pushed
the
patch-1
branch
from
38d85da to
7e4f1aa
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
tmf/org.eclipse.tracecompass.tmf.ui/src/org/eclipse/tracecompass/tmf/ui/viewers/xychart/linechart/TmfCommonXLineChartTooltipProvider.java (2)
114-121:⚠️ Potential issue | 🟠 MajorEscape
keybefore embedding it into HTML tooltip markup.Line 115/Line 120 uses raw series IDs inside HTML. Labels containing
<,&, or quotes can break rendering and inject unintended markup.Proposed fix
protected String formatSeriesLabel(IXYSeries series, String key) { - String label = (key == null) ? "" : key; //$NON-NLS-1$ + String label = escapeHtml((key == null) ? "" : key); //$NON-NLS-1$ Color color = series.getColor(); if (color != null) { RGBA rgba = color.getRGBA(); RGBAColor rgbaColor = new RGBAColor(rgba.rgb.red, rgba.rgb.green, rgba.rgb.blue, rgba.alpha); label = String.format(TmfCommonXLineChartTooltipProvider.HTML_COLOR_TOOLTIP, rgbaColor, label); } return label; } + + private static String escapeHtml(String value) { + return value + .replace("&", "&") //$NON-NLS-1$ //$NON-NLS-2$ + .replace("<", "<") //$NON-NLS-1$ //$NON-NLS-2$ + .replace(">", ">") //$NON-NLS-1$ //$NON-NLS-2$ + .replace("\"", """) //$NON-NLS-1$ //$NON-NLS-2$ + .replace("'", "'"); //$NON-NLS-1$ //$NON-NLS-2$ + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tmf/org.eclipse.tracecompass.tmf.ui/src/org/eclipse/tracecompass/tmf/ui/viewers/xychart/linechart/TmfCommonXLineChartTooltipProvider.java` around lines 114 - 121, The series label is inserted raw into HTML in formatSeriesLabel(IXYSeries series, String key), which can break rendering or allow injection; before using key (the series ID) in the String.format(...) with HTML_COLOR_TOOLTIP, HTML-escape special characters (&, <, >, " and ') and preserve the existing null/empty handling so the escaped value is used instead of the raw key; update formatSeriesLabel to replace or use a utility to perform HTML escaping on key prior to embedding it in the tooltip string.
99-112:⚠️ Potential issue | 🔴 CriticalCompilation issue:
addItem(...)is invoked outsideTmfAbstractToolTipHandlercontext.At Line 108 and Line 110,
addItem(...)is called fromTmfCommonXLineChartTooltipProvider. That method is defined on the tooltip handler, so this call site is invalid and breaks compilation.Proposed fix
+ `@FunctionalInterface` + protected interface TooltipItemSink { + void add(ToolTipString label, ToolTipString value); + } + - protected void addSeriesTooltipItem(IXYSeries series, String key, int index, Format format) { + protected void addSeriesTooltipItem(IXYSeries series, String key, int index, Format format, TooltipItemSink sink) { double[] ySeries = series.getYSeries(); if (ySeries == null || index < 0 || index >= ySeries.length) { return; } String label = formatSeriesLabel(series, key); double yValue = ySeries[index]; - if (format == null) { - addItem(null, ToolTipString.fromHtml(label), ToolTipString.fromDecimal(yValue)); - } else { - addItem(null, ToolTipString.fromHtml(label), ToolTipString.fromString(format.format(yValue))); - } + ToolTipString value = (format == null) + ? ToolTipString.fromDecimal(yValue) + : ToolTipString.fromString(format.format(yValue)); + sink.add(ToolTipString.fromHtml(label), value); } @@ - addSeriesTooltipItem(xySeries, key, index, format); + addSeriesTooltipItem(xySeries, key, index, format, (label, value) -> addItem(null, label, value));Also applies to: 162-162
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tmf/org.eclipse.tracecompass.tmf.ui/src/org/eclipse/tracecompass/tmf/ui/viewers/xychart/linechart/TmfCommonXLineChartTooltipProvider.java` around lines 99 - 112, The calls to addItem(...) inside TmfCommonXLineChartTooltipProvider are invalid because addItem is a method on TmfAbstractToolTipHandler; update addSeriesTooltipItem (and the similar call in addEventsTooltipItem) to obtain the tooltip handler (via getToolTipHandler()) and invoke handler.addItem(...), guarding with a null check on the handler before calling; ensure you pass the same arguments (ToolTipString.fromHtml(label), ToolTipString.fromDecimal/ fromString(format...)) and remove the direct addItem(...) calls in TmfCommonXLineChartTooltipProvider.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@tmf/org.eclipse.tracecompass.tmf.ui/src/org/eclipse/tracecompass/tmf/ui/viewers/xychart/linechart/TmfCommonXLineChartTooltipProvider.java`:
- Around line 114-121: The series label is inserted raw into HTML in
formatSeriesLabel(IXYSeries series, String key), which can break rendering or
allow injection; before using key (the series ID) in the String.format(...) with
HTML_COLOR_TOOLTIP, HTML-escape special characters (&, <, >, " and ') and
preserve the existing null/empty handling so the escaped value is used instead
of the raw key; update formatSeriesLabel to replace or use a utility to perform
HTML escaping on key prior to embedding it in the tooltip string.
- Around line 99-112: The calls to addItem(...) inside
TmfCommonXLineChartTooltipProvider are invalid because addItem is a method on
TmfAbstractToolTipHandler; update addSeriesTooltipItem (and the similar call in
addEventsTooltipItem) to obtain the tooltip handler (via getToolTipHandler())
and invoke handler.addItem(...), guarding with a null check on the handler
before calling; ensure you pass the same arguments
(ToolTipString.fromHtml(label), ToolTipString.fromDecimal/
fromString(format...)) and remove the direct addItem(...) calls in
TmfCommonXLineChartTooltipProvider.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6527fdc3-f10e-4b10-9d1f-1a74fb0812fc
📒 Files selected for processing (1)
tmf/org.eclipse.tracecompass.tmf.ui/src/org/eclipse/tracecompass/tmf/ui/viewers/xychart/linechart/TmfCommonXLineChartTooltipProvider.java
Refactor TmfCommonXLineChartTooltipProvider to make the tooltip population logic extensible for subclasses via protected hooks, while preserving the default tooltip behavior. Also remove unused maxLen calculation that was left over from an earlier implementation. Files: - org.eclipse.tracecompass.tmf.ui.viewers.xychart.linechart.TmfCommonXLineChartTooltipProvider
TranHoangThanhDuy
force-pushed
the
patch-1
branch
from
4e48b21 to
79afdc2
Compare
|
Hello @jonahgraham , @MatthewKhouzam , This is a refactor of TmfCommonXLineChartTooltipProvider. Could someone please review my change? |
jonahgraham
left a comment
jonahgraham
left a comment
There was a problem hiding this comment.
The goal is to make the tooltip handling easier to extend (via small overridable hooks) while preserving the existing default behavior (time item + per-series value).
Thanks for the PR - the general idea seems good to me. In addition I like removing the calculation of maxLen that was no longer used anymore.
I am concerned that too much new API is added that isn't maintainable in the future, or makes this class too hard to update/evolve. Each new API method should be documented for its purpose and only make methods API that are need to be API.
Therefore, please change the new protected methods to private or document them.
Additionally, can you also complete the "what it does/how to test/etc" fields from the PR template in the top comment.
| return getChartViewer().getWindowDuration() != 0; | ||
| } | ||
|
|
||
| protected int getHoveredIndex(List<IXYSeries> series, double xCoordinate) { |
There was a problem hiding this comment.
Does this need to be new API? Can extenders have a different algorithm to calculate the index?
There was a problem hiding this comment.
Good point. It doesn’t strictly need to be new API. I extracted it mainly to keep fill() smaller/readable and to avoid repeating the index calculation logic.
If you prefer not to expose this as an extension point, I can make getHoveredIndex(...) private.
How about this idea?
There was a problem hiding this comment.
Good idea - having the functionality extracted to make fill easier to read is a good idea. Lets keep all these new methods private unless explicitly needed to support extenders.
MatthewKhouzam
left a comment
MatthewKhouzam
left a comment
There was a problem hiding this comment.
I like this, let's get it in. I gave a light nit on the logic.
| } | ||
|
|
||
| String firstValidSeriesKey = null; | ||
| for (IXYSeries xySeries : series) { |
There was a problem hiding this comment.
can you make a copy of the list which is valid only? this could simplify the logic
|
Also, please ssh / gpg sign your commit to prevent impersonation attacks. |
|
Note that this code does not compile. Please let me know if you are having trouble setting up the development environment so you can develop and test your changes properly Error from the build log:
|
3 participants
Refactor TmfCommonXLineChartTooltipProvider to make the tooltip population logic extensible for subclasses via protected hooks, while preserving the default tooltip behavior.
Also remove unused maxLen calculation that was left over from an earlier implementation.
Files:
What it does
How to test
Follow-ups
Review checklist
Summary by CodeRabbit