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

List:       koffice-devel
Subject:    Re: Patch for KPresenter (Rename page title )
From:       Toshitaka Fujioka <toshitaka () kde ! gr ! jp>
Date:       2001-05-08 21:59:38
[Download RAW message or body]

On Sunday 06 May 2001 00:53, David Faure wrote:
> 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.

Probably I think that I was able to fix (probably ...).

> > "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 :)

Widget (e.g. Label) was plied up when I used KDialogBase and didn't work fine.
(Probably it is my mistake.)

It worked fine when I used KDialog.

> 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 :)

Works fine.

Is a patch of KDialog or KLineEditDlg which good? 

> 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.

void KPresenterDoc::loadTitle( const QDomElement &element )
{
    QDomElement title=element.firstChild().toElement();
    while ( !title.isNull() ) {
        if ( title.tagName()=="Title" ) {
	    if ( title.hasAttribute("title") ) {
	        if ( !(title.attribute("title")).isEmpty() )
		    manualTitleList.append(title.attribute("title"));
                     else
		    manualTitleList.append(QString::null);
	    }
        }
        title=title.nextSibling().toElement();
    }
}

Is it OK?

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

I see, fixed.

> 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 ?

I'm sorry. This was my mistake. :-(
This problem was fixed when I changed a tag name of XML in "PAGE_TITLE" 
from "PAGETITLES".  "_" was problem.
When loadNativeFormat(...) was called, false returned with title.isNull().

> 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.

I changed in loadXML( const QDomDocument &doc ) of "kpresenter_doc.cc".

> 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()).

I see, fixed.

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

Thank you.
_______________________________________________
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]