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
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
TranHoangThanhDuy
force-pushed
the
patch-1
branch
from
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
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.
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.
|
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:
|
TranHoangThanhDuy
force-pushed
the
patch-1
branch
from
1a33cc3 to
7bbfeea
Compare
|
Hi @jonahgraham , |
TranHoangThanhDuy
force-pushed
the
patch-1
branch
from
d85eb5b to
160eff8
Compare
|
@jonahgraham , @MatthewKhouzam , |
TranHoangThanhDuy
force-pushed
the
patch-1
branch
7 times, most recently
from
ad88702 to
398e0c3
Compare
jonahgraham
left a comment
jonahgraham
left a comment
There was a problem hiding this comment.
I'll run this change up in my dev environment once the CI reports it compiles - at which point I can complete my review.
@jonahgraham , @MatthewKhouzam , I'm building it using the command line and it will take some time to confirm the build results. However, I've used this source code for another project that uses Trace Compass and it worked normally. Therefore, I think we can continue reviewing this PR.
@TranHoangThanhDuy this sounds like you have not setup a dev environment to edit and test out changes in TraceCompass - please follow instructions in https://github.com/eclipse-tracecompass/org.eclipse.tracecompass/blob/master/DEVELOPMENT_ENV_SETUP.md
I recommend "Using the Eclipse Installer (Oomph)" part of the document. I have just tested it and the instructions worked fine. There are three errors in the workspace that can be ignored #387
| void addItem(ToolTipString key, ToolTipString value); | ||
| } | ||
|
|
||
| protected void addAdditionalTooltipItems(ITooltipItemAdder adder, double xCoordinate) { |
There was a problem hiding this comment.
Methods that are API should be documented as to their purpose (to be overridden or only called by subclasses)
There was a problem hiding this comment.
I added the description for APIs.
| return label; | ||
| } | ||
|
|
||
| protected String getFirstValidSeriesKey() { |
There was a problem hiding this comment.
Sorry for missing it.
That method is designed for using in the subclass.
In fact, I needed to get the first valid series key in the subclass implementation, so I created this method.
Hi @jonahgraham -san, Besides the three errors you pointed out, I encountered another one, but it wasn't a major issue: |
TranHoangThanhDuy
force-pushed
the
patch-1
branch
from
398e0c3 to
64dd8d6
Compare
TranHoangThanhDuy
force-pushed
the
patch-1
branch
from
64dd8d6 to
fd9280a
Compare
|
The project is working towards a release in the coming days, once that is done a full review from a project committer can be completed. We discussed this during today's project meeting |
|
Hi @jonahgraham, @MatthewKhouzam, |
arfio
left a comment
arfio
left a comment
There was a problem hiding this comment.
Thank you for this patch, just minor fixes and I will gladly approve. I like to remove as many warnings as possible as sometimes, they will highlight possible errors.
|
|
||
| import java.text.Format; | ||
| import java.util.Arrays; | ||
| import java.util.function.BiConsumer; |
There was a problem hiding this comment.
When using Eclipse IDE, use ctrl+shift+o to fix/sort imports.
There was a problem hiding this comment.
Thank you for this helpful guide.
| * time offset). | ||
| * @since 9.2 | ||
| */ | ||
| protected void addAdditionalTooltipItems(BiConsumer<ToolTipString, ToolTipString> adder, double xCoordinate) { |
There was a problem hiding this comment.
Use @nonnull to suppress type annotation warnings when using this method ->
addAdditionalTooltipItems(BiConsumer<@NonNull ToolTipString, @NonNull ToolTipString> adder, double xCoordinate)
| ITmfTimestamp time = TmfTimestamp.fromNanos(timeNanos); | ||
| adder.accept( | ||
| ToolTipString.fromString(Messages.TmfCommonXLineChartTooltipProvider_time), | ||
| ToolTipString.fromTimestamp(time.toString(), time.toNanos())); |
There was a problem hiding this comment.
Use Objects.requireNonNull() method to suppress null annotation warning. This method is ok to use when it is guaranteed that the value will never be null. In this case the timestamp is derived from an unboxed value, so it should not be null and the message is a constant.
There was a problem hiding this comment.
I used Objects.requireNonNull() method. Could you please check it again?
| * formatter). If {@code null}, a default decimal formatting is used. | ||
| * @since 9.2 | ||
| */ | ||
| protected void addSeriesTooltipItem(BiConsumer<ToolTipString, ToolTipString> adder, IXYSeries xySeries, int index, Format format) { |
There was a problem hiding this comment.
Same as above, use @nonnull on the ToolTipString types
| } else { | ||
| adder.accept( | ||
| ToolTipString.fromHtml(label), | ||
| ToolTipString.fromString(format.format(yValue))); |
There was a problem hiding this comment.
Use Objects.requireNonNull() as the yValue is unboxed, it should never be null
| * @return The formatted label (potentially containing HTML). | ||
| * @since 9.2 | ||
| */ | ||
| protected static final String formatSeriesLabel(IXYSeries xySeries) { |
| * The hovered X coordinate in data space (i.e., in the same domain as | ||
| * the series X values, typically nanoseconds relative to the viewer's | ||
| * time offset). | ||
| * @since 9.2 |
There was a problem hiding this comment.
The version number should be upgrade from 9.2.0 to 9.3.0 in the file tmf/org.eclipse.tracecompass.tmf.ui/META-INF/MANIFEST.MF. Then all these descriptions should be updated to since 9.3
There was a problem hiding this comment.
Thanks.
I have upgraded version number in MANIFEST.MF to 9.3.0 and method descriptions are also updated.
Please check it.
There was a problem hiding this comment.
I forgot that you also need to update the pom.xml
There was a problem hiding this comment.
Hi @arfio ,
I updated the pom.xml in my local env
...
<artifactId>org.eclipse.tracecompass.tmf.ui</artifactId>
<version>9.3.0-SNAPSHOT</version> <!-- this line was updated -->
...Then some errors occur.
It requires some locations update to 9.3.0
/org.eclipse.tracecompass.tmf.ui/src/org/eclipse/tracecompass/tmf/ui/widgets/timegraph/dialogs/MultiTreePatternFilter.java
/org.eclipse.tracecompass.tmf.ui/src/org/eclipse/tracecompass/tmf/ui/viewers/tree/AbstractSelectTreeViewer3.java
Should I update those?
There was a problem hiding this comment.
Hi @arfio
I have updated to fix issue I mentioned above in the files
-
/org.eclipse.tracecompass.tmf.ui/src/org/eclipse/tracecompass/tmf/ui/widgets/timegraph/dialogs/MultiTreePatternFilter.java
-
/org.eclipse.tracecompass.tmf.ui/src/org/eclipse/tracecompass/tmf/ui/viewers/tree/AbstractSelectTreeViewer3.java
If this is unnecessary, please let me know.
There was a problem hiding this comment.
I removed these changes, and the build works without any errors. Also, as these files were not changed, I think you should change it back to 9.2.
There was a problem hiding this comment.
@TranHoangThanhDuy Sorry I should have been clearer, you should keep the pom.xml and the MANIFEST.MF change and the 9.3 in the file you modified. The errors that you have in the other 2 files are because the baseline was updated and I think you need to refresh it.
There was a problem hiding this comment.
Hi @arfio , thank you for making it clear.
I have synchronized my branch with eclipse-tracecompass:master.
Then, I updated pom.xml, MANIFEST.MF and the file I modified to 9.3.
Please help me confirm.
TranHoangThanhDuy
force-pushed
the
patch-1
branch
from
fd9280a to
21caedb
Compare
TranHoangThanhDuy
force-pushed
the
patch-1
branch
4 times, most recently
from
a16d3d4 to
0ce0612
Compare
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
0ce0612 to
17833a3
Compare
5 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