Skip to content

Conversation

@ShadowK2494
Copy link

@ShadowK2494 ShadowK2494 commented Dec 5, 2025

What it does

  • Makes AbstractSelectTreeViewer2 injectable to improve flexibility and integration with dependency injection frameworks.
  • Adds AbstractSelectTreeViewer3, which extends the tree viewer functionality to support column-aware filtering using SpecificColumnPatternFilter.
  • Introduces MultiTreePatternFilter2 to allow filtering based on a specific column index, enhancing usability for multi-column trees.
  • Updates bundle version to 9.2.0 to reflect new features and API changes.

How to test

  1. Open a view that uses AbstractSelectTreeViewer3 or configure a tree viewer with SpecificColumnPatternFilter .
  2. Apply multiple filter patterns separated by / and verify that filtering works correctly for the specified column index.
  3. Check that dependency injection works for AbstractSelectTreeViewer2 without breaking existing functionality.

Follow-ups

  • Consider adding unit tests for column-aware filtering logic.
  • Update documentation for new classes and usage examples.

Review checklist

  • Verify that existing tree viewers still work as expected:
    • Test with Memory Transaction View (uses AbstractSelectTreeViewer2)
  • Confirm that column-aware filtering works in views using AbstractSelectTreeViewer3
  • Validate that dependency injection for AbstractSelectTreeViewer2 does not break existing functionality
  • Check bundle version updated to 9.2.0 and no regressions in plugin startup
  • As an author, I have thoroughly tested my changes and carefully followed the instructions in this template

Summary by CodeRabbit

  • New Features

    • Column-specific pattern filtering for tree/timegraph viewers.
    • Increased extensibility of tree viewers to support subclassing and column-based customization.
  • Chores

    • Project/bundle version updated to 9.2.0 (artifact version bumped).

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link

coderabbitai bot commented Dec 5, 2025

Walkthrough

Bumps TMF UI bundle and POM versions; relaxes a tree viewer constructor to protected; adds AbstractSelectTreeViewer3 which builds a multi-select, full-row TriStateFilteredCheckboxTree with a column-specific filter; makes MultiTreePatternFilter column index configurable and adds SpecificColumnPatternFilter.

Changes

Cohort / File(s) Summary
Bundle & POM version
tmf/org.eclipse.tracecompass.tmf.ui/META-INF/MANIFEST.MF, tmf/org.eclipse.tracecompass.tmf.ui/pom.xml
Bundle-Version updated to 9.2.0.qualifier in MANIFEST.MF; POM version bumped from 9.1.2-SNAPSHOT to 9.2.0-SNAPSHOT. No other manifest fields changed.
Tree viewer base / subclass
tmf/org.eclipse.tracecompass.tmf.ui/src/org/eclipse/tracecompass/tmf/ui/viewers/tree/AbstractSelectTreeViewer2.java, tmf/org.eclipse.tracecompass.tmf.ui/src/org/eclipse/tracecompass/tmf/ui/viewers/tree/AbstractSelectTreeViewer3.java
Constructor visibility of AbstractSelectTreeViewer2(...) changed from private to protected. New public abstract AbstractSelectTreeViewer3 added; its constructor creates a TriStateFilteredCheckboxTree configured for multi-selection and full-row highlight and installs a SpecificColumnPatternFilter for a specified column index.
Pattern filtering classes
tmf/org.eclipse/tracecompass/tmf.ui/src/.../widgets/timegraph/dialogs/MultiTreePatternFilter.java, tmf/org.eclipse/tracecompass/tmf.ui/src/.../widgets/timegraph/dialogs/SpecificColumnPatternFilter.java
MultiTreePatternFilter replaces hard-coded column index with a protected getIndexColumnFilter() (default 0). New SpecificColumnPatternFilter class added to store a target column index, provide a public constructor, and override getIndexColumnFilter() to return the configured index.

Sequence Diagram(s)

sequenceDiagram
    participant Parent as Composite (parent)
    participant Viewer as AbstractSelectTreeViewer3
    participant Tree as TriStateFilteredCheckboxTree
    participant Filter as SpecificColumnPatternFilter

    Parent ->> Viewer: new AbstractSelectTreeViewer3(parent, legendIndex, id, indexColumnFilter)
    Viewer ->> Tree: create TriStateFilteredCheckboxTree(multi-select, full-row)
    Viewer ->> Filter: new SpecificColumnPatternFilter(indexColumnFilter)
    Viewer ->> Tree: installFilter(Filter)
    Tree ->> Filter: evaluate rows using getIndexColumnFilter()
    Note right of Tree: Filter applies patterns only on configured column
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

  • Verify constructor visibility change does not expose/internalize unintended APIs.
  • Review AbstractSelectTreeViewer3 initialization (selection mode, full-row highlight) and lifecycle integration.
  • Check bounds-checking and null-safety for configured column indices in filtering.
  • Confirm MANIFEST.MF and POM version strings are consistent and intended.

Suggested reviewers

  • bhufmann

"I hopped through branches and tuned a view,
I nudged a constructor to welcome the new crew,
A chosen column where patterns may play,
Filters whisper softly and prune what they may,
Hop, filter, bloom — a small rabbit's hooray."

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main changes: making AbstractSelectTreeViewer2 injectable, adding AbstractSelectTreeViewer3, and introducing SpecificColumnPatternFilter for column-aware filtering, which directly aligns with the changeset.
Docstring Coverage ✅ Passed Docstring coverage is 83.33% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (3)
tmf/org.eclipse.tracecompass.tmf.ui/src/org/eclipse/tracecompass/tmf/ui/widgets/timegraph/dialogs/SpecificColumnPatternFilter.java (2)

27-27: Consider more descriptive field naming.

The field name index is generic. Consider using a more descriptive name following the Eclipse/TraceCompass conventions, such as fColumnIndex or fIndexColumnFilter (with f prefix for fields).

Apply this diff for improved clarity:

-    private int index = 0;
+    private final int fColumnIndex;

And update the constructor and getter:

     public SpecificColumnPatternFilter(int indexColumnFilter) {
-        this.index = indexColumnFilter;
+        this.fColumnIndex = indexColumnFilter;
     }
     
     @Override
     protected int getIndexColumnFilter() {
-        return this.index;
+        return this.fColumnIndex;
     }

34-36: Consider validation for column index parameter.

The constructor accepts any integer value for indexColumnFilter, including negative values. While the Eclipse SWT framework might handle negative indices gracefully, consider adding validation to ensure the column index is non-negative or documenting the expected range.

Add validation if negative indices are not supported:

     public SpecificColumnPatternFilter(int indexColumnFilter) {
+        if (indexColumnFilter < 0) {
+            throw new IllegalArgumentException("Column index must be non-negative");
+        }
         this.index = indexColumnFilter;
     }

Alternatively, document the acceptable range in the Javadoc:

     /**
      * Creates a new filter with the specified column index.
-     * @param indexColumnFilter the index of the column to apply the filter on
+     * @param indexColumnFilter the index of the column to apply the filter on (must be >= 0)
      */
tmf/org.eclipse.tracecompass.tmf.ui/src/org/eclipse/tracecompass/tmf/ui/viewers/tree/AbstractSelectTreeViewer3.java (1)

44-44: Consider removing redundant inline comment.

The inline comment "Initialize the tree viewer with a filtered checkbox tree and column-based filtering" restates what's already clear from the constructor's Javadoc and the code itself.

Apply this diff to remove the redundant comment:

-        // Initialize the tree viewer with a filtered checkbox tree and column-based filtering
         super(parent, new TriStateFilteredCheckboxTree(parent,
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 835eca3 and 84b5fff.

📒 Files selected for processing (5)
  • tmf/org.eclipse.tracecompass.tmf.ui/META-INF/MANIFEST.MF (3 hunks)
  • tmf/org.eclipse.tracecompass.tmf.ui/src/org/eclipse/tracecompass/tmf/ui/viewers/tree/AbstractSelectTreeViewer2.java (1 hunks)
  • tmf/org.eclipse.tracecompass.tmf.ui/src/org/eclipse/tracecompass/tmf/ui/viewers/tree/AbstractSelectTreeViewer3.java (1 hunks)
  • tmf/org.eclipse.tracecompass.tmf.ui/src/org/eclipse/tracecompass/tmf/ui/widgets/timegraph/dialogs/MultiTreePatternFilter.java (2 hunks)
  • tmf/org.eclipse.tracecompass.tmf.ui/src/org/eclipse/tracecompass/tmf/ui/widgets/timegraph/dialogs/SpecificColumnPatternFilter.java (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tmf/org.eclipse.tracecompass.tmf.ui/src/org/eclipse/tracecompass/tmf/ui/viewers/tree/AbstractSelectTreeViewer3.java (2)
tmf/org.eclipse.tracecompass.tmf.ui/src/org/eclipse/tracecompass/tmf/ui/widgets/timegraph/dialogs/SpecificColumnPatternFilter.java (1)
  • SpecificColumnPatternFilter (26-46)
tmf/org.eclipse.tracecompass.tmf.ui/src/org/eclipse/tracecompass/tmf/ui/widgets/timegraph/dialogs/TriStateFilteredCheckboxTree.java (1)
  • TriStateFilteredCheckboxTree (43-249)
🔇 Additional comments (7)
tmf/org.eclipse.tracecompass.tmf.ui/src/org/eclipse/tracecompass/tmf/ui/viewers/tree/AbstractSelectTreeViewer2.java (1)

223-224: LGTM! Constructor visibility change enables subclassing.

Changing the constructor from private to protected appropriately enables the extensibility required by AbstractSelectTreeViewer3 without breaking the existing public API.

tmf/org.eclipse.tracecompass.tmf.ui/src/org/eclipse/tracecompass/tmf/ui/widgets/timegraph/dialogs/MultiTreePatternFilter.java (2)

77-78: LGTM! Refactoring enables column-specific filtering.

The change from a hard-coded column index to getIndexColumnFilter() appropriately enables subclasses to customize the filtering column while preserving the default behavior (column 0).


89-96: LGTM! Well-documented extensibility point.

The new protected method provides a clean extensibility point for subclasses. The default return value of 0 maintains backward compatibility.

tmf/org.eclipse.tracecompass.tmf.ui/META-INF/MANIFEST.MF (3)

5-5: LGTM! Version bump is appropriate.

The version bump to 9.2.0 correctly reflects the addition of new public API elements (getIndexColumnFilter(), SpecificColumnPatternFilter, AbstractSelectTreeViewer3).


156-156: LGTM! Optional import is appropriate.

The traceeventlogger package is correctly marked as optional, which is appropriate for an optional dependency used for logging/tracing purposes.


33-35: Verify necessity of new bundle dependencies.

The added bundle dependencies (org.eclipse.equinox.p2.ui and org.eclipse.ui.browser) should be verified as required for the changes in this PR (column-aware filtering, tree viewer extensibility). Search the codebase for imports or usage of these packages in the modified Java files to confirm they support the new functionality, or remove them if unused.

tmf/org.eclipse.tracecompass.tmf.ui/src/org/eclipse/tracecompass/tmf/ui/viewers/tree/AbstractSelectTreeViewer3.java (1)

43-48: LGTM! Clean extension of AbstractSelectTreeViewer2.

The implementation correctly leverages the newly protected constructor of AbstractSelectTreeViewer2 and integrates SpecificColumnPatternFilter for column-aware filtering. The constructor parameters are well-documented.

Comment on lines 1 to 10
/*******************************************************************************
* Copyright (c) 2017 Ericsson
*
* All rights reserved. This program and the accompanying materials are
* made available under the terms of the Eclipse Public License 2.0 which
* accompanies this distribution, and is available at
* https://www.eclipse.org/legal/epl-2.0/
*
* SPDX-License-Identifier: EPL-2.0
*******************************************************************************/
Copy link

@coderabbitai coderabbitai bot Dec 5, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Update copyright year for new file.

The copyright year is listed as 2017, but this is a new file being added in 2025. Update the copyright to reflect the correct year (e.g., "Copyright (c) 2025 Ericsson").

Apply this diff to update the copyright:

-/*******************************************************************************
- * Copyright (c) 2017 Ericsson
- *
+/*******************************************************************************
+ * Copyright (c) 2025 Ericsson
+ *
📝 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.

Suggested change
/*******************************************************************************
* Copyright (c) 2017 Ericsson
*
* All rights reserved. This program and the accompanying materials are
* made available under the terms of the Eclipse Public License 2.0 which
* accompanies this distribution, and is available at
* https://www.eclipse.org/legal/epl-2.0/
*
* SPDX-License-Identifier: EPL-2.0
*******************************************************************************/
/*******************************************************************************
* Copyright (c) 2025 Ericsson
*
* All rights reserved. This program and the accompanying materials are
* made available under the terms of the Eclipse Public License 2.0 which
* accompanies this distribution, and is available at
* https://www.eclipse.org/legal/epl-2.0/
*
* SPDX-License-Identifier: EPL-2.0
*******************************************************************************/
🤖 Prompt for AI Agents
In
tmf/org.eclipse.tracecompass.tmf.ui/src/org/eclipse/tracecompass/tmf/ui/widgets/timegraph/dialogs/SpecificColumnPatternFilter.java
around lines 1 to 10, the file header lists "Copyright (c) 2017 Ericsson" but
this is a new file added in 2025; update the copyright year to 2025 (e.g.,
"Copyright (c) 2025 Ericsson") and ensure the SPDX identifier and license text
remain unchanged.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please change the company name to your company, or you a an individual, Ericsson does not own the code.

Copy link

Choose a reason for hiding this comment

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

@MatthewKhouzam, thank you for the correction! You're absolutely right—the copyright holder should reflect the actual owner of the code, not Ericsson.

@ShadowK2494, please update the copyright header to replace "Ericsson" with either your company name (if this code is being contributed on behalf of your employer) or your own name (if you're contributing as an individual). Also update the year from 2017 to 2025, since this is a new file.

For example:

/*******************************************************************************
 * Copyright (c) 2025 [Your Company Name or Your Name]
 *
 * All rights reserved. This program and the accompanying materials are
 * made available under the terms of the Eclipse Public License 2.0 which
 * accompanies this distribution, and is available at
 * https://www.eclipse.org/legal/epl-2.0/
 *
 * SPDX-License-Identifier: EPL-2.0
 *******************************************************************************/

@MatthewKhouzam
Copy link
Contributor

Hi @ShadowK2494, Thank you for the PR, we will look at it soon, but until then, could you please sign the ECA? It is an essential step in our way of working, to make sure things are clean.

@@ -0,0 +1,49 @@
/*******************************************************************************
* Copyright (c) 2017, 2025 Ericsson, Draeger Auriga
Copy link
Contributor

Choose a reason for hiding this comment

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

Update please

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the reminder! I have signed the ECA and updated the commit as requested.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

♻️ Duplicate comments (1)
tmf/org.eclipse.tracecompass.tmf.ui/src/org/eclipse/tracecompass/tmf/ui/widgets/timegraph/dialogs/SpecificColumnPatternFilter.java (1)

1-10: Fix copyright year and holder in header.

This is a new file added in 2025, but the header says Copyright (c) 2015 Hong Anh. Update the year to 2025 and ensure the holder matches the actual copyright owner (your name or your company, per how you’re contributing under the ECA).

For example:

 /*******************************************************************************
- * Copyright (c) 2015 Hong Anh
+ * Copyright (c) 2025 Your Name or Your Company
  *

Keep the EPL-2.0 text and SPDX identifier unchanged.

🧹 Nitpick comments (2)
tmf/org.eclipse.tracecompass.tmf.ui/src/org/eclipse/tracecompass/tmf/ui/widgets/timegraph/dialogs/SpecificColumnPatternFilter.java (2)

26-36: Make the column index field immutable.

index is set only in the constructor and never changed; making it final better expresses intent and avoids accidental reassignment later.

-public class SpecificColumnPatternFilter extends MultiTreePatternFilter {
-    private int index = 0;
+public class SpecificColumnPatternFilter extends MultiTreePatternFilter {
+    private final int index;
@@
     public SpecificColumnPatternFilter(int indexColumnFilter) {
         this.index = indexColumnFilter;
     }

29-32: Clarify column index semantics in Javadoc.

Consider stating explicitly that the column index is 0‑based so callers don’t have to infer it from the underlying Tree APIs.

For example:

-     * @param indexColumnFilter the index of the column to apply the filter on
+     * @param indexColumnFilter the 0-based index of the column to apply the filter on
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 84b5fff and 2d69e68.

📒 Files selected for processing (2)
  • tmf/org.eclipse.tracecompass.tmf.ui/src/org/eclipse/tracecompass/tmf/ui/viewers/tree/AbstractSelectTreeViewer3.java (1 hunks)
  • tmf/org.eclipse.tracecompass.tmf.ui/src/org/eclipse/tracecompass/tmf/ui/widgets/timegraph/dialogs/SpecificColumnPatternFilter.java (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • tmf/org.eclipse.tracecompass.tmf.ui/src/org/eclipse/tracecompass/tmf/ui/viewers/tree/AbstractSelectTreeViewer3.java

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
tmf/org.eclipse.tracecompass.tmf.ui/src/org/eclipse/tracecompass/tmf/ui/widgets/timegraph/dialogs/SpecificColumnPatternFilter.java (1)

26-45: Consider validating the column index and making it immutable.

Functionally this is fine, but two small improvements would harden the API:

  • Guard against invalid indices (e.g., negative values), which are unlikely to be meaningful as column indices.
  • Make index final since it’s set only in the constructor.

For example:

-public class SpecificColumnPatternFilter extends MultiTreePatternFilter {
-    private int index = 0;
+public class SpecificColumnPatternFilter extends MultiTreePatternFilter {
+    private final int index;
@@
-    public SpecificColumnPatternFilter(int indexColumnFilter) {
-        this.index = indexColumnFilter;
+    public SpecificColumnPatternFilter(int indexColumnFilter) {
+        if (indexColumnFilter < 0) {
+            throw new IllegalArgumentException("indexColumnFilter must be >= 0");
+        }
+        this.index = indexColumnFilter;
     }

This keeps usage explicit and fails fast on accidental misuse.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2d69e68 and 5d66467.

📒 Files selected for processing (1)
  • tmf/org.eclipse.tracecompass.tmf.ui/src/org/eclipse/tracecompass/tmf/ui/widgets/timegraph/dialogs/SpecificColumnPatternFilter.java (1 hunks)
🔇 Additional comments (1)
tmf/org.eclipse.tracecompass.tmf.ui/src/org/eclipse/tracecompass/tmf/ui/widgets/timegraph/dialogs/SpecificColumnPatternFilter.java (1)

1-10: Header now matches contribution (year and author).

License header looks correct for a new 2025 file and uses the proper EPL-2.0 SPDX identifier. Nothing to change here.

…TreeViewer3 and MultiTreePatternFilter2 for column-aware filtering

Signed-off-by: Hong Anh <honganh.book@gmail.com>
@ShadowK2494 ShadowK2494 force-pushed the feature/tmf-ui-tree-filters branch from 5d66467 to 667b23e Compare December 8, 2025 09:18
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (2)
tmf/org.eclipse.tracecompass.tmf.ui/src/org/eclipse/tracecompass/tmf/ui/widgets/timegraph/dialogs/SpecificColumnPatternFilter.java (2)

27-27: Consider making the field final and removing redundant initialization.

The index field is set in the constructor and never modified afterward. Making it final would better express this immutability and prevent accidental modification.

Apply this diff:

-    private int index = 0;
+    private final int index;

The initialization to 0 is redundant since the constructor always sets it.


29-36: Consider validating the column index parameter.

The constructor accepts any int value for indexColumnFilter, including negative numbers. If an invalid column index is provided, it could lead to unexpected behavior when getColumnText() is called in the parent class's isLeafMatch() method.

Consider adding validation:

 public SpecificColumnPatternFilter(int indexColumnFilter) {
+    if (indexColumnFilter < 0) {
+        throw new IllegalArgumentException("Column index must be non-negative, got: " + indexColumnFilter);
+    }
     this.index = indexColumnFilter;
 }

Alternatively, document the valid range in the Javadoc:

 /**
  * Creates a new filter with the specified column index.
- * @param indexColumnFilter the index of the column to apply the filter on
+ * @param indexColumnFilter the index of the column to apply the filter on (must be >= 0)
  */
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5d66467 and 667b23e.

📒 Files selected for processing (5)
  • tmf/org.eclipse.tracecompass.tmf.ui/META-INF/MANIFEST.MF (3 hunks)
  • tmf/org.eclipse.tracecompass.tmf.ui/src/org/eclipse/tracecompass/tmf/ui/viewers/tree/AbstractSelectTreeViewer2.java (1 hunks)
  • tmf/org.eclipse.tracecompass.tmf.ui/src/org/eclipse/tracecompass/tmf/ui/viewers/tree/AbstractSelectTreeViewer3.java (1 hunks)
  • tmf/org.eclipse.tracecompass.tmf.ui/src/org/eclipse/tracecompass/tmf/ui/widgets/timegraph/dialogs/MultiTreePatternFilter.java (2 hunks)
  • tmf/org.eclipse.tracecompass.tmf.ui/src/org/eclipse/tracecompass/tmf/ui/widgets/timegraph/dialogs/SpecificColumnPatternFilter.java (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • tmf/org.eclipse.tracecompass.tmf.ui/src/org/eclipse/tracecompass/tmf/ui/viewers/tree/AbstractSelectTreeViewer3.java
🔇 Additional comments (7)
tmf/org.eclipse.tracecompass.tmf.ui/META-INF/MANIFEST.MF (3)

5-5: LGTM! Bundle version bump is appropriate for new API additions.

The minor version increment from 9.1.2 to 9.2.0 correctly reflects the addition of new public APIs (AbstractSelectTreeViewer3, SpecificColumnPatternFilter, and the protected getIndexColumnFilter() method).


156-156: LGTM! Import is correctly marked as optional.

The org.eclipse.tracecompass.traceeventlogger import is used in AbstractSelectTreeViewer2 (lines 75-76) and is appropriately marked with resolution:=optional for optional dependency support.


34-35: Verify that new bundle dependencies are actually used.

The PR adds org.eclipse.equinox.p2.ui and org.eclipse.ui.browser bundle dependencies to the manifest, but these are not referenced in any of the provided code changes. The PR description focuses on tree viewer filtering and does not mention P2 provisioning or browser functionality. Before merging, confirm these dependencies are required by searching the modified code for imports or usages of org.eclipse.equinox.p2.ui.* and org.eclipse.ui.browser.* packages. If unused, remove them to avoid unnecessary bundle bloat.

tmf/org.eclipse.tracecompass.tmf.ui/src/org/eclipse/tracecompass/tmf/ui/widgets/timegraph/dialogs/MultiTreePatternFilter.java (2)

77-78: LGTM! Clean refactoring to support dynamic column selection.

The change from a hard-coded column index to calling getIndexColumnFilter() is well-executed and maintains backward compatibility.


88-96: LGTM! Well-designed extension point for subclasses.

The new getIndexColumnFilter() method provides a clean template method pattern that allows subclasses to customize the filtering column while maintaining backward compatibility with a sensible default.

tmf/org.eclipse.tracecompass.tmf.ui/src/org/eclipse/tracecompass/tmf/ui/viewers/tree/AbstractSelectTreeViewer2.java (1)

223-224: LGTM! Constructor visibility change enables subclassing.

Changing the constructor from private to protected correctly enables AbstractSelectTreeViewer3 to extend this class. This is a safe API change since private members were not accessible to external code anyway.

Note: The PR title mentions making this class "injectable," but this visibility change alone doesn't add dependency injection support—it simply enables inheritance. If dependency injection framework support (e.g., JSR-330 @Inject) was the goal, additional annotations would be needed.

tmf/org.eclipse.tracecompass.tmf.ui/src/org/eclipse/tracecompass/tmf/ui/widgets/timegraph/dialogs/SpecificColumnPatternFilter.java (1)

38-45: LGTM! Clean override implementation.

The method correctly overrides the parent's getIndexColumnFilter() to return the configured column index.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 416ccd4 and 617a3ba.

📒 Files selected for processing (1)
  • tmf/org.eclipse.tracecompass.tmf.ui/META-INF/MANIFEST.MF (3 hunks)
🔇 Additional comments (2)
tmf/org.eclipse.tracecompass.tmf.ui/META-INF/MANIFEST.MF (2)

33-35: Validate new Require-Bundle entries across all target platforms

The added/updated requirements for:

  • org.eclipse.jdt.annotation;bundle-version="[2.0.0,3.0.0)";resolution:=optional
  • org.eclipse.equinox.p2.ui;bundle-version="2.8.500"
  • org.eclipse.ui.browser;bundle-version="3.8.300"

look fine, but please confirm:

  • These bundles (with at least the specified versions) are present in all supported product configurations.
  • The hard requirements on org.eclipse.equinox.p2.ui and org.eclipse.ui.browser are intentional (i.e., views using the new functionality will never be instantiated without them).

If any usages are optional or behind feature flags, consider marking the corresponding bundles as optional as well.


156-156: Good use of optional Import-Package for org.eclipse.tracecompass.traceeventlogger

Marking org.eclipse.tracecompass.traceeventlogger as resolution:=optional is appropriate if the logger is an optional integration; it avoids wiring failures when the package is absent. Just ensure all call sites handle the package not being available (e.g., by checking class availability or using service lookups defensively).

MatthewKhouzam
MatthewKhouzam previously approved these changes Dec 11, 2025
Copy link
Contributor

@MatthewKhouzam MatthewKhouzam left a comment

Choose a reason for hiding this comment

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

Looks great! Welcome to the tree!

@ShadowK2494
Copy link
Author

Looks great! Welcome to the tree!
Hi @MatthewKhouzam
My commit bumps the OSGi version to 9.2.0.qualifier, but the project is still on 9.1.2.qualifier, which breaks Tycho CI.
Is the project ready to move to 9.2.0, or should I rework this change to stay on 9.1.2?

@MatthewKhouzam
Copy link
Contributor

The API can be incremented, no problem!

@MatthewKhouzam
Copy link
Contributor

Hey, I think you need to update the pom too!

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 617a3ba and aaf3da3.

📒 Files selected for processing (2)
  • tmf/org.eclipse.tracecompass.tmf.ui/META-INF/MANIFEST.MF (1 hunks)
  • tmf/org.eclipse.tracecompass.tmf.ui/pom.xml (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • tmf/org.eclipse.tracecompass.tmf.ui/META-INF/MANIFEST.MF


<artifactId>org.eclipse.tracecompass.tmf.ui</artifactId>
<version>9.1.2-SNAPSHOT</version>
<version>9.2.0-SNAPSHOT</version>
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify version consistency across the tmf.ui bundle

echo "=== Checking MANIFEST.MF version in tmf.ui ==="
fd -t f "MANIFEST.MF" tmf/org.eclipse.tracecompass.tmf.ui --exec cat {} \; | grep -i "bundle-version\|implementation-version"

echo -e "\n=== Checking pom.xml in tmf.ui directory ==="
cat -n tmf/org.eclipse.tracecompass.tmf.ui/pom.xml | head -30

echo -e "\n=== Searching for other version references (9.1.2 or 9.2.0) in tmf.ui ==="
rg "9\.[12]\." tmf/org.eclipse.tracecompass.tmf.ui --type xml --type properties

echo -e "\n=== Checking feature.xml files in parent directories that might reference tmf.ui ==="
fd -t f "feature.xml" | head -20

Repository: eclipse-tracecompass/org.eclipse.tracecompass

Length of output: 2844


🏁 Script executed:

#!/bin/bash
# Check feature.xml files that reference the tmf.ui bundle

echo "=== Searching feature.xml files for tmf.ui references ==="
fd -t f "feature.xml" | xargs grep -l "org.eclipse.tracecompass.tmf.ui" 2>/dev/null | while read file; do
  echo -e "\n--- $file ---"
  grep -n "org.eclipse.tracecompass.tmf.ui" "$file" -A 1 -B 1
done

echo -e "\n=== Checking if any feature.xml specifies bundle versions for tmf.ui ==="
fd -t f "feature.xml" | xargs grep "org.eclipse.tracecompass.tmf.ui" 2>/dev/null | grep -E "version|plugin"

Repository: eclipse-tracecompass/org.eclipse.tracecompass

Length of output: 3757


Version bump is consistent across pom.xml and MANIFEST.MF.

The minor version increment from 9.1.2-SNAPSHOT to 9.2.0-SNAPSHOT appropriately reflects the new public APIs (AbstractSelectTreeViewer3, SpecificColumnPatternFilter) and visibility changes. The MANIFEST.MF Bundle-Version is correctly set to 9.2.0.qualifier, and feature.xml imports use "greaterOrEqual" constraints that are compatible with 9.2.0.

No other module POMs require version updates; the parent pom version (11.0.0-SNAPSHOT) intentionally differs from child module versions in this project structure.

Verify that Tycho CI builds successfully with this version bump.

🤖 Prompt for AI Agents
In tmf/org.eclipse.tracecompass.tmf.ui/pom.xml around line 23, the pom version
was bumped to 9.2.0-SNAPSHOT; confirm and ensure Tycho builds succeed by running
the CI/tycho build locally or via CI, verify MANIFEST.MF Bundle-Version is
9.2.0.qualifier and feature.xml imports remain compatible, and if the Tycho
build fails, adjust any module POMs or manifest entries to match the 9.2.0
versioning or fix dependency constraints so the Tycho CI passes.

@ShadowK2494
Copy link
Author

Hey, I think you need to update the pom too!

@MatthewKhouzam Thanks for the reminder! The pom.xml is now updated. Please let me know if there are any additional steps or checks needed before merging.

@ShadowK2494
Copy link
Author

Hi @MatthewKhouzam,could you please review and approve my pull request? The workflows are awaiting maintainer approval, and merging is currently blocked. Thank you!

@ShadowK2494
Copy link
Author

Hi @MatthewKhouzam ! It's just a kindly reminder. Could you help me take a look at my pull request?
Thanks in advance and happy new year 🥳

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants