Skip to content

tmf: Expose state system through data provider factory #325

New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom
Merged
bhufmann merged 1 commit into from
Oct 9, 2025
Merged

tmf: Expose state system through data provider factory #325

bhufmann merged 1 commit into from
Oct 9, 2025

Conversation

@Yibhir0
Copy link

@Yibhir0 Yibhir0 commented Sep 23, 2025 •
edited
Loading

What it does

  • It exposes StateSystemDataProvider through StateSystemDataProviderFactory
  • Adds DESCRIPTOR for the clients
  • Sets autoexpand level to 1
  • Implements fetchStyle
  • Improve tests

How to test

  • Run trace server
  • Run Vscode extension
  • Open a trace and click on state system explorer
  • you should see all the states

Follow-ups

Make sure that view is updated when new analysis is executed (maybe refresh view when another view is executed). TBD.

Review checklist

  • As an author, I have thoroughly tested my changes and carefully followed the instructions in this template

PatrickTasse
PatrickTasse requested changes Sep 26, 2025
View reviewed changes
...org/eclipse/tracecompass/internal/tmf/core/statesystem/provider/StateSystemDataProvider.java Outdated

static {
ImmutableMap.Builder<@NonNull String, @NonNull OutputElementStyle> builder = new ImmutableMap.Builder<>();
IPaletteProvider fPalette = new RotatingPaletteProvider.Builder().setNbColors(NUM_COLORS).build();
Copy link
Contributor

@PatrickTasse PatrickTasse Sep 26, 2025

Choose a reason for hiding this comment

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

f-prefix is to indentify class variables. For local variables it would just be 'palette'.

...org/eclipse/tracecompass/internal/tmf/core/statesystem/provider/StateSystemDataProvider.java Outdated
builder.put(key, outputStyle);
}

Map<String, Object> fMapUNKOWN = ImmutableMap.of(StyleProperties.BACKGROUND_COLOR, COLOR_UNKOWN);
Copy link
Contributor

@PatrickTasse PatrickTasse Sep 26, 2025

Choose a reason for hiding this comment

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

Would be 'unknown' but you could just inline it into the constructor below.

Copy link
Author

@Yibhir0 Yibhir0 Sep 29, 2025

Choose a reason for hiding this comment

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

Spelling mistake. I got it now.

...org/eclipse/tracecompass/internal/tmf/core/statesystem/provider/StateSystemDataProvider.java Outdated
Status status = fetchTreeIsComplete ? Status.COMPLETED : Status.RUNNING;
String msg = fetchTreeIsComplete ? CommonStatusMessage.COMPLETED : CommonStatusMessage.RUNNING;
return new TmfModelResponse<>(new TmfTreeModel<>(Collections.emptyList(), entryList), status, msg);
TmfTreeModel<TimeGraphEntryModel> treeModel = new TmfTreeModel<>(Collections.emptyList(), entryList);
Copy link
Contributor

@PatrickTasse PatrickTasse Sep 26, 2025

Choose a reason for hiding this comment

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

It should create two headers inspired by TmfStateSystemExplorer.COLUMN_NAMES

Copy link
Contributor

@PatrickTasse PatrickTasse Sep 26, 2025

Choose a reason for hiding this comment

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

Then in AttributeEntryModel constructor, the labels passed to super constructor should be a list of [ name, quark ]

@Yibhir0 Yibhir0 changed the title Exposed state system through data provider factory tmf: Expose state system through data provider factory Sep 29, 2025
PatrickTasse
PatrickTasse requested changes Sep 30, 2025
View reviewed changes
...org/eclipse/tracecompass/internal/tmf/core/statesystem/provider/StateSystemDataProvider.java Outdated
Status status = fetchTreeIsComplete ? Status.COMPLETED : Status.RUNNING;
String msg = fetchTreeIsComplete ? CommonStatusMessage.COMPLETED : CommonStatusMessage.RUNNING;
TmfTreeModel<TimeGraphEntryModel> treeModel = new TmfTreeModel<>(Collections.emptyList(), entryList);
TmfTreeModel<TimeGraphEntryModel> treeModel = new TmfTreeModel<>( new ArrayList<>(Arrays.asList( Messages.TreeNodeColumnLabel, Messages.QuarkColumnLabel)), entryList);
Copy link
Contributor

@PatrickTasse PatrickTasse Sep 30, 2025

Choose a reason for hiding this comment

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

wrapping into ArrayList not necessary, Arrays.asList() does the job.

Copy link
Contributor

@PatrickTasse PatrickTasse Sep 30, 2025

Choose a reason for hiding this comment

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

In the AttributeEntryModel constructor, both column values should be passed as the label list to the super constructor.

...org/eclipse/tracecompass/internal/tmf/core/statesystem/provider/StateSystemDataProvider.java Outdated
Map<String, Object> fMapUNKOWN = ImmutableMap.of(StyleProperties.BACKGROUND_COLOR, COLOR_UNKOWN);
OutputElementStyle unknownoutputStyle = new OutputElementStyle(null, fMapUNKOWN);
builder.put(UNKNOWN, unknownoutputStyle);
builder.put( UNKNOWN , new OutputElementStyle(null, ImmutableMap.of(StyleProperties.BACKGROUND_COLOR, COLOR_UNKNOWN)));
Copy link
Contributor

@PatrickTasse PatrickTasse Sep 30, 2025

Choose a reason for hiding this comment

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

Add a TRANSPARENT style with 0.0f opacity and use for the blank states added to module and state system entries. This will prevent the default white color to be used by the web client.

PatrickTasse
PatrickTasse requested changes Oct 2, 2025
View reviewed changes
Copy link
Contributor

@PatrickTasse PatrickTasse left a comment

Choose a reason for hiding this comment

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

Please fix commit header and message to match the PR's header and description.

In both, fix 'auto scale level' to 'autoexpand level'.

@Yibhir0 Yibhir0 force-pushed the yibhir branch 2 times, most recently from 732cce3 to efdfa61 Compare October 2, 2025 16:40
bhufmann
bhufmann requested changes Oct 2, 2025
View reviewed changes
Copy link
Contributor

@bhufmann bhufmann left a comment

Choose a reason for hiding this comment

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

Code looks good. Please fix your ECA setup so that he eclipse foundation check is successful. Please also add you signed-off to the commit message. Please update the commit message to with a description what is done and remove list of previous commit titles.

As a follow-up PR, I suggest working on refreshing the analysis available when opening the view (fetchTree). Right now only state systems are shown that had been opened before the state system explorer is opened the first time. Note, whenever a view is opened a new state system might opened or create. Opening the state system explorer should include those.

@Yibhir0 Yibhir0 force-pushed the yibhir branch 3 times, most recently from 1bcb937 to 8679a5e Compare October 3, 2025 20:13
@theibhir
tmf:Expose state system through data provider factory
54a8ab4 
It exposes StateSystemDataProvider through StateSystemDataProviderFactory
Adds DESCRIPTOR for the clients
Sets autoexpand level to 1
Implements fetchStyle
Improves tests of all data providers

Signed-off-by: Yassine Ibhir <[email protected]>
bhufmann
bhufmann approved these changes Oct 9, 2025
View reviewed changes
Copy link
Contributor

@bhufmann bhufmann left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks!

@bhufmann bhufmann merged commit 33f89b8 into eclipse-tracecompass:master Oct 9, 2025
4 checks passed
@bhufmann bhufmann mentioned this pull request Oct 9, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bhufmann bhufmann bhufmann approved these changes

@PatrickTasse PatrickTasse PatrickTasse approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Yibhir0 @bhufmann @PatrickTasse @theibhir