Conversation
- Microsoft.Toolkit.Mvvm updated to preview 4 - WpfAnalyzers enabled - Trimmode Enabled - Modified startup code location for Appcentre - Intelisense / Resharper code cleanup and auto optimisation run
There was a problem hiding this comment.
I'm not approving this!
Reasons:
- Empty line endings are removed
- Empty spaces are removed (they're inconsistently removed here and there)
SetCurrentValue()is un-necessarily introduced! (here and there)- Text after
#endregion - And some other changes.
Any non-cleanup change made by you are approved.
Open a new PR with just the changes you made.
Changes made by Intellisense are stupid and inconsistent. Please don't get me wrong for saying this.
| } | ||
|
|
||
| #endregion | ||
| #endregion App activation & Single instancing |
There was a problem hiding this comment.
Text after #endregion will make maintaining the code complex. It would be better the way it was.
| } | ||
| } | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
Why are line endings removed everywhere?
It's a convention to keep a empty line at the end of a code file, isn't it?
Could this be reverted?
| xmlns:ui="http://schemas.modernwpf.com/2019" | ||
| xmlns:utils="clr-namespace:ModernFlyouts.Utilities" | ||
| xmlns:resx="clr-namespace:ModernFlyouts.Properties" | ||
| xmlns:resx="clr-namespace:ModernFlyouts.Properties" xmlns:modernflyouts="clr-namespace:ModernFlyouts" d:DataContext="{d:DesignInstance Type=modernflyouts:AirplaneModeFlyoutHelper}" |
There was a problem hiding this comment.
No need to set the design time datacontext as the flyout will only be initialized at runtime.
There is a check in the helpers to design time and not do anything.
This will just make the xaml complex. No use. Revert it
| private const string SecondTemplatePartName = "SecondContentPresenter"; | ||
| #endregion | ||
|
|
||
| #endregion Constants and Statics |
There was a problem hiding this comment.
Could you just revert this?
It's hard to keep adding the same text on the start and end of a region. It requires both titles to be same. Please?
| TimeBar.Value = 0; | ||
|
|
||
| TimelineInfoButton.Visibility = Visibility.Collapsed; | ||
| TimelineInfoButton.SetCurrentValue(VisibilityProperty, Visibility.Collapsed); |
There was a problem hiding this comment.
No no not this please! Revert them!!
| UpdatePlaybackInfo(session); | ||
|
|
||
| await SetThumbnailAsync(mediaInfo.Thumbnail, playback.PlaybackType); | ||
| await SetThumbnailAsync(mediaInfo.Thumbnail, session.GetPlaybackInfo().PlaybackType); |
There was a problem hiding this comment.
Revert this one too.
The GetPlaybackInfo() call is expensive. Calling it multiple times will result in increase in memory usage.
| ThumbnailBackgroundBrush.SetCurrentValue(Brush.OpacityProperty, 0.0); | ||
| ThumbnailImageBrush.SetCurrentValue(Brush.OpacityProperty, 0.0); | ||
| TextBlockGrid.SetCurrentValue(OpacityProperty, 0.0); | ||
| mediaArtistBlockTranslateTransform.SetCurrentValue(TranslateTransform.YProperty, 0.0); | ||
| mediaTitleBlockTranslateTransform.SetCurrentValue(TranslateTransform.YProperty, 0.0); |
| T1.SetCurrentValue(System.Windows.Media.TranslateTransform.YProperty, (double)-40); | ||
| if (ContentGrid != null) ContentGrid.SetCurrentValue(OpacityProperty, (double)0); |
There was a problem hiding this comment.
Please revert every change that introduced SetCurrentValue. This is insane! What happened to Intellisense!
Is it on drugs?
| var defaultPosition = toDefault ? FlyoutHandler.Instance.DefaultFlyoutPosition : AppDataHelper.FlyoutPosition; | ||
|
|
||
| Left = defaultPosition.X - leftShadowMargin; Top = defaultPosition.Y; | ||
| Left = defaultPosition.X - leftShadowMargin; SetCurrentValue(TopProperty, defaultPosition.Y); |
There was a problem hiding this comment.
This is inconsistent!
Left is untouched but Top is modified!
| public const string ChevronLeft = "\uE00E"; | ||
|
|
||
| public const string ChevronRight = "\uE00F"; | ||
|
|
||
| public const string ChevronUp = "\uE010"; | ||
|
|
||
| public const string ChevronDown = "\uE011"; | ||
|
|
||
| public const string Refresh = "\uE149"; |
There was a problem hiding this comment.
It removed empty spaces only in some places.
Other places are left untouched.
|
Open a new PR only with these changes. Sorry for not approving this PR 😅. The cleanup made a large mess!
|
|
Wow, after looking a the things you pointed out i see why people don't use intelisense.
all good, the reason i made this as a PR was because i was questioning how good intelisense's changes were. |
Due to the extensive changes (largely done by intelisense code cleanup) just want to get your approval @ShankarBUS and to make sure no bugs.