[Okular-devel] Review Request: QML components and touch optimized okular version

Marco Martin notmart at gmail.com
Tue Oct 2 12:19:47 UTC 2012 


> On Oct. 1, 2012, 7:54 p.m., Albert Astals Cid wrote:
> > You seem to be missing a Message.sh file

i added a couple of messages.sh and catalogs.
org.kde.active.documentviewer.pot for the app, both qml and c++ parts
org.kde.okular.pot for the qml components: maybe that should be together okular.pot?


> On Oct. 1, 2012, 7:54 p.m., Albert Astals Cid wrote:
> > active/app/active-documentviewer.desktop, line 6
> > <http://git.reviewboard.kde.org/r/106666/diff/1/?file=87952#file87952line6>
> >
> >     Forcing graphicssystem from the desktop file is a bit weird, what if i start it from the command line?

ah, was an old workaround for meego, that should of course go


> On Oct. 1, 2012, 7:54 p.m., Albert Astals Cid wrote:
> > active/app/active-documentviewer.desktop, line 13
> > <http://git.reviewboard.kde.org/r/106666/diff/1/?file=87952#file87952line13>
> >
> >     This is not correct, active-documentviewer will only support pdf if the poppler generator is built, you should probably imitate the billions of *.desktop files we have in the generators folders

are they assembled in a single desktop file in the end?
what about generating a desktop file with the right mimetypes with cmake?


> On Oct. 1, 2012, 7:54 p.m., Albert Astals Cid wrote:
> > active/app/package/contents/ui/Bookmarks.qml, line 5
> > <http://git.reviewboard.kde.org/r/106666/diff/1/?file=87953#file87953line5>
> >
> >     The chosen license looks a bit strage given all the rest of Okular code is just GPL, is this a conscious selection or just something that happened by c&p?

i usually prefer lgpl for qml files to make it less of an hassle if some piece has to be moved in a component, but if here the preference is for gpl i can relicense it.

btw i can see quite some problems if the okular core library is going to be used around


> On Oct. 1, 2012, 7:54 p.m., Albert Astals Cid wrote:
> > active/app/package/contents/ui/main.qml, line 42
> > <http://git.reviewboard.kde.org/r/106666/diff/1/?file=87962#file87962line42>
> >
> >     Not sure if you are interested in following okular settings, but we have lots of options to define how the caption of the app has to look like

not sure if i would want configuration ui for it, but following the desktop app settings may be useful.
where they are stored, where i can read more about?


> On Oct. 1, 2012, 7:54 p.m., Albert Astals Cid wrote:
> > active/app/src/main.cpp, line 50
> > <http://git.reviewboard.kde.org/r/106666/diff/1/?file=87965#file87965line50>
> >
> >     Do we really need to expose that to the user? I mean would a user do
> >     
> >     active-documentviewer -h
> >     
> >     and then think "oh, let's use a QGLWidget"?
> >     
> >     I do think so.

yeah, shouldn't. that also come as an old workaround, may be dropped.


> On Oct. 1, 2012, 7:54 p.m., Albert Astals Cid wrote:
> > active/components/documentitem.h, line 47
> > <http://git.reviewboard.kde.org/r/106666/diff/1/?file=87967#file87967line47>
> >
> >     I guess we need some docu here, what's matchingPages supposed to be?

all pages that match a search term, i documented all the properties of the components now


> On Oct. 1, 2012, 7:54 p.m., Albert Astals Cid wrote:
> > active/components/documentitem.cpp, line 113
> > <http://git.reviewboard.kde.org/r/106666/diff/1/?file=87968#file87968line113>
> >
> >     Looks like we are losing some precision here by only returning the page number, no?

yes.
it was kept simple since the ui only used that.

it should get more complete (it has a page plus a qrect right?)
since qml can only understand qobjects, this should become either a list of qobjects, or probably a model, even just QStandardItemModel


> On Oct. 1, 2012, 7:54 p.m., Albert Astals Cid wrote:
> > active/components/pageitem.cpp, line 187
> > <http://git.reviewboard.kde.org/r/106666/diff/1/?file=87972#file87972line187>
> >
> >     You sure this makes sense? I think pagepainter will set antialiasing at its own will

ok, so this wouldn't work.
would be nice to be possible to tell pagepainter to not use antialiasing (to be consistent with usual api of qml, often items are drawn without aa when transforming, animating etc, to be a tad faster)


> On Oct. 1, 2012, 7:54 p.m., Albert Astals Cid wrote:
> > active/components/pageitem.cpp, line 201
> > <http://git.reviewboard.kde.org/r/106666/diff/1/?file=87972#file87972line201>
> >
> >     Oh, i see you are still using PagePainter, thus besides using okularcore you also use the API of a class, was never meant to be reused, that'll make it a bit more tricky for the desktop client development, but should not be that bad i guess
> >     
> >     Wait, you copied the code? That's a no go, i don't want to fix bugs twice in that huge file

that was in the "maybe i'll have to move this on aother repo" category ;)

i changed it now to use pagepainter from ui/


- Marco


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/106666/#review19712
-----------------------------------------------------------


On Oct. 1, 2012, 11:32 a.m., Marco Martin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106666/
> -----------------------------------------------------------
> 
> (Updated Oct. 1, 2012, 11:32 a.m.)
> 
> 
> Review request for Okular.
> 
> 
> Description
> -------
> 
> This patch provides some QML imports org.kde.okular.* to use some features of okularcore directly from qml (basically, components for documents and page rendering)
> 
> Then there is a small application, mostly QML, that use those components to build a document reader optimized for touch devices, used on Plasma Active
> 
> 
> Diffs
> -----
> 
>   CMakeLists.txt a057e82 
>   active/CMakeLists.txt PRE-CREATION 
>   active/app/CMakeLists.txt PRE-CREATION 
>   active/app/active-documentviewer.desktop PRE-CREATION 
>   active/app/package/contents/ui/Bookmarks.qml PRE-CREATION 
>   active/app/package/contents/ui/Browser.qml PRE-CREATION 
>   active/app/package/contents/ui/FullScreenDelegate.qml PRE-CREATION 
>   active/app/package/contents/ui/TableOfContents.qml PRE-CREATION 
>   active/app/package/contents/ui/Thumbnails.qml PRE-CREATION 
>   active/app/package/contents/ui/ThumbnailsBase.qml PRE-CREATION 
>   active/app/package/contents/ui/TreeDelegate.qml PRE-CREATION 
>   active/app/package/contents/ui/bookmark.png PRE-CREATION 
>   active/app/package/contents/ui/bookmark.svgz PRE-CREATION 
>   active/app/package/contents/ui/main.qml PRE-CREATION 
>   active/app/package/metadata.desktop PRE-CREATION 
>   active/app/src/CMakeLists.txt PRE-CREATION 
>   active/app/src/main.cpp PRE-CREATION 
>   active/components/CMakeLists.txt PRE-CREATION 
>   active/components/documentitem.h PRE-CREATION 
>   active/components/documentitem.cpp PRE-CREATION 
>   active/components/okularplugin.h PRE-CREATION 
>   active/components/okularplugin.cpp PRE-CREATION 
>   active/components/pageitem.h PRE-CREATION 
>   active/components/pageitem.cpp PRE-CREATION 
>   active/components/pagepainter.h PRE-CREATION 
>   active/components/pagepainter.cpp PRE-CREATION 
>   active/components/qmldir PRE-CREATION 
>   active/components/test.qml PRE-CREATION 
>   active/components/thumbnailitem.h PRE-CREATION 
>   active/components/thumbnailitem.cpp PRE-CREATION 
>   active/components/tocmodel.h PRE-CREATION 
>   active/components/tocmodel.cpp PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/106666/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Marco Martin
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/okular-devel/attachments/20121002/1572fb24/attachment-0001.html>

More information about the Okular-devel mailing list