Adding TransferSpeedColumn configuration to display bits/bytes + binary/decimal prefixes

[tpill90]

Context

Currently the TransferSpeedColumn is displaying the transfer speed using the binary/IEC definition of a kilobyte/megabyte/gigabyte (1024^x)

Typically, network transfer speeds are specified using the decimal definition (1000^x), for example Ethernet (10/100/1000BASE-T), or internet connection speeds (100mbit = 100,000,000,000)

https://en.wikipedia.org/wiki/Binary_prefix#Data_transmission_and_clock_rates

Describe the solution you'd like

I would like to propose adding the ability to configure which binary prefix (1000^x or 1024^x) is used for display, as well as configuring to display either bits/s or bytes/s. This would allow developers to be able to tweak formatting to better match their use case.

The default display format would be to continue to use the current format, 1024^x , as to ensure that there are no unexpected changes to the existing behavior when updating to a newer version of the package.

Using a data transfer rate of 1,000,000 bytes, the expected output for each configurablee option would be as follows :

Binary Prefix	Bits/Bytes	Expected Output
Binary	bytes	.95 MiB/s
Decimal	bytes	1 MB/s
Binary	bits	7.63 Mibit/s
Decimal	bits	8 Mbit/s

Testing

These changes have been in use for about a month now in my projects steam-lancache-prefill and battlenet-lancache-prefill, and have been able to be tested and proven to be working, as well as being able to smooth over any unexpected edge cases.

Example output of the change :

Related commit, where changes started to be used

---

Please upvote 👍 this pull request if you are interested in it.

[CLAassistant]

All committers have signed the CLA.

[tpill90]

@patriksvensson @phil-scott-78 Could I kindly ask for your thoughts and feedback on these changes whenever you have some time to take a look? I'd be more than happy to discuss the changes in further details, and I would be glad to know if there is anything I can do to help get these changes included. Thank you!

[tpill90]

@microsoft-github-policy-service agree

[FrankRay78]

Hey @tpill90, looks like an interesting, well drafted PR - are you still around to help me get this reviewed/merged? (assuming I might need your help, although perhaps we get lucky with a straight merge...). I assume you'd like this for https://github.com/tpill90/steam-lancache-prefill ? (which looks awesome btw)?

Update
An initial cursory glance seems to reveal Live\Progress\Columns\TransferSpeedColumn missing test coverage (in main branch, not your PR). You've done a great job of updating DownloadColumnTests, and it would seem reasonable to take this opportunity to retrofit some initial tests for TransferSpeedColumn as well.

[FrankRay78]

I like this PR @tpill90 and have started reviewing it. Solid code.

[FrankRay78]

Initial review feedback and suggested changes here: tpill90#1

[FrankRay78]

Brain dump of my review comments below.

I used to dread this kind of thing when contributing @tpill90, but the review is a sign that I feel your contribution is worth spending time on, and getting it over the line.

I'm not 'the boss' and you don't need to silently accept everything I say in this, rather, we are peers, your coding skills are probably superior to mine, and we hash out the details until we are both happy. Truly excellent library code is what we want. Feel free to disagree with anything I say, but provide an explanation when you do, so I can consider the merits.

Your fork/branch is really old, I'd suggest you rebase (as I can't actually compile it - I've been working off a recent main fork with your commits cherry-picked).

Noted your comment about testing TransferSpeedColumn - I'll need to give it some thought.

[FrankRay78]

We should probably default this to Decimal, to maintain backward compatibility with existing users of Spectre.Console (if we feel it should be Binary OOTB, I'll need to discuss this with the other maintainers first)

[tpill90]

I had to do some more testing to remember why I ended up picking Binary as the default here. Spectre.Console was actually already doing its transfer rates in binary, it was just displaying the incorrect unit.

I did some testing with my 300mbit internet connection, and tested how the transfer rate is being displayed in the nuget version of the library, and compared it against the default of FileSizeBase.Binary in my PR. Both versions display the transfer rate as "36" which roughly corresponds to the conversion of 300mbit to 35.7628MiB. Give or take a little since network connections aren't perfectly flat.

Now if I rerun my PR version using FileSizeBase.Decimal then I get around 38 MB/s, which matches the conversion of 300mbit to 37.5MB. Again, fluctuating network transfers make it a bit off, but its in the correct ballpark.

[FrankRay78]

Really good stuff 👏

I want to check with the other maintainers before we commit this, as whilst you've identified a bug in the current main branch, changing the default to Binary will break any output validation (that relies on the suffix) our existing users may have. Equally, I know the converse is true, ie. a default of Decimal means the suffix remains the same, but the calculated speed will change (although I strongly suspect this is less likely to be a 'breaking change').

Would you be kind enough to offer your opinion @patriksvensson, on the above?

[FrankRay78]

Careful, this needs to align to the same default value hardcoded in FileSize

[FrankRay78]

Note to self - final decision whether to standardise on Binary or Decimal is pending input from the other Spectre.Console maintainers (which has been asked above, in another review comment).

[FrankRay78]

FileSizeTests are fantastic - it exercises Decimal/Binary/Bits Y/Bits N combinations. We should have a similar structure for DownloadedColumnTests I think ie. we need three more test methods (but see my review comment a few lines down for the implication of this).

[FrankRay78]

Note to self - better explained the implications further down, here: #904 (comment)

[FrankRay78]

Having the new FileSize embedded inside the DownloadedColumn.Render means that we cannot configure the Fixture for Decimal/Binary/Bits Y/Bits N combinations - we need a better way to do this (this same issue will affect Spectre.Console users when they come to use the DownloadedColumn).

I'm wondering if we should create a FileSizeSettings class, that has the following properties on it (stolen from TransferSpeedColumn):

    /// <summary>
    /// Gets or sets the <see cref="FileSizeBase"/> to use.
    /// </summary>
    public FileSizeBase Base { get; set; } = FileSizeBase.Binary;

    /// <summary>
    /// Gets or sets a value indicating whether or not to display the transfer speed in bits.
    /// </summary>
    public bool DisplayBits { get; set; }

and somehow FileSizeSettings can be set on both DownloadedColumn and TransferSpeedColumn, both by the Spectre.Console user and also when using ProgressColumnFixture

Other suggestions/ideas welcome.

[tpill90]

I'm not sure I'm tracking here. Wouldn't it just be as simple as:

.Columns(new ProgressColumn[] 
    {
        new DownloadedColumn() { Base = FileSizeBase.Decimal },  
		new TransferSpeedColumn { Base = FileSizeBase.Decimal }
    })

[FrankRay78]

Apologies, I didn't explain myself very well. See the following test:

    public void Should_Return_Correct_Value(double value, double total, string expected)
    {
        // Given
        var fixture = new ProgressColumnFixture<DownloadedColumn>(value, total);
        fixture.Column.Culture = CultureInfo.InvariantCulture;

ProgressColumnFixture is actually creating the DownloadedColumn object within it's own constructor, see here:

    public T Column { get; }
    public ProgressTask Task { get; set; }

    public ProgressColumnFixture(double completed, double total)
    {
        Column = new T();    //    <----  HERE
        Task = new ProgressTask(1, "Foo", total);

So currently, the unit tests cannot set properties like you suggest here:

        new DownloadedColumn() { Base = FileSizeBase.Decimal },  
		new TransferSpeedColumn { Base = FileSizeBase.Decimal }

I think we need the ability to pass in an already instantiated ProgressColumn into the ProgressColumnFixture ?

[tpill90]

I see what you're saying now. I just ended up doing them like this, which was already in line with what the original tests were doing with setting the Culture.

 var fixture = new ProgressColumnFixture<DownloadedColumn>(value, total);
 fixture.Column.Culture = CultureInfo.InvariantCulture;
 fixture.Column.Base = FileSizeBase.Binary;
 fixture.Column.ShowBits = false;

I could certainly refactor it out to have the column be passed in if needed.

[FrankRay78]

There is no rush/pressure @tpill90 with this. Just to say I will be happy to progress the PR as and when you are ready. It won't be one that languishes. In the meantime, be well 👏.

[tpill90]

Hey Frank, I apologize for not getting a chance to look at it yet. I'm going to try to get to it today. Thanks for your help on this

[FrankRay78]

I'll look at your comments/changes soon @tpill90 👏

[FrankRay78]

FYI. The build is broken on your branch, following the rename from DisplayBits to ShowBits, see:

[FrankRay78]

Note to self - default value TBD by Spectre.Console maintainers.

[patriksvensson]

I vote for Binary

[tpill90]

I pushed up another commit which should take care of the latest comments you've left on the review @FrankRay78 .

A follow up question, what documentation should I be updating with this PR? I'll gladly update it, I just was waiting for things to get closer to being merged before going through all of it 😄

[FrankRay78]

I've been thinking about documentation. Strictly speaking, the xml comments on things like the ShowBits property will end up in the auto generated API reference, however I notice the Progress page code example doesn't include the Download progress column, see: https://spectreconsole.net/live/progress

I think it might be enough to just add that into the example, so it becomes obvious to casual readers that such a thing exists.