Implement tempo and time signature reporting #1073

day-garwood · 2024-05-10T12:28:43Z

Introduces a new action to report the current tempo and time signature of the project, specifically the tempo reported at the position of the play cursor.

see issue #179 for discussion.

Note: This isn’t linked to the keymap yet. Still figuring that part out.

Any feedback would be appreciated since I'm new here. Thanks.

Introduces a new action to report the current tempo and time signature of the project, specifically the tempo reported at the position of the play cursor. see issue jcsteh#179 for discussion.

AppVeyorBot · 2024-05-10T12:35:31Z

Build succeeded! Build osara pr1073-1418,359eb84f completed (commit 359eb84f18 by @day-garwood)
Downloads:

Windows

Mac

installer/osara_pr1073-1418,359eb84f.dmg

ScottChesworth · 2024-05-10T12:56:08Z

Woo, it's excellent to see you landing code here dude! Let's have a think about where we want to put it on the key map. FYI doing that requires access to Windows and Mac, we've got @jennykbrennan handling the Mac side at the moment.
Here's how to add to the key map on Windows though, just so you know in future:

Make sure you're starting with an unmodified copy of the latest OSARA map. Easiest way to do that is backup reaper-kb.ini if you've got any customizations you want to preserve, then install latest OSARA and say Yes to the key map replacement prompt.
Open REAPER, add the new binding as usual.
Take reaper-kb.ini from your resource path, go to src\config\windows in your local clone of OSARA, delete the existing copy and replace it. You'll likely find an ini.bak version there too, I usually delete that as well. It'll be recreated in the next step.
Now run python tools\sortKeymap.py config\windows\reaper-kb.ini. This is a tool Robbie wrote that makes the diff easier to track.
Add the name of the action and its new binding to readme.md.

That's it. Commit, push, switch back to your prior reaper-kb.ini to restore customizations if needed.

day-garwood · 2024-05-10T13:23:09Z

To be fair, I couldn't actually find anywhere I would put it - of course the logical choice would be something linked to T but everything I tried was taken, so I think I'll leave that to the experienced keymappers for now.
Good to know for future though. Thanks.

ScottChesworth · 2024-05-10T15:23:28Z

Adding tempo time sig markers is on Shift+C. The C part isn't strong but it's been there forever and T is already jam packed so I guess that'll be staying where it is.
What if that becomes double-pressable? Once to hear tempo and time sig, twice to bring up the dialog for that marker type?

Updated "OSARA: Report current tempo and time signature" to "OSARA: Manage current tempo and time signature" to accommodate dual functionality. A single press announces the current tempo and time signature for enhanced accessibility, while a double press triggers the insertion of a tempo/time signature change marker. Also updated the Windows keymap to reflect this change: "Shift+C" now activates this new function, replacing "Tempo envelope: Insert tempo/time signature change marker at edit cursor...".

day-garwood · 2024-05-11T09:53:02Z

I've just pushed a new commit so you can see what that would look like.

AppVeyorBot · 2024-05-11T09:58:01Z

Build succeeded! Build osara pr1073-1419,0e53777f completed (commit 0e53777faa by @day-garwood)
Downloads:

Windows

Mac

installer/osara_pr1073-1419,0e53777f.dmg

jcsteh · 2024-05-14T04:01:29Z

Welcome, @day-garwood. I just wanted to let you know that I've seen this and I'm keen to review it when I can. Just a bit flat out here with very little headspace at the moment, so it might take me a few more days to get to it. Thanks for your work.

AppVeyorBot · 2024-05-14T10:51:55Z

Build succeeded! Build osara pr1073-1421,ad75caad completed (commit ad75caadf5 by @ScottChesworth)
Downloads:

Windows

Mac

installer/osara_pr1073-1421,ad75caad.dmg

jcsteh

Thanks for your work on this, and thanks for putting time and effort into contributing to OSARA. Functionally, this is great, but there are a few code style and readability nits. I tend to be a bit picky about code readability and polish. I can make these changes myself if this is overwhelming, but I figured you might like to learn from the code review.

jcsteh · 2024-05-14T10:57:43Z

readme.md

@@ -222,7 +222,8 @@ SWS/FNG: Time stretch selected items (fine): Control+Alt+=
 - Markers: Insert region from time selection: Shift+R

 #### Time Signature/Tempo Markers
- Move edit cursor to previous tempo or time signature change: Shift+;
+- OSARA: Report tempo and time signature at play cursor (press twice to add/edit tempo marker): Shift+C


@ScottChesworth Hmm. I don't love the "press twice..." in parentheses here. First, it's not really an action name any more at this point; it's providing instructions, which feels inconsistent with everything else. Second, it makes adding/editing seem like an afterthought, rather than a primary function of the action which just happens to be on a second press to save keystrokes.

"OSARA: Report/add/edit tempo and time signature at play cursor" feels like it would be more consistent. However, I do get your point regarding shortcut help. I guess using a semi-colon rather than parentheses would at least solve my second concern somewhat; i.e. "OSARA: Report tempo and time signature at play cursor; press twice to add/edit tempo marker".

jcsteh · 2024-05-14T11:00:15Z

src/reaper_osara.cpp

@@ -3583,6 +3583,26 @@ void cmdUnselAllTracksItemsPoints(Command* command) {
 		outputMessage(translate("unselected tracks/items/envelope points"));
 }

+void reportTempoTimeSig() {
+ReaProject* proj=EnumProjects(-1, nullptr, 0);


While it's not necessary to get the code to compile, code blocks should be indented for good style. We use a tab character for each level of indentation. Unfortunately, we don't have automatic code formatting working yet, so this has to be done manually.

jcsteh · 2024-05-14T11:00:54Z

src/reaper_osara.cpp

+ReaProject* proj=EnumProjects(-1, nullptr, 0);
+if(proj==nullptr) return;
+double tempo=0;
+int timesig1=0;


Please rename these to be more descriptive; e.g. num and denom. This makes it clearer what each of these actually is: numerator and denominator.

jcsteh · 2024-05-14T11:09:58Z

src/reaper_osara.cpp

+int timesig2=0;
+double pos=GetPlayPosition();
+TimeMap_GetTimeSigAtTime(proj, pos, &timesig1, &timesig2, &tempo);
+outputMessage(formatDouble(tempo, 1, false)+", "+to_string(timesig1)+"/"+to_string(timesig2));


Rather than concatenating strings like this, it would be cleaner and easier to follow to use format(). This way, it's much easier to see what the output will look like. Something like this:

outputMessage(format("{}, {}/{}", formatDouble(tempo, 1, false), timesig1, timesig2));

You could even name the format template arguments, though that might be overkill for only three arguments for an untranslatable message:

outputMessage(format("{tempo}, {num}/{denom}", "tempo"_a=formatDouble(tempo, 1, false), "num"_a=timesig1, "denom"_a=timesig2));

I could go either way.

jcsteh · 2024-05-14T11:16:05Z

src/reaper_osara.cpp

+
+void cmdManageTempoTimeSigMarkers(Command* command) {
+if(lastCommandRepeatCount==0)
+{


Brace on the same line as the statement please, like this:

if (lastCommandRepeatCount == 0) {

jcsteh · 2024-05-14T11:17:23Z

src/reaper_osara.cpp

+reportTempoTimeSig();
+return;
+}
+Main_OnCommand(40256, 0);


It'd be great if you could add a comment at the end of this line to explain what action this is:

Main_OnCommand(40256, 0); // Tempo envelope: Insert tempo/time signature change marker at edit cursor...

day-garwood · 2024-05-14T12:09:14Z

Thanks for the suggestions. I will make these changes now.
The code formatting will definitely take some getting used to, I'm not used to dealing with indentations with a screen reader (especially using Notepad2 which can sometimes butcher a text selection workflow if you're not aware that a line is indented) and I'm certainly not used to the braces on same line method. It's not impossible but it does slow me down a lot. Not sure if that's because I'm primarily a TTS user rather than Braille or whether I'm just an oddball who struggles to process all that extra verbosity when you do things like turn all punctuation and indentation information on in addition to reading the actual text.
I'd be interested to know how you and other contributors work with some of these quirks being screen reader users yourselves.
Cheers.

jcsteh · 2024-05-14T12:17:22Z

Ideally, we'd have auto code formatting so we just wouldn't have to worry about it. There's been some work on that in #940, but it causes some problems and so isn't quite ready to merge yet. Sorry about that.

Personally, I actually find it easier to follow code with indentation, but it did take some practice. I use NVDA and I have indentation reporting set to tones, which means I can hear the indentation simultaneously with speech. That cuts down on both time and speech clutter.

I always have my punctuation set to all, not just when I'm coding. But I'm also ridiculous in that I generally have my speech rate set somewhere between 650 and 960 WPM. :)

@jcsteh

Formatted code to conform to overall project and based on additional suggestions by @jcsteh in PR jcsteh#1073. Removed keymap entries from config and readme due to ongoing discussion about possible reassignment (see issue jcsteh#1076).

AppVeyorBot · 2024-05-14T13:08:26Z

Build succeeded! Build osara pr1073-1422,92c4c2e8 completed (commit 92c4c2e8d3 by @day-garwood)
Downloads:

Windows

Mac

installer/osara_pr1073-1422,92c4c2e8.dmg

src/reaper_osara.cpp

jcsteh

Thank you.

jcsteh · 2024-05-14T21:48:59Z

@ScottChesworth, shall I land this now and we can update the Mac key map later or do you want to get the Mac key map updated here? Normally, I'd just land it and update both key maps later, but it feels different to have one key map updated and one not.

AppVeyorBot · 2024-05-14T21:54:02Z

Build succeeded! Build osara pr1073-1425,bddfef83 completed (commit bddfef8393 by @jcsteh)
Downloads:

Windows

Mac

installer/osara_pr1073-1425,bddfef83.dmg

ScottChesworth · 2024-05-15T00:05:35Z

Don't land just yet, will get @jennykbrennan to do Mac mapping tomorrow.

day-garwood · 2024-05-15T07:12:23Z

Ooh nice. Since this isn't my project thought I'd go for clarity over brevity, but I also prefer boolean checks. Yay.

…ursor; press twice to add/edit tempo markers per Jamie's suggestion, added it to ReadMe.

AppVeyorBot · 2024-05-15T20:58:28Z

Build succeeded! Build osara pr1073-1426,b2811d4e completed (commit b2811d4ee9 by @ScottChesworth)
Downloads:

Windows

Mac

installer/osara_pr1073-1426,b2811d4e.dmg

AppVeyorBot · 2024-05-15T21:27:09Z

Build succeeded! Build osara pr1073-1427,f4985419 completed (commit f498541936 by @ScottChesworth)
Downloads:

Windows

Mac

installer/osara_pr1073-1427,f4985419.dmg

Implement tempo and time signature reporting

b8875eb

Introduces a new action to report the current tempo and time signature of the project, specifically the tempo reported at the position of the play cursor. see issue jcsteh#179 for discussion.

ScottChesworth mentioned this pull request May 11, 2024

Key map update for June 2024 #1076

Open

3 tasks

style cleanup, renamed functions and new action

04b197a

jcsteh requested changes May 14, 2024

View reviewed changes

Minor changes, mainly formatting

daaca4c

Formatted code to conform to overall project and based on additional suggestions by @jcsteh in PR jcsteh#1073. Removed keymap entries from config and readme due to ongoing discussion about possible reassignment (see issue jcsteh#1076).

jcsteh reviewed May 14, 2024

View reviewed changes

src/reaper_osara.cpp Outdated Show resolved Hide resolved

Update src/reaper_osara.cpp

1902e66

jcsteh approved these changes May 14, 2024

View reviewed changes

Rename new action to OSARA: Report tempo and time signature at play c…

bd090ab

…ursor; press twice to add/edit tempo markers per Jamie's suggestion, added it to ReadMe.

Update Windows and Mac key maps, new action on Shift+C

674bb6a

jcsteh merged commit 293fe50 into jcsteh:master May 15, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement tempo and time signature reporting #1073

Implement tempo and time signature reporting #1073

day-garwood commented May 10, 2024

AppVeyorBot commented May 10, 2024

ScottChesworth commented May 10, 2024

day-garwood commented May 10, 2024

ScottChesworth commented May 10, 2024

day-garwood commented May 11, 2024

AppVeyorBot commented May 11, 2024

jcsteh commented May 14, 2024

AppVeyorBot commented May 14, 2024

jcsteh left a comment

jcsteh May 14, 2024

jcsteh May 14, 2024

jcsteh May 14, 2024

jcsteh May 14, 2024

jcsteh May 14, 2024

jcsteh May 14, 2024

day-garwood commented May 14, 2024

jcsteh commented May 14, 2024

AppVeyorBot commented May 14, 2024

jcsteh left a comment

jcsteh commented May 14, 2024

AppVeyorBot commented May 14, 2024

ScottChesworth commented May 15, 2024

day-garwood commented May 15, 2024

AppVeyorBot commented May 15, 2024

AppVeyorBot commented May 15, 2024

Implement tempo and time signature reporting #1073

Implement tempo and time signature reporting #1073

Conversation

day-garwood commented May 10, 2024

AppVeyorBot commented May 10, 2024

ScottChesworth commented May 10, 2024

day-garwood commented May 10, 2024

ScottChesworth commented May 10, 2024

day-garwood commented May 11, 2024

AppVeyorBot commented May 11, 2024

jcsteh commented May 14, 2024

AppVeyorBot commented May 14, 2024

jcsteh left a comment

Choose a reason for hiding this comment

jcsteh May 14, 2024

Choose a reason for hiding this comment

jcsteh May 14, 2024

Choose a reason for hiding this comment

jcsteh May 14, 2024

Choose a reason for hiding this comment

jcsteh May 14, 2024

Choose a reason for hiding this comment

jcsteh May 14, 2024

Choose a reason for hiding this comment

jcsteh May 14, 2024

Choose a reason for hiding this comment

day-garwood commented May 14, 2024

jcsteh commented May 14, 2024

AppVeyorBot commented May 14, 2024

jcsteh left a comment

Choose a reason for hiding this comment

jcsteh commented May 14, 2024

AppVeyorBot commented May 14, 2024

ScottChesworth commented May 15, 2024

day-garwood commented May 15, 2024

AppVeyorBot commented May 15, 2024

AppVeyorBot commented May 15, 2024