Remove the SelectionChangeSubscriber mechanism from the grid and implement some basic selection change notification through SelectionController.

Change SelectionListener interface so it receives the set of lines added and removed from selection, instead of just the complete new selection.
Update VisualTool<> to use SelectionListener to receive selection change notifications.

This change (temporarily, I hope) breaks feature selection in visual drag mode, when changing selection via the grid. This is caused by the grid selection change first clearing the entire selection, which sends a separate notification about selection clear. This causes the last visual feature to be deselected, and then the visual tool base reselects the active line, causing a new notification for selection to be sent. The active line happens to be the newly clicked line, and the selection notification enters during the externalChange guard being set, and is then ignored for feature update purposes. When control returns to the original SelectRow call in the grid, the line to be selected has already been selected and then nothing happens.
The best fix is to avoid two notifications being required to deselect all then reselect one line in the first place, so making the grid selection handling saner is the best fix.

Originally committed to SVN as r4602.

aegisub/src/base_grid.cpp

@@ -80,7 +80,6 @@ BaseGrid::BaseGrid(wxWindow* parent, wxWindowID id, const wxPoint& pos, const wx
 	holding = false;
 	byFrame = false;
 	lineHeight = 1; // non-zero to avoid div by 0
-	selChangeSub = NULL;
 
 	// Set scrollbar
 	scrollBar = new wxScrollBar(this,GRID_SCROLLBAR,wxDefaultPosition,wxDefaultSize,wxSB_VERTICAL);
@@ -142,6 +141,10 @@ void BaseGrid::UpdateStyle() {
 /// @brief Clears grid 
 ///
 void BaseGrid::Clear () {
+	Selection lines_removed;
+	GetSelectedSet(lines_removed);
+	AnnounceSelectedSetChanged(Selection(), lines_removed);
+
 	diagMap.clear();
 	diagPtrMap.clear();
 	selMap.clear();
@@ -209,7 +212,11 @@ void BaseGrid::MakeCellVisible(int row, int col,bool center) {
 ///
 void BaseGrid::SelectRow(int row, bool addToSelected, bool select) {
 	if (row < 0 || (size_t)row >= selMap.size()) return;
-	if (!addToSelected) ClearSelection();
+
+	if (!addToSelected) {
+		// Sends change notifications for itself
+		ClearSelection();
+	}
 
 	if (select != !!selMap[row]) {
 		selMap[row] = select;
@@ -224,7 +231,14 @@ void BaseGrid::SelectRow(int row, bool addToSelected, bool select) {
 			RefreshRect(wxRect(0,(row+1-yPos)*lineHeight,w,lineHeight),false);
 		}
 
-		if (selChangeSub) selChangeSub->OnSelectionChange(!addToSelected, row, select);
+		Selection lines_added;
+		Selection lines_removed;
+		if (select)
+			lines_added.insert(diagPtrMap[row]);
+		else
+			lines_removed.insert(diagPtrMap[row]);
+
+		AnnounceSelectedSetChanged(lines_added, lines_removed);
 	}
 }
 
@@ -233,6 +247,9 @@ void BaseGrid::SelectRow(int row, bool addToSelected, bool select) {
 /// @brief Selects visible lines 
 ///
 void BaseGrid::SelectVisible() {
+	Selection lines_removed;
+	GetSelectedSet(lines_removed);
+
 	int rows = GetRows();
 	bool selectedOne = false;
 	for (int i=0;i<rows;i++) {
@@ -247,6 +264,11 @@ void BaseGrid::SelectVisible() {
 			}
 		}
 	}
+
+	Selection lines_added;
+	GetSelectedSet(lines_added);
+
+	AnnounceSelectedSetChanged(lines_added, lines_removed);
 }
 
 
@@ -254,11 +276,16 @@ void BaseGrid::SelectVisible() {
 /// @brief Unselects all cells 
 ///
 void BaseGrid::ClearSelection() {
+	Selection lines_removed;
+	GetSelectedSet(lines_removed);
+
 	int rows = selMap.size();
 	for (int i=0;i<rows;i++) {
 		selMap[i] = false;
 	}
 	Refresh(false);
+
+	AnnounceSelectedSetChanged(Selection(), lines_removed);
 }
 
 
@@ -309,8 +336,8 @@ int BaseGrid::GetLastSelRow() const {
 
 
 /// @brief Gets all selected rows 
-/// @param[out] cont 
-/// @return 
+/// @param[out] cont Is the selection contiguous, i.e. free from holes
+/// @return Array with indices of selected lines
 ///
 wxArrayInt BaseGrid::GetSelection(bool *cont) const {
 	// Prepare
@@ -1180,3 +1207,16 @@ wxArrayInt BaseGrid::GetRangeArray(int n1,int n2) const {
 }
 
 
+
+// SelectionController
+
+
+void BaseGrid::GetSelectedSet(Selection &selection) const {
+	for (size_t i = 0; i < selMap.size(); ++i) {
+		if (selMap[i] != 0) {
+			selection.insert(GetDialogue((int)i));
+		}
+	}
+}
+
+

aegisub/src/base_grid.h

@@ -60,11 +60,9 @@ class FrameMain;
 /// DOCME
 typedef std::list<AssEntry*>::iterator entryIter;
 
-class SelectionChangeSubscriber {
-public:
-	virtual void OnSelectionChange(bool clear, int row, bool selected) = 0;
-};
 
+typedef SelectionController<AssDialogue> SubtitleSelectionController;
+typedef SelectionListener<AssDialogue> SubtitleSelectionListener;
 
 
 /// DOCME
@@ -72,7 +70,7 @@ public:
 /// @brief DOCME
 ///
 /// DOCME
-class BaseGrid : public wxWindow, public BaseSelectionController<AssEntry> {
+class BaseGrid : public wxWindow, public BaseSelectionController<AssDialogue> {
 private:
 
 	/// DOCME
@@ -99,8 +97,6 @@ private:
 	/// DOCME
 	wxBitmap *bmp;
 
-	SelectionChangeSubscriber* selChangeSub;
-
 	void OnPaint(wxPaintEvent &event);
 	void OnSize(wxSizeEvent &event);
 	void OnScroll(wxScrollEvent &event);
@@ -133,10 +129,10 @@ protected:
 
 public:
 	// SelectionController implementation
-	virtual void SetActiveLine(AssEntry *new_line) { }
-	virtual AssEntry * GetActiveLine() const { return 0; }
+	virtual void SetActiveLine(AssDialogue *new_line) { }
+	virtual AssDialogue * GetActiveLine() const { return 0; }
 	virtual void SetSelectedSet(const Selection &new_selection) { }
-	virtual void GetSelectedSet(Selection &selection) const { }
+	virtual void GetSelectedSet(Selection &selection) const;
 	virtual void NextLine() { }
 	virtual void PrevLine() { }
 
@@ -181,10 +177,6 @@ public:
 
 	AssDialogue *GetDialogue(int n) const;
 
-	void RegisterSelectionChange(SelectionChangeSubscriber* sel) {
-		selChangeSub = sel;
-	}
-
 	BaseGrid(wxWindow* parent, wxWindowID id, const wxPoint& pos = wxDefaultPosition, const wxSize& size = wxDefaultSize, long style = wxWANTS_CHARS, const wxString& name = wxPanelNameStr);
 	~BaseGrid();
 

aegisub/src/selection_controller.h

@@ -47,13 +47,21 @@ public:
 	/// Virtual destructor for safety
 	virtual ~SelectionListener() { }
 
-	/// @brief Called when the active subtitle line changes
-	/// @param new_line The subtitle line that is now the active line
+	/// @brief Called when the active line changes
+	/// @param new_line The line that is now the active line
+	///
+	/// In case new_line is 0, the active line was changed to none.
 	virtual void OnActiveLineChanged(ItemDataType *new_line) = 0;
 
 	/// @brief Called when the selected set changes
-	/// @param new_selection The subtitle line set that is now the selected set
-	virtual void OnSelectedSetChanged(const Selection &new_selection) = 0;
+	/// @param lines_added   Lines added to the selection
+	/// @param lines_removed Lines removed from the selection
+	///
+	/// Conceptually, removal happens before addition, for the purpose of this change notification.
+	///
+	/// In case the two sets overlap, the intersection are lines that were previously selected
+	/// and remain selected.
+	virtual void OnSelectedSetChanged(const Selection &lines_added, const Selection &lines_removed) = 0;
 };
 
 
@@ -165,11 +173,11 @@ protected:
 	}
 
 	/// Call OnSelectedSetChangedon all listeners
-	void AnnounceSelectedSetChanged(const Selection &new_selection)
+	void AnnounceSelectedSetChanged(const Selection &lines_added, const Selection &lines_removed)
 	{
 		for (SelectionListenerSet::iterator listener = listeners.begin(); listener != listeners.end(); ++listener)
 		{
-			(*listener)->OnSelectedSetChanged(new_selection);
+			(*listener)->OnSelectedSetChanged(lines_added, lines_removed);
 		}
 	}
 

aegisub/src/visual_tool.cpp

@@ -92,7 +92,7 @@ VisualTool<FeatureType>::VisualTool(VideoDisplay *parent, VideoState const& vide
 {
 	if (VideoContext::Get()->IsLoaded()) {
 		frame_n = VideoContext::Get()->GetFrameN();
-		VideoContext::Get()->grid->RegisterSelectionChange(this);
+		VideoContext::Get()->grid->AddSelectionListener(this);
 	}
 
 	PopulateFeatureList();
@@ -100,7 +100,7 @@ VisualTool<FeatureType>::VisualTool(VideoDisplay *parent, VideoState const& vide
 
 template<class FeatureType>
 VisualTool<FeatureType>::~VisualTool() {
-	VideoContext::Get()->grid->RegisterSelectionChange(NULL);
+	VideoContext::Get()->grid->RemoveSelectionListener(this);
 }
 
 template<class FeatureType>

aegisub/src/visual_tool.h

@@ -79,7 +79,7 @@ public:
 /// @brief DOCME
 /// DOCME
 template<class FeatureType>
-class VisualTool : public IVisualTool, public SelectionChangeSubscriber {
+class VisualTool : public IVisualTool, protected SubtitleSelectionListener {
 private:
 	agi::OptionValue* realtime; /// Realtime updating option
 	int dragStartX; /// Starting x coordinate of the current drag, if any
@@ -187,6 +187,12 @@ protected:
 	typedef typename std::vector<FeatureType>::iterator feature_iterator;
 	typedef typename std::vector<FeatureType>::const_iterator feature_const_iterator;
 
+protected:
+	// SubtitleSelectionListener implementation
+	// (overridden by deriving classes too)
+	virtual void OnActiveLineChanged(AssDialogue *new_line) { }
+	virtual void OnSelectedSetChanged(const Selection &lines_added, const Selection &lines_removed) { }
+
 public:
 	/// @brief Handler for all mouse events
 	/// @param event Shockingly enough, the mouse event
@@ -202,9 +208,6 @@ public:
 	/// @brief Called by stuff when there's stuff
 	void Refresh();
 
-	/// Called by the grid when the selection changes
-	virtual void OnSelectionChange(bool, int, bool) { }
-
 	/// @brief Constructor
 	/// @param parent The VideoDisplay to use for coordinate conversion
 	/// @param video Video and mouse information passing blob

aegisub/src/visual_tool_drag.cpp

@@ -121,27 +121,24 @@ void VisualToolDrag::DoRefresh() {
 	UpdateToggleButtons();
 }
 
-void VisualToolDrag::OnSelectionChange(bool clear, int row, bool selected) {
+void VisualToolDrag::OnSelectedSetChanged(const Selection &added, const Selection &removed) {
 	if (!externalChange) return;
 	externalChange = false;
-	if (clear) {
-		ClearSelection(false);
-	}
-	if (selected) {
-		for (size_t i = 0; i < features.size(); i++) {
-			if (features[i].lineN == row && features[i].type == DRAG_START) {
-				AddSelection(i);
-				break;
-			}
+
+	// Remove all deselected lines
+	for (size_t i = 0; i < features.size(); i++) {
+		if (removed.find(features[i].line) != removed.end()) {
+			RemoveSelection(i);
 		}
 	}
-	else {
-		for (size_t i = 0; i < features.size(); i++) {
-			if (features[i].lineN == row) {
-				RemoveSelection(i);
-			}
+
+	// And add all newly selected lines
+	for (size_t i = 0; i < features.size(); i++) {
+		if (added.find(features[i].line) != added.end() && features[i].type == DRAG_START) {
+			AddSelection(i);
 		}
 	}
+
 	externalChange = true;
 }
 

aegisub/src/visual_tool_drag.h

@@ -82,11 +82,13 @@ private:
 	void UpdateToggleButtons();
 	void DoRefresh();
 
+protected:
+	// Overriding SubtitleSelectionListener inherited from base VisualTool<>
+	virtual void OnSelectedSetChanged(const Selection &lines_added, const Selection &lines_removed);
+
 public:
 	VisualToolDrag(VideoDisplay *parent, VideoState const& video, wxToolBar *toolbar);
 
-	void OnSelectionChange(bool clear, int row, bool selected);
-
 	void Draw();
 	bool Update();
 	void OnSubTool(wxCommandEvent &event);