View Issue Details

IDCategoryLast Update
0007994bugs2020-06-05 10:15
ReportermpkAssigned To 
Reproducibilityalways 
Status newResolutionopen 
Product Version6.0-pre1 
Fixed in Version 
Summary0007994: Split affects topmost region instead of active region
DescriptionThe 's' keyboard shortcut when used with Mouse as edit point now splits the topmost region rather than the selected region. This is a behaviour change from Ardour-5. The selected region is split when Playhead is used as the edit point.
TagsNo tags attached.

Activities

mpk

2020-04-17 15:36

reporter   ~0021374

I see this is an intentional change:

commit 22deebb42fa32e618ddfb9fbdaa93449a57b1717
Author: Ben Loftis <ben@harrisonconsoles.com>
Date: Tue Feb 12 10:29:03 2019 -0600

    Selection-after-split behavior (gtk2 part)

    * When splitting in MouseObject, entered_region should get priority over selected regions.
        This fixes the unexpected case where you try to split an unselected a region, but
           a) nothing happens OR
           b) some other region (maybe off-screen) is split


But I question the logic and consistency here. The behaviour makes it awkward to split regions in a lower layer which is often used when editing with the "later is higher" editing model. It also seems strange and confusing that this behaviour is specific to the Mouse Edit Point.

Attached is a patch to only prioritize the entered_regionview if it does not overlap a selected region at the edit position.

0001-Only-prioritize-entered_regionview-if-it-doesn-t-ove.patch (1,750 bytes)
From ad7734ca75e5c807b83e8cd021866b90f094dca5 Mon Sep 17 00:00:00 2001
From: Mark Knoop <mark@markknoop.com>
Date: Fri, 17 Apr 2020 16:28:25 +0100
Subject: [PATCH] Only prioritize entered_regionview if it doesn't overlap
 selection

---
 gtk2_ardour/editor_ops.cc | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/gtk2_ardour/editor_ops.cc b/gtk2_ardour/editor_ops.cc
index 0348940907..14f54f16c0 100644
--- a/gtk2_ardour/editor_ops.cc
+++ b/gtk2_ardour/editor_ops.cc
@@ -6586,7 +6586,15 @@ Editor::split_region ()
 		//  * nothing happens OR
 		//  * some other region (maybe off-screen) is split.
 		//NOTE:  if the entered_regionview is /part of the selection/ then we should operate on the selection as usual
-		if (_edit_point == EditAtMouse && entered_regionview && !entered_regionview->selected()) {
+
+		// entered_regionview should only be prioritized if it does not overlap a selected region at the edit position
+		const samplepos_t erpos = entered_regionview->region()->position();
+		const samplepos_t erend = erpos + entered_regionview->region()->length();
+		const samplepos_t pos = get_preferred_edit_position();
+
+		if (_edit_point == EditAtMouse && entered_regionview && !entered_regionview->selected() &&
+				((erend < selection->regions.start() || erpos > selection->regions.end_sample()) ||
+				 (pos < selection->regions.start() || pos > selection->regions.end_sample()))) {
 			rs.add (entered_regionview);
 		} else {
 			rs = selection->regions;   //might be empty
@@ -6604,7 +6612,6 @@ Editor::split_region ()
 			}
 		}
 
-		const samplepos_t pos = get_preferred_edit_position();
 		const int32_t division = get_grid_music_divisions (0);
 		MusicSample where (pos, division);
 
-- 
2.25.2

BenLoftis

2020-04-17 21:06

developer   ~0021377

Yes, this change was made intentionally for the Mixbus commercial product. It has been released for about 15 months.

The intent was to address complaints like "I am finding splits in my regions that I didn't make". This was found to be the result of users expecting the Object tool to work on the "highlighted" (entered_region) region, but instead it was operating elsewhere. Either in lower layers that were current invisible, or in regions on a different track.

Since this change, I don't recall any reports of that problem.

If you want to split multiple regions as you've described, there are 3 good tools for that: The Cut tool, the Range tool, and EditPoint=Playhead (all of which can use a track selection). Those operations work on the whole layer stack.

mpk

2020-04-18 09:17

reporter   ~0021381

Thanks Ben, I appreciate the response and recognise that this is unlikely to change :) However if I may at least request a configurable option for this behaviour?

Consider the attached screenshot as an example. In this situation virtually every action in the Region menu (via keyboard shortcuts) operates on the selected (red) regions [Audio-1.1, Audio-1.23]: trim start and end, boost/cut gain, set fade in/out, mute, raise/lower etc. Except Split, which now operates on Audio-1.24 - a region visually distinguished only by the fade handles, not really "highlighted" (perhaps this is different in Mixbus?).

I guess I don't quite understand why a user would have selected regions and NOT want the action to operate on them.

Regarding your suggested alternative tools:

- The Cut tool is not able to take a region selection so will not help in this situation
- The Range tool will split all the layered regions, not just the selected regions
- Yes, using EditPoint=Playhead does work as expected, but requires changing to a different editing model and loses the efficiencies of EditPoint=Mouse.

split-situation.png (89,992 bytes)
split-situation.png (89,992 bytes)

x42

2020-04-18 23:22

administrator   ~0021383

I think "entered_regionview should only be prioritized if it does not overlap a selected region at the edit position" as proposed by the patch is the best of both worlds.

@BenLoftis this should still prevent your "I am finding splits in my regions that I didn't make" issue. If a selection is elsewhere, entered_regionview is used. Am I missing something?

mpk

2020-04-19 10:53

reporter   ~0021386

TBH this still seems pretty messy to me - it's not logical why the Split action in particular should be special-cased to behave differently.

Perhaps what @BenLoftis‘s user really wants is an additional mouse mode (or sub-mode, like the Smart switch) where the region under the pointer (entered_regionview) is ALWAYS prioritised over the selection for all region actions.

The current situation seems to be analogous to a word processor where Ctrl-i italicises the current selection, but Ctrl-B emboldens the word under the mouse pointer.

mpk

2020-06-01 07:22

reporter   ~0024338

Any thoughts on a resolution for this? Is it worth me working up a patch to make this behaviour optional?

mpk

2020-06-02 10:30

reporter   ~0024346

OK, here's a patch to make this behaviour configurable.

0001-Add-configuration-option-for-new-split-action-behavi.patch (3,202 bytes)
From fdaf120f17860bbfa27eae121510db1242a8c7b8 Mon Sep 17 00:00:00 2001
From: Mark Knoop <mark@markknoop.com>
Date: Tue, 2 Jun 2020 11:16:38 +0100
Subject: [PATCH] Add configuration option for new split action behaviour

---
 gtk2_ardour/editor_ops.cc                  |  2 +-
 gtk2_ardour/rc_option_editor.cc            | 10 ++++++++++
 libs/ardour/ardour/rc_configuration_vars.h |  1 +
 3 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/gtk2_ardour/editor_ops.cc b/gtk2_ardour/editor_ops.cc
index 08dd4b4220..32140e3a2a 100644
--- a/gtk2_ardour/editor_ops.cc
+++ b/gtk2_ardour/editor_ops.cc
@@ -6587,7 +6587,7 @@ Editor::split_region ()
 		//  * nothing happens OR
 		//  * some other region (maybe off-screen) is split.
 		//NOTE:  if the entered_regionview is /part of the selection/ then we should operate on the selection as usual
-		if (_edit_point == EditAtMouse && entered_regionview && !entered_regionview->selected()) {
+		if (Config->get_split_region_ignores_selection() && _edit_point == EditAtMouse && entered_regionview && !entered_regionview->selected()) {
 			rs.add (entered_regionview);
 		} else {
 			rs = selection->regions;   //might be empty
diff --git a/gtk2_ardour/rc_option_editor.cc b/gtk2_ardour/rc_option_editor.cc
index d7d8854ae7..14bee8c710 100644
--- a/gtk2_ardour/rc_option_editor.cc
+++ b/gtk2_ardour/rc_option_editor.cc
@@ -2636,6 +2636,16 @@ RCOptionEditor::RCOptionEditor ()
 
 	add_option (_("Editor"), new OptionEditorHeading (_("Split/Separate")));
 
+  bo = new BoolOption (
+				"split-region-ignores-selection",
+				_("Ignore region selection when requesting split in Object mode"),
+				sigc::mem_fun (*_rc_config, &RCConfiguration::get_split_region_ignores_selection),
+				sigc::mem_fun (*_rc_config, &RCConfiguration::set_split_region_ignores_selection)
+				);
+  add_option (_("Editor"), bo);
+  Gtkmm2ext::UI::instance()->set_tip (bo->tip_widget(),
+			_("<b>When enabled</b> split affects the region under the mouse pointer, not the selected regions."));
+
 	ComboOption<RangeSelectionAfterSplit> *rras = new ComboOption<RangeSelectionAfterSplit> (
 		    "range-selection-after-separate",
 		    _("After a Separate operation, in Range mode"),
diff --git a/libs/ardour/ardour/rc_configuration_vars.h b/libs/ardour/ardour/rc_configuration_vars.h
index 87dd884f2f..167e83c585 100644
--- a/libs/ardour/ardour/rc_configuration_vars.h
+++ b/libs/ardour/ardour/rc_configuration_vars.h
@@ -111,6 +111,7 @@ CONFIG_VARIABLE (bool, automation_follows_regions, "automation-follows-regions",
 CONFIG_VARIABLE (bool, region_boundaries_from_selected_tracks, "region-boundaries-from-selected-tracks", true)
 CONFIG_VARIABLE (bool, region_boundaries_from_onscreen_tracks, "region-boundaries-from-onscreen_tracks", true)
 CONFIG_VARIABLE (FadeShape, default_fade_shape, "default-fade-shape", FadeConstantPower)
+CONFIG_VARIABLE (bool, split_region_ignores_selection, "split-region-ignores-selection", false)
 CONFIG_VARIABLE (RangeSelectionAfterSplit, range_selection_after_split, "range-selection-after-split", PreserveSel)
 CONFIG_VARIABLE (RegionSelectionAfterSplit, region_selection_after_split, "region-selection-after-split", None)
 
-- 
2.26.2

paul

2020-06-04 16:02

administrator   ~0024381

The analogy with a word processor isn't really fair.

Both of those examples (italicize and embolden) only require knowledge of the target of the operation: which word?

Split requires 2 things: the target of the operation (which region(s)?) and the location of the operation. Neither the region selection nor the mouse location alone provide all the required information.

mpk

2020-06-05 08:54

reporter   ~0024395

Sure, it's not an exact analogy, but the fact that currently the selection is ignored is the point.

colinf

2020-06-05 09:12

updater   ~0024399

A radical thought perhaps, but isn't the idea of a persistent "selection" when Edit point=mouse actually redundant? Shouldn't "the selection" be considered to be whatever is under the mouse pointer (plus any other equivalent regions) at any particular moment? If the mouse is over the track headers, it should be a track selection: if it's over a region, it should be a region selection.

I rarely use EP=mouse, possibly because it's not always clear in my mental model of it what will happen, depending on what (if anything) is selected. Having the selection explicitly and visually follow the mouse would make it absolutely clear what any editing operations were about to apply to.

mpk

2020-06-05 10:15

reporter   ~0024400

This could be a possibility, perhaps for a new edit mode (essentially what I suggested in 0007994:0021386). But this mode would not be useful for the particular case which prompted this issue, being that it is now impossible to split a region that is not the topmost.

(AFAICT, the use-case for which the change was made seems to be a situation where the user should just use the Cut tool anyway.)

Issue History

Date Modified Username Field Change
2020-04-08 11:57 mpk New Issue
2020-04-17 15:36 mpk File Added: 0001-Only-prioritize-entered_regionview-if-it-doesn-t-ove.patch
2020-04-17 15:36 mpk Note Added: 0021374
2020-04-17 21:06 BenLoftis Note Added: 0021377
2020-04-18 09:17 mpk File Added: split-situation.png
2020-04-18 09:17 mpk Note Added: 0021381
2020-04-18 23:22 x42 Note Added: 0021383
2020-04-19 10:53 mpk Note Added: 0021386
2020-06-01 07:22 mpk Note Added: 0024338
2020-06-02 10:30 mpk File Added: 0001-Add-configuration-option-for-new-split-action-behavi.patch
2020-06-02 10:30 mpk Note Added: 0024346
2020-06-04 16:02 paul Note Added: 0024381
2020-06-05 08:54 mpk Note Added: 0024395
2020-06-05 09:12 colinf Note Added: 0024399
2020-06-05 10:15 mpk Note Added: 0024400