Add project and take markers action #1133

ScottChesworth · 2024-07-06T19:16:35Z

This improves support for take markers, by making adding them context sensitive. Navigation of them will be added to the key map in the next commit.

AppVeyorBot · 2024-07-06T19:24:45Z

Build succeeded! Build osara pr1133-1569,e50f98de completed (commit e50f98dedd by @ScottChesworth)
Downloads:

Windows

Mac

installer/osara_pr1133-1569,e50f98de.dmg

AppVeyorBot · 2024-07-06T19:57:27Z

Build succeeded! Build osara pr1133-1570,ee47c18f completed (commit ee47c18ff6 by @ScottChesworth)
Downloads:

Windows

Mac

installer/osara_pr1133-1570,ee47c18f.dmg

jcsteh

There are a few problems here:

We shouldn't be using the same function (cmdInsertMarker) to both intercept a REAPER command and implement a custom OSARA command. Otherwise, the custom OSARA command might end up overriding functionality provided by an intercepted REAPER command. It's okay for a custom OSARA command to call the cmd function for an intercepted REAPER command, though. So, I think cmdInsertMarker should be renamed to cmdInsertProjectMarker and a new function cmdInsertMarker should be added.
We add a custom OSARA command which calls Item: Quick add take marker at play position or edit cursor, but we don't support that action itself. Some users may wish to use it directly. So, we need cmdInsertTakeMarker as well, and then cmdInsertMarker would call either that or cmdInsertProjectMarker depending on focus.
You define item focus here as 1 or more selected items. But that's almost always true as soon as you've moved to an item, even if you've since focused the track or the ruler. I haven't tested this, but I'll wager that if you select an item, move to a new track, scrub and move by bar, you'll still end up adding a take marker instead of a project marker. I think this needs to use fakeFocus.
A minor nit: strictly speaking, the new OSARA commands should probably be in the Miscellaneous Actions section of the readme because they're custom OSARA commands. The earlier sections are under "Supported REAPER and Extension Actions" and this is not a REAPER action.

ScottChesworth · 2024-07-10T18:00:36Z

Gotcha, I can refactor that. Did you have specific fakeFocus states in mind? I started off trying to use FOCUS_ITEM and FOCUS_RULER because I figured it would be nice to Page through long takes dropping markers, but it felt like the lesser used marker type was taking precedence too often that way, so I was gonna tell people to clear selection with Shift+Escape instead. Maybe I should try removing item selection from the conditionals, rely entirely on fakeFocus instead? Hmm.

jcsteh · 2024-07-10T22:41:49Z

I was thinking take markers would only be FOCUS_ITEM, but I guess I can see why that would make this action far less useful, precisely because you can't drop take markers while moving through the project. I guess the guidance could be that you need to select the item under the cursor to force focus back to the item when you want to drop a take marker, but that seems quite annoying too. Ug.

I'm not really sure what to suggest here. I'm not sure we can really make this context sensitive without encountering problems like this. I don't love the idea of having to clear selection to drop a project marker; it feels a bit unintuitive. I'm probably a bit biased though because I use project markers all the time, but I've never used take markers and I don't see that changing any time soon. That said, I can just keep using the existing actions, so if I'm the odd one out here, that's fine.

ScottChesworth · 2024-07-10T23:50:30Z

I don't think you'll be the odd one out, using it more tonight, I'm not liking it yet either. Will keep trying stuff. I'm exploring the context sensitive approach as there's nowhere logical left to put adding and editing take markers. T, A, K, E, M and R are all crammed.

…for adding take markers now

…ed in review

ScottChesworth · 2024-07-11T21:37:27Z

Refactored with a lot of help from @Timtam, changed to using fakeFocus instead of item selection because while not ideal, it makes the context sensitivity less weighted toward take which feels better in situ. I'll come back tomorrow and fix the key map conflict.

AppVeyorBot · 2024-07-12T20:12:52Z

Build succeeded! Build osara pr1133-1585,549d0a14 completed (commit 549d0a140f by @ScottChesworth)
Downloads:

Windows

Mac

installer/osara_pr1133-1585,549d0a14.dmg

ScottChesworth · 2024-07-12T20:14:02Z

Wew, resolved conflicts. Can you take a gander at this approach when you get time @jcsteh?

jcsteh

Thanks for working on this. Sorry it's taken me so long to get to a second round of review.

jcsteh · 2024-07-23T10:08:02Z

src/reaper_osara.cpp

+
+Main_OnCommand(command->gaccel.accel.cmd, 0);


I think this line accidentally got split.

Suggested change

Main_OnCommand(command->gaccel.accel.cmd, 0);

Main_OnCommand(command->gaccel.accel.cmd, 0);

jcsteh · 2024-07-23T10:13:56Z

src/reaper_osara.cpp

+}
+
+void cmdhInsertTakeMarker(int command) {
+	if(CountSelectedMediaItems(0)>0) {


If there are no selected items, this means we will never pass the command to REAPER. Practically, this should be fine, since REAPER will probably do nothing in the case of 0 selected items. However, when intercepting commands, I do think we should always pass them to REAPER, on the very slight chance that REAPER does something we don't expect, either now or in future. Otherwise, the action will just do nothing, even if it would have done something without OSARA installed.

You could either:

Check if the item count is 0, call Main_OnCommand and return; or

Don't check at all and rely on the fact that 0 items means the loops will do nothing.

src/reaper_osara.cpp

jcsteh · 2024-07-23T10:16:14Z

src/reaper_osara.cpp

+		}
+		if (addedMarkers == 0) {
+			return; // not inserted
+		} else {


There's no need for an else block if you always return in the if above it. This also simplifies things by avoiding a level of nesting.

jcsteh · 2024-07-23T10:16:40Z

src/reaper_osara.cpp

+		if (addedMarkers == 0) {
+			return; // not inserted
+		} else {
+			// Translators: Reported when using REAPER's quick add take marker action. If more than one take marker is inserted, [] will be replaced with the number of markers. E.G. "2 take markers inserted".


[] should be {}

Suggested change

// Translators: Reported when using REAPER's quick add take marker action. If more than one take marker is inserted, [] will be replaced with the number of markers. E.G. "2 take markers inserted".

// Translators: Reported when using REAPER's quick add take marker action. If more than one take marker is inserted, {} will be replaced with the number of markers. E.G. "2 take markers inserted".

jcsteh · 2024-07-23T10:17:11Z

src/reaper_osara.cpp

+			// Translators: Reported when using REAPER's quick add take marker action. If more than one take marker is inserted, [] will be replaced with the number of markers. E.G. "2 take markers inserted".
+			outputMessage(format(
+				translate_plural("take marker inserted", "{} take markers inserted", addedMarkers), addedMarkers));
+			return;


There's no need for a return if it's the last thing in the function and the function returns nothing.

Suggested change

return;

jcsteh · 2024-07-23T10:19:10Z

src/reaper_osara.cpp

+}
+
+void cmdInsertProjectOrTakeMarker(Command* command) {
+	if  (fakeFocus == FOCUS_ITEM && CountSelectedMediaItems(nullptr) > 0)


How often will fakeFocus be item with 0 items selected? I'm just wondering whether we should only look at fakeFocus and not the selected item count. That will mean we do nothing if no items are selected, but it's consistent with other commands I think? Also, we don't check the selection count for edit marker below.

Hmm well it isn't difficult to get that fakeFocus item with 0 items selected situation, it can often be the case after an item is deleted. Now in that situation we go back to the action inserting project markers until the user explicitly selects another item.

jcsteh · 2024-07-23T10:20:15Z

src/reaper_osara.cpp

+	double start, end;
+	GetSet_LoopTimeRange(false, true, &start, &end, false);
+	if (start != end && fakeFocus == FOCUS_ITEM) {
+		Main_OnCommand(43181, 0); // Item: Add/edit take marker at time selection


The action is called "OSARA: Add project or take marker at cursor (depending on focus)". It doesn't mention time selection. Should it?

Urgh, just realised that I renamed the wrong action. Time selection is only relevant when using the add/edit one. Another commit coming up shortly.

jcsteh · 2024-07-23T10:20:39Z

src/reaper_osara.cpp

+		Main_OnCommand(42385, 0); // Item: Add/edit take marker at play position or edit cursor
+	} else {
+		Main_OnCommand(40171, 0); // Markers: Insert and/or edit marker at current position
+		return;


Unnecessary return.

Suggested change

return;

AppVeyorBot · 2024-07-26T10:15:56Z

Build succeeded! Build osara pr1133-1587,a2ba8695 completed (commit a2ba869513 by @ScottChesworth)
Downloads:

Windows

Mac

installer/osara_pr1133-1587,a2ba8695.dmg

AppVeyorBot · 2024-07-26T10:34:02Z

Build succeeded! Build osara pr1133-1588,819a7b5e completed (commit 819a7b5e41 by @ScottChesworth)
Downloads:

Windows

Mac

installer/osara_pr1133-1588,819a7b5e.dmg

jcsteh

Thanks. Approving this for merge once these comments are addressed.

jcsteh · 2024-07-26T11:26:03Z

src/reaper_osara.cpp

+	for(int i = 0; i < CountSelectedMediaItems(nullptr); ++ i) {
+		MediaItem* item = GetSelectedMediaItem(nullptr, i);
+		MediaItem_Take* take = GetActiveTake(item);
+		preMarkers += GetNumTakeMarkers(take);


Sorry, I missed this in the prior review. It's possible for an item to be empty and contain no takes, so you need to only do this if take is valid. Otherwise, we will possibly crash.

Suggested change

preMarkers += GetNumTakeMarkers(take);

if (take) {

preMarkers += GetNumTakeMarkers(take);

}

jcsteh · 2024-07-26T11:26:12Z

src/reaper_osara.cpp

+	for(int i = 0; i < CountSelectedMediaItems(nullptr); ++ i) {
+		MediaItem* item = GetSelectedMediaItem(nullptr, i);
+		MediaItem_Take* take = GetActiveTake(item);
+		postMarkers += GetNumTakeMarkers(take);


jcsteh · 2024-07-26T11:27:42Z

src/reaper_osara.cpp

@@ -5252,6 +5303,8 @@ Command COMMANDS[] = {
 	{MAIN_SECTION, {DEFACCEL, _t("OSARA: Check for update")}, "OSARA_UPDATE", cmdCheckForUpdate},
 	{MAIN_SECTION, {DEFACCEL, _t("OSARA: Open online documentation")}, "OSARA_OPENDOC", cmdOpenDoc},
 	{MAIN_SECTION, {DEFACCEL, _t("OSARA: Report tempo and time signature at play cursor; press twice to add/edit tempo markers")}, "OSARA_MANAGETEMPOTIMESIGMARKERS", cmdManageTempoTimeSigMarkers},
+	{MAIN_SECTION, {DEFACCEL, _t("OSARA: Add project or take marker at cursor (depending on focus)")}, "OSARA_ADDPROJTAKEMARKER", cmdInsertProjectOrTakeMarker},
+	{MAIN_SECTION, {DEFACCEL, _t("OSARA: Add/edit project or take marker at cursor (depending on focus) sets time selection as take marker length")}, "OSARA_ADDEDITPROJTAKEMARKER", cmdInsertOrEditMarker},


This is bothering me slightly linguistically. Perhaps this:
OSARA: Add/edit project or take marker at cursor (depending on focus), sets time selection as take marker length
or this:
OSARA: Add/edit project or take marker at cursor (depending on focus) and set time selection as take marker length

Timtam · 2024-07-26T11:34:02Z

Just FYI @jcsteh, we tested explicitly with items that don't have takes and didn't cause a crash, probably because REAPER is smart enough to just return 0 if you hand it an invalid take. Its fair to ensure we don't cause unexpected behaviour, but REAPER is smart enough to don't crash too. An extra safety layer doesn't hurt though.

jcsteh · 2024-07-26T11:35:54Z

Yeah, I did wonder whether REAPER might be smart enough to handle that. I'm fairly sure I've seen a crash by my failure to null check takes in the past, but maybe that wasn't a REAPEr function I was calling; I can't recall.

If you've explicitly tested it and it doesn't crash, I can live without that change.

jcsteh · 2024-07-26T11:38:58Z

The commit I'm talking about is 8944a12, but maybe that was caused by something else I was doing with a null take there. My commit comment is a bit vague on the details of the actual crash. :)

jcsteh

Durp, meant to actually hit approve on this. Feel free to merge once you've addressed the comments one way or another, even if that's an intentional decision to ignore the null check guidance given that you tested it and it works fine.

ScottChesworth added 2 commits July 4, 2024 21:32

Add OSARA action, now adding project and take markers is contextual

7b59a65

New OSARA action to make add/edit project/take markers context sensitive

6eb4409

ScottChesworth requested a review from jcsteh July 6, 2024 19:16

ScottChesworth added 2 commits July 6, 2024 20:44

Win and Mac key map update to support adding and navigating take markers

b0c54f8

Update take marker keystrokes in ReadMe

03a1679

jcsteh requested changes Jul 10, 2024

View reviewed changes

ScottChesworth added 2 commits July 11, 2024 20:57

Refactored per code review suggestions, using fakeFocus = FOCUS_ITEM …

cb90dfb

…for adding take markers now

Moved scattered OSARA actions to correct section in ReadMe as suggest…

19a9474

…ed in review

Resolve key map conflicts

5a531e5

jcsteh requested changes Jul 23, 2024

View reviewed changes

Refactored per suggestions in review

8c0662e

ScottChesworth requested a review from jcsteh July 26, 2024 10:09

Corrected name of action

ca63f2c

jcsteh reviewed Jul 26, 2024

View reviewed changes

jcsteh approved these changes Jul 26, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add project and take markers action #1133

Add project and take markers action #1133

ScottChesworth commented Jul 6, 2024

AppVeyorBot commented Jul 6, 2024

AppVeyorBot commented Jul 6, 2024

jcsteh left a comment

ScottChesworth commented Jul 10, 2024

jcsteh commented Jul 10, 2024

ScottChesworth commented Jul 10, 2024

ScottChesworth commented Jul 11, 2024

AppVeyorBot commented Jul 12, 2024

ScottChesworth commented Jul 12, 2024

jcsteh left a comment

jcsteh Jul 23, 2024

jcsteh Jul 23, 2024

jcsteh Jul 23, 2024

jcsteh Jul 23, 2024

jcsteh Jul 23, 2024

jcsteh Jul 23, 2024

ScottChesworth Jul 26, 2024

jcsteh Jul 23, 2024

ScottChesworth Jul 26, 2024

jcsteh Jul 23, 2024

AppVeyorBot commented Jul 26, 2024

AppVeyorBot commented Jul 26, 2024

jcsteh left a comment

jcsteh Jul 26, 2024

jcsteh Jul 26, 2024

jcsteh Jul 26, 2024

Timtam commented Jul 26, 2024

jcsteh commented Jul 26, 2024

jcsteh commented Jul 26, 2024

jcsteh left a comment


	Main_OnCommand(command->gaccel.accel.cmd, 0);
	Main_OnCommand(command->gaccel.accel.cmd, 0);

	// Translators: Reported when using REAPER's quick add take marker action. If more than one take marker is inserted, [] will be replaced with the number of markers. E.G. "2 take markers inserted".
	// Translators: Reported when using REAPER's quick add take marker action. If more than one take marker is inserted, {} will be replaced with the number of markers. E.G. "2 take markers inserted".

Add project and take markers action #1133

Are you sure you want to change the base?

Add project and take markers action #1133

Conversation

ScottChesworth commented Jul 6, 2024

AppVeyorBot commented Jul 6, 2024

AppVeyorBot commented Jul 6, 2024

jcsteh left a comment

Choose a reason for hiding this comment

ScottChesworth commented Jul 10, 2024

jcsteh commented Jul 10, 2024

ScottChesworth commented Jul 10, 2024

ScottChesworth commented Jul 11, 2024

AppVeyorBot commented Jul 12, 2024

ScottChesworth commented Jul 12, 2024

jcsteh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AppVeyorBot commented Jul 26, 2024

AppVeyorBot commented Jul 26, 2024

jcsteh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Timtam commented Jul 26, 2024

jcsteh commented Jul 26, 2024

jcsteh commented Jul 26, 2024

jcsteh left a comment

Choose a reason for hiding this comment