Introduce Autofit choice for groups per row #82

haydar-metin · 2024-02-23T13:46:55Z

What it does

Introduces the Autofit element for Groups per Row.

IMPORTANT: The selected Groups per Row is based on the available choices! That means, it is not possible to render, e.g., 7 groups per rows (for now). HOWEVER, code-wise it should be still possible if requested.

How to test

Select Autofit in the options for Groups per Row
Change Bytes per Word, Words per Group or toggle the columns
Resize either the view or the colums
Go to 2.

Review checklist

as an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

as a reviewer, I agree to behave in accordance with the review guidelines

Closes #73

planger

Thank you, this looks and works great!
I personally would find it even better if we'd allow any number of groups per row with the Autofit option active. I don't really see a reason why not allowing e.g. 3, 7, or 10. But then on the other hand I'm not a typical user, so I might be wrong. Better to let Jens and Colin chime in on this one.
Thank you!

src/webview/components/memory-table.tsx

src/common/typescript.ts

src/webview/components/memory-table.tsx

src/webview/components/options-widget.tsx

colin-grant-work · 2024-02-23T18:37:04Z

src/webview/utils/window.ts

+/**
+ * Calculates the width of the provided text within the container
+ *
+ * @see https://stackoverflow.com/a/21015393


In general, it is not good practice to copy Stack Overflow code, as it implicates the including project in the licensing of the included code, which is a copy-left license:

https://stackoverflow.com/help/licensing
https://creativecommons.org/licenses/by-sa/4.0/

I updated the link to use the API doc as I only provided the link in the beginning to read more about the topic if necessary

haydar-metin · 2024-02-26T14:40:58Z

@colin-grant-work I moved the calculations to the column contribution in commit 413ca77
@planger The groups per row can now be any value independently of the choices we have. The advantage is, the full width will be now used.

planger · 2024-02-27T09:36:16Z

Great, thanks for the update! From my point of view this works very well now. @colin-grant-work @jreineckearm what do you think?

src/common/typescript.ts

src/plugin/manifest.ts

src/common/typescript.ts

src/webview/columns/column-fitting.ts

colin-grant-work · 2024-02-28T00:33:01Z

src/webview/columns/data-column.tsx

+     *
+     * **Note**
+     *
+     * The returned `groups per row` value is based on {@link CONFIG_GROUPS_PER_ROW_NUMERIC_CHOICES}


I don't see code that would enforce this. In the for loop below, we appear to decrement by one and return whenever the value works, which certainly doesn't guarantee a power of two.

My error. Removed the note, as Autofit should use the width as whole.

colin-grant-work · 2024-02-28T00:42:15Z

src/webview/components/memory-table.tsx

+            if (row) {
+                const fitting = SupportedColumnFitting.findAll(this.props.columnOptions.map(c => c.contribution), ColumnGroupsPerRowFit.is);
+                return Math.max(...fitting.map(s => s.calculateGroupsPerRow(row, options)));
+            }


I think my previous suggestion to come up with an API for column behaviors may have produced an excess of abstraction.

In fact, we can only get one value out of this calculation, and only one column has the right to calculate that value. Rather than pretending that ColumnGroupsPerRowFit could apply to more than one column, it would be better to just hardcode a reference to the calculation in the data ColumnContribution. It is still better for the code for calculating that value to be in that file, but there really isn't any way around just making that column responsible for this calculation.

Yes, I removed the not necessary abstraction again. I just let the fittingType in the API for basic behaviors such as content-width fitting (e.g., the address column)

src/webview/columns/data-column.tsx

jreineckearm · 2024-03-04T16:17:52Z

@haydar-metin , @planger , I have just tested this. Looks really great!
I've only noticed that it often doesn't make full use of the remaining space. In some scenarios, a fixed "Group per Row" setting fits in more than the autofit.

Example:
Started of with autofit. It fits in 7 groups per row with 1 word per group.

Then switched to using 8 groups per row, column width untouched. Which manages to fit an additional group in.

When resizing the column, it often looks like it's trying to wait for two groups to fit in before adding another one.
Is there any way to further optimize the width utilization?

jreineckearm · 2024-03-04T16:18:55Z

BTW: looking forward to see this working in combination with the auto-append scrolling. :-)

haydar-metin · 2024-03-05T12:41:39Z

@jreineckearm Thank you for testing it!

I now use the exact values (905e061) where possible. Unfortunately, the browser-engine rounds the values or uses different approaches to calculate the width. For this reason, we can only approximate the calculation. Improving this aspect would require us to further investigate how the engine renders it to exactly match (e.g., the width of the eight-bits varies). For this reason, there will be some edge cases where Autofit will require more space.
30ab368 hides now the second row that we can see for a second if we resize the webview.

jreineckearm · 2024-03-07T07:27:13Z

Thanks, @haydar-metin ! Space utilization is much improved with the latest change! :-)

haydar-metin self-assigned this Feb 23, 2024

haydar-metin requested a review from planger February 23, 2024 13:49

planger reviewed Feb 23, 2024

View reviewed changes

src/webview/components/memory-table.tsx Outdated Show resolved Hide resolved

colin-grant-work reviewed Feb 23, 2024

View reviewed changes

haydar-metin marked this pull request as draft February 26, 2024 10:11

haydar-metin marked this pull request as ready for review February 26, 2024 14:43

colin-grant-work reviewed Feb 28, 2024

View reviewed changes

haydar-metin force-pushed the issues/73_auto_fitting branch from 2bc98bd to 106a12a Compare February 28, 2024 12:10

haydar-metin added 4 commits March 5, 2024 10:01

Introduce Autofit choice for groups per row

817bf8f

Improve types according feedback

a4e72e0

Clean up

7dee4b2

Improve approximation by using exact values

905e061

haydar-metin force-pushed the issues/73_auto_fitting branch from 7e4dc5b to 905e061 Compare March 5, 2024 11:52

Hide second line for data column if groups-per-row autofit is active

30ab368

colin-grant-work approved these changes Mar 6, 2024

View reviewed changes

colin-grant-work merged commit 0f2411e into eclipse-cdt-cloud:main Mar 6, 2024
5 checks passed

tortmayr mentioned this pull request Mar 7, 2024

Context menu enhancements #94

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce Autofit choice for groups per row #82

Introduce Autofit choice for groups per row #82

haydar-metin commented Feb 23, 2024 •

edited

Loading

planger left a comment

colin-grant-work Feb 23, 2024

haydar-metin Feb 26, 2024

haydar-metin commented Feb 26, 2024 •

edited

Loading

planger commented Feb 27, 2024

colin-grant-work Feb 28, 2024

haydar-metin Feb 28, 2024 •

edited

Loading

colin-grant-work Feb 28, 2024

haydar-metin Feb 28, 2024

jreineckearm commented Mar 4, 2024

jreineckearm commented Mar 4, 2024

haydar-metin commented Mar 5, 2024

jreineckearm commented Mar 7, 2024

Introduce Autofit choice for groups per row #82

Introduce Autofit choice for groups per row #82

Conversation

haydar-metin commented Feb 23, 2024 • edited Loading

What it does

How to test

Review checklist

Reminder for reviewers

planger left a comment

Choose a reason for hiding this comment

colin-grant-work Feb 23, 2024

Choose a reason for hiding this comment

haydar-metin Feb 26, 2024

Choose a reason for hiding this comment

haydar-metin commented Feb 26, 2024 • edited Loading

planger commented Feb 27, 2024

colin-grant-work Feb 28, 2024

Choose a reason for hiding this comment

haydar-metin Feb 28, 2024 • edited Loading

Choose a reason for hiding this comment

colin-grant-work Feb 28, 2024

Choose a reason for hiding this comment

haydar-metin Feb 28, 2024

Choose a reason for hiding this comment

jreineckearm commented Mar 4, 2024

jreineckearm commented Mar 4, 2024

haydar-metin commented Mar 5, 2024

jreineckearm commented Mar 7, 2024

haydar-metin commented Feb 23, 2024 •

edited

Loading

haydar-metin commented Feb 26, 2024 •

edited

Loading

haydar-metin Feb 28, 2024 •

edited

Loading