eclipse-cdt-cloud / vscode-memory-inspector

vscode memory inspector
https://open-vsx.org/extension/eclipse-cdt/memory-inspector
Eclipse Public License 2.0
6 stars 10 forks source link

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

Closed haydar-metin closed 6 months ago

haydar-metin commented 6 months ago

What it does

Reverts commit 30ab368 and introduces an alternative solution

How to test

Commit: 62354e33629c19073b416615391aa12b146ee437

  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: ed152d290872744bf8859f420a77d59937905143

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 commented 6 months ago

@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 commented 6 months ago

@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 commented 6 months ago

@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 commented 6 months ago

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

jreineckearm commented 6 months ago

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 commented 6 months ago

@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 commented 6 months ago

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