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

[Albert Astals Cid]

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


You seem to be missing a Message.sh file


active/CMakeLists.txt
<http://git.reviewboard.kde.org/r/106666/#comment15680>

    Adding in the comments section (don't remember which of the two "") how to get the ActiveApp seems a good idea and it'd maybe even get people to build it ;-)



active/app/active-documentviewer.desktop
<http://git.reviewboard.kde.org/r/106666/#comment15679>

    Forcing graphicssystem from the desktop file is a bit weird, what if i start it from the command line?



active/app/active-documentviewer.desktop
<http://git.reviewboard.kde.org/r/106666/#comment15678>

    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



active/app/package/contents/ui/Bookmarks.qml
<http://git.reviewboard.kde.org/r/106666/#comment15677>

    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?



active/app/package/contents/ui/main.qml
<http://git.reviewboard.kde.org/r/106666/#comment15681>

    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



active/app/src/main.cpp
<http://git.reviewboard.kde.org/r/106666/#comment15682>

    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.



active/components/documentitem.h
<http://git.reviewboard.kde.org/r/106666/#comment15683>

    I guess we need some docu here, what's matchingPages supposed to be?



active/components/documentitem.cpp
<http://git.reviewboard.kde.org/r/106666/#comment15684>

    const & here



active/components/documentitem.cpp
<http://git.reviewboard.kde.org/r/106666/#comment15685>

    Looks like we are losing some precision here by only returning the page number, no?



active/components/pageitem.cpp
<http://git.reviewboard.kde.org/r/106666/#comment15687>

    You sure this makes sense? I think pagepainter will set antialiasing at its own will



active/components/pageitem.cpp
<http://git.reviewboard.kde.org/r/106666/#comment15686>

    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


- Albert Astals Cid


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/20121001/9d963edd/attachment-0001.html>