[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kfm-devel
Subject:    Re: Problems with KHTMView (and KHTMLPart) constructors
From:       Germain Garand <germain () ebooksfrance ! org>
Date:       2007-12-16 23:49:12
Message-ID: 200712170049.12898.germain () ebooksfrance ! org
[Download RAW message or body]

Hi!

Le Mardi 11 Décembre 2007 14:07, Eeli Kaikkonen a écrit :
> Hi, I'm new here. I had some bugs which I wanted to talk about and I
> told about them in #khtml but nobody answered there. This is more than
> just one bug report, therefore I thought it would be proper to use this
> forum rather than bugs.kde.org.
>
> The heart of the issue is how the KHTMLPart and KHTMLView constructors
> work. It is not possible to set either Part for View later or vice
> versa. This is not usually a problem but if I need to subclass both of
> them it is.

I agree this is an embarrassing problem (along with the repeated breakage, 
thank you for your patience!)

We are not going to change the API at this point, but please check if the 
attached patch may address your concerns.

It adds:
1) the possibility of constructing a KHTMLView beforehand with a 0L part
   pointer. That pointer will be updated by the KHTMLPart constructor.
2) some documentation describing 1) and also view creation from 
   part constructor initialization list
3) a unit test asserting 1)
4) a fix for the early KHTMLGlobal usage problem

Comments and review by other list members welcome.

Greetings,
Germain

["patch" (text/x-diff)]

diff --git a/khtml/khtml_global.h b/khtml/khtml_global.h
index b0ef453..fd484cb 100644
--- a/khtml/khtml_global.h
+++ b/khtml/khtml_global.h
@@ -45,6 +45,8 @@ namespace DOM
  */
 class KHTML_EXPORT KHTMLGlobal
 {
+    friend class KHTMLViewPrivate;
+
 public:
     KHTMLGlobal();
     ~KHTMLGlobal();
diff --git a/khtml/khtml_part.cpp b/khtml/khtml_part.cpp
index f5fc5c4..2427f85 100644
--- a/khtml/khtml_part.cpp
+++ b/khtml/khtml_part.cpp
@@ -226,6 +226,7 @@ KHTMLPart::KHTMLPart( KHTMLView *view, QObject *parent, GUIProfile prof )
     // TODO KDE4 - don't load plugins yet
     //setComponentData( KHTMLGlobal::componentData(), false );
     assert( view );
+    view->setPart( this );
     init( view, prof );
 }
 
@@ -5726,7 +5727,7 @@ void KHTMLPart::setFontScaleFactor(int percent)
   if (d->m_fontScaleFactor == percent) return;
   d->m_fontScaleFactor = percent;
 
-  if(d->m_view) {
+  if (d->m_view && d->m_doc) {
     QApplication::setOverrideCursor( Qt::WaitCursor );
     if (d->m_doc->styleSelector())
       d->m_doc->styleSelector()->computeFontSizes(d->m_doc->logicalDpiY(), d->m_fontScaleFactor);
diff --git a/khtml/khtml_part.h b/khtml/khtml_part.h
index c2bc90f..df7f636 100644
--- a/khtml/khtml_part.h
+++ b/khtml/khtml_part.h
@@ -245,14 +245,31 @@ public:
    * holding the document data (DOM document), and the KHTMLView,
    * derived from QScrollView, in which the document content is
    * rendered in. You can specify two different parent objects for a
-   * KHTMLPart, one parent for the KHTMLPart document and on parent
+   * KHTMLPart, one parent for the KHTMLPart document and one parent
    * for the KHTMLView. If the second @p parent argument is 0L, then
    * @p parentWidget is used as parent for both objects, the part and
    * the view.
    */
   KHTMLPart( QWidget *parentWidget = 0,
              QObject *parent = 0, GUIProfile prof = DefaultGUI );
-
+  /**
+   * Constructs a new KHTMLPart.
+   *
+   * This constructor is useful if you wish to subclass KHTMLView.
+   * If the @p view passed  as first argument to the constructor was built with a
+   * null KHTMLPart pointer, then the newly created KHTMLPart will be assigned as the view's part.
+   *
+   * Therefore, you might either initialize the view as part of the initialization list of
+   * your derived KHTMLPart class constructor:
+   * \code
+   *   MyKHTMLPart() : KHTMLPart( new MyKHTMLView( this ), ...
+   * \endcode
+   * Or separately build the KHTMLView beforehand:
+   * \code
+   *   KHTMLView * v = KHTMLView( 0L, parentWidget());
+   *   KHTMLPart * p = KHTMLPart( v ); // p will be assigned to v, so that v->part() == p
+   * \endcode
+   */
   KHTMLPart( KHTMLView *view, QObject *parent = 0, GUIProfile prof = DefaultGUI );
 
   /**
diff --git a/khtml/khtmlview.cpp b/khtml/khtmlview.cpp
index d7c5113..464facc 100644
--- a/khtml/khtmlview.cpp
+++ b/khtml/khtmlview.cpp
@@ -247,9 +247,12 @@ public:
 	accessKeysActivated = false;
 	accessKeysPreActivate = false;
 
-        // defaultHTMLSettings is available since a KHTMLPart has been created first
-        // and has ref()'ed the factory for us.
+        // the view might have been built before the part it will be assigned to,
+        // so exceptionally, we need to directly ref/deref KHTMLGlobal to 
+        // account for this transitory case.
+        KHTMLGlobal::ref();
         accessKeysEnabled = KHTMLGlobal::defaultHTMLSettings()->accessKeysEnabled();
+        KHTMLGlobal::deref();
 
         emitCompletedAfterRepaint = CSNone;
         m_mouseEventsTarget = 0;
@@ -520,6 +523,12 @@ KHTMLView::~KHTMLView()
     delete d;
 }
 
+void KHTMLView::setPart(KHTMLPart *part)
+{
+    assert(part && !m_part);
+    m_part = part;
+}
+
 void KHTMLView::init()
 {
     // Do not access the part here. It might not be fully constructed.
diff --git a/khtml/khtmlview.h b/khtml/khtmlview.h
index 41ba192..85d3361 100644
--- a/khtml/khtmlview.h
+++ b/khtml/khtmlview.h
@@ -372,6 +372,8 @@ private:
 
     QStack<QRegion>* clipHolder() const;
     void setClipHolder( QStack<QRegion>* ch );
+    
+    void setPart(KHTMLPart *part);
 
     /**
      * Paints the HTML document to a QPainter.
diff --git a/khtml/testkhtml.cpp b/khtml/testkhtml.cpp
index 0acdd02..f480f51 100644
--- a/khtml/testkhtml.cpp
+++ b/khtml/testkhtml.cpp
@@ -123,7 +123,6 @@ int main(int argc, char *argv[])
 
 
     int ret = a.exec();
-    delete fac;
     return ret;
 }
 
diff --git a/khtml/tests/CMakeLists.txt b/khtml/tests/CMakeLists.txt
index 77bf289..150d0b3 100644
--- a/khtml/tests/CMakeLists.txt
+++ b/khtml/tests/CMakeLists.txt
@@ -1,6 +1,8 @@
 set( EXECUTABLE_OUTPUT_PATH ${CMAKE_CURRENT_BINARY_DIR} )
 
 include_directories(${KDE4_KPARTS_INCLUDES} ${CMAKE_CURRENT_SOURCE_DIR}/..)
+kde4_add_unit_test( khtmlparttest khtmlparttest.cpp )
+target_link_libraries( khtmlparttest ${QT_QTTEST_LIBRARY} ${KDE4_KDECORE_LIBS} khtml )
 
 # msvc linker doesn't like "#define protected public"
 if(NOT MSVC)
diff --git a/khtml/tests/khtmlparttest.cpp b/khtml/tests/khtmlparttest.cpp
new file mode 100644
index 0000000..36c3539
--- /dev/null
+++ b/khtml/tests/khtmlparttest.cpp
@@ -0,0 +1,49 @@
+/* This file is part of the KDE project
+ *
+ * Copyright (C) 2007 Germain Garand <germain@ebooksfrance.org>
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Library General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Library General Public License for more details.
+ *
+ * You should have received a copy of the GNU Library General Public License
+ * along with this library; see the file COPYING.LIB.  If not, write to
+ * the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
+ * Boston, MA 02110-1301, USA.
+ */
+
+#include <khtml_part.h>
+#include <qtest_kde.h>
+#include <khtmlview.h>
+#include "khtmlparttest.h"
+#include <csignal>
+#include <cstdlib>
+
+QTEST_KDEMAIN( KHTMLPartTest, GUI )
+
+void __abort(int) {
+    std::signal(SIGABRT, SIG_DFL);
+    std::signal(SIGSEGV, SIG_DFL);
+    QVERIFY( false );
+} 
+
+void KHTMLPartTest::initTestCase()
+{
+    std::signal(SIGABRT, &__abort);
+    std::signal(SIGSEGV, &__abort);
+}
+
+void KHTMLPartTest::testConstructKHTMLViewBeforePart()
+{
+    // test that a KHTMLView can be constructed before a KHTMLPart
+    m_view = new KHTMLView(0, 0);
+    m_part = new KHTMLPart(m_view);
+    QVERIFY(true);
+    QVERIFY(m_view->part() == m_part);
+}
diff --git a/khtml/tests/khtmlparttest.h b/khtml/tests/khtmlparttest.h
new file mode 100644
index 0000000..fa6df8d
--- /dev/null
+++ b/khtml/tests/khtmlparttest.h
@@ -0,0 +1,36 @@
+/* This file is part of the KDE project
+ *
+ * Copyright (C) 2007 Germain Garand <germain@ebooksfrance.org>
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Library General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Library General Public License for more details.
+ *
+ * You should have received a copy of the GNU Library General Public License
+ * along with this library; see the file COPYING.LIB.  If not, write to
+ * the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
+ * Boston, MA 02110-1301, USA.
+ */
+
+#include <QtCore/QObject>
+
+class KHTMLPart;
+class KHTMLView;
+
+class KHTMLPartTest : public QObject
+{
+Q_OBJECT
+private Q_SLOTS:
+    void initTestCase();
+    void testConstructKHTMLViewBeforePart();
+
+private:
+    KHTMLPart* m_part;
+    KHTMLView* m_view;
+};


[prev in list] [next in list] [prev in thread] [next in thread]