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

List:       kde-core-devel
Subject:    Re: KParts::ReadWritePart
From:       David Faure <david () mandrakesoft ! com>
Date:       2002-01-24 15:59:26
[Download RAW message or body]

On Thursday 24 January 2002 15:31, Matthias Kretz wrote:
> Hi,
> 
> I'm currently working on a ReadWritePart and I have some questions to the
> code:
> - What is m_bClosing good for?
As the comment above it says: to remove the local tempfile after uploading
it, if we're closing the document.

> First, it will be set to true once and never back to false.
Oops. Should be set to false again at the end of slotUploadFinished (unconditionnally). 
Hmm, not good enough (for local files this isn't called). Reimplemented openURL 
and set it to false there.

> Second, using this, ReadOnlyPart::closeURL() will get called
> twice.
Ouch! There's indeed a bug. Fixed, see patch attached.

> May I remove this flag or is it doing something I didn't see yet?
We can't remove it. We need to call to remove the temp file _after_ uploading, 
not before - there might be a race condition otherwise.

> - m_bModified is not set back to false if a new document is opened and the old
> one wasn't saved. Should the ReadWritePart have it's own openURL() method or
> should I just call setModified( false ) in ReadWritePart::closeURL() after
> case KMessageBox::No :?

Since I just added an openURL reimplementation it's easy to set modified to 
false there too. Hmm, no, we can't do that, we have to let 
ReadOnlyPart::openURL call closeURL() first (which can trigger the 
"want to save?" dialog if modified).

"Cheating" after selecting No sounds easier. Done.

The patch for the above fixes is attached.
Untested. Can you test and commit if it works ?

KOffice is rather unaffected since it reimplements most of this stuff for other reasons.
But... does Kate use KParts::ReadWritePart ?

> - The Message Box shows Yes, No, Cancel. While this is a standart KMessageBox
> it has the same icons for No and Cancel. I would like to change it to
> KStdGuiItem::save() and KStdGuiItem::dontSave() (or should this be
> KStdGuiItem::discard()? ). Or what about:
> int res = m_url.isEmpty() ?
>       // not only changes, but a new document
>       KMessageBox::warningYesNoCancel( widget(),
>           i18n( "This is a new document.\nDo you want to save it ?" ),
>           QString::null, KStdGuiItem::saveAs(), KStdGuiItem::discard() ) :
>       // document already exists but was changed
>       KMessageBox::warningYesNoCancel( widget(),
>           i18n( "The document has been modified.\nDo you want to save it ?" ),
>           QString::null, KStdGuiItem::save(), KStdGuiItem::dontSave() );

No clue about KStdGuiItem, hope Martijn will reply.

-- 
David FAURE, david@mandrakesoft.com, faure@kde.org
http://people.mandrakesoft.com/~david, http://www.konqueror.org
KDE 3.0: Konquering the Desktops

["kparts.diff" (text/x-diff)]

? tests.chapter
Index: part.cpp
===================================================================
RCS file: /home/kde/kdelibs/kparts/part.cpp,v
retrieving revision 1.105
diff -u -p -r1.105 part.cpp
--- part.cpp	2002/01/21 00:08:13	1.105
+++ part.cpp	2002/01/24 15:58:41
@@ -431,6 +431,12 @@ void ReadWritePart::setModified()
   setModified( true );
 }
 
+bool ReadWritePart::openURL( const KURL &url )
+{
+    m_bClosing = false;
+    return ReadOnlyPart::openURL( url );
+}
+
 bool ReadWritePart::closeURL()
 {
   abortLoad(); //just in case
@@ -446,10 +452,11 @@ bool ReadWritePart::closeURL()
       {
           KURL url = KFileDialog::getSaveURL();
           if (url.isEmpty()) return false;
-          return saveAs( url ) && ReadOnlyPart::closeURL();
+          return saveAs( url );
       }
-      return save() && ReadOnlyPart::closeURL();
+      return save();
     case KMessageBox::No :
+      setModified( false ); // the user isn't interested in the changes, forget them
       return true;
     default : // case KMessageBox::Cancel :
       return false;
@@ -503,6 +510,11 @@ bool ReadWritePart::saveToURL()
   {
     setModified( false );
     emit completed();
+    if ( m_bClosing && m_bTemp ) // We're finished with this document -> remove temp file
+    {
+      unlink( QFile::encodeName(m_file) );
+      m_bTemp = false;
+    }
     return true; // Nothing to do
   }
   else
@@ -520,8 +532,11 @@ void ReadWritePart::slotUploadFinished( 
   else
   {
     setModified( false );
-    if ( m_bClosing )
-      ReadOnlyPart::closeURL();
+    if ( m_bClosing && m_bTemp ) // We're finished with this document -> remove temp file
+    {
+      unlink( QFile::encodeName(m_file) );
+      m_bTemp = false;
+    }
     emit completed();
   }
 }
Index: part.h
===================================================================
RCS file: /home/kde/kdelibs/kparts/part.h,v
retrieving revision 1.87
diff -u -p -r1.87 part.h
--- part.h	2001/10/26 21:54:45	1.87
+++ part.h	2002/01/24 15:58:41
@@ -480,6 +480,11 @@ public:
 
 public slots:
   /**
+   * Reimplemented for internal reasons, see @ref KParts::ReadOnlyPart::openURL
+   */
+  virtual bool openURL( const KURL &url );
+
+  /**
    * Call @ref setModified() whenever the contents get modified.
    * This is a slot for convenience, so that you can connect it
    * to a signal, like textChanged().


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