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

Revert autofit smoothing introduced in PR #82 and use debouncing resize events #99

Merged

Conversation

haydar-metin
Copy link
Contributor

What it does

Reverts commit 30ab368 and introduces an alternative solution

How to test

Commit: 62354e3

  1. Set the Bytes per Word and Words per Row to 1 and Groups per Row to Autofit
  2. Decrease the width of the webview (e.g., Open the devtools and resize that)

Increasing the width of the webview: No issues - new groups will be appended
Decreasing the width of the webview: The content of the data-column will break until the autofit calculation runs.

Commit: ed152d2

Now, we wait until the user stops the movement (200ms) and trigger the change afterwards. The only disadvantage we have is, that we need to wait a little.

Peek 2024-03-08 13-05

Source Problem

There is a time delay:

  1. User resizes webview
  2. Browser renders elements, e.g., the data-column will break lines if it can not fit anymore
  3. We retrieve the resize event and trigger the autofit
  4. The columns will be rendered again

Consequently, there is flickering.

Alternative solution

Remove CSS solution which causes issues and execute the autofitting after the user stops with debounce.

Review checklist

Reminder for reviewers

@haydar-metin
Copy link
Contributor Author

haydar-metin commented Mar 8, 2024

@tortmayr Sorry for the headache I have caused for your PR! I reverted the commit that caused the issues for you.

@jreineckearm, @colin-grant-work What do you think about the new solution?

@tortmayr
Copy link
Contributor

tortmayr commented Mar 8, 2024

@haydar-metin
I have already provided a fix in my PR so that copy&paste also works with the previous approach.
Using a instead of a div for the root element of address columns
And add special handling to the onCopy listener of the memory table for the autofit case. If autofit is enabled,
the listener removes the groups-per-row-autofit class from the table to copy the correct selection and then adds it again.

So from my side both approaches work and we should probably go with the solution that offers the better look & feel.
In any case, we should probably not merge #94 until this is resolved.

@haydar-metin
Copy link
Contributor Author

@tortmayr Unfortunately, the old approach also caused other issues (#93). That means we need to revert it. The question would be how to solve the flickering. I'm not sure, if there is an easier way to solve this problem than the one I have proposed.

@tortmayr
Copy link
Contributor

tortmayr commented Mar 8, 2024

@tortmayr I see, then i will revert my changes regarding this in #94.

tortmayr added a commit to eclipsesource/vscode-memory-inspector that referenced this pull request Mar 8, 2024
tortmayr added a commit to eclipsesource/vscode-memory-inspector that referenced this pull request Mar 8, 2024
@tortmayr tortmayr mentioned this pull request Mar 8, 2024
1 task
@jreineckearm
Copy link
Contributor

Thanks, @haydar-metin , I tested it and I see now the effect that you meant despite the debounce.
I had a bit of a play with a smaller delay and maxWait (in DebounceSettings) settings. For example }, 100, { maxWait: 500 });
It keeps rendering the line breaks but felt a little smoother. But not quite sure if that was rather wishful thinking.

@jreineckearm
Copy link
Contributor

@haydar-metin , I'd suggest to merge this change as it is after resolving merge conflicts. It fixes the copy functionality. Maybe reduce the debounce to 100ms which looks a little smoother already. We can look into further enhancing the rendering again at a later point.

@haydar-metin
Copy link
Contributor Author

@jreineckearm I reduced the delay - it feels smoother now.

@haydar-metin haydar-metin marked this pull request as draft March 12, 2024 10:26
@haydar-metin haydar-metin marked this pull request as ready for review March 12, 2024 10:30
@planger planger self-requested a review March 12, 2024 11:13
Copy link
Contributor

@planger planger left a comment

Choose a reason for hiding this comment

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

Thanks, looks good to me! 👍

@planger planger merged commit ed4cdff into eclipse-cdt-cloud:main Mar 12, 2024
5 checks passed
@planger planger deleted the revert-auto-fitting-rendering branch March 12, 2024 11:15
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.

None yet

4 participants