Review vgmrecorder.tcl

[m9710797]

Hi,

I reviewed the vgmrecorder.tcl script and made some tweaks and small bug fixes.

BIG DISCLAIMER: I changed the code, but did NOT actually test anything. Please review carefully before accepting these changes.

Some of the changes are just cleanups or small changes in coding style. Feel free to discard any of those changes.

Here's a list of the more important changes:

• Simplified (opl4_register_1, opl4_register_2, active_fm_register) -> (opl4_register, active_fm_register).
• 'abort' variable was only used inside vgm_rec_end, made it a parameter instead of a global variable.
• Removed some unused variables / dead code / obsolete help text.
• Fixed some typos/consistency in help texts.
• More consistent code formatting.
• Bug fix in 'vgm_rec_set_filename': 'string trim $filename ".vgm" removes any sequence containing the characters 'v', 'g', 'm' and '.', both at the left and at the right. So 'myfile.gm' gets trimmed to 'yfile'.
• Small simplification e.g. 'lindex $args [expr {$idx + 1}]' -> 'lindex $args $idx+1'. (Unfortunately this convenient syntax works only for specific commands, mostly commands that work with list indices.
• Instead of using "puts" to display a string in the console, 'return' the string from the proc. When used interactively the string also ends up in the console. It's a bit shorter, but more importantly in this way the string is available for further scripts that may want to call the 'vgm_rec' command.
• Removed redundant error check for 'already recording' from vgm_rec_start.
• Not sure if this is actually more readable: merged successive 'append' or 'lappend' commands to the same variable into a single command. Disadvantage: long command line must be escaped to be able to split over multiple lines.
• Some asynchronous actions (callbacks) use both 'puts' and 'message' to show some message. I removed the 'puts' version because it's not useful for a regular user, he isn't using the console when this triggers. (It might have been useful while developing the script). Some uses of 'puts' remain, but as far as I can tell these should normally never trigger (it are more like debug prints for developing the script).
• The 'error' Tcl command stops executing the Tcl proc, so I removed any dead code after 'error' (e.g. a 'return' statement).
• Bug fix in vgm_rec_end: the condition for the 'title_address' for 'mbwave_basic_title_hack' was wrong (copy/paste error).

Wouter

[niekvlessert]

Hallo Wouter,

Bedankt voor de inzet! Ik zal de pull request toepassen en testen.

Groeten,

Niek

On Wed, Nov 28, 2018 at 7:04 PM Wouter Vermaelen notifications@github.com wrote:

Hi,

I reviewed the vgmrecorder.tcl script and made some tweaks and small bug fixes.

BIG DISCLAIMER: I changed the code, but did NOT actually test anything. Please review carefully before accepting these changes.

Some of the changes are just cleanups or small changes in coding style. Feel free to discard any of those changes.

Here's a list of the more important changes:

• Simplified (opl4_register_1, opl4_register_2, active_fm_register) -> (opl4_register, active_fm_register).
• 'abort' variable was only used inside vgm_rec_end, made it a parameter instead of a global variable.
• Removed some unused variables / dead code / obsolete help text.
• Fixed some typos/consistency in help texts.
• More consistent code formatting.
• Bug fix in 'vgm_rec_set_filename': 'string trim $filename ".vgm" removes any sequence containing the characters 'v', 'g', 'm' and '.', both at the left and at the right. So 'myfile.gm' gets trimmed to 'yfile'.
• Small simplification e.g. 'lindex $args [expr {$idx + 1}]' -> 'lindex $args $idx+1'. (Unfortunately this convenient syntax works only for specific commands, mostly commands that work with list indices.
• Instead of using "puts" to display a string in the console, 'return' the string from the proc. When used interactively the string also ends up in the console. It's a bit shorter, but more importantly in this way the string is available for further scripts that may want to call the 'vgm_rec' command.
• Removed redundant error check for 'already recording' from vgm_rec_start.
• Not sure if this is actually more readable: merged successive 'append' or 'lappend' commands to the same variable into a single command. Disadvantage: long command line must be escaped to be able to split over multiple lines.
• Some asynchronous actions (callbacks) use both 'puts' and 'message' to show some message. I removed the 'puts' version because it's not useful for a regular user, he isn't using the console when this triggers. (It might have been useful while developing the script). Some uses of 'puts' remain, but as far as I can tell these should normally never trigger (it are more like debug prints for developing the script).
• The 'error' Tcl command stops executing the Tcl proc, so I removed any dead code after 'error' (e.g. a 'return' statement).
• Bug fix in vgm_rec_end: the condition for the 'title_address' for 'mbwave_basic_title_hack' was wrong (copy/paste error).

Wouter

You can view, comment on, or merge this pull request online at:

https://github.com/niekvlessert/openmsx_tcl_vgm_export/pull/5 Commit Summary

• Hi,

File Changes

• M vgmrecorder.tcl https://github.com/niekvlessert/openmsx_tcl_vgm_export/pull/5/files#diff-0 (445)

Patch Links:

• https://github.com/niekvlessert/openmsx_tcl_vgm_export/pull/5.patch
• https://github.com/niekvlessert/openmsx_tcl_vgm_export/pull/5.diff

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/niekvlessert/openmsx_tcl_vgm_export/pull/5, or mute the thread https://github.com/notifications/unsubscribe-auth/AA91TP4n72pqbR8SlWk64N3qxASY5iVLks5uztBCgaJpZM4Y4JpI .