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

Retain row and state selection after refresh #49

Merged
merged 1 commit into from Oct 19, 2020
Merged

Retain row and state selection after refresh #49

merged 1 commit into from Oct 19, 2020

Conversation

ghost
Copy link

@ghost ghost commented Oct 2, 2020

fixes #50

Signed-off-by: muddana-satish [email protected]

@tahini
Copy link
Contributor

tahini commented Oct 5, 2020

When zooming out, the selection is lost once more. Also, when a state is selected at high zoom level, the selected time may be beside that state when we zoom in. See the screecast here: this is taken with your PR in timeline-chart and theia-trace-extension

rowSelection

@ghost
Copy link
Author

ghost commented Oct 5, 2020

I think we should test it with actual states. The GAP states doesn't have fixed range and mostly depends on the zoom levels. Could you please test it with actual states.

@ghost
Copy link
Author

ghost commented Oct 6, 2020

@tahini
This is how it looks for the actual states.

State Selection

I believe this functionality is correct. The GAP states keep changing based on the zoom level. So it is highly unlikely to identify the state. Any thoughts on this?

@tahini
Copy link
Contributor

tahini commented Oct 6, 2020

I thought I had selected a state at first! I clicked on something, very thin, true, but still, it highlighted the visible state and the state remains hightlighted as I zoom in. This may be unrelated with this PR, but it's a bug anyway. If the state appears to be selected, then I'd expect to zoom in and remain in that highlighted state.

@ghost
Copy link
Author

ghost commented Oct 6, 2020

Discussed with @tahini about the scope of this PR.

As part of this PR, I fixed the issue only for the actual states and excluded GAP states. Since GAP states do not have fixed start and end ranges, the current changes doesn't work on GAP states. I created a new issue #51 for addressing this.

@tahini
Copy link
Contributor

tahini commented Oct 8, 2020

Code looks good, but I don't know if it fixes #50. In your screencast, you also select a gap state. Can you reproduce bug #50 with a normal state?

@ghost
Copy link
Author

ghost commented Oct 8, 2020

Code looks good, but I don't know if it fixes #50. In your screencast, you also select a gap state. Can you reproduce bug #50 with a normal state?

I'm sorry for the confusion. I added a new screencast showing two issues that this PR fixes.

Copy link
Contributor

@tahini tahini left a comment

Choose a reason for hiding this comment

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

I could reproduce the bug and it works for normal states.

As a suggestion when/if you're going to look at the gap states also, instead of row id, start, end to compare with previous selection, maybe you could use row id and the intersection of the currently selected time instead (but I have no idea how the selected time works, so maybe I'm completely wrong)

@ghost
Copy link
Author

ghost commented Oct 14, 2020

I could reproduce the bug and it works for normal states.

As a suggestion when/if you're going to look at the gap states also, instead of row id, start, end to compare with previous selection, maybe you could use row id and the intersection of the currently selected time instead (but I have no idea how the selected time works, so maybe I'm completely wrong)

The problem with this approach is say at initial zoom level we have selected a GAP state. As you zoom in, the same GAP state can get split into two or more GAP states and it's possible that no state has intersection in our selection range.

I believe splitting the same state into multiple states based on the zoom level would make it difficult to identify the initial selected state. I would have to check this behavior in trace compass.

@ghost ghost changed the title Retained the row and state selection after refresh Retain row and state selection after refresh Oct 15, 2020
@MatthewKhouzam MatthewKhouzam merged commit a0e2dfb into eclipse-cdt-cloud:master Oct 19, 2020
@ghost ghost deleted the row-deselects branch October 28, 2020 06:50
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.

Zooming in deselects Row in Timegraph chart
2 participants