ubuntu-touch-coreapps-reviewers team mailing list archive

[Stefano Verzegnassi <stefano92.100@xxxxxxxxx>]

Review: Needs Fixing

I found a few things that require a fix:

  * I'd prefer to use the new PageHeader, instead of the "old" page.head property.
    e.g. http://paste.ubuntu.com/14495479/

  * We'd probably need to display the page number of the current page.[1] In the code I posted above, it would be a matter of adding the "subtitle" property in the ListItemLayout.

  * I'm not sure of the background color. Shouldn't it be black?[1]

  * If the night-mode is active, it affects also the presentation view. It should be disabled when the user plays a presentation.

  * I've seen some strange output from PdfViewDelegate. Connections to the pinch area should be disabled when the component is used from the presentation view (e.g. using a bool).

  * The ShaderEffectSource in PdfViewDelegate should be disabled when in presentation mode, otherwise a blurry render of the page is visible after resizing the window[2]

  * Just a cosmetic fix, I'd use the "slideshow" icon, instead of "media-playback-start".


---

I've pushed the patch for these things in a separate branch:
    https://code.launchpad.net/~verzegnassi-stefano/ubuntu-docviewer-app/pdf-presentations-mp-review


I have still some doubt about the mouse/touch gestures.
While the "double-click-to-show-header" gesture looks good to me, I'm not sure about the "swipe-to-go-prev/next" touch gesture.
I'd probably prefer to split the mouseArea into three parts, as done in some Google application:
    https://drive.google.com/open?id=0By4kAplbFcE6QmJlR1ZydlVCSDA
Anyway, this is not important at the moment; I'm okay with the current solution.


Overall, your MP looks good, and it's pretty similar to how I was thinking of doing this. Big thumb up and thank you very much!


======

[1] My design proposal: https://drive.google.com/open?id=0By4kAplbFcE6MDNJMjBmNklVY28
     Current design: https://drive.google.com/open?id=0By4kAplbFcE6QWF4cXVfQm9uR28

[2] screenshot - https://drive.google.com/open?id=0By4kAplbFcE6RXh6Q2NELWlJbjg
-- 
https://code.launchpad.net/~mhall119/ubuntu-docviewer-app/pdf-presentations/+merge/282512
Your team Ubuntu Document Viewer Developers is subscribed to branch lp:ubuntu-docviewer-app.