Version 0.5.0 and 0.6.0 #6

fabianoriccardi · 2022-04-03T13:11:43Z

No description provided.

… files

…ion is deprecated

…ication can capture ThreadInterruptedException; show appropriate message if user stops the test

…d verification phase

…nforms app

…dependencies

dkrahmer

I like most of your changes. I have a few tweaks I would like you to make before I accept your PR. See file comments for details. I may have additional feedback but I wanted send you what I have so far.

Other feedback:

The .sln file should be updated with the VS 2022 header, including MinimumVisualStudioVersion.
The logo looks great!

dkrahmer · 2022-04-03T18:34:19Z

MediaTester/AboutForm.cs

+
+		private void linkLabelRepository_LinkClicked(object sender, LinkLabelLinkClickedEventArgs e)
+		{
+			string url = "http://www.github.com/fabianoriccardi/MediaTester";


This should link to the head repo (dkrahmer) instead of your fork.

dkrahmer · 2022-04-03T19:33:24Z

MediaTester/Main.Designer.cs

+            this.ChooseTargetButton.Location = new System.Drawing.Point(306, 32);
+            this.ChooseTargetButton.Margin = new System.Windows.Forms.Padding(3, 2, 3, 2);
+            this.ChooseTargetButton.Name = "ChooseTargetButton";
+            this.ChooseTargetButton.Size = new System.Drawing.Size(28, 31);


The button is too small and only shows ".." instead of "..." when I run the app.

Of course the button can be enlarged, but this seems weird, I see it correclty. Which OS do you use? May you attach a screeshot?

This is a screenshot when running. It looks the same in the VS2022 designer view.

I don't understand why it happens, however I have resized it.

dkrahmer · 2022-04-03T20:11:37Z

MediaTester/Main.Designer.cs

+            this.button1.Size = new System.Drawing.Size(50, 32);
+            this.button1.TabIndex = 21;
+            this.button1.UseVisualStyleBackColor = true;
+            this.button1.Click += new System.EventHandler(this.button1_Click);


A few things:

button1 and the event method should have descriptive names since they are referenced in non-designer code.

My original "About..." link was kinda hacky but this new button is too big and covers an unrelated control. Maybe change this to a small image of a question mark in a circle (?) placed in the lower right of the window, in the far right corner of the status bar.

Add a tooltip that shows "About..." so the user knows what it is without having to click on it.

dkrahmer · 2022-04-03T20:24:59Z

MediaTesterCli/MediaTesterCli.csproj

-    <Configuration Condition=" '$(Configuration)' == '' ">Debug</Configuration>
-    <Platform Condition=" '$(Platform)' == '' ">AnyCPU</Platform>
-    <ProjectGuid>{82DBB478-8EF7-45B1-8200-F28BEF178019}</ProjectGuid>
+    <TargetFramework>net6.0</TargetFramework>


For best compatibility, I would like this to have multiple build targets. Framework 4.7.2 (for Windows) with Costura.Fody and net6.0 (for everything else). Let me know if you need help with getting multiple build targets setup with a conditional Costura.Fody nuget package.

Yes, I would ask your help for this.

dkrahmer · 2022-04-03T20:28:14Z

MediaTesterCli/Program.cs

 			{
 				while (string.IsNullOrEmpty(testDirectory))
 				{
 					Console.WriteLine();
 					Console.WriteLine();
-					Console.Write("Please enter a drive letter or path to test: ");
+					Console.Write("Please enter a drive letter or a full path: ");


Does it really need to specify "a full path"? The following seems sufficient:

Console.Write("Please enter a drive letter or path: ");

dkrahmer · 2022-04-03T22:19:42Z

CodeMaid.config

@@ -0,0 +1,15 @@
+<?xml version="1.0" encoding="utf-8"?>


This project uses .editorconfig. This CodeMaid.config file should not be committed since it is your personal preferences. Please remove and add to .gitignore

dkrahmer · 2022-04-03T22:21:23Z

README.md

+
+- MediaTester: the GUI frontend. This project is based on WinForms for .NET Framework 4.7.2, for the best compatibility with the latests versions of Windows 10/11.
+
+The code is developed in VS2022 and automatically styled with CodeMaid.


Remove reference to CodeMaid. You are welcome to use it but it is not part of this project. This project uses .editorconfig for style preferences.

dkrahmer · 2022-04-03T22:24:54Z

MediaTester/App.config

+	<runtime>
+		<assemblyBinding xmlns="urn:schemas-microsoft-com:asm.v1">


Is this assembly binding necessary?

dkrahmer · 2022-04-03T22:25:40Z

MediaTester/Properties/AssemblyInfo.cs

@@ -7,9 +7,9 @@
 [assembly: AssemblyTitle("MediaTester")]
 [assembly: AssemblyDescription("")]
 [assembly: AssemblyConfiguration("")]
-[assembly: AssemblyCompany("KrahmerSoft")]
+[assembly: AssemblyCompany("")]


Add back "KrahmerSoft"

dkrahmer · 2022-04-03T22:30:34Z

MediaTester/AboutForm.Designer.cs

+            this.button1.TabIndex = 3;
+            this.button1.Text = "OK";
+            this.button1.UseVisualStyleBackColor = true;
+            this.button1.Click += new System.EventHandler(this.button1_Click);


button1 and the event method should have descriptive names since they are referenced in non-designer code.

dkrahmer · 2022-04-05T22:44:40Z

MediaTester/Main.cs

+		/// <summary>
+		/// Remove test files after write+verify. This method do no update the progress bar!
+		/// </summary>
+		private void RemoveTempDataFilesNoProgressBar()


Why do we need this? It seems redundant.

dkrahmer · 2022-04-05T22:48:13Z

MediaTesterLib/MediaTester.cs

-		public WriteBlockCompleteHandler AfterWriteBlock;
-		public VerifyBlockCompleteHandler AfterQuickTest;
-		public VerifyBlockCompleteHandler AfterVerifyBlock;
-		public ExceptionHandler OnException;


Why did you change all of these names? I am fine with changing from delegate to event but the names should stay the same.

dkrahmer · 2022-04-05T22:52:59Z

MediaTesterLib/MediaTester.cs

+			else if (_options.MaxBytesToTest > GetAvailableBytes())
+			{
+				TotalTargetBytes = GetAvailableBytes();
+				OnExceptionThrown(new Exception($"Not enough space to test the requested size. Downsizing to the available disk space: {TotalTargetBytes:#,##0} bytes"));


Is this really an exception?

dkrahmer · 2022-04-05T22:55:01Z

MediaTesterLib/MediaTester.cs

 		}

+		/// <summary>
+		/// Verify a test file. TODO this function can be split and simplified.


Should not have TODO in a method summary

dkrahmer · 2022-04-05T22:55:25Z

MediaTesterLib/MediaTester.cs

+		/// <param name="bytesVerified"></param>
+		/// <param name="bytesFailed"></param>
+		/// <param name="updateTotalBytes"></param>
+		/// <returns></returns>
 		private bool VerifyTestFile(int testFileIndex, string testFilePath, out int bytesVerified, out int bytesFailed, bool updateTotalBytes = false)


Why are there so many changes to this method?

dkrahmer · 2022-04-05T22:56:12Z

MediaTesterLib/MediaTester.cs

+		/// Creates and writes all the test files.
+		/// </summary>
+		/// <returns></returns>
+		private bool GenerateTestFiles()


Why are there so many changes to this method?

dkrahmer · 2022-04-06T18:03:55Z

MediaTester/Main.Designer.cs

@@ -631,7 +605,7 @@ private void InitializeComponent()
 		private System.Windows.Forms.Label FailedBytesLabel;
 		private System.Windows.Forms.Label VerifiedBytesLabel;
 		private System.Windows.Forms.Label WrittenBytesLabel;
-		private System.Windows.Forms.Label ReadSpeedLabel;
+		private System.Windows.Forms.Label VerifySpeedLabel;


Why did you rename this?

dkrahmer · 2022-04-06T18:17:40Z

@fabianoriccardi, I have spend some more time reviewing this PR. I like a lot of your improvements but you also included many changes based on personal preference, rather than necessity. Also, much of the added text contains grammatical errors. I am not interested in a complete overhaul or restyle of my applications. I think you should break this PR up into smaller PRs that focus on bug fixes rather than personal preference.

Doug Krahmer and others added 30 commits February 2, 2020 12:01

Enhance quick tests by checking the beginning and end of each file.

cb3b9df

Add additional log information

9346b04

upgrade to .NET 4.8

d02ae6d

fix typos

76b9ac5

upgrade to .NET 6 (add dpi-awareness)

49b4243

fix ui (progress bar)

06dfd6f

thread abort() is now thread interrupt()

55f0cb7

fix overlapping label

5a76c37

rewrite event in modern c# (use event instead of delegate)

60e6358

typo

0297e3e

code style (with codemaid)

950dd82

enable codemaid format on save

1e38755

update json library and add relative docs

318b936

add docs

6a5789f

code cleanup and reordering

4dd272e

add docs

2b833b8

connect AfterVerifyBlock and remove old delegate

feb50e3

minor ui fix

2fad1e4

docs

2a86e06

fix CLI version accordingly to event-delegate changes in MediaTesterLib

1c89199

add info for building releases

175f073

remove no more needed nuget dependencies and remove FodyWeaver config…

a41611c

… files

remove App.config files since this format in .NET Core and newer vers…

bb3904e

…ion is deprecated

switch from Newtonsoft.Json to System.Text.Json

e3dea1d

indent json in option file

59ceccd

re-throw generic exceptions from mediatester library so the main appl…

55b7c57

…ication can capture ThreadInterruptedException; show appropriate message if user stops the test

replacing delegates with lambda functions, refactoring

29f1b37

avoid crash if the drive is removed during the test, both in write an…

c51d35c

…d verification phase

remove another delegate 😅

f238e16

minor refactor and docs

37cb7c3

fabianoriccardi added 20 commits February 19, 2022 18:24

update readme

3a2b545

add note about installation

5f1a072

modify about page

b1139e5

add app icon, add link to github repo

975a8b3

add about icon, minor fixes

c49e4da

fix for single-file app deployment

cedfcdf

version bump, update readme for the current release

916f87f

Merge branch 'development'

b81fcab

[lib] switch to .net standard 2.1

90d10a3

[lib] switch to .net standard 2.0

ae315df

[ui] switch to .net 4.8

a5a1042

set font Segoe UI 9pt (default font in modern win app), fix about icon

80fce23

[ui] switch to .net 4.7.2, the oldest version supporting DPI-aware wi…

402bf73

…nforms app

[ui] add icon to executable and enable single-file app

d6c89a5

move dpi-awareness flag into app.manifest (embedded in executable)

388d11a

fix broken dependency System.Runtime.CompilerServices.Unsafe, update …

d69c06c

…dependencies

fix wrong dpi resizing policy and ui quirks

f570c1e

update readme

3d7f7f3

bump versions

f58aca7

Merge branch 'development'

827790e

dkrahmer requested changes Apr 3, 2022

View reviewed changes

various fixes and cleanup

496118e

dkrahmer reviewed Apr 5, 2022

View reviewed changes

dkrahmer reviewed Apr 6, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Version 0.5.0 and 0.6.0 #6

Version 0.5.0 and 0.6.0 #6

fabianoriccardi commented Apr 3, 2022

dkrahmer left a comment

dkrahmer Apr 3, 2022

dkrahmer Apr 3, 2022

fabianoriccardi Apr 4, 2022

dkrahmer Apr 5, 2022

fabianoriccardi Apr 5, 2022

dkrahmer Apr 3, 2022

dkrahmer Apr 3, 2022

fabianoriccardi Apr 5, 2022

dkrahmer Apr 3, 2022

dkrahmer Apr 3, 2022

dkrahmer Apr 3, 2022

dkrahmer Apr 3, 2022

dkrahmer Apr 3, 2022

dkrahmer Apr 3, 2022

dkrahmer Apr 5, 2022

dkrahmer Apr 5, 2022

dkrahmer Apr 5, 2022

dkrahmer Apr 5, 2022

dkrahmer Apr 5, 2022

dkrahmer Apr 5, 2022

dkrahmer Apr 6, 2022

dkrahmer commented Apr 6, 2022


		- MediaTester: the GUI frontend. This project is based on WinForms for .NET Framework 4.7.2, for the best compatibility with the latests versions of Windows 10/11.

		The code is developed in VS2022 and automatically styled with CodeMaid.

		<runtime>
		<assemblyBinding xmlns="urn:schemas-microsoft-com:asm.v1">

Version 0.5.0 and 0.6.0 #6

Are you sure you want to change the base?

Version 0.5.0 and 0.6.0 #6

Conversation

fabianoriccardi commented Apr 3, 2022

dkrahmer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkrahmer commented Apr 6, 2022