Add viewer for long strings in GridValue #5018

adamint · 2024-07-22T21:42:50Z

Fixes #4412, and converts the copy button to a menu where the user can select whether to open a popup viewer or copy the cell text.

Menu appearance:

Dialog appearance:

Per spec, adds a plaintext, xml, and json formatting option. The initial formatting option is selected by trying to parse the string using an xml/json parser.

demo:
https://github.com/user-attachments/assets/5d0ef395-13f3-4e59-9a3f-9b6842469ca2

Normally, we would trigger popup opening just by adding an OnClick listener to the FluentMenuItems, but we are encountering a bug (probably in fluentui-blazor or fluent webcomponents) causing this to be swallowed sometimes when enclosed in PropertyGrid. @vnbaaij is aware, I am looking into this. A workaround is invoking a function from JS similar to how the copy button currently works.

Microsoft Reviewers: Open in CodeFlow

…-view

mitchdenny · 2024-07-22T22:32:24Z

This is nice. This might be relevant for DevTunnels support where we want to be able to render a QR code associated with a particular endpoint so people can point their mobile device at it.

tlmii · 2024-07-22T22:49:11Z

Just curious: Do we have any indication how common it is that people just want to copy? Are we trading quick one-click access to that most-common task for two-clicks for all tasks? I'm fine accepting either "the two clicks aren't that bad" or "copy isn't that much more common than what we expect visualize will be" or even "we don't know and we'll see if users complain after we do this", I just want to make sure that's an intentional usability decision.

JamesNK · 2024-07-23T00:03:43Z

This looks good.

I haven't tested it yet, but from my first impression of the screenshots, here are some ideas to make it better:

Add a new, default option, called Auto. When the popup opens, we try to guess the value type. For example, we try to parse it as JSON, and if that succeeds then the value is automatically displayed as JSON. If that doesn't work (catch the exception) then we try to parse it as XML, and so on. If nothing works, then display as plaintext. The default option would say something like "Auto (JSON)", depending on what we end up displaying.
An auto option makes the user do less work to get a better result.
There should be a copy button on the popup. I could imagine someone options the popup to see the full value, and then decides they want to copy it to the clip board. We don't want to make them close the popup and then choose the copy value. The exception details popup has a copy button. Reuse that design?
If there is a copy button here, an interesting question is what gets copied? The formatted value (has new lines and indenting), or the original value?
I think line numbers would be useful here. I'm guessing that currently long lines wrap (like they do in the console page) so they would help indicate when content has a new line vs content that is just long. A lot of the CSS from the console page could be reused as all the same display choices apply here.
I think there should be some differentiation in background color between the dialog and the scrollable content area. Lower priority.

adamint · 2024-07-23T00:11:39Z

Add a new, default option, called Auto. When the popup opens, we try to guess the value type. For example, we try to parse it as JSON, and if that succeeds then the value is automatically displayed as JSON. If that doesn't work (catch the exception) then we try to parse it as XML, and so on. If nothing works, then display as JSON. The default option would say something like "Auto (JSON)", depending on what we end up displaying. An auto option makes the user do less work to get a better result.

This is already done by default when you open the dialog, do you feel that a separate option is important to have rather than preselecting?

There should be a copy button on the popup. I could imagine someone options the popup to see the full value, and then decides they want to copy it to the clip board. We don't want to make them close the popup and then choose the copy value. The exception details popup has a copy button. Reuse that design?
If there is a copy button here, an interesting question is what gets copied? The formatted value (has new lines and indenting), or the original

I would assume the formatted value so as not to confuse the user, or both options?

adamint · 2024-07-23T00:12:01Z

Just curious: Do we have any indication how common it is that people just want to copy? Are we trading quick one-click access to that most-common task for two-clicks for all tasks? I'm fine accepting either "the two clicks aren't that bad" or "copy isn't that much more common than what we expect visualize will be" or even "we don't know and we'll see if users complain after we do this", I just want to make sure that's an intentional usability decision.

I don’t think we have any information on that. @kvenkatrajan

JamesNK · 2024-07-23T00:17:59Z

Just curious: Do we have any indication how common it is that people just want to copy? Are we trading quick one-click access to that most-common task for two-clicks for all tasks? I'm fine accepting either "the two clicks aren't that bad" or "copy isn't that much more common than what we expect visualize will be" or even "we don't know and we'll see if users complain after we do this", I just want to make sure that's an intentional usability decision.

I think we can mitigate this issue by only displaying the ... menu option when we think there will be multiple lines. For example, the Source column on the resource grid is always an image name with one line. We should just display the copy button as we did before. But the details view for a resource should display ... for its values because the value could be anything, e.g. JSON in an OTEL span's attribute value.

JamesNK · 2024-07-23T00:19:45Z

This is already done by default when you open the dialog, do you feel that a separate option is important to have rather than preselecting?

Nice, I didn't know that. I don't know which is best, I'll try it out and see what it's like. Preselecting like is already happening is probably fine. That's the important part.

JamesNK · 2024-07-23T00:20:52Z

I would assume the formatted value so as not to confuse the user, or both options?

Let's go with copying the formatted value. If someone wants to copy without formatting, then they can select plaintext and then copy. We can change if there is feedback.

JamesNK · 2024-07-23T00:26:53Z

Here is an idea: color formatting of JSON and XML keywords. We could do it server side, but that would be a fair amount of work. There are JS libraries that do this that we could use:

https://github.com/highlightjs/highlight.js

We know the content type so we can tell the JS library for highlight as JSON for JSON, etc.

drewnoakes

LGTM though would be good to get a UI expert's input. I share Tim's view about the extra click for copy, so would be interested to know if we have additional options here.

src/Aspire.Dashboard/Components/Dialogs/TextVisualizerDialog.razor.cs

src/Aspire.Dashboard/Components/Dialogs/TextVisualizerDialog.razor

src/Aspire.Dashboard/Components/Dialogs/TextVisualizerDialog.razor.cs

drewnoakes · 2024-07-23T00:18:40Z

src/Aspire.Dashboard/Components/Dialogs/TextVisualizerDialog.razor.cs

+            doc.WriteTo(utf8JsonWriter);
+            utf8JsonWriter.Flush();
+
+            _formattedText = Encoding.Default.GetString(memoryStream.ToArray());


I don't think it's correct to use the default encoding here. The Utf8JsonWriter will write UTF8 bytes, which can only be decoded as UTF8.

+1. Add some unit tests with various width unicode characters.

src/Aspire.Dashboard/Components/Dialogs/TextVisualizerDialog.razor.cs

drewnoakes · 2024-07-23T00:20:26Z

src/Aspire.Dashboard/Components/Dialogs/TextVisualizerDialog.razor.cs

+            TryFormatXml();
+        }
+        else if (_selectedOption == JsonFormat)
+        {
+            TryFormatJson();


If these "try" operations fail, should we set _selectedOption back to the item representing no formatting?

I don't think so. The 'JSON' view for a non-JSON string is just the same as the unformatted view.

What happens if you have invalid JSON or xml? Is it only going to format if its valid? I almost want the errors if the user explicitly selects json, but it's not valid json.

src/Aspire.Dashboard/Extensions/FluentUIExtensions.cs

JamesNK · 2024-07-23T02:58:25Z

Large content doesn't overflow with a scrollbar:

And long content doesn't wrap:

I think the content here should act like the console logs text display:

Vertical scroll bar when content exceeds available height.
Text wrapping when content exceeds available width. Ensure that a single long word also wraps (console logs does this).
Line number so you can see whether content wrapped because it's long or there was a new line.

Console logs does all of this already. CSS can either be copied, or ideally, reused.

JamesNK · 2024-07-23T03:00:27Z

Menu is cutoff when located in details view and the details view is narrow:

JamesNK · 2024-07-23T03:07:24Z

I wrote a log with a fairly large JSON attribute (64kb minified). There is an error when trying to view it:

JamesNK · 2024-07-23T03:37:56Z

Add an example of formatted data to stress app: #5021

JamesNK · 2024-07-23T03:56:23Z

The button is currently always highlighted. In other places in the dashboard (e.g. resource type filter) the button is only highlighted if a non-default value is chosen. I think that should be done here as well. e.g. Plaintext = unhighlighted, anything else = highlighted
I think that clicking on an option should close the menu. There is a MenuButton control in FluentUI. I think it might be the right kind of control to use in this situation - https://www.fluentui-blazor.net/MenuButton
Nothing happens when clicking on an invalid option, e.g. selecting XML for non-XML content. I would have expected an error message. Since we're checking the content when the visualizer launches, what about disabling invalid options in the select? For example, disable XML and JSON options if the content is a regular sentence. Then we don't need to worry about showing an error message.

src/Aspire.Dashboard/Components/Layout/MainLayout.razor.cs

…-view # Conflicts: # src/Aspire.Dashboard/Components/Controls/GridValue.razor

adamint · 2024-08-07T16:55:59Z

Some UI things I noticed:

The dialog vertical size doesn’t shrink to match the content. It should take the whole screen to show one line.

Also, there should be a max width on the dialog, so it doesn’t become too wide.

Max width is now set to 600px @JamesNK.

JamesNK · 2024-08-08T06:24:11Z

I think 1000px is the right spot. Enough room for most values, without growing too large on wide screens.

src/Aspire.Dashboard/Components/Layout/MainLayout.razor.cs

src/Aspire.Dashboard/Components/Dialogs/TextVisualizerDialog.razor

JamesNK · 2024-08-08T07:32:17Z

I made style fixes. It's quicker for me to do it rather than communicate here. Also, there was a regression in the old console viewer that I fixed.

JamesNK · 2024-08-08T07:32:31Z

src/Aspire.Dashboard/Components/Layout/MainLayout.razor.cs

+                var effectiveTheme = await _jsModule.InvokeAsync<string>("getCurrentTheme");
+                ThemeManager.EffectiveTheme = effectiveTheme;
+
+                await InvokeAsync(StateHasChanged);


Be careful with calling StateHasChanged and minimize how often it is used. I think in this case, it could be limited to just the new text dialog. Listening to theme change and calling state change in the dialog would be better than doing it globally.

src/Aspire.Dashboard/Components/Layout/MainLayout.razor.cs

src/Aspire.Dashboard/Components/Dialogs/TextVisualizerDialog.razor.cs

…ualizer action

JamesNK

The only major remaining issue is the interaction with splitters. It would be good to know a fix is coming in fluentui.

adamint · 2024-08-09T15:03:11Z

/azp run

azure-pipelines · 2024-08-09T15:03:23Z

Azure Pipelines successfully started running 1 pipeline(s).

adamint · 2024-08-10T02:22:25Z

/azp run

azure-pipelines · 2024-08-10T02:22:36Z

Azure Pipelines successfully started running 1 pipeline(s).

adamint · 2024-08-10T17:59:34Z

/azp run

azure-pipelines · 2024-08-10T17:59:45Z

Azure Pipelines successfully started running 1 pipeline(s).

…-view

…ossible

Adam Ratzman and others added 6 commits July 17, 2024 11:08

Move GridValue copy button inside a menu

9b9eb28

add empty dialog

1aeb0de

Merge branch 'refs/heads/main' into dev/adamint/4412-multiline-string…

10f7cd2

…-view

test

1714da7

Style dialog, work around fluentui onclick issue for FluentMenuItem

eebf7a4

Remove hardcoded string

86c1a5c

adamint requested review from JamesNK, vnbaaij, tlmii, samsp-msft and kvenkatrajan July 22, 2024 21:42

dotnet-issue-labeler bot added the area-dashboard label Jul 22, 2024

adamint added this to the 8.2 milestone Jul 22, 2024

drewnoakes reviewed Jul 23, 2024

View reviewed changes

JamesNK mentioned this pull request Jul 23, 2024

Add value formatting to stress playground app #5021

Merged

JamesNK reviewed Jul 23, 2024

View reviewed changes

src/Aspire.Dashboard/Components/Layout/MainLayout.razor.cs Show resolved Hide resolved

Merge branch 'refs/heads/main' into dev/adamint/4412-multiline-string…

4b262ab

…-view # Conflicts: # src/Aspire.Dashboard/Components/Controls/GridValue.razor

change width/height display of dialog

6cd0349

JamesNK reviewed Aug 8, 2024

View reviewed changes

src/Aspire.Dashboard/Components/Layout/MainLayout.razor.cs Outdated Show resolved Hide resolved

Update src/Aspire.Dashboard/Components/Layout/MainLayout.razor.cs

1593d0d

JamesNK reviewed Aug 8, 2024

View reviewed changes

src/Aspire.Dashboard/Components/Dialogs/TextVisualizerDialog.razor Outdated Show resolved Hide resolved

Style fixes

1892bc8

JamesNK reviewed Aug 8, 2024

View reviewed changes

src/Aspire.Dashboard/Components/Layout/MainLayout.razor.cs Outdated Show resolved Hide resolved

Change text visualizer theming from server to client side

bb2d8d9

JamesNK reviewed Aug 8, 2024

View reviewed changes

src/Aspire.Dashboard/Components/Dialogs/TextVisualizerDialog.razor.cs Outdated Show resolved Hide resolved

adamint added 2 commits August 8, 2024 14:49

Move parameter to codebehind, properly disconnect observer

d52819f

Allow users to click the menu item icons to prompt copy/open text vis…

18cdf6c

…ualizer action

adamint requested a review from JamesNK August 8, 2024 19:17

build-analysis bot mentioned this pull request Aug 8, 2024

Failing test Aspire.Dashboard.Tests.Integration.Playwright.AppBarTests.AppBar_Change_Theme #4851

Closed

JamesNK approved these changes Aug 9, 2024

View reviewed changes

adamint added 2 commits August 10, 2024 14:04

Merge branch 'refs/heads/main' into dev/adamint/4412-multiline-string…

e29961f

…-view

set width to 0 on cell menubutton to show as much column content as p…

bdedc1e

…ossible

adamint merged commit 55eb46f into dotnet:main Aug 12, 2024
11 checks passed

JamesNK mentioned this pull request Aug 12, 2024

Use key as text visualizer title in property grids #5259

Merged

16 tasks

JamesNK mentioned this pull request Sep 2, 2024

Update explore doc with text visualizer dotnet/docs-aspire#1587

Closed

github-actions bot locked and limited conversation to collaborators Sep 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add viewer for long strings in GridValue #5018

Add viewer for long strings in GridValue #5018

adamint commented Jul 22, 2024 •

edited by drewnoakes

Loading

mitchdenny commented Jul 22, 2024

tlmii commented Jul 22, 2024

JamesNK commented Jul 23, 2024 •

edited

Loading

adamint commented Jul 23, 2024

adamint commented Jul 23, 2024

JamesNK commented Jul 23, 2024

JamesNK commented Jul 23, 2024

JamesNK commented Jul 23, 2024

JamesNK commented Jul 23, 2024

drewnoakes left a comment

drewnoakes Jul 23, 2024

JamesNK Jul 23, 2024

drewnoakes Jul 23, 2024

adamint Jul 23, 2024

samsp-msft Aug 1, 2024

JamesNK commented Jul 23, 2024 •

edited

Loading

JamesNK commented Jul 23, 2024

JamesNK commented Jul 23, 2024

JamesNK commented Jul 23, 2024

JamesNK commented Jul 23, 2024 •

edited

Loading

adamint commented Aug 7, 2024 •

edited

Loading

JamesNK commented Aug 8, 2024

JamesNK commented Aug 8, 2024

JamesNK Aug 8, 2024

JamesNK left a comment

adamint commented Aug 9, 2024

azure-pipelines bot commented Aug 9, 2024

adamint commented Aug 10, 2024

azure-pipelines bot commented Aug 10, 2024

adamint commented Aug 10, 2024

azure-pipelines bot commented Aug 10, 2024

Add viewer for long strings in GridValue #5018

Add viewer for long strings in GridValue #5018

Conversation

adamint commented Jul 22, 2024 • edited by drewnoakes Loading

Microsoft Reviewers: Open in CodeFlow

mitchdenny commented Jul 22, 2024

tlmii commented Jul 22, 2024

JamesNK commented Jul 23, 2024 • edited Loading

adamint commented Jul 23, 2024

adamint commented Jul 23, 2024

JamesNK commented Jul 23, 2024

JamesNK commented Jul 23, 2024

JamesNK commented Jul 23, 2024

JamesNK commented Jul 23, 2024

drewnoakes left a comment

Choose a reason for hiding this comment

drewnoakes Jul 23, 2024

Choose a reason for hiding this comment

JamesNK Jul 23, 2024

Choose a reason for hiding this comment

drewnoakes Jul 23, 2024

Choose a reason for hiding this comment

adamint Jul 23, 2024

Choose a reason for hiding this comment

samsp-msft Aug 1, 2024

Choose a reason for hiding this comment

JamesNK commented Jul 23, 2024 • edited Loading

JamesNK commented Jul 23, 2024

JamesNK commented Jul 23, 2024

JamesNK commented Jul 23, 2024

JamesNK commented Jul 23, 2024 • edited Loading

adamint commented Aug 7, 2024 • edited Loading

JamesNK commented Aug 8, 2024

JamesNK commented Aug 8, 2024

JamesNK Aug 8, 2024

Choose a reason for hiding this comment

JamesNK left a comment

Choose a reason for hiding this comment

adamint commented Aug 9, 2024

azure-pipelines bot commented Aug 9, 2024

adamint commented Aug 10, 2024

azure-pipelines bot commented Aug 10, 2024

adamint commented Aug 10, 2024

azure-pipelines bot commented Aug 10, 2024

adamint commented Jul 22, 2024 •

edited by drewnoakes

Loading

JamesNK commented Jul 23, 2024 •

edited

Loading

JamesNK commented Jul 23, 2024 •

edited

Loading

JamesNK commented Jul 23, 2024 •

edited

Loading

adamint commented Aug 7, 2024 •

edited

Loading