From 5b1970d7d93c7e7a41db25b5f608592347bced1a Mon Sep 17 00:00:00 2001 From: Steven Kirk Date: Mon, 21 May 2018 10:14:11 +0200 Subject: [PATCH 1/7] Fix scrolling in PR details view. Added a `ScollingVerticalStackPanel` which can be used to mark controls as "fixed" meaning they don't scroll horizontally. Mark all controls in the PR details as fixed except the changes tree. Fixes #1698 --- .../Controls/ScollingVerticalStackPanel.cs | 178 ++++++++++++++++++ src/GitHub.UI/GitHub.UI.csproj | 1 + .../GitHubPane/PullRequestDetailView.xaml | 55 +++--- .../GitHubPane/PullRequestFilesView.xaml.cs | 2 + 4 files changed, 205 insertions(+), 31 deletions(-) create mode 100644 src/GitHub.UI/Controls/ScollingVerticalStackPanel.cs diff --git a/src/GitHub.UI/Controls/ScollingVerticalStackPanel.cs b/src/GitHub.UI/Controls/ScollingVerticalStackPanel.cs new file mode 100644 index 0000000000..30c76ae952 --- /dev/null +++ b/src/GitHub.UI/Controls/ScollingVerticalStackPanel.cs @@ -0,0 +1,178 @@ +using System; +using System.Windows; +using System.Windows.Controls; +using System.Windows.Controls.Primitives; +using System.Windows.Media; + +namespace GitHub.UI.Controls +{ + /// + /// A vertical stack panel which implements its own logical scrolling, allowing controls to be + /// fixed horizontally in the scroll area. + /// + /// + /// This panel is needed by the PullRequestDetailsView because of #1698: there is no default + /// panel in WPF which allows the horizontal scrollbar to always be present at the bottom while + /// also making the PR description etc be fixed horizontally (non-scrollable) in the viewport. + /// + public class ScollingVerticalStackPanel : Panel, IScrollInfo + { + const int lineSize = 16; + const int mouseWheelSize = 48; + + /// + /// Attached property which when set to True on a child control, will cause it to be fixed + /// horizontally within the scrollable viewport. + /// + public static readonly DependencyProperty IsFixedProperty = + DependencyProperty.RegisterAttached( + "IsFixed", + typeof(bool), + typeof(ScollingVerticalStackPanel), + new FrameworkPropertyMetadata(false, FrameworkPropertyMetadataOptions.AffectsMeasure)); + + public bool CanHorizontallyScroll + { + get { return true; } + set { } + } + + public bool CanVerticallyScroll + { + get { return true; } + set { } + } + + public double ExtentHeight { get; private set; } + public double ExtentWidth { get; private set; } + public double HorizontalOffset { get; private set; } + public double VerticalOffset { get; private set; } + public double ViewportHeight { get; private set; } + public double ViewportWidth { get; private set; } + public ScrollViewer ScrollOwner { get; set; } + + public static bool GetIsFixed(FrameworkElement control) + { + return (bool)control.GetValue(IsFixedProperty); + } + + public static void SetIsFixed(FrameworkElement control, bool value) + { + control.SetValue(IsFixedProperty, value); + } + + public void LineDown() => SetVerticalOffset(VerticalOffset + lineSize); + public void LineLeft() => SetHorizontalOffset(HorizontalOffset - lineSize); + public void LineRight() => SetHorizontalOffset(HorizontalOffset + lineSize); + public void LineUp() => SetVerticalOffset(VerticalOffset - lineSize); + public void MouseWheelDown() => SetVerticalOffset(VerticalOffset + mouseWheelSize); + public void MouseWheelLeft() => SetHorizontalOffset(HorizontalOffset - mouseWheelSize); + public void MouseWheelRight() => SetHorizontalOffset(HorizontalOffset + mouseWheelSize); + public void MouseWheelUp() => SetVerticalOffset(VerticalOffset - mouseWheelSize); + public void PageDown() => SetVerticalOffset(VerticalOffset + ViewportHeight); + public void PageLeft() => SetHorizontalOffset(HorizontalOffset - ViewportWidth); + public void PageRight() => SetHorizontalOffset(HorizontalOffset + ViewportWidth); + public void PageUp() => SetVerticalOffset(VerticalOffset - ViewportHeight); + + public Rect MakeVisible(Visual visual, Rect rectangle) + { + var transform = visual.TransformToVisual(this); + var rect = transform.TransformBounds(rectangle); + var offsetX = HorizontalOffset; + var offsetY = VerticalOffset; + + if (rect.Bottom > ViewportHeight) + { + var delta = rect.Bottom - ViewportHeight; + offsetY += delta; + rect.Y -= delta; + } + + if (rect.Y < 0) + { + offsetY += rect.Y; + } + + // We technially should be trying to also show the right-hand side of the rect here + // using the same technique that we just used to show the bottom of the rect above, + // but in the case of the PR details view, the left hand side of the item is much + // more important than the right hand side and it actually feels better to not do + // this. If this control is used elsewhere and this behavior is required, we could + // put in a switch to enable it. + + if (rect.X < 0) + { + offsetX += rect.X; + } + + SetHorizontalOffset(offsetX); + SetVerticalOffset(offsetY); + + return rect; + } + + public void SetHorizontalOffset(double offset) + { + var value = Math.Max(0, offset); + + if (value != HorizontalOffset) + { + HorizontalOffset = value; + InvalidateArrange(); + } + } + + public void SetVerticalOffset(double offset) + { + var value = Math.Max(0, offset); + + if (value != VerticalOffset) + { + VerticalOffset = value; + InvalidateArrange(); + } + } + + protected override Size MeasureOverride(Size availableSize) + { + var maxWidth = 0.0; + var height = 0.0; + + foreach (FrameworkElement child in Children) + { + var isFixed = GetIsFixed(child); + var childConstraint = new Size( + isFixed ? availableSize.Width : double.PositiveInfinity, + double.PositiveInfinity); + child.Measure(childConstraint); + maxWidth = Math.Max(maxWidth, child.DesiredSize.Width); + height += child.DesiredSize.Height; + } + + ExtentWidth = maxWidth; + ExtentHeight = height; + + return new Size( + Math.Min(maxWidth, availableSize.Width), + Math.Min(height, availableSize.Height)); + } + + protected override Size ArrangeOverride(Size finalSize) + { + var y = -VerticalOffset; + + foreach (FrameworkElement child in Children) + { + var isFixed = GetIsFixed(child); + var x = isFixed ? 0 : -HorizontalOffset; + child.Arrange(new Rect(x, y, child.DesiredSize.Width, child.DesiredSize.Height)); + y += child.DesiredSize.Height; + } + + ViewportWidth = finalSize.Width; + ViewportHeight = finalSize.Height; + ScrollOwner?.InvalidateScrollInfo(); + return finalSize; + } + } +} diff --git a/src/GitHub.UI/GitHub.UI.csproj b/src/GitHub.UI/GitHub.UI.csproj index b5e6c4b839..1a06d44c73 100644 --- a/src/GitHub.UI/GitHub.UI.csproj +++ b/src/GitHub.UI/GitHub.UI.csproj @@ -85,6 +85,7 @@ True OcticonPaths.resx + Spinner.xaml diff --git a/src/GitHub.VisualStudio/Views/GitHubPane/PullRequestDetailView.xaml b/src/GitHub.VisualStudio/Views/GitHubPane/PullRequestDetailView.xaml index 85419b6c70..565346e75a 100644 --- a/src/GitHub.VisualStudio/Views/GitHubPane/PullRequestDetailView.xaml +++ b/src/GitHub.VisualStudio/Views/GitHubPane/PullRequestDetailView.xaml @@ -162,26 +162,15 @@ - - - - - - - - - - - - - - - - - - - - + + + + View on GitHub @@ -255,9 +244,9 @@ + HeaderText="Description" + IsExpanded="True" + ghfvs:ScollingVerticalStackPanel.IsFixed="true"> @@ -282,7 +271,7 @@ HeaderText="Reviewers" IsExpanded="True" Margin="0 8 0 0" - Grid.Row="2"> + ghfvs:ScollingVerticalStackPanel.IsFixed="true"> @@ -294,13 +283,17 @@ - - - + Grid.Row="4" + IsExpanded="True" + HeaderText="{Binding Files.ChangedFilesCount, StringFormat={x:Static prop:Resources.ChangesCountFormat}}" + Margin="0 8 10 0" + ghfvs:ScollingVerticalStackPanel.IsFixed="true"/> + + + + \ No newline at end of file diff --git a/src/GitHub.VisualStudio/Views/GitHubPane/PullRequestFilesView.xaml.cs b/src/GitHub.VisualStudio/Views/GitHubPane/PullRequestFilesView.xaml.cs index 2f2956ef45..9b87f84b8c 100644 --- a/src/GitHub.VisualStudio/Views/GitHubPane/PullRequestFilesView.xaml.cs +++ b/src/GitHub.VisualStudio/Views/GitHubPane/PullRequestFilesView.xaml.cs @@ -7,6 +7,7 @@ using GitHub.Exports; using GitHub.UI.Helpers; using GitHub.ViewModels.GitHubPane; +using GitHub.VisualStudio.UI.Helpers; namespace GitHub.VisualStudio.Views.GitHubPane { @@ -17,6 +18,7 @@ public partial class PullRequestFilesView : UserControl public PullRequestFilesView() { InitializeComponent(); + PreviewMouseWheel += ScrollViewerUtilities.FixMouseWheelScroll; } protected override void OnMouseDown(MouseButtonEventArgs e) From 9652a1d0be167d06889e163d59e014f1c0a2c783 Mon Sep 17 00:00:00 2001 From: Steven Kirk Date: Mon, 21 May 2018 10:28:10 +0200 Subject: [PATCH 2/7] Make CA happy. CA you don't know what you're talking about. --- src/GitHub.UI/Controls/ScollingVerticalStackPanel.cs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/GitHub.UI/Controls/ScollingVerticalStackPanel.cs b/src/GitHub.UI/Controls/ScollingVerticalStackPanel.cs index 30c76ae952..5dbe47d2b9 100644 --- a/src/GitHub.UI/Controls/ScollingVerticalStackPanel.cs +++ b/src/GitHub.UI/Controls/ScollingVerticalStackPanel.cs @@ -1,4 +1,5 @@ using System; +using System.Diagnostics.CodeAnalysis; using System.Windows; using System.Windows.Controls; using System.Windows.Controls.Primitives; @@ -51,11 +52,13 @@ public bool CanVerticallyScroll public double ViewportWidth { get; private set; } public ScrollViewer ScrollOwner { get; set; } + [SuppressMessage("Microsoft.Design", "CA1011:ConsiderPassingBaseTypesAsParameters", Justification = "Can only be applied to controls")] public static bool GetIsFixed(FrameworkElement control) { return (bool)control.GetValue(IsFixedProperty); } + [SuppressMessage("Microsoft.Design", "CA1011:ConsiderPassingBaseTypesAsParameters", Justification = "Can only be applied to controls")] public static void SetIsFixed(FrameworkElement control, bool value) { control.SetValue(IsFixedProperty, value); From 1db2c08d70e3cd80ff6f0b89513b8a97aeefaad4 Mon Sep 17 00:00:00 2001 From: Steven Kirk Date: Mon, 21 May 2018 12:13:46 +0200 Subject: [PATCH 3/7] Fix laggy scrolling when no horizontal scrollbar. --- .../Controls/ScollingVerticalStackPanel.cs | 26 ++++++++++++++++--- 1 file changed, 23 insertions(+), 3 deletions(-) diff --git a/src/GitHub.UI/Controls/ScollingVerticalStackPanel.cs b/src/GitHub.UI/Controls/ScollingVerticalStackPanel.cs index 5dbe47d2b9..1b37623467 100644 --- a/src/GitHub.UI/Controls/ScollingVerticalStackPanel.cs +++ b/src/GitHub.UI/Controls/ScollingVerticalStackPanel.cs @@ -136,6 +136,11 @@ public void SetVerticalOffset(double offset) } } + protected override void ParentLayoutInvalidated(UIElement child) + { + base.ParentLayoutInvalidated(child); + } + protected override Size MeasureOverride(Size availableSize) { var maxWidth = 0.0; @@ -155,6 +160,8 @@ protected override Size MeasureOverride(Size availableSize) ExtentWidth = maxWidth; ExtentHeight = height; + UpdateScrollInfo(new Size(maxWidth, height), new Size(ViewportWidth, ViewportHeight)); + return new Size( Math.Min(maxWidth, availableSize.Width), Math.Min(height, availableSize.Height)); @@ -172,10 +179,23 @@ protected override Size ArrangeOverride(Size finalSize) y += child.DesiredSize.Height; } - ViewportWidth = finalSize.Width; - ViewportHeight = finalSize.Height; - ScrollOwner?.InvalidateScrollInfo(); + UpdateScrollInfo(new Size(ExtentWidth, ExtentHeight), finalSize); return finalSize; } + + void UpdateScrollInfo(Size extent, Size viewport) + { + var changed = extent.Width != ExtentWidth || + extent.Height != ExtentHeight || + viewport.Width != ViewportWidth || + viewport.Height != ViewportHeight; + + ExtentWidth = extent.Width; + ExtentHeight = extent.Height; + ViewportWidth = viewport.Width; + ViewportHeight = viewport.Height; + + if (changed) ScrollOwner?.InvalidateScrollInfo(); + } } } From 4c5d0430bb45e17e4cb85edc761761d74e62573a Mon Sep 17 00:00:00 2001 From: Steven Kirk Date: Mon, 21 May 2018 12:24:28 +0200 Subject: [PATCH 4/7] Make horizontal scrollbar disappear when not needed. --- src/GitHub.UI/Controls/ScollingVerticalStackPanel.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/GitHub.UI/Controls/ScollingVerticalStackPanel.cs b/src/GitHub.UI/Controls/ScollingVerticalStackPanel.cs index 1b37623467..7ccabe1665 100644 --- a/src/GitHub.UI/Controls/ScollingVerticalStackPanel.cs +++ b/src/GitHub.UI/Controls/ScollingVerticalStackPanel.cs @@ -160,7 +160,7 @@ protected override Size MeasureOverride(Size availableSize) ExtentWidth = maxWidth; ExtentHeight = height; - UpdateScrollInfo(new Size(maxWidth, height), new Size(ViewportWidth, ViewportHeight)); + UpdateScrollInfo(new Size(maxWidth, height), availableSize); return new Size( Math.Min(maxWidth, availableSize.Width), From 538d23144a9721cb84a091a1d83796e4c8c8d769 Mon Sep 17 00:00:00 2001 From: Steven Kirk Date: Mon, 21 May 2018 13:23:00 +0200 Subject: [PATCH 5/7] Maybe got it right this time? There's ZERO official documentation on implementing `IScrollInfo` so it's a trial-and-error process. --- .../Controls/ScollingVerticalStackPanel.cs | 15 +++------------ 1 file changed, 3 insertions(+), 12 deletions(-) diff --git a/src/GitHub.UI/Controls/ScollingVerticalStackPanel.cs b/src/GitHub.UI/Controls/ScollingVerticalStackPanel.cs index 7ccabe1665..beab2f50ee 100644 --- a/src/GitHub.UI/Controls/ScollingVerticalStackPanel.cs +++ b/src/GitHub.UI/Controls/ScollingVerticalStackPanel.cs @@ -116,7 +116,7 @@ public Rect MakeVisible(Visual visual, Rect rectangle) public void SetHorizontalOffset(double offset) { - var value = Math.Max(0, offset); + var value = Math.Max(0, Math.Min(offset, ExtentWidth - ViewportWidth)); if (value != HorizontalOffset) { @@ -127,7 +127,7 @@ public void SetHorizontalOffset(double offset) public void SetVerticalOffset(double offset) { - var value = Math.Max(0, offset); + var value = Math.Max(0, Math.Min(offset, ExtentHeight - ViewportHeight)); if (value != VerticalOffset) { @@ -157,9 +157,6 @@ protected override Size MeasureOverride(Size availableSize) height += child.DesiredSize.Height; } - ExtentWidth = maxWidth; - ExtentHeight = height; - UpdateScrollInfo(new Size(maxWidth, height), availableSize); return new Size( @@ -185,17 +182,11 @@ protected override Size ArrangeOverride(Size finalSize) void UpdateScrollInfo(Size extent, Size viewport) { - var changed = extent.Width != ExtentWidth || - extent.Height != ExtentHeight || - viewport.Width != ViewportWidth || - viewport.Height != ViewportHeight; - ExtentWidth = extent.Width; ExtentHeight = extent.Height; ViewportWidth = viewport.Width; ViewportHeight = viewport.Height; - - if (changed) ScrollOwner?.InvalidateScrollInfo(); + ScrollOwner?.InvalidateScrollInfo(); } } } From 64eb5592b45955a22dbcfb2d9e7582f2c5c1dd29 Mon Sep 17 00:00:00 2001 From: Steven Kirk Date: Mon, 21 May 2018 13:52:23 +0200 Subject: [PATCH 6/7] Only take visible controls into account for Extent. --- .../Controls/ScollingVerticalStackPanel.cs | 20 ++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/src/GitHub.UI/Controls/ScollingVerticalStackPanel.cs b/src/GitHub.UI/Controls/ScollingVerticalStackPanel.cs index beab2f50ee..49b7c030a7 100644 --- a/src/GitHub.UI/Controls/ScollingVerticalStackPanel.cs +++ b/src/GitHub.UI/Controls/ScollingVerticalStackPanel.cs @@ -153,7 +153,12 @@ protected override Size MeasureOverride(Size availableSize) isFixed ? availableSize.Width : double.PositiveInfinity, double.PositiveInfinity); child.Measure(childConstraint); - maxWidth = Math.Max(maxWidth, child.DesiredSize.Width); + + if (height - VerticalOffset < availableSize.Height) + { + maxWidth = Math.Max(maxWidth, child.DesiredSize.Width); + } + height += child.DesiredSize.Height; } @@ -167,16 +172,24 @@ protected override Size MeasureOverride(Size availableSize) protected override Size ArrangeOverride(Size finalSize) { var y = -VerticalOffset; + var thisRect = new Rect(finalSize); + var visibleMaxWidth = 0.0; foreach (FrameworkElement child in Children) { var isFixed = GetIsFixed(child); var x = isFixed ? 0 : -HorizontalOffset; - child.Arrange(new Rect(x, y, child.DesiredSize.Width, child.DesiredSize.Height)); + var childRect = new Rect(x, y, child.DesiredSize.Width, child.DesiredSize.Height); + child.Arrange(childRect); y += child.DesiredSize.Height; + + if (childRect.IntersectsWith(thisRect) && childRect.Right > visibleMaxWidth) + { + visibleMaxWidth = childRect.Right; + } } - UpdateScrollInfo(new Size(ExtentWidth, ExtentHeight), finalSize); + UpdateScrollInfo(new Size(visibleMaxWidth, ExtentHeight), new Size(finalSize.Width, finalSize.Height)); return finalSize; } @@ -184,6 +197,7 @@ void UpdateScrollInfo(Size extent, Size viewport) { ExtentWidth = extent.Width; ExtentHeight = extent.Height; + ScrollOwner?.InvalidateScrollInfo(); ViewportWidth = viewport.Width; ViewportHeight = viewport.Height; ScrollOwner?.InvalidateScrollInfo(); From 2f3f06d102ae76ef37694ab7cc146e2c78b95455 Mon Sep 17 00:00:00 2001 From: Steven Kirk Date: Mon, 21 May 2018 16:01:31 +0200 Subject: [PATCH 7/7] Typo oops! --- ...lStackPanel.cs => ScrollingVerticalStackPanel.cs} | 4 ++-- src/GitHub.UI/GitHub.UI.csproj | 2 +- .../Views/GitHubPane/PullRequestDetailView.xaml | 12 ++++++------ 3 files changed, 9 insertions(+), 9 deletions(-) rename src/GitHub.UI/Controls/{ScollingVerticalStackPanel.cs => ScrollingVerticalStackPanel.cs} (98%) diff --git a/src/GitHub.UI/Controls/ScollingVerticalStackPanel.cs b/src/GitHub.UI/Controls/ScrollingVerticalStackPanel.cs similarity index 98% rename from src/GitHub.UI/Controls/ScollingVerticalStackPanel.cs rename to src/GitHub.UI/Controls/ScrollingVerticalStackPanel.cs index 49b7c030a7..5e423ba8c1 100644 --- a/src/GitHub.UI/Controls/ScollingVerticalStackPanel.cs +++ b/src/GitHub.UI/Controls/ScrollingVerticalStackPanel.cs @@ -16,7 +16,7 @@ namespace GitHub.UI.Controls /// panel in WPF which allows the horizontal scrollbar to always be present at the bottom while /// also making the PR description etc be fixed horizontally (non-scrollable) in the viewport. /// - public class ScollingVerticalStackPanel : Panel, IScrollInfo + public class ScrollingVerticalStackPanel : Panel, IScrollInfo { const int lineSize = 16; const int mouseWheelSize = 48; @@ -29,7 +29,7 @@ public class ScollingVerticalStackPanel : Panel, IScrollInfo DependencyProperty.RegisterAttached( "IsFixed", typeof(bool), - typeof(ScollingVerticalStackPanel), + typeof(ScrollingVerticalStackPanel), new FrameworkPropertyMetadata(false, FrameworkPropertyMetadataOptions.AffectsMeasure)); public bool CanHorizontallyScroll diff --git a/src/GitHub.UI/GitHub.UI.csproj b/src/GitHub.UI/GitHub.UI.csproj index 1a06d44c73..ae7d07f227 100644 --- a/src/GitHub.UI/GitHub.UI.csproj +++ b/src/GitHub.UI/GitHub.UI.csproj @@ -85,7 +85,7 @@ True OcticonPaths.resx - + Spinner.xaml diff --git a/src/GitHub.VisualStudio/Views/GitHubPane/PullRequestDetailView.xaml b/src/GitHub.VisualStudio/Views/GitHubPane/PullRequestDetailView.xaml index 565346e75a..67f493c3a5 100644 --- a/src/GitHub.VisualStudio/Views/GitHubPane/PullRequestDetailView.xaml +++ b/src/GitHub.VisualStudio/Views/GitHubPane/PullRequestDetailView.xaml @@ -166,11 +166,11 @@ Margin="0,0,-8,0" HorizontalScrollBarVisibility="Auto" VerticalScrollBarVisibility="Auto"> - + + ghfvs:ScrollingVerticalStackPanel.IsFixed="true"> View on GitHub @@ -246,7 +246,7 @@ + ghfvs:ScrollingVerticalStackPanel.IsFixed="true"> @@ -271,7 +271,7 @@ HeaderText="Reviewers" IsExpanded="True" Margin="0 8 0 0" - ghfvs:ScollingVerticalStackPanel.IsFixed="true"> + ghfvs:ScrollingVerticalStackPanel.IsFixed="true"> @@ -287,13 +287,13 @@ IsExpanded="True" HeaderText="{Binding Files.ChangedFilesCount, StringFormat={x:Static prop:Resources.ChangesCountFormat}}" Margin="0 8 10 0" - ghfvs:ScollingVerticalStackPanel.IsFixed="true"/> + ghfvs:ScrollingVerticalStackPanel.IsFixed="true"/> - + \ No newline at end of file