Clarify download debug progress message by vadi2 · Pull Request #4713 · Mudlet/Mudlet

vadi2 · 2021-01-29T15:56:34Z

Brief overview of PR changes/additions

Saw this message and had no idea what it means:

Turns out it was talking about download progress :)

Motivation for adding to Mudlet

Clearer debug message

Other info (issues closed, discussion etc)

add-deployment-links · 2021-01-29T15:56:36Z

Hey there! Thanks for helping Mudlet improve. 🌟

Test versions

You can directly test the changes here:

No need to install anything - just unzip and run.
Let us know if it works well, and if it doesn't, please give details.

SlySven · 2021-01-30T20:35:59Z

src/TLuaInterpreter.cpp

        raiseDownloadProgressEvent(L, urlString, bytesDownloaded, totalBytes);
        if (mudlet::debugMode) {
-            TDebug(QColor(Qt::white), QColor(Qt::blue)) << "\n" << bytesDownloaded << "/" << totalBytes << " for file "
+            TDebug(QColor(Qt::white), QColor(Qt::blue)) << "\n" << bytesDownloaded << "/" << totalBytes << " bytes downloaded for file "


Do we need the \n in that line as that is separating this line from the preceding message generated by the raiseDownloadProgressEvent(...) or can we not guarantee that that text will be there? If we cannot perhaps we should use:

Suggested change

TDebug(QColor(Qt::white), QColor(Qt::blue)) << "\n" << bytesDownloaded << "/" << totalBytes << " bytes downloaded for file "

TDebug(QColor(Qt::white), QColor(Qt::blue)) << "\nDownload progress: " << bytesDownloaded << " of: " << totalBytes << " bytes for file: "

SlySven

How about something else that will stand alone better?

vadi2 · 2021-01-31T09:54:05Z

It's a bit more verbose and doesn't fit with the initial style of the message:

Don't think I want to overhaul all of them to be more verbose

SlySven · 2021-02-02T14:00:38Z

It's a bit more verbose and doesn't fit with the initial style of the message:

IMHO it is not so bad - and what if there are several large downloads going on. Does that message only show up at the end or after each "chunk" is received? If the latter then - depending on the size of the chunks - it might work quite well - or be horrible spammy... 😜

You could always change the beginning from:

- "Download progress:"-
+ "DOWNLOAD:"

vadi2 · 2021-02-02T14:02:16Z

I think we should keep it simple as it is. This PR is a super simple one fixing a problem, there was not enough info in the original message, but the message itself is fine.

Delwing

I am aware that message can be spamy, but if you go to debug console while downloading file... this might be the reason :)

Kebap · 2021-02-02T15:03:11Z

Hooray for bikeshedding!

Let's review the texts in .. text maybe? Before jumping into code.
What would you like to read in debug window?

STATUS QUO:

downloadFile: script is downloading from URL
A/B for file URL

(Where URL and A and B are to be filled with variable values)
Agree, the second text seems outrageous and needs change!

VADI1:

downloadFile: script is downloading from URL
A/B bytes downloaded for file URL

This adds the hint "download" but still seems a bit arbitrary to me.

SLYSVEN1:

downloadFile: script is downloading from URL
Download progress: A of: B bytes for file: URL

I like the idea. Probably the : after of was a typo?

SLYSVEN2:

downloadFile: script is downloading from URL
DOWNLOAD: A of: B bytes for file: URL

This is going a bit further off-road and tries to close up to LUA: messages maybe?

Here is my take: Let's name the function in the second line again! Also less verbose.

KEBAP1:

downloadFile: script is downloading from URL
downloadFile: A/B bytes ready for URL

..or to be even less verbose in the first line as well:

KEBAP2:

downloadFile: start download from URL
downloadFile: A/B bytes ready for URL

Delwing · 2021-02-02T15:06:31Z

I'd vote for KEBAP2

vadi2 · 2021-02-02T15:07:23Z

To be honest there's a lot more other things that need doing, I don't have the energy for this. I just fixed a nonsensical message to make sense; if you'd like to change it entirely let's open up a separate discussion for this. Otherwise we won't go anywhere and the message still don't make sense in the release, is that a win?

SlySven · 2021-02-02T19:44:18Z

TBH In SlySven2 I was thinking of the "LUA" messages. And in both that and the prior one the of: was intentional as it was introducing the second (of two numbers). I am not sure how widely using / as a separator is understood compared to of - if others feel it is better then fair enough.

I cannot recall the exact behaviour but I have a nasty suspicion that the second number is often not available in many download cases - so it might be simpler to only bother showing it if it is more than the first anyhow.

I didn't realise this might be considered bikeshedding (and I didn't realise that was the definition - or that it had that negative a connotation!)

Since we have a review process for everything - it is not surprising that we have different views - the input from others can avoid us merging in something that doesn't have some sort of consensus - KEBAPs is so-so I guess but "of" (for the file) is even shorter than "ready for" and doesn't necessarily imply that the file is complete.

Now we have collected up these deckchairs then Vadim can tell/show us whether there is any change in his decision, we can rubber-stamp it and we can get on and steer this vessel away (hopefully) from the large chunks of ices out there.

vadi2 · 2021-02-02T19:56:35Z

Let's go with KEBAP2 then, we are more or less aligned on it.

SlySven

Lets suck this and ~~crash and burn~~ see.

* Clarify download progress message Co-authored-by: Kebap <kebap_spam@gmx.net>

Clarify download progress message

f924ebf

vadi2 requested a review from a team as a code owner January 29, 2021 15:56

vadi2 requested review from a team January 29, 2021 15:56

SlySven reviewed Jan 30, 2021

View reviewed changes

Delwing approved these changes Feb 2, 2021

View reviewed changes

use texts like KEBAP2

79d61e5

SlySven approved these changes Feb 2, 2021

View reviewed changes

Kebap merged commit 8cf869e into Mudlet:development Feb 2, 2021

vadi2 deleted the clarify-download-message branch February 14, 2021 18:37

Chris7 pushed a commit to Chris7/Mudlet that referenced this pull request Jan 2, 2022

Clarify download debug progress message (Mudlet#4713)

164cb90

* Clarify download progress message Co-authored-by: Kebap <kebap_spam@gmx.net>

	TDebug(QColor(Qt::white), QColor(Qt::blue)) << "\n" << bytesDownloaded << "/" << totalBytes << " bytes downloaded for file "
	TDebug(QColor(Qt::white), QColor(Qt::blue)) << "\nDownload progress: " << bytesDownloaded << " of: " << totalBytes << " bytes for file: "

Uh oh!

Conversation

vadi2 commented Jan 29, 2021

Brief overview of PR changes/additions

Motivation for adding to Mudlet

Other info (issues closed, discussion etc)

Uh oh!

add-deployment-links bot commented Jan 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test versions

Uh oh!

SlySven Jan 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SlySven left a comment

Choose a reason for hiding this comment

Uh oh!

vadi2 commented Jan 31, 2021

Uh oh!

SlySven commented Feb 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vadi2 commented Feb 2, 2021

Uh oh!

Delwing left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kebap commented Feb 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Delwing commented Feb 2, 2021

Uh oh!

vadi2 commented Feb 2, 2021

Uh oh!

SlySven commented Feb 2, 2021

Uh oh!

vadi2 commented Feb 2, 2021

Uh oh!

SlySven left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

add-deployment-links bot commented Jan 29, 2021 •

edited

Loading

SlySven Jan 30, 2021 •

edited

Loading

SlySven commented Feb 2, 2021 •

edited

Loading

Delwing left a comment •

edited

Loading

Kebap commented Feb 2, 2021 •

edited

Loading