Fix a number of crashes when views are destroyed. Avoid deleting widgets...

Fix a number of crashes when views are destroyed.  Avoid deleting widgets explicitly where possible, using QObject parent-child mechanism instead.  Defer deletion using object->deleteLater() instead of 'delete object'.  Make filter chains own their filters and take responsibility for deleting them, instead of having other objects deleting filters without removing them from the chain.  Add API documentation to the filter classes.

svn path=/branches/work/konsole-split-view/; revision=624111

konsole/Filter.cpp

@@ -36,6 +36,18 @@
 #include "Filter.h"
 #include "TerminalCharacterDecoder.h"
 
+FilterChain::~FilterChain()
+{
+    QMutableListIterator<Filter*> iter(*this);
+    
+    while ( iter.hasNext() )
+    {
+        Filter* filter = iter.next();
+        iter.remove();
+        delete filter;
+    }
+}
+
 void FilterChain::addFilter(Filter* filter)
 {
     append(filter);

konsole/Filter.h

@@ -29,12 +29,46 @@
 
 class ca;
 
+/**
+ * A filter processes blocks of text looking for certain patterns (such as URLs or keywords from a list)
+ * and marks the areas which match the filter's patterns as 'hotspots'.
+ *
+ * Each hotspot has a type identifier associated with it ( such as a link or a highlighted section ),
+ * and an action.  When the user performs some activity such as a mouse-click in a hotspot area ( the exact
+ * action will depend on what is displaying the block of text which the filter is processing ), the hotspot's
+ * activate() method should be called.  Depending on the type of hotspot this will trigger a suitable response.
+ *
+ * For example, if a hotspot represents a URL then a suitable action would be opening that URL in a web browser.
+ * Hotspots may have more than one action, in which case the list of actions can be obtained using the 
+ * actions() method.
+ *
+ * Different subclasses of filter will return different types of hotspot.
+ * Subclasses must reimplement the process() method to examine a block of text and identify sections of interest.
+ * When processing the text they should create instances of Filter::HotSpot subclasses for sections of interest
+ * and add them to the filter's list of hotspots using addHotSpot()
+ */
 class Filter
 {
 public:
+    /**
+    * Represents an area of text which matched the pattern a particular filter has been looking for.
+    *
+    * Each hotspot has a type identifier associated with it ( such as a link or a highlighted section ),
+    * and an action.  When the user performs some activity such as a mouse-click in a hotspot area ( the exact
+    * action will depend on what is displaying the block of text which the filter is processing ), the hotspot's
+    * activate() method should be called.  Depending on the type of hotspot this will trigger a suitable response.
+    *
+    * For example, if a hotspot represents a URL then a suitable action would be opening that URL in a web browser.
+    * Hotspots may have more than one action, in which case the list of actions can be obtained using the 
+    * actions() method.  These actions may then be displayed in a popup menu or toolbar for example. 
+    */
     class HotSpot
     {
     public:
+       /** 
+        * Constructs a new hotspot which covers the area from (@p startLine,@p startColumn) to (@p endLine,@p endColumn)
+        * in a block of text.
+        */
        HotSpot(int startLine , int startColumn , int endLine , int endColumn);
        virtual ~HotSpot();
 
@@ -48,15 +82,26 @@ public:
             Marker
        }; 
 
+       /** Returns the line when the hotspot area starts */
        int startLine() const;
+       /** Returns the line where the hotspot area ends */
        int endLine() const;
+       /** Returns the column on startLine() where the hotspot area starts */
        int startColumn() const;
+       /** Returns the column on endLine() where the hotspot area ends */
        int endColumn() const;
+       /** 
+        * Returns the type of the hotspot.  This is usually used as a hint for views on how to represent
+        * the hotspot graphically.  eg.  Link hotspots are typically underlined when the user mouses over them
+        */
        Type type() const;
+       /** Causes the primary action associated with a hotspot to be triggered. */
        virtual void activate() = 0; 
+       /** Returns a list of actions associated with the hotspot which can be used in a menu or toolbar */
        virtual QList<QAction*> actions();
 
     protected:
+       /** Sets the type of a hotspot.  This should only be set once */
        void setType(Type type);
 
     private:
@@ -70,18 +115,33 @@ public:
 
     virtual ~Filter();
 
+    /** Causes the filter to process the block of text currently in its internal buffer */
     virtual void process() = 0;
 
+    /** 
+     * Empties the filters internal buffer and resets the line count back to 0.
+     * All hotspots are deleted. 
+     */
     void reset();
+
+    /** Adds a new line of text to the filter and increments the line count */
     void addLine(const QString& string);
 
+    /** Returns the hotspot which covers the given @p line and @p column, or 0 if no hotspot covers that area */
     HotSpot* hotSpotAt(int line , int column) const;
+
+    /** Returns the list of hotspots identified by the filter */
     QList<HotSpot*> hotSpots() const;
+
+    /** Returns the list of hotspots identified by the filter which occur on a given line */
     QList<HotSpot*> hotSpotsAtLine(int line) const;
 
 protected:
+    /** Adds a new hotspot to the list */
     void addHotSpot(HotSpot*);
+    /** Returns the internal buffer */
     QString& buffer();
+    /** Converts a character position within buffer() to a line and column */
     void getLineColumn(int position , int& startLine , int& startColumn);
 
 private:
@@ -91,29 +151,50 @@ private:
     QString _buffer;
 };
 
+/** 
+ * A filter which searches for sections of text matching a regular expression and creates a new RegExpFilter::HotSpot 
+ * instance for them.
+ *
+ * Subclasses can reimplement newHotSpot() to return custom hotspot types when matches for the regular expression
+ * are found. 
+ */
 class RegExpFilter : public Filter
 {
 public:
+    /** 
+     * Type of hotspot created by RegExpFilter.  The capturedTexts() method can be used to find the text
+     * matched by the filter's regular expression.
+     */
     class HotSpot : public Filter::HotSpot
     {
     public:
         HotSpot(int startLine, int startColumn, int endLine , int endColumn);
         virtual void activate();
 
+        /** Sets the captured texts associated with this hotspot */
         void setCapturedTexts(const QStringList& texts);
+        /** Returns the texts found by the filter when matching the filter's regular expression */
         QStringList capturedTexts() const;
     private:
         QStringList _capturedTexts;
     };
 
+    /** Constructs a new regular expression filter */
     RegExpFilter();
 
+    /** Sets the regular expression which the filter searches for in blocks of text */
     void setRegExp(const QRegExp& text);
+    /** Returns the regular expression which the filter searches for in blocks of text */
     QRegExp regExp() const;
 
+    /** Reimplemented to search the filter's text buffer for text matching regExp() */
     virtual void process();
 
 protected:
+    /** 
+     * Called when a match for the regular expression is encountered.  Subclasses should reimplement this
+     * to return custom hotspot types
+     */
     virtual RegExpFilter::HotSpot* newHotSpot(int startLine,int startColumn,
                                     int endLine,int endColumn);
 
@@ -122,9 +203,15 @@ private:
 };
 
 class FilterObject;
+
+/** A filter which matches URLs in blocks of text */
 class UrlFilter : public RegExpFilter 
 {
 public:
+    /** 
+     * Hotspot type created by UrlFilter instances.  The activate() method opens a web browser 
+     * at the given URL when called.
+     */
     class HotSpot : public RegExpFilter::HotSpot 
     {
     public:
@@ -132,6 +219,11 @@ public:
         virtual ~HotSpot();
 
         virtual QList<QAction*> actions();
+
+        /** 
+         * Open a web browser at the current URL.  The url itself can be determined using
+         * the capturedTexts() method.
+         */
         virtual void activate();
     private:
         FilterObject* _urlObject;
@@ -154,26 +246,70 @@ private:
     Filter::HotSpot* _filter;
 };
 
+/** 
+ * A chain which allows a group of filters to be processed as one. 
+ * The chain owns the filters added to it and deletes them when the chain itself is destroyed.
+ *
+ * Use addFilter() to add a new filter to the chain.  
+ * When new text to be filtered arrives, use addLine() to add each additional
+ * line of text which needs to be processed and then after adding the last line, use
+ * process() to cause each filter in the chain to process the text.
+ *
+ * After processing a block of text, the reset() method can be used to set the filter chain's
+ * internal cursor back to the first line.
+ *
+ * The hotSpotAt() method will return the first hotspot which covers a given position.
+ *
+ * The hotSpots() and hotSpotsAtLine() method return all of the hotspots in the text and on
+ * a given line respectively.
+ */
 class FilterChain : protected QList<Filter*>
 {
 public:
+    virtual ~FilterChain();
+
+    /** Adds a new filter to the chain.  The chain will delete this filter when it is destroyed */
     void addFilter(Filter* filter);
+    /** Removes a filter from the chain.  The chain will no longer delete the filter when destroyed */
     void removeFilter(Filter* filter);
+    /** Returns true if the chain contains @p filter */
     bool containsFilter(Filter* filter);
+    /** Removes all filters from the chain */
     void clear();
 
+    /** Resets each filter in the chain */
     void reset();
+    /** 
+     * Adds a new line of text to each filter in the chain 
+     * TODO:  This is inefficient memory-wise because a new line is added for each filter.
+     * Only one buffer should be used for the entire chain.
+     */
     void addLine(const QString& line);
+    /**
+     * Processes each filter in the chain 
+     */
     void process();
+
+    /** Returns the first hotspot which occurs at @p line, @p column or 0 if no hotspot was found */
     Filter::HotSpot* hotSpotAt(int line , int column) const;
+    /** Returns a list of all the hotspots in all the chain's filters */
     QList<Filter::HotSpot*> hotSpots() const;
+    /** Returns a list of all hotspots at the given line in all the chain's filters */
     QList<Filter::HotSpot> hotSpotsAtLine(int line) const;
 
 };
 
+/** A filter chain which processes character images from terminal displays */
 class TerminalImageFilterChain : public FilterChain
 {
 public:
+    /**
+     * Set the current terminal image to @p image.
+     *
+     * @param image The terminal image
+     * @param lines The number of lines in the terminal image
+     * @param columns The number of columns in the terminal image
+     */
     void addImage(const ca* const image , int lines , int columns);  
 };
 #endif //FILTER_H

konsole/SPLIT-VIEW-TODO

-
-Monday - 8th Jan 07
-===================
+Tuesday - 16th Jan 07
+=====================
     - More reliable syncing of menus when selected view changes.
       Currently the _pluggedController in ViewManager is only changed if the newly
       selected view gets the focus, and that might not happen automatically in some circumstances.
 
       Add a signal to ViewContainer which is emitted when the selected view changes.
 
+      [PARTIAL-FIXED Works with ListViewContainer, just need to make TabbedViewContainer emit
+       a viewActivated() signal]
+
+    - Midnight Commander not always set to the right size when creating a new view
+
+Monday - 8th Jan 07
+===================
+
     - Calling viewRemoved() in ViewContainer::viewDestroyed() causes crashes at exit.
       Start Konsole, create several shells then click X button on main window to close --> crash.
       Debug this.
-    
-    - Midnight Commander not always set to the right size when creating a new view
+   
+      [WORKS - Don't delete objects directly in ViewContainer if possible, rely on the QObject parent-child
+      mechanism to do that instead.  Use object->deleteLater() rather than 'delete object'
+      -- Not absolutely sure about the safety of that fix.  See if there is a more correct solution]
+
 
     - Using ListViewContainer as view container:
 
@@ -22,18 +32,17 @@ Monday - 8th Jan 07
       5.  The new main window now has two list view items for the same terminal display (as expected)
       6.  Trying to activate the second entry in the list causes a crash
 
+      [FIXED - Crash caused by session-view controller deleting a filter when destroyed.  
+               Filter chains now delete the filters themselves ]
+
     - Using ListViewContainer as view container:
 
       1.  Create a new shell
       2.  Split the view
       3.  Unsplit the view using View -> Remove split.  Causes a crash
 
-    - Cannot create a selection when the view is split.  
-        
-        1.  Create a new shell
-        2.  Split the view
-        3.  Try to select a block of text with the mouse in one of the views
-        4.  The selection will disappear instantly 
+      [FIXED - See above ]
+
 
 
 == NOTE: This TODO refers to the old Konsole code, before the start of a new front-end

konsole/SessionController.cpp

@@ -41,14 +41,11 @@ SessionController::SessionController(TESession* session , TEWidget* view, QObjec
     connect( _session , SIGNAL(updateTitle()) , this , SLOT(sessionTitleChanged()) );
 
     // install filter on the view to highlight URLs
-    _viewUrlFilter = new UrlFilter();
-    view->filterChain()->addFilter( _viewUrlFilter );
+    view->filterChain()->addFilter( new UrlFilter() );
 }
 
 SessionController::~SessionController()
 {   
-    // delete filters
-    delete _viewUrlFilter;
 }
 
 bool SessionController::eventFilter(QObject* watched , QEvent* event)

konsole/SessionController.h

@@ -41,7 +41,9 @@ public:
     /** Reimplemented to watch for events happening to the view */
     virtual bool eventFilter(QObject* watched , QEvent* event);
 
+    /** Returns the session associated with this controller */
     TESession* session() { return _session; }
+    /** Returns the view associated with this controller */
     TEWidget*  view()    { return _view;    }
 
 signals:
@@ -53,6 +55,7 @@ signals:
     void focused( SessionController* controller );
 
 private slots:
+    // menu item handlers
     void copy();
     void paste();
     void clear();

konsole/ViewContainer.cpp

@@ -40,6 +40,11 @@
 #include "ViewContainer.h"
 #include "ViewProperties.h"
 
+ViewContainer::~ViewContainer()
+{
+    emit destroyed(this);
+
+}
 void ViewContainer::addView(QWidget* view , ViewProperties* item)
 {
     _views << view;
@@ -173,8 +178,8 @@ void TabbedViewContainer::closeTabClicked()
 
 TabbedViewContainer::~TabbedViewContainer()
 {
-    delete _tabContextMenu;
-    delete _tabWidget;
+    _tabContextMenu->deleteLater();
+    _tabWidget->deleteLater();
 }
 
 void TabbedViewContainer::setNewSessionMenu(QMenu* menu)
@@ -330,22 +335,19 @@ ListViewContainer::ListViewContainer(QObject* parent)
     : ViewContainer(parent)
 {
     _splitter = new QSplitter;
-    _stackWidget = new QStackedWidget;
-    _listWidget = new QListWidget;
+    _stackWidget = new QStackedWidget(_splitter);
+    _listWidget = new QListWidget(_splitter);
     _listWidget->setTextElideMode( Qt::ElideLeft );
     _listWidget->setHorizontalScrollBarPolicy( Qt::ScrollBarAlwaysOff );
     _splitter->addWidget(_listWidget);
     _splitter->addWidget(_stackWidget);
-
-    
+        
     connect( _listWidget , SIGNAL(currentRowChanged(int)) , this , SLOT(rowChanged(int)) ); 
 }
 
 ListViewContainer::~ListViewContainer()
 {
-    delete _listWidget;
-    delete _stackWidget;
-    delete _splitter;
+    _splitter->deleteLater();
 }
 
 QWidget* ListViewContainer::containerWidget() const
@@ -388,9 +390,13 @@ void ListViewContainer::setActiveView( QWidget* view )
 
 void ListViewContainer::rowChanged( int row )
 {
-    _stackWidget->setCurrentIndex( row );
+    // row may be -1 if the last row has been removed from the model
+    if ( row >= 0 )
+    {
+        _stackWidget->setCurrentIndex( row );
 
-    emit activeViewChanged( _stackWidget->currentWidget() );
+        emit activeViewChanged( _stackWidget->currentWidget() );
+    }
 }
 
 void ListViewContainer::updateTitle( ViewProperties* properties )

konsole/ViewContainer.h

@@ -66,7 +66,13 @@ Q_OBJECT
     
 public:
     ViewContainer(QObject* parent) : QObject(parent) {}
-    virtual ~ViewContainer() { emit destroyed(this); }
+
+    /** 
+     * Called when the ViewContainer is destroyed.  When reimplementing this in 
+     * subclasses, use object->deleteLater() to delete any widgets or other objects
+     * instead of 'delete object'.
+     */
+    virtual ~ViewContainer();
 
     /** Returns the widget which contains the view widgets */
     virtual QWidget* containerWidget() const = 0;
@@ -161,6 +167,7 @@ private slots:
     void showContextMenu(QWidget* widget , const QPoint& position);
     void currentTabChanged(int index);
 
+
 private:
     KTabWidget* _tabWidget; 
     QList<QAction*> _viewActions;

konsole/ViewManager.cpp

@@ -144,9 +144,15 @@ void ViewManager::focusActiveView()
     }
 }
 
+
 void ViewManager::viewActivated( QWidget* view )
 {
-    view->setFocus(Qt::MouseFocusReason);
+    Q_ASSERT( view != 0 );
+
+    // focus the activated view, this will cause the SessionController
+    // to notify the world that the view has been focused and the appropriate UI
+    // actions will be plugged in.
+    view->setFocus(Qt::OtherFocusReason);
 }
 
 void ViewManager::viewFocused( SessionController* controller )
@@ -163,6 +169,8 @@ void ViewManager::viewFocused( SessionController* controller )
         _mainWindow->setPlainCaption( controller->session()->displayTitle() );
 
         _pluggedController = controller;
+
+        kDebug() << "Plugged actions for " << controller->session()->displayTitle() << endl;
     }
 }
 
@@ -267,6 +275,8 @@ ViewContainer* ViewManager::createContainer()
     connect( container , SIGNAL(closeRequest(QWidget*)) , this , SLOT(viewCloseRequest(QWidget*)) );
 */
     ViewContainer* container = new ListViewContainer(_viewSplitter);
+
+    connect( container , SIGNAL(activeViewChanged(QWidget*)) , this , SLOT(viewActivated(QWidget*)));
     return container;
 }
 

konsole/ViewManager.h

@@ -42,6 +42,9 @@ class ViewSplitter;
  * KonsoleMainWindow widget passed as an argument to the constructor.
  *
  * To create new terminal displays inside the container widget, use the createView() method.
+ *
+ * ViewContainers can be merged together, that is, the views contained in one container can be moved
+ * into another using the merge() method.
  */
 class ViewManager : public QObject
 {
@@ -75,7 +78,9 @@ signals:
     void viewDetached(TESession* session);
 
 private slots:
+    // called when the "Split View" menu item is selected
     void splitView(bool splitView);
+    // called when the "Detach View" menu item is selected
     void detachActiveView();
     // called when a session terminates - the view manager will delete any
     // views associated with the session
@@ -83,7 +88,13 @@ private slots:
     // called when the container requests to close a particular view
     void viewCloseRequest(QWidget* widget);
 
+    // controller detects when an associated view is given the focus
+    // and emits a signal.  ViewManager listens for that signal
+    // and then plugs the action into the UI
     void viewFocused( SessionController* controller );
+
+    // called when the active view in a ViewContainer changes, so
+    // that we can plug the appropriate actions into the UI
     void viewActivated( QWidget* view );
 
 private:

konsole/ViewProperties.h

@@ -26,8 +26,11 @@
 
 
 /** 
- * Provides access to information such as the title and icon associated with a document
- * in a view container 
+ * Encapsulates user-visible information about the terminal session currently being displayed in a view,
+ * such as the icon and title associated with that session.
+ *
+ * This can be used by navigation widgets in a ViewContainer sub-class to provide a tab, label or other
+ * item for switching between views.
  */
 class ViewProperties : public QObject 
 {