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

List:       koffice-devel
Subject:    Re: Patch for KPresenter (Rename page title )
From:       David Faure <david () mandrakesoft ! com>
Date:       2001-05-05 14:56:47
[Download RAW message or body]

On Saturday 05 May 2001 12:12, Toshitaka Fujioka wrote:
> I'm sorry, I did not understand intention of your question.
> "savePageTitle" saves a title of a page to copy temporarily.
> I don't store this part in XML.
> It is "QDomElement KPresenterDoc::saveTitle( QDomDocument &doc )" that
> save page title in XML.

Yes, so the question is, what is savePageTitle for ?
+    // For "Copy Page" and "Duplicate Page"
Well, that's what I thought : it's a hack. If you copy page and
paste into another document, it won't work. That's why I was referring
to saving the title as part of the XML - and loading it, of course.
You might want to use saveOnlyPage in completeSaving, to only save
the title of the current page. That's what is done for everything
else upon saving (saveOnlyPage is -1 for "all pages", otherwise it's
the page to save). Just check completeSaving is called, I'm not sure.

> "savePageTitle" rename "saveCurrentPageTitle".
> 
> I attaced patch. (CVS of 05/05)
> If a patch is wrong, please advise.

Sorry I still see many things not right in that patch :(

1 - please use KDialogBase for dialogs. It provides the buttons,
and the slots, etc. And use QVBoxLayout( this, KDialog::marginHint(), KDialog::spacingHint() )
or even KDialogBase::makeVBoxMainWidget() ... see KDialogBase's docu,
it has all you need and more :)

Maybe you can even use KLineEditDlg in fact, if you only need
one edit line in the dialog. This way you can completely remove
the new files - and still have the same functionality :)

2 - 
+void KPresenterDoc::loadTitle( const QDomElement &element )
+{
+    QDomElement title=element.firstChild().toElement();
+    while ( !title.isNull() ) {
+        if ( title.tagName()=="Title" ) {
+            if ( title.hasAttribute("title") )
+                manualTitleList.append(title.attribute("title"));
+        }
+        title=title.nextSibling().toElement();
+    }
This is going to break if some pages have a manual title and some not
I'd remove the hasAttribute() test, so that an empty string is
appended in the case where the page have an automatic title.

3-
+        QString title = manualTitleList[pgNum];
+        (*manualTitleList.at(pgNum)) = title;
The second line is obviously useless.

4-
+     // A count of manualTitleList doubles. Maybe bug of QStringList or my mistake.
+    if ( getPageNums() <= manualTitleList.count() ) {
+        for ( unsigned int i = manualTitleList.count(); i > getPageNums()-1; --i )
+            manualTitleList.remove( manualTitleList.fromLast() );
+    }
Is this really necessary ?

5-
+    // Initialization of manualTitleList
+    // for version before adding this special feature (save/load page title to file)
+    if ( doc->manualTitleList.isEmpty() ) {
+        for ( unsigned int i = 0; i <= doc->getPageNums() - 1; ++i )
+            doc->manualTitleList.append(QString::null);
+    }
Hmm, I'd rather do that upon loading. Either the title list was found,
or we need to do the above. Ok, not very important, just cleaner.

6-
SideBar::renamePageTitle is a bit complex. If tndlg.exec() returned false,
we don't need to rebuild the items.
And if it returned true, why not simply do
+                (*doc->manualTitleList.at(pageNumber)) = renameTitle;
before the for() loop ?
Then you don't even need the for() loop at all, you can simply call updateItem(pagenr)
and it will update the item in the list - removing much code duplication
(and many variables in renamePageTitle()).


Other than that, it's starting to look good :)
Sorry that it's so hard to get it right :)

-- 
David FAURE, david@mandrakesoft.com, faure@kde.org
http://perso.mandrakesoft.com/~david/, http://www.konqueror.org/
KDE, Making The Future of Computing Available Today
_______________________________________________
Koffice-devel mailing list
Koffice-devel@master.kde.org
http://master.kde.org/mailman/listinfo/koffice-devel

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