[Okular-devel] Re: Meta Data Feature

[Christopher Reichert]

Pino,

Thank you for the time you took to review the code. I think I may have sent
an outdated patch as Albert addressed a few of these issues with me as well.
However, every other point you bring up is valid.


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



>
> > +        // 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. 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?

>> +                                     i18n( "Warning " ),

> + trailing space in i18n() string text, and a bit too generic

Hmmm, any suggestions?


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




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

I understand what you mean and this is why I have held off a little on
fixing some of the issues that will take a longer time. Does anybody else
have any thoughts on this?
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/okular-devel/attachments/20110612/deb0186b/attachment-0001.htm 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: metadata.patch
Type: text/x-patch
Size: 20084 bytes
Desc: not available
Url : http://mail.kde.org/pipermail/okular-devel/attachments/20110612/deb0186b/attachment-0001.patch