[Okular-devel] Re: Meta Data Feature

[Pino Toscano]

Alle domenica 12 giugno 2011, Christopher Reichert ha scritto:
> > > +    m_model = new QFileSystemModel( this );
> > 
> > please use a KDirModel
> 
> I just want to know why? Maybe KDirModel may be a better KDE api
> option but its not necessarily easy to set up. This would require me
> to reimplement quite a few things in the dialog( which is not a
> problem I just want to know that its justified). If a KDirModel is
> necessary then I will definitely need more time.

Usually KDE classes are more controllable in their behaviour, there's 
less risk for unexpected behaviour change.
What are the problems with KDirModel?

> > > +        // 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)
> 
> Because the proxy model would have to implement catching these which
> would have taken a few more lines of code.

see QSortFilterProxyModel::filterAcceptsColumn()

> That's not a big deal but
> I will wait until the KDirModel.
> 
> > > +        m_dlg->MetaFileList->setColumnWidth( 0, 350);
> > 
> > why this hardcoded value?
> 
> This has been bugging me for a while. I cant get the view to resize
> the table to contents efficiently. The resizeColumnsToContents() is
> a slot I havent quite figured out yet. Any suggestions?

a slot is just a normal function which is also available in the 
metaobject of the class it belongs to. you can call it straight away

> >> +                                     i18n( "Warning " ),
> > 
> > + trailing space in i18n() string text, and a bit too generic
> 
> Hmmm, any suggestions?

i18n("Metadata Deletion")

> > > +            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
> 
> I have refactored this process a bit. While I agree that a regular
> expression may be heavy duty it does guarantees a match and delete of
> a file size of varying digit lengths as well as a .xml that __has__
> to be at the end.

again, you don't really need a regexp for this. let's pick your last 
version:
  if (  fileName.endsWith( ".xml" ) ) 
  {
    // remove the prepensions and extensions from meta data filename to
    // beautify its display to the user
    fileName.remove( QRegExp(".xml$") );
    fileName.remove( QRegExp("^[0-9]+\\.") );
    return fileName;
  }

if a fileName enters this if(), it _will_ have ".xml" at the end 
already, so you don't need to enforce it again.
- a proper QString::chop(4) will just remove the ending
- a proper loop from the start skipping digits until there's a dot will
  do the deletion job done by the other regexp
(or, even better, do a proper QString::mid() to extract the part and cut 
prefix and extension at once)
sure, the code will be slightly longer, but much faster than with two 
regexp (which does matter in the data() function of a model, which 
should be as much fast as possible to avoid making any view using it 
slow as molasses)

> +    m_docdataDir = KStandardDirs().saveLocation( "data", 
"okular/docdata", false );

still slow, please use KGlobal::dirs()

> +    return sourceModel()->data( mapToSource( index ), role );

just call the base QSortFilterProxyModel::data() instead

> +    protected:
> +        void MetaDataExp(bool);

unimplemented, wrong naming (=> remove)

> +        MetaFileProxyModel( QObject *parent);

make the parent as optional argument (= 0), uniformity with qobject's

plus,
* there are trailing spaces in a couple of lines of the patch, please
  remove them
* ui file: there is one signal connection done in the ui and one in the
  code: please do both in the same place, otherwise it can be confusing
* for i18n strings and strings in ui file: please follow the proper
  capitalization, see [1] 

[1] http://techbase.kde.org/Projects/Usability/HIG/Capitalization

-- 
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/20110619/15d26783/attachment.sig