Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ProgressBar.WriteLine does not work correctly on multiline writes or when lines are longer then ConsoleWidth #102

Open
EddyToo opened this issue Dec 6, 2022 · 3 comments

Comments

@EddyToo
Copy link

EddyToo commented Dec 6, 2022

The DefaultConsoleWrite method assumes that the written height will always be 1 which is obviously not the case when writing multiline strings or when the string contents does not fit on a single line.
The effect is that anything beyond the first line will be overwritten

The method also increases the _originalCursorTop with the lines written, ignoring the effect scrolling beyond the size of the buffer has. Since 5 messages can be written for each draw operation it can lead to _originalCursorTop exceeding WindowHeight.
This causes different visual issues because the code looses track of where the top of the progressbar was/should be

This issue is related to #79

@EddyToo
Copy link
Author

EddyToo commented Dec 6, 2022

Suggested patch to resolve this issue and #79

diff --git a/src/ShellProgressBar.Example/Examples/MessageBeforeAndAfterExample.cs b/src/ShellProgressBar.Example/Examples/MessageBeforeAndAfterExample.cs
index 4280eef..eaaf98e 100644
--- a/src/ShellProgressBar.Example/Examples/MessageBeforeAndAfterExample.cs
+++ b/src/ShellProgressBar.Example/Examples/MessageBeforeAndAfterExample.cs
@@ -8,7 +8,7 @@ namespace ShellProgressBar.Example.Examples
 		protected override Task StartAsync()
 		{
 			Console.WriteLine("This should not be overwritten");
-			const int totalTicks = 10;
+			int totalTicks = Console.WindowHeight;
 			var options = new ProgressBarOptions
 			{
 				ForegroundColor = ConsoleColor.Yellow,
@@ -18,9 +18,27 @@ namespace ShellProgressBar.Example.Examples
 			};
 			using (var pbar = new ProgressBar(totalTicks, "showing off styling", options))
 			{
-				TickToCompletion(pbar, totalTicks, sleep: 500, i =>
+				TickToCompletion(pbar, totalTicks, sleep: 250, i =>
 				{
-					pbar.WriteErrorLine($"This should appear above:{i}");
+					if (i % 5 == 0)
+					{
+						// Single line
+						pbar.WriteErrorLine($"[{i}] This{Environment.NewLine}[{i}] is{Environment.NewLine}[{i}] over{Environment.NewLine}[{i}] 4 lines");
+						return;
+					}
+					if (i % 4 == 0)
+					{
+						// Single line
+						pbar.WriteErrorLine($"[{i}] This has{Environment.NewLine}[{i}] 2 lines.");
+						return;
+					}
+					if (i % 3 == 0)
+					{
+						// Single line
+						pbar.WriteErrorLine($"[{i}] This is a very long line {new string('.', Console.BufferWidth)} and should be split over 2 lines");
+						return;
+					}
+					pbar.WriteErrorLine($"[{i}] This should appear above");
 				});
 			}
 
diff --git a/src/ShellProgressBar.Example/Program.cs b/src/ShellProgressBar.Example/Program.cs
index b947538..0acc86e 100644
--- a/src/ShellProgressBar.Example/Program.cs
+++ b/src/ShellProgressBar.Example/Program.cs
@@ -66,6 +66,9 @@ namespace ShellProgressBar.Example
 				case "test":
 					await RunTestCases(token);
 					return;
+				case "scrolltest":
+					await RunTestCases(token, Console.WindowHeight+5);
+					return;
 				case "example":
 					var nth = args.Length > 1 ? int.Parse(args[1]) : 0;
 					await RunExample(nth, token);
@@ -88,12 +91,16 @@ namespace ShellProgressBar.Example
 			await example.Start(token);
 		}
 
-		private static async Task RunTestCases(CancellationToken token)
+		private static async Task RunTestCases(CancellationToken token, int writeNumOfRowBefore = 0)
 		{
 			var i = 0;
 			foreach (var example in TestCases)
 			{
 				if (i > 0) Console.Clear(); //not necessary but for demo/recording purposes.
+
+				for (int r = 0; r< writeNumOfRowBefore; r++)
+					Console.WriteLine($"Writing output before test. Row {r+1}/{writeNumOfRowBefore}");
+
 				await example.Start(token);
 				i++;
 			}
diff --git a/src/ShellProgressBar/ProgressBar.cs b/src/ShellProgressBar/ProgressBar.cs
index a76e525..9208fa8 100644
--- a/src/ShellProgressBar/ProgressBar.cs
+++ b/src/ShellProgressBar/ProgressBar.cs
@@ -2,7 +2,6 @@
 using System.Collections.Concurrent;
 using System.Collections.Generic;
 using System.Linq;
-using System.Runtime.CompilerServices;
 using System.Runtime.InteropServices;
 using System.Threading;
 using System.Threading.Tasks;
@@ -15,11 +14,10 @@ namespace ShellProgressBar
 
 		private readonly ConsoleColor _originalColor;
 		private readonly Func<ConsoleOutLine, int> _writeMessageToConsole;
-		private readonly int _originalWindowTop;
-		private readonly int _originalWindowHeight;
 		private readonly bool _startedRedirected;
 		private int _originalCursorTop;
 		private int _isDisposed;
+		private int _lastDrawBottomPos;
 
 		private Timer _timer;
 		private int _visibleDescendants = 0;
@@ -41,8 +39,6 @@ namespace ShellProgressBar
 			try
 			{
 				_originalCursorTop = Console.CursorTop;
-				_originalWindowTop = Console.WindowTop;
-				_originalWindowHeight = Console.WindowHeight + _originalWindowTop;
 				_originalColor = Console.ForegroundColor;
 			}
 			catch
@@ -56,7 +52,7 @@ namespace ShellProgressBar
 			if (this.Options.EnableTaskBarProgress)
 				TaskbarProgress.SetState(TaskbarProgress.TaskbarStates.Normal);
 
-			if (this.Options.DisplayTimeInRealTime)
+			if (this.Options.DisplayTimeInRealTime) 
 				_timer = new Timer((s) => OnTimerTick(), null, 500, 500);
 			else //draw once
 				_timer = new Timer((s) =>
@@ -102,18 +98,22 @@ namespace ShellProgressBar
 
 		private void EnsureMainProgressBarVisible(int extraBars = 0)
 		{
+			var lastVisibleRow = Console.WindowHeight + Console.WindowTop;
+
 			var pbarHeight = this.Options.DenseProgressBar ? 1 : 2;
-			var neededPadding = Math.Min(_originalWindowHeight - pbarHeight, (1 + extraBars) * pbarHeight);
-			var difference = _originalWindowHeight - _originalCursorTop;
-			var write = difference <= neededPadding ? Math.Max(0, Math.Max(neededPadding, difference)) : 0;
+			var neededPadding = Math.Min(lastVisibleRow - pbarHeight, (1 + extraBars) * pbarHeight);
+			var difference = lastVisibleRow - _originalCursorTop;
+			var write = difference <= neededPadding ? Math.Min(Console.WindowHeight, Math.Max(0, Math.Max(neededPadding, difference))) : 0;
+
+			if (write == 0)
+				return;
 
 			var written = 0;
 			for (; written < write; written++)
 				Console.WriteLine();
-			if (written == 0) return;
 
-			Console.CursorTop = _originalWindowHeight - (written);
-			_originalCursorTop = Console.CursorTop - 1;
+			Console.CursorTop = Console.WindowHeight + Console.WindowTop - write;
+			_originalCursorTop = Console.CursorTop -1;
 		}
 
 		private void GrowDrawingAreaBasedOnChildren() => EnsureMainProgressBarVisible(_visibleDescendants);
@@ -345,7 +345,12 @@ namespace ShellProgressBar
 
 			DrawChildren(this.Children, indentation, ref cursorTop, Options.PercentageFormat);
 
-			ResetToBottom(ref cursorTop);
+			if (Console.CursorTop < _lastDrawBottomPos)
+			{
+				// The bar shrunk. Need to clean the remaining rows
+				ClearLines(_lastDrawBottomPos - Console.CursorTop);
+			}
+			_lastDrawBottomPos = Console.CursorTop;
 
 			Console.SetCursorPosition(0, _originalCursorTop);
 			Console.ForegroundColor = _originalColor;
@@ -355,35 +360,60 @@ namespace ShellProgressBar
 			_timer = null;
 		}
 
+		private static void ClearLines(int numOfLines)
+		{
+			// Use bufferwidth and not only the visible width. (currently identical on all platforms)
+			Console.Write(new string(' ', Console.BufferWidth * numOfLines));
+		}
+
+		private static string _resetString = "";
+		private static void ClearCurrentLine()
+		{
+			if (_resetString.Length != Console.BufferWidth + 2)
+			{
+				// Use buffer width and not only the visible width. (currently identical on all platforms)
+				_resetString = $"\r{new string(' ', Console.BufferWidth)}\r";
+			}
+			Console.Write(_resetString);
+		}
+
 		private void WriteConsoleLine(ConsoleOutLine m)
 		{
-			var resetString = new string(' ', Console.WindowWidth);
-			Console.Write(resetString);
-			Console.Write("\r");
 			var foreground = Console.ForegroundColor;
 			var background = Console.BackgroundColor;
-			var written = _writeMessageToConsole(m);
+			ClearCurrentLine();
+			var moved = _writeMessageToConsole(m);
 			Console.ForegroundColor = foreground;
 			Console.BackgroundColor = background;
-			_originalCursorTop += written;
+			_originalCursorTop += moved;
 		}
 
 		private static int DefaultConsoleWrite(ConsoleOutLine line)
 		{
-			if (line.Error) Console.Error.WriteLine(line.Line);
-			else Console.WriteLine(line.Line);
-			return 1;
-		}
+			var fromPos = Console.CursorTop;
 
-		private void ResetToBottom(ref int cursorTop)
-		{
-			var resetString = new string(' ', Console.WindowWidth);
-			var windowHeight = _originalWindowHeight;
-			if (cursorTop >= (windowHeight - 1)) return;
-			do
+			// First line was already cleared by WriteConsoleLine().
+			// Would be cleaner to do it here, but would break backwards compatibility for those
+			// who implemented their own writer function.
+			bool isClearedLine = true;
+			foreach (var outLine in line.Line.SplitToConsoleLines())
 			{
-				Console.Write(resetString);
-			} while (++cursorTop < (windowHeight - 1));
+				// Skip slower line clearing if we scrolled on last write
+				if (!isClearedLine)
+					ClearCurrentLine();
+
+				int lastCursorTop = Console.CursorTop;
+				if (line.Error)
+					Console.Error.WriteLine(outLine);
+				else
+					Console.WriteLine(outLine);
+
+				// If the cursorTop is still on same position we are at the end of the buffer and scrolling happened.
+				isClearedLine = lastCursorTop == Console.CursorTop;
+			}
+
+			// Return how many rows the cursor actually moved by.
+			return Console.CursorTop - fromPos;
 		}
 
 		private static void DrawChildren(IEnumerable<ChildProgressBar> children, Indentation[] indentation,
@@ -392,12 +422,12 @@ namespace ShellProgressBar
 			var view = children.Where(c => !c.Collapse).Select((c, i) => new {c, i}).ToList();
 			if (!view.Any()) return;
 
-			var windowHeight = Console.WindowHeight;
+			var lastVisibleRow = Console.WindowHeight + Console.WindowTop;
 			var lastChild = view.Max(t => t.i);
 			foreach (var tuple in view)
 			{
-				//Dont bother drawing children that would fall off the screen
-				if (cursorTop >= (windowHeight - 2))
+				// Dont bother drawing children that would fall off the screen and don't want to scroll top out of view
+				 if (cursorTop >= (lastVisibleRow - 2))
 					return;
 
 				var child = tuple.c;
@@ -500,7 +530,7 @@ namespace ShellProgressBar
 			{
 				var pbarHeight = this.Options.DenseProgressBar ? 1 : 2;
 				var openDescendantsPadding = (_visibleDescendants * pbarHeight);
-				var newCursorTop = Math.Min(_originalWindowHeight, _originalCursorTop + pbarHeight + openDescendantsPadding);
+				var newCursorTop = Math.Min(Console.WindowHeight+Console.WindowTop, _originalCursorTop + pbarHeight + openDescendantsPadding);
 				Console.CursorVisible = true;
 				Console.SetCursorPosition(0, newCursorTop);
 			}
diff --git a/src/ShellProgressBar/StringExtensions.cs b/src/ShellProgressBar/StringExtensions.cs
index bc3071b..0d00102 100644
--- a/src/ShellProgressBar/StringExtensions.cs
+++ b/src/ShellProgressBar/StringExtensions.cs
@@ -12,5 +12,32 @@ namespace ShellProgressBar
                 return phrase;
             return phrase.Substring(0, length - 3) + "...";
         }
+
+        /// <summary>
+        /// Splits a string into it's indiviudal lines and then again splits these individual lines
+        /// into multiple lines if they exceed the width of the console.
+        /// </summary>
+        /// <param name="str"></param>
+        /// <returns></returns>
+        public static IEnumerable<string> SplitToConsoleLines(this string str)
+        {
+	        int width = Console.BufferWidth;
+	        var lines = str.Split(new string[] { Environment.NewLine }, StringSplitOptions.None);
+
+	        foreach (var line in lines)
+	        {
+		        if (line.Length > width)
+		        {
+			        for (int i = 0; i < line.Length; i += width)
+			        {
+				        yield return line.Substring(i, Math.Min(width, line.Length - i));
+			        }
+		        }
+		        else
+		        {
+			        yield return line;
+		        }
+	        }
+        }
     }
 }

@KoalaBear84
Copy link

It would be great if issues with this are being resolved, as it is almost unusable to use the WriteLine. I was glad I saw the ProgressBar.WriteLine method, but after that already smashed to the floor again because it isn't usable 😂

@tishige
Copy link

tishige commented Jan 20, 2023

Thank you!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants