Allow arbitrary content in music box messages #12271
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
MarnixCroes
approved these changes
Jan 17, 2024
There was a problem hiding this comment.
tack c62de8b
maybe add a tooltip (Play) to the icon?
I know it seems obvious, but most apps (youtube etc) have tooltips even for a Play button.
| @@ -28,7 +29,7 @@ public partial class CoinJoinStateViewModel : ViewModelBase | |||
| private const string WaitingForBlameRoundMessage = "Awaiting the blame round"; | |||
| private const string WaitingRoundMessage = "Awaiting a round"; | |||
| private const string PlebStopMessage = "Coinjoin may be uneconomical"; | |||
| private const string PlebStopMessageBelow = "Add more funds or press 'Play' to bypass"; | |||
| public const string PlebStopMessageBelow = "Add more funds or press 'Play' to bypass"; | |||
There was a problem hiding this comment.
ultra nit: to have the same with line 23
maybe change if you retouch, or feel free to ignore
Suggested change
| public const string PlebStopMessageBelow = "Add more funds or press 'Play' to bypass"; | |
| public const string PlebStopMessageBelow = "Add more funds or press Play to bypass"; |
soosr
reviewed
Jan 22, 2024
Comment on lines
15
to
37
| <converters:KeyToDictionaryItemConverter x:Key="MusicBoxMessageConverter"> | ||
| <ResourceDictionary> | ||
| <StackPanel x:Key="{x:Static wallets:CoinJoinStateViewModel.PressPlayToStartMessage}"> | ||
| <TextBlock Inlines=""> | ||
| <Run>Press </Run> | ||
| <InlineUIContainer> | ||
| <PathIcon Data="{StaticResource play_regular}" /> | ||
| </InlineUIContainer> | ||
| <Run> to start</Run> | ||
| </TextBlock> | ||
| </StackPanel> | ||
| <StackPanel x:Key="{x:Static wallets:CoinJoinStateViewModel.PlebStopMessageBelow}"> | ||
| <TextBlock Inlines=""> | ||
| <Run>Add more funds or press </Run> | ||
| <InlineUIContainer> | ||
| <PathIcon Data="{StaticResource play_regular}" /> | ||
| </InlineUIContainer> | ||
| <Run> to bypass</Run> | ||
| </TextBlock> | ||
| </StackPanel> | ||
| </ResourceDictionary> | ||
| </converters:KeyToDictionaryItemConverter> | ||
| </UserControl.Resources> |
There was a problem hiding this comment.
This part bothers me, as the message is duplicated or at least the one in the ViewModel is useless...
What do you think about having a converter that generates Content from String?
For example, the string Press Play to start string is bound to the ContentControl in XAML, with a converter.
The convert in this case would start to build the content programmatically.
Basically split the string by space, so we have a list of words, then just iterate through them.
// Create a TextBlock
// Init the inlines
if (word == "Play")
// add the Play icon to TextBlock.Inlines
else
// add a TextBlock with the word to TextBlock.Inlines
Return TextBlock;With we would be more flexible, the messages won't be duplicated plus we can change the messages more easily and dynamically.
Also, we could do the same for Stop or Pause icon.
WDYT?
There was a problem hiding this comment.
Nice! I like your approach. I've already implemented it. Please tell me if it's as you expected.
MarnixCroes
suggested changes
Jan 24, 2024
There was a problem hiding this comment.
4a31a35, this crashes when pressing pause
press play -> press stop -> crash
Exception type: System.InvalidOperationException
Message: The control already has a visual parent.
Stack Trace: at Avalonia.Visual.ValidateVisualChild(Visual c)
at Avalonia.Collections.AvaloniaList`1.Add(T item)
at Avalonia.Controls.TextBlock.MeasureOverride(Size availableSize)
at Avalonia.Layout.Layoutable.MeasureCore(Size availableSize)
at Avalonia.Layout.Layoutable.Measure(Size availableSize)
at Avalonia.Layout.LayoutHelper.MeasureChild(Layoutable control, Size availableSize, Thickness padding, Thickness borderThickness)
at Avalonia.Controls.Presenters.ContentPresenter.MeasureOverride(Size availableSize)
at Avalonia.Layout.Layoutable.MeasureCore(Size availableSize)
at Avalonia.Layout.Layoutable.Measure(Size availableSize)
at Avalonia.Layout.LayoutManager.Measure(Layoutable control)
at Avalonia.Layout.LayoutManager.ExecuteMeasurePass()
at Avalonia.Layout.LayoutManager.InnerLayoutPass()
at Avalonia.Layout.LayoutManager.ExecuteLayoutPass()
at Avalonia.Media.MediaContext.FireInvokeOnRenderCallbacks()
at Avalonia.Media.MediaContext.RenderCore()
at Avalonia.Media.MediaContext.Render()
at Avalonia.Threading.DispatcherOperation.InvokeCore()
at Avalonia.Threading.DispatcherOperation.Execute()
at Avalonia.Threading.Dispatcher.ExecuteJob(DispatcherOperation job)
at Avalonia.Threading.Dispatcher.ExecuteJobsCore(Boolean fromExplicitBackgroundProcessingCallback)
at Avalonia.Threading.Dispatcher.Signaled()
at Avalonia.X11.X11PlatformThreading.CheckSignaled()
at Avalonia.X11.X11PlatformThreading.RunLoop(CancellationToken cancellationToken)
at Avalonia.Threading.DispatcherFrame.Run(IControlledDispatcherImpl impl)
at Avalonia.Threading.Dispatcher.PushFrame(DispatcherFrame frame)
at Avalonia.Threading.Dispatcher.MainLoop(CancellationToken cancellationToken)
at Avalonia.Controls.ApplicationLifetimes.ClassicDesktopStyleApplicationLifetime.Start(String[] args)
at Avalonia.ClassicDesktopStyleApplicationLifetimeExtensions.StartWithClassicDesktopLifetime(AppBuilder builder, String[] args, ShutdownMode shutdownMode)
at WalletWasabi.Fluent.Desktop.WasabiAppExtensions.<>c__DisplayClass0_0.<RunAsGuiAsync>b__0() in WalletWasabi.Fluent.Desktop/Program.cs:line 190
at WalletWasabi.Daemon.WasabiApplication.RunAsync(Func`1 afterStarting) in WalletWasabi.Daemon/WasabiAppBuilder.cs:line 78
at WalletWasabi.Fluent.Desktop.WasabiAppExtensions.RunAsGuiAsync(WasabiApplication app) in WalletWasabi.Fluent.Desktop/Program.cs:line 147
at WalletWasabi.Fluent.Desktop.Program.Main(String[] args) in WalletWasabi.Fluent.Desktop/Program.cs:line 63
Inner Exception:
Oh yeah, I discovered the problem. It seems that x:Shared="False" still doesn't work in Avalonia UI. I had to take an alternate aproach. It works now. Thanks for reporting! |
MarnixCroes
approved these changes
Jan 24, 2024
There was a problem hiding this comment.
f74345a works
imo having tooltip for play, pause and stop in the musicbox is good to add.
can be for a follow-up
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This allows any content where we previously had non-formatted text. It allows more visual hints in the MusicBox and could be used in other parts of the application.
Example:
Fixes #11567