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.

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

Persist state of view after pin/unpin #782

Merged
merged 2 commits into from
Jul 25, 2022
Merged

Persist state of view after pin/unpin #782

merged 2 commits into from
Jul 25, 2022

Conversation

hriday-panchasara
Copy link
Contributor

  • This is an improvement of the feature in PR Ability to Pin Views #739
  • When a view is pinned/unpinned it is re-rerendered onto the layout as it is moving to a different container. This re-rendering does not happen when dragging views since they are re-arranged in the same container. So when a view is pinned/unpinned, the charts lose their state and the user has to redo the selections.
  • This patch tries to persist the state of the view that's being pinned and unpinned. Luckily, we are able to pass the state of the chart as a payload when firing the signal to pin/unpin a view.
  • This is being used by the xy-charts to pass the checkedSeries and collapsedNodes of the tree, and the timegraph-charts to pass the collapsedNodes and collapsedMarkerNodes.
  • By doing so we ensure that the chart state stays the same and the user does not have to re-do their selections
  • Unfortunately I could not find a way to persist the scroll position of the tree or the order of tree columns of the xy chart. If there are any suggestions I'll try them out.
  • The xy-output-component currently does not support data-provider styles and randomly assigns series colors. Therefore, sometimes after pin/unpin I've noticed minor changes in the overall color scheme. We'll be working on implementing data-provider styles for xy in the near future, so I'm assuming this issue would not pose as a blocker.

Fixes #762

Signed-off-by: hriday-panchasara hriday.panchasara@ericsson.com

@hriday-panchasara hriday-panchasara force-pushed the Issue/762-persist-state-of-pinned-view branch from d67ffff to 86003c2 Compare July 8, 2022 19:47
PatrickTasse
PatrickTasse requested changes Jul 11, 2022
View reviewed changes
packages/base/src/signals/signal-manager.ts Outdated
@@ -26,8 +26,8 @@ export declare interface SignalManager {
fireTraceServerStartedSignal(): void;
fireUndoSignal(): void;
fireRedoSignal(): void;
firePinView(output: OutputDescriptor): void;
fireUnPinView(): void;
firePinView(payload: { output: OutputDescriptor, checkedSeries?: number[], collapsedNodes?: number[], collapsedMarkerNodes?: number[]}): void;
Copy link
Contributor

Choose a reason for hiding this comment

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

It feels like the persisted state of outputs should be opaque as it can be different for every output (e.g. there are no checkedSeries in timegraph). It doesn't feel right to have a merge of all possible state properties here.

I would perhaps keep the output and persistedState as separate parameters in the signal. I'm not sure if the persisted state can be an 'any' object or a choice of state classes defined in each specific output.

But I think signal-manager, abstract-output-component and trace-context-component should pass the persisted state as a 'black box' object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion Patrick, I've added another commit which passes the persisted state as a 'black-box' object. I was able to specify the persisted state as an 'any' object with the help of disable es-lint statements.

bhufmann
bhufmann requested changes Jul 20, 2022
View reviewed changes
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.

It needs a rebase. Heads-up that the merge of xy-output-component will be non-trivial due to the overview addition.

I tested it and it works fine. I wonder if you could persist the selectedRow of the timegraphs?

@hriday-panchasara
Persist state of view after pin/unpin
48a9336 
Keep track of tree selections of the pinned/unpinned chart so that users don't have to redo them

Fixes eclipse-cdt-cloud#762

Signed-off-by: hriday-panchasara <hriday.panchasara@ericsson.com>
@hriday-panchasara hriday-panchasara force-pushed the Issue/762-persist-state-of-pinned-view branch from 2a67c51 to 48a9336 Compare July 22, 2022 16:51
bhufmann
bhufmann previously approved these changes Jul 22, 2022
View reviewed changes
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.

It looks good to me. Nice improvement. Thanks.

I'm going to open a follow-up issue to track other things that could be persisted as well, and would delay this PR to implement.

@bhufmann
Copy link
Collaborator

Note that there is a network issue that prevents a successful build. I'll re-trigger build once it's back to normal.

@hriday-panchasara
Persist events table selection after pin/unpin
52c3805 
helps fix eclipse-cdt-cloud#762

Signed-off-by: hriday-panchasara <hriday.panchasara@ericsson.com>
@hriday-panchasara
Copy link
Contributor Author

hriday-panchasara commented Jul 22, 2022 •
edited
Loading

@bhufmann Thanks for the reviews! I've added another commit to persist the selected rows of the events table. For the selectedRow of timegraphs, it would be possible to persist once PR #788 is merged

@bhufmann
Copy link
Collaborator

@bhufmann Thanks for the reviews! I've added another commit to persist the selected rows of the events table. For the selectedRow of timegraphs, it would be possible to persist once PR #788 is merged

Sounds good. I'm ok to merge this PR and adding the selected row for time graphs afterwards.

@bhufmann bhufmann merged commit 0e186e0 into eclipse-cdt-cloud:master Jul 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PatrickTasse PatrickTasse PatrickTasse approved these changes

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

Persist state of pinned view
3 participants
@hriday-panchasara @bhufmann @PatrickTasse