redesign technique for naming/creating regions for MIDI clone (or other non-capture driven MIDI region creation operations).

See comments in Session::new_midi_source_name() for details.

libs/ardour/ardour/audio_diskstream.h

@@ -139,7 +139,6 @@ class LIBARDOUR_API AudioDiskstream : public Diskstream
 	void set_block_size (pframes_t);
 	int  internal_playback_seek (framecnt_t distance);
 	int  can_internal_playback_seek (framecnt_t distance);
-	std::list<boost::shared_ptr<Source> > steal_write_sources();
 	void reset_write_sources (bool, bool force = false);
 	void non_realtime_input_change ();
 	void non_realtime_locate (framepos_t location);

libs/ardour/ardour/diskstream.h

@@ -71,6 +71,8 @@ class LIBARDOUR_API Diskstream : public SessionObject, public PublicDiskstream
 
 	virtual bool set_name (const std::string& str);
 
+	virtual std::string steal_write_source_name () { return std::string(); }
+
 	boost::shared_ptr<ARDOUR::IO> io() const { return _io; }
 	void set_track (ARDOUR::Track *);
 

libs/ardour/ardour/file_source.h

@@ -44,7 +44,7 @@ class LIBARDOUR_API MissingSource : public std::exception
 /** A source associated with a file on disk somewhere */
 class LIBARDOUR_API FileSource : virtual public Source {
 public:
-	virtual ~FileSource () {}
+	virtual ~FileSource ();
 
 	virtual const std::string& path() const { return _path; }
 

libs/ardour/ardour/midi_diskstream.h

@@ -111,7 +111,7 @@ class LIBARDOUR_API MidiDiskstream : public Diskstream
 	void set_block_size (pframes_t);
 	int  internal_playback_seek (framecnt_t distance);
 	int  can_internal_playback_seek (framecnt_t distance);
-	std::list<boost::shared_ptr<Source> > steal_write_sources();
+	std::string steal_write_source_name();
 	void reset_write_sources (bool, bool force = false);
 	void non_realtime_input_change ();
 	void non_realtime_locate (framepos_t location);

libs/ardour/ardour/public_diskstream.h

@@ -38,7 +38,7 @@ public:
 	virtual bool destructive () const = 0;
 	virtual std::list<boost::shared_ptr<Source> > & last_capture_sources () = 0;
 	virtual void set_capture_offset () = 0;
-	virtual std::list<boost::shared_ptr<Source> > steal_write_sources () = 0;
+	virtual std::string steal_write_source_name () = 0;
 	virtual void reset_write_sources (bool, bool force = false) = 0;
 	virtual float playback_buffer_load () const = 0;
 	virtual float capture_buffer_load () const = 0;

libs/ardour/ardour/track.h

@@ -120,7 +120,7 @@ class LIBARDOUR_API Track : public Route, public PublicDiskstream
 	bool destructive () const;
 	std::list<boost::shared_ptr<Source> > & last_capture_sources ();
 	void set_capture_offset ();
-	std::list<boost::shared_ptr<Source> > steal_write_sources();
+	std::string steal_write_source_name ();
 	void reset_write_sources (bool, bool force = false);
 	float playback_buffer_load () const;
 	float capture_buffer_load () const;

libs/ardour/audio_diskstream.cc

@@ -1926,14 +1926,6 @@ AudioDiskstream::use_new_write_source (uint32_t n)
 	return 0;
 }
 
-list<boost::shared_ptr<Source> >
-AudioDiskstream::steal_write_sources()
-{
-	/* not possible to steal audio write sources */
-	list<boost::shared_ptr<Source> > ret;
-	return ret;
-}
-
 void
 AudioDiskstream::reset_write_sources (bool mark_write_complete, bool /*force*/)
 {

libs/ardour/file_source.cc

@@ -81,6 +81,10 @@ FileSource::FileSource (Session& session, const XMLNode& node, bool /*must_exist
         prevent_deletion ();
 }
 
+FileSource::~FileSource()
+{
+}
+
 void
 FileSource::prevent_deletion ()
 {
@@ -88,6 +92,8 @@ FileSource::prevent_deletion ()
          */
 
         if (Glib::file_test (_path, Glib::FILE_TEST_EXISTS)) {
+		cerr << " ... " << _path << " already  exists, marking immutable\n";
+
                 if (!(_flags & Destructive)) {
                         mark_immutable ();
                 } else {
@@ -527,10 +533,13 @@ FileSource::set_source_name (const string& newname, bool destructive)
 		return -1;
 	}
 
-        if (::rename (oldpath.c_str(), newpath.c_str()) != 0) {
+	if (Glib::file_test (oldpath.c_str(), Glib::FILE_TEST_EXISTS)) { 
-                error << string_compose (_("cannot rename file %1 to %2 (%3)"), oldpath, newpath, strerror(errno)) << endmsg;
+		/* rename only needed if file exists on disk */
-                return -1;
+		if (::rename (oldpath.c_str(), newpath.c_str()) != 0) {
-        }
+			error << string_compose (_("cannot rename file %1 to %2 (%3)"), oldpath, newpath, strerror(errno)) << endmsg;
+			return -1;
+		}
+	}
 
 	_name = Glib::path_get_basename (newpath);
 	_path = newpath;

libs/ardour/midi_diskstream.cc

@@ -1213,25 +1213,24 @@ MidiDiskstream::use_new_write_source (uint32_t n)
 
 	return 0;
 }
-
+/**
-list<boost::shared_ptr<Source> >
+ * We want to use the name of the existing write source (the one that will be
-MidiDiskstream::steal_write_sources()
+ * used by the next capture) for another purpose. So change the name of the
+ * current source, and return its current name.
+ *
+ * Return an empty string if the change cannot be accomplished.
+ */
+std::string
+MidiDiskstream::steal_write_source_name ()
 {
-	list<boost::shared_ptr<Source> > ret;
+	std::string our_new_name = _session.new_midi_source_name (_write_source->name());
+	std::string our_old_name = _write_source->name();
+	
+	if (_write_source->set_source_name (our_new_name, false)) {
+		return string();
+	}
 
-	/* put some data on the disk, even if its just a header for an empty file */
+	return our_old_name;
-	boost::dynamic_pointer_cast<SMFSource> (_write_source)->ensure_disk_file ();
-
-	/* never let it go away */
-	_write_source->mark_nonremovable ();
-
-	ret.push_back (_write_source);
-
-	/* get a new one */
-
-	use_new_write_source (0);
-
-	return ret;
 }
 
 void

libs/ardour/midi_source.cc

@@ -335,21 +335,19 @@ MidiSource::mark_streaming_write_completed ()
 boost::shared_ptr<MidiSource>
 MidiSource::clone (const string& path, Evoral::MusicalTime begin, Evoral::MusicalTime end)
 {
-	string newname = PBD::basename_nosuffix(_name.val());
 	string newpath;
 
+	/* get a new name for the MIDI file we're going to write to
+	 */
+	
 	if (path.empty()) {
-
+		string newname = PBD::basename_nosuffix(_name.val());
-		/* get a new name for the MIDI file we're going to write to
+		newname = bump_name_once (newname, '-');
-		 */
+		newname += ".mid";
-		
+		newpath = _session.new_source_path_from_name (DataType::MIDI, newname);
-		do {
-			newname = bump_name_once (newname, '-');
-			newpath = Glib::build_filename (_session.session_directory().midi_path(), newname + ".mid");
-			
-		} while (Glib::file_test (newpath, Glib::FILE_TEST_EXISTS));
 	} else {
 		/* caller must check for pre-existing file */
+		assert (!Glib::file_test (path, Glib::FILE_TEST_EXISTS));
 		newpath = path;
 	}
 

libs/ardour/session.cc

@@ -88,6 +88,7 @@
 #include "ardour/smf_source.h"
 #include "ardour/source_factory.h"
 #include "ardour/speakers.h"
+#include "ardour/track.h"
 #include "ardour/utils.h"
 
 #include "midi++/port.h"
@@ -3587,6 +3588,7 @@ Session::new_midi_source_name (const string& base)
 	legalized = legalize_for_path (base);
 
 	// Find a "version" of the file name that doesn't exist in any of the possible directories.
+
 	for (cnt = 1; cnt <= limit; ++cnt) {
 
 		vector<space_and_path>::iterator i;
@@ -3626,23 +3628,37 @@ Session::new_midi_source_name (const string& base)
 boost::shared_ptr<MidiSource>
 Session::create_midi_source_for_session (Track* track, string const & n)
 {
-	/* try to use the existing write source for the track, to keep numbering sane
+	std::string name;
-	 */
 
 	if (track) {
-		/*MidiTrack* mt = dynamic_cast<Track*> (track);
+		/* the caller passes in the track the source will be used in,
-		  assert (mt);
+		   so that we can keep the numbering sane. 
+		   
+		   Rationale: a track with the name "Foo" that has had N
+		   captures carried out so far will already have a write source
+		   named "Foo-N+1.mid" waiting to be used for the next capture.
+
+		   If we call new_midi_source_name() we will get "Foo-N+2". But
+		   there is no region corresponding to "Foo-N+1", so when
+		   "Foo-N+2" appears in the track, the gap presents the user
+		   with odd behaviour - why did it skip past Foo-N+1?
+
+		   We could explain this to the user in some odd way, but
+		   instead we rename "Foo-N+1.mid" as "Foo-N+2.mid", and then
+		   use "Foo-N+1" here.
+
+		   If that attempted rename fails, we get "Foo-N+2.mid" anyway.
 		*/
 
-		list<boost::shared_ptr<Source> > l = track->steal_write_sources ();
+		MidiTrack* mt = dynamic_cast<MidiTrack*> (track);
-
+		assert (mt);
-		if (!l.empty()) {
+		name = track->steal_write_source_name ();
-			assert (boost::dynamic_pointer_cast<MidiSource> (l.front()));
+	}
-			return boost::dynamic_pointer_cast<MidiSource> (l.front());
+
-		}
+	if (name.empty()) {
+		name = new_midi_source_name (n);
 	}
 
-	const string name = new_midi_source_name (n);
 	const string path = new_source_path_from_name (DataType::MIDI, name);
 
 	return boost::dynamic_pointer_cast<SMFSource> (

libs/ardour/track.cc

@@ -579,10 +579,10 @@ Track::set_capture_offset ()
 	_diskstream->set_capture_offset ();
 }
 
-list<boost::shared_ptr<Source> >
+std::string
-Track::steal_write_sources()
+Track::steal_write_source_name()
 {
-        return _diskstream->steal_write_sources ();
+        return _diskstream->steal_write_source_name ();
 }
 
 void