[Okular-devel] Re: Meta Data Feature
Pino Toscano
pino at kde.org
Fri Jun 10 01:46:00 CEST 2011
Hi,
Alle sabato 28 maggio 2011, Christopher Reichert ha scritto:
> +#include <config-okular.h>
you don't need this
> + m_model = new QFileSystemModel( this );
please use a KDirModel
> + m_docdataDir =
> KStandardDirs().localkdedir().append("share/apps/okular/docdata"); +
this is doubly wrong!
please make use of the KStandardDirs of the okular kpart (ok, you cannot
access to it right now, so for now use the one in KGlobal), and use
KStandardDirs::saveLocation() for the apps path (IIRC it should be used
somewhere in part.cpp or document.cpp)
> + // Hide file size and type
> + m_dlg->MetaFileList->hideColumn( 1 );
> + m_dlg->MetaFileList->hideColumn( 2 );
you have a proxy model, so make that proxy do the column hiding job (not
sure it is needed that way with KDirModel)
> + m_dlg->MetaFileList->setColumnWidth( 0, 350);
why this hardcoded value?
> +
> + //m_dlg->MetaFileList->header()->setResizeMode(
> QHeaderView::Interactive );
> + //m_dlg->MetaFileList->header()->resize(10,20);
no need for commented code in new code
> + KGuiItem yesButton( i18n( "Yes" ), QString(),
> + i18n( "This is a tooltip" ),
> + i18n( "This is a WhatsThis help text." ) );
unused?
> + if ( KMessageBox::questionYesNo( 0, i18n( "Are you sure you want
> to do this?\n\n"
missing a parent widget ("this" can be enough) for the message box
> + "Warning: deleting
> meta data will result\n"
> + "in the loss of
> bookmarks and annotations." ),
there are no bookmarks in .xml files
> + i18n( "Warning " ),
trailing space in i18n() string text, and a bit too generic
> + KStandardGuiItem::yes(),
put a custom gui item with the "delete" text (or dig whether
KStandardGuiItem has one)
> + i18n( "Do ask me again" )) ==
> KMessageBox::Yes )
this would be a dangerous operation so
- add the dangerous operation to the questionYesNo() call
- do not add the "do not ask me" string
> + if ( KMessageBox::questionYesNo( 0, i18n( "Are you sure you want
> to do this?\n\n"
> + "Warning: deleting
> meta data will result\n"
> + "in the loss of
> bookmarks and annotations." ),
> + i18n( "Warning " ),
> + KStandardGuiItem::yes(),
> + KStandardGuiItem::no(),
> + i18n( "Do ask me again" )) ==
> KMessageBox::Yes )
same notes as above
> + QDirIterator it(m_docdataDir, QDir::Files |
> QDir::NoDotAndDotDot );
> + QDir dir(m_docdataDir);
> + while ( it.hasNext() )
> + {
> + dir.remove( it.next() );
> + }
this must remove only the xml files, not any arbitrary file which could
be there
> + if ( role == Qt::DisplayRole )
switch for the implemented roles
> + QRegExp rx("*.xml");
> + rx.setPatternSyntax(QRegExp::Wildcard);
> +
> + if ( rx.exactMatch( modifiedData.toString() ))
as it is, a regexp is just overkill, QString::endsWith() does the same
> + // beautify its display to the users
> + modifiedData =
> modifiedData.toString().remove(QRegExp(".xml$"));
> + modifiedData =
> modifiedData.toString().remove(QRegExp("^[0-9]+\."));
pretty inefficient; you're getting the string out of the variant, do a
removal of some text using two regexps (which are not needed), and then
converting to qvariant again; just get the string out once, do the
replacements and reassign back to the variant
> + if (role == Qt::SizeHintRole) {
> + if ( orientation == Qt::Horizontal )
> + {
> + switch (section)
> + {
> + // 0 == File Name; 3 == Modified
> + // --this sizes the actual Header size so it looks
> proper + case 0: case 3:
> + //return QSize(20,20);
> + default:
> + return QVariant();
> + }
> + }
> + }
i don't think you need to play with the size hint of the header
elements, just leave whatever the style does
> + else if ( role == Qt::DisplayRole ) {
> + if ( orientation == Qt::Horizontal )
> + {
> + switch (section)
> + {
> + case 0:
> + return i18n("File Name");
> + case 3:
> + return i18n("Modified");
> + default:
> + return QVariant();
> + }
> + }
> + }
check again whether this is still needed with KDirModel
> +#ifndef _DLGMETADATA_H
> +#define _DLGMETADATA_H
OKULAR_CONF_DLGMETADATA_H would be better
> +class QHeaderView;
> +class KMessageBox;
unneeded
> + protected:
> + void MetaDataExp(bool);
unneeded
> + Ui_DlgMetaDataBase *m_dlg;
this go to the private section
> + private slots:
> + void delMetaFile();
del_ete_MetaFile(), no gain in making it shorter
> + private:
> + KMessageBox *warningDlg;
> + QHeaderView *m_header;
unneeded
> Index: conf/dlgmetadatabase.ui
- you don't need a simple widget inside the groupbox, just lay out on
the groupbox itself
- the spacer next to the checkbox is not needed
- i'm not sure having both configuration and "handling" in the very same
ui is ideal... maybe one could rename the "personal identity"
configuration page to "personal data", and put just the metadata
checkbox there, while leaving the actual handling reachable from
somewhere else (settings -> handle (personal) meta data?)
> Index: core/document.cpp
> ===================================================================
> --- core/document.cpp (revision 1230563)
> +++ core/document.cpp (working copy)
> @@ -1027,6 +1027,16 @@
> int pageToKick = m_allocatedTextPagesFifo.takeFirst();
> m_pagesVector.at(pageToKick)->setTextPage( 0 ); // deletes
> the textpage }
> +
> + if ( Okular::Settings::saveMetaData() )
> + {
> + if ( m_saveBookmarksTimer )
> + {
> + m_saveBookmarksTimer->start( 5 * 60 * 1000 );
> + }
> + }
> + else if ( m_saveBookmarksTimer &&
> !Okular::Settings::saveMetaData() )
> + m_saveBookmarksTimer->stop();
two if() for the same condition... you can simplify it like
if (Okular::Settings::saveMetaData())
...
else
...
a couple of general notes:
- there are lots of trailing spaces around, please remove them
- not that the okular code shines for coding style, but at least please
keep one single style in your code...
--
Pino Toscano
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 190 bytes
Desc: This is a digitally signed message part.
Url : http://mail.kde.org/pipermail/okular-devel/attachments/20110610/84133182/attachment.sig
More information about the Okular-devel
mailing list