[Audacity-devel] Bug 1402 - Pull Request

[Mark Young]

Hi all,



I have now taken care of Bug 1402 and raised a Pull Request at
https://github.com/audacity/audacity/pull/168



Hope this is acceptable.



Regards,

Mark



Mark Young

tip2tail Ltd

www.tip2tail.scot



Registered in Scotland: SC542338

[Steve the Fiddle]

Thanks for the fix Mark.

Some comments about it (others may want to chip in if I'm leading you astray).

My first impression is that the code is rather long for what it does.
Part of that impression comes from the copious commenting.
I think that generally the Audacity code is not commented enough, but
I'd try to strike a happy medium.

Example:

if (iMinutes < 1) {
// Less than a minute...
sFormatted = _("Less than 1 minute");
return sFormatted;
}

The comment "// Less than a minute" is stating the obvious, so not
really necessary. If code is over-commented, then less actual code is
visible in an editor, making it harder to read rather than easier.
Better to leave out stating the obvious, which for this function is
probably all of the comments.

---

Regarding the actual message, I think there should be a full stop
(period) at the end so that it reads:

Planned recording duration: 260 hours and 36 minutes.
Recording time remaining on disk: 200 hours and 38 minutes.

---

Should we perhaps round the minutes? It looks strange to me that a
recording of 59 mins and 59 seconds is reported as 59 minutes.
Something like:

// How many minutes will this recording require?
int iMinsRecording = m_TimeSpan_Duration.GetMinutes();
+ if (m_TimeSpan_Duration.GetSeconds() >= 30)
+ iMinsRecording += 1;

---

There is also a status bar message that reports the amount of time
remaining. That message is calculated and formatted somewhere else and
also needs attention. It would probably be better if we used the same
code in both places rather than having to fix the same problem in two
places.


Steve

Post by Mark Young
Hi all,
I have now taken care of Bug 1402 and raised a Pull Request at
https://github.com/audacity/audacity/pull/168
Hope this is acceptable.
Regards,
Mark
Mark Young
tip2tail Ltd
www.tip2tail.scot
Registered in Scotland: SC542338
------------------------------------------------------------------------------
_______________________________________________
audacity-devel mailing list
https://lists.sourceforge.net/lists/listinfo/audacity-devel

------------------------------------------------------------------------------

[Mark Young]

Hi Steve,

I tend to comment as I write - I appreciate however there could be too many.

In this fix I was resolving the issue requested rather than re-writing the
whole routine. However I do see that it could be much more concise. I
will edit this later and submit another PR.

Also, will add the full stop! :)

Thanks.
Mark

Post by Steve the Fiddle
Thanks for the fix Mark.
Some comments about it (others may want to chip in if I'm leading you astray).
My first impression is that the code is rather long for what it does.
Part of that impression comes from the copious commenting.
I think that generally the Audacity code is not commented enough, but
I'd try to strike a happy medium.
if (iMinutes < 1) {
// Less than a minute...
sFormatted = _("Less than 1 minute");
return sFormatted;
}
The comment "// Less than a minute" is stating the obvious, so not
really necessary. If code is over-commented, then less actual code is
visible in an editor, making it harder to read rather than easier.
Better to leave out stating the obvious, which for this function is
probably all of the comments.
---
Regarding the actual message, I think there should be a full stop
Planned recording duration: 260 hours and 36 minutes.
Recording time remaining on disk: 200 hours and 38 minutes.
---
Should we perhaps round the minutes? It looks strange to me that a
recording of 59 mins and 59 seconds is reported as 59 minutes.
// How many minutes will this recording require?
int iMinsRecording = m_TimeSpan_Duration.GetMinutes();
+ if (m_TimeSpan_Duration.GetSeconds() >= 30)
+ iMinsRecording += 1;
---
There is also a status bar message that reports the amount of time
remaining. That message is calculated and formatted somewhere else and
also needs attention. It would probably be better if we used the same
code in both places rather than having to fix the same problem in two
places.
Steve

Post by Mark Young
Hi all,
I have now taken care of Bug 1402 and raised a Pull Request at
https://github.com/audacity/audacity/pull/168
Hope this is acceptable.
Regards,
Mark
Mark Young
tip2tail Ltd
www.tip2tail.scot
Registered in Scotland: SC542338
------------------------------------------------------------

------------------

Post by Mark Young
_______________________________________________
audacity-devel mailing list
https://lists.sourceforge.net/lists/listinfo/audacity-devel

[Mark Young]

Hi all,

I have now revisited this. Changes are: using wxPLURAL, one routine for
both the dialog and Status Bar, and rounding the minutes rather than simply
showing the int value of it.

https://github.com/audacity/audacity/pull/169

Hope this can be merged.

Mark

Post by Steve the Fiddle
Thanks for the fix Mark.
Some comments about it (others may want to chip in if I'm leading you astray).
My first impression is that the code is rather long for what it does.
Part of that impression comes from the copious commenting.
I think that generally the Audacity code is not commented enough, but
I'd try to strike a happy medium.
if (iMinutes < 1) {
// Less than a minute...
sFormatted = _("Less than 1 minute");
return sFormatted;
}
The comment "// Less than a minute" is stating the obvious, so not
really necessary. If code is over-commented, then less actual code is
visible in an editor, making it harder to read rather than easier.
Better to leave out stating the obvious, which for this function is
probably all of the comments.
---
Regarding the actual message, I think there should be a full stop
Planned recording duration: 260 hours and 36 minutes.
Recording time remaining on disk: 200 hours and 38 minutes.
---
Should we perhaps round the minutes? It looks strange to me that a
recording of 59 mins and 59 seconds is reported as 59 minutes.
// How many minutes will this recording require?
int iMinsRecording = m_TimeSpan_Duration.GetMinutes();
+ if (m_TimeSpan_Duration.GetSeconds() >= 30)
+ iMinsRecording += 1;
---
There is also a status bar message that reports the amount of time
remaining. That message is calculated and formatted somewhere else and
also needs attention. It would probably be better if we used the same
code in both places rather than having to fix the same problem in two
places.
Steve

Post by Mark Young
Hi all,
I have now taken care of Bug 1402 and raised a Pull Request at
https://github.com/audacity/audacity/pull/168
Hope this is acceptable.
Regards,
Mark
Mark Young
tip2tail Ltd
www.tip2tail.scot
Registered in Scotland: SC542338
------------------------------------------------------------

------------------

Post by Mark Young
_______________________________________________
audacity-devel mailing list
https://lists.sourceforge.net/lists/listinfo/audacity-devel