Skip to content
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.

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

Select chart components from the events table #746

Closed

Conversation

Rodrigoplp-work
Copy link
Contributor

@Rodrigoplp-work Rodrigoplp-work commented May 6, 2022

Select corresponding chart components from the events table.

Clicking on a line in the events table will cause a chart like "Thread
Status" to scroll vertically to the row which has the element that
corresponds to the clicked event.

Only works if the events contains matching information about start time,
PID, TID or unique process name.

This PR requires the following pull requests:

2022-05-06.17-16-14.mp4

Signed-off-by: Rodrigo Pinto rodrigo.pinto@calian.ca

Copy link
Collaborator

@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.

Thanks for the contribution. It was fun to see this useful feature getting shape. I have request for change in the search algorithm for the row.

@@ -288,6 +298,59 @@ export class TimegraphOutputComponent extends AbstractTreeOutputComponent<Timegr
}
}

private findElement(payload: { [key: string]: string | { [key: string]: string }}): TimeGraphEntry | number | undefined {
const startTimestampString: string = payload['startTimestamp'].toString();
const found = this.state.timegraphTree.findIndex(e => e.start === BigInt(startTimestampString));
Copy link
Collaborator

Choose a reason for hiding this comment

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

The entry start time in not relevant in most cases not matching the selected event time. So, you can remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

}
else if (found === -1 && typeof(payload.load) !== 'string') {
// Parse identifiers from "Contents" field
const contents = payload.load.Contents.split(', ');
Copy link
Collaborator

Choose a reason for hiding this comment

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

So, your trying to match the Content column with the metadata. However, the idea of the this feature was to match the metadata with the columns of the event in the table, where the key is the header name and value the content of the cell. So, what you need to do is find the most intersecting row, i.e. most number of matching key/value pairs from metadata and event header/cell pairs. To test I tried to implement it with the following code:

    private doHandleSelectionChangedSignal(payload: { [key: string]: string }) {
        const offset = this.props.viewRange.getOffset() || BigInt(0);
        const startTimestamp = payload['startTimestamp'];
        const endTimestamp = payload['endTimestamp'];
        if (startTimestamp !== undefined && endTimestamp !== undefined) {
            const selectionRangeStart = BigInt(startTimestamp) - offset;
            const selectionRangeEnd = BigInt(endTimestamp) - offset;
            const foundElement = this.findElement(payload);
            // Scroll vertically
            if (foundElement) {
                const rowIndex = this.state.dataRows.findIndex(d => d.id === foundElement.id);
                if (this.timeGraphTreeRef.current) {
                    this.timeGraphTreeRef.current.scrollTop = 20 * rowIndex;
                }
                // Select row
                const row = this.state.dataRows.find(d => d.id === foundElement.id);
                if (row) {
                    this.chartLayer.selectRow(row);
                    // TODO select state at row/starttime
                }
            }
            // Scroll horizontally
            this.props.unitController.selectionRange = {
                start: selectionRangeStart,
                end: selectionRangeEnd
            };
            this.chartCursors.maybeCenterCursor();
        }
    }

    private findElement(payload: { [key: string]: string | { [key: string]: string }}): TimeGraphEntry | undefined {
        let element: TimeGraphEntry | undefined = undefined;
        let max = 0;
        if (payload && payload.load) {
            this.state.timegraphTree.forEach(el => {
                if (el.metadata) {
                    let cnt = 0;
                    Object.entries(el.metadata).some(([key, value]) => {
                        if (typeof (payload.load) !== 'string') {
                            const val = payload.load[key];
                            if (val !== undefined) {
                                const num = Number(val);
                                if ((num !== undefined && num === value[0]) || (val === value[0])) {
                                    cnt++;
                                }
                            }
                        }
                    });
                    if (cnt > max) {
                        max = cnt;
                        element = el;
                    }
                }
            });
        }
        return element;
    }

The is probably a better way to implement the search algorithm findElement with some stream and lambdas and some javascript magic which you are more familiar that me. Feel free to reimplement the findElement method.

I tested my code with the Thread Status and Resources view.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the clarification and suggestions!
I tried a reducer in place of the forEach in findElement, but the solution was not much shorter than yours, just more abstract. So I preferred to kept your solution with a few minor adjustments, and cleaned up unused code.

@Rodrigoplp-work Rodrigoplp-work force-pushed the click-event branch 2 times, most recently from 1cfeb39 to 0ed55c5 Compare May 30, 2022 21:57
@Rodrigoplp-work Rodrigoplp-work requested a review from bhufmann May 30, 2022 22:21
@@ -27,7 +27,6 @@ import hash from 'traceviewer-base/lib/utils/value-hash';
import ColumnHeader from './utils/filter-tree/column-header';
import { TimeGraphAnnotationComponent } from 'timeline-chart/lib/components/time-graph-annotation';
import { Entry } from 'tsp-typescript-client';
import { isEqual } from 'lodash';

Copy link
Collaborator

Choose a reason for hiding this comment

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

you accidentally removed this import

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True. Fixed.

const foundElement = this.findElement(payload);
// Scroll vertically
if (foundElement) {
const rowIndex = this.state.dataRows.findIndex(d => d.id === foundElement.id);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This actually doesn't work. this.state.dataRows contains the visible row components and the row component id is not equal to the corresponding tree entry id. Also, when that particular element is under a collapsed node then it is also not part of the dataRows.

Right now, the code base doesn't have an API that would ensure visibility based on tree entry ID. That will have to been done to be able to support this vertical scrolling.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok.

Select corresponding chart components from the events table.

Clicking on a line in the events table will cause a chart like "Thread
Status" to scroll vertically to the row which has the element that
corresponds to the clicked event.

Only works if the events contains matching information about start time,
PID, TID or unique process name.

This PR requires the following pull requests:

https://git.eclipse.org/r/c/tracecompass.incubator/org.eclipse.tracecompass.incubator/+/193246
eclipse-cdt-cloud/tsp-typescript-client#62

Signed-off-by: Rodrigo Pinto <rodrigo.pinto@calian.ca>
@bhufmann
Copy link
Collaborator

bhufmann commented Jun 6, 2022

This PR is moved to #760.

@bhufmann bhufmann closed this Jun 6, 2022
@Rodrigoplp-work Rodrigoplp-work deleted the click-event branch August 25, 2022 19:04
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