ubuntu-touch-coreapps-reviewers team mailing list archive

[Nicholas Skaggs <nskaggs@xxxxxxxxxx>]

Review: Needs Fixing

Some comments :-)

Diff comments:

> === modified file 'po/com.ubuntu.docviewer.pot'
> --- po/com.ubuntu.docviewer.pot	2015-04-27 16:02:40 +0000
> +++ po/com.ubuntu.docviewer.pot	2015-05-04 20:16:06 +0000
> @@ -8,7 +8,7 @@
>  msgstr ""
>  "Project-Id-Version: \n"
>  "Report-Msgid-Bugs-To: \n"
> -"POT-Creation-Date: 2015-04-27 18:02+0200\n"
> +"POT-Creation-Date: 2015-05-04 21:48+0200\n"
>  "PO-Revision-Date: YEAR-MO-DA HO:MI+ZONE\n"
>  "Last-Translator: FULL NAME <EMAIL@ADDRESS>\n"
>  "Language-Team: LANGUAGE <LL@xxxxxx>\n"
> @@ -34,7 +34,7 @@
>  
>  #: ../src/app/docviewer-application.cpp:164
>  #: ../src/app/qml/documentPage/DocumentPage.qml:25
> -#: /home/stefano/tmp/build-ch-imported-documents-name-Desktop-Default/po/com.ubuntu.docviewer.desktop.in.in.h:1
> +#: /home/letozaf/autopilot-tests/build-test-toc-Desktop-Default/po/com.ubuntu.docviewer.desktop.in.in.h:1
>  msgid "Document Viewer"
>  msgstr ""
>  
> @@ -257,12 +257,12 @@
>  msgstr ""
>  
>  #. TRANSLATORS: "Contents" refers to the "Table of Contents" of a PDF document.
> -#: ../src/app/qml/pdfView/PdfContentsPage.qml:26
> +#: ../src/app/qml/pdfView/PdfContentsPage.qml:32
>  #: ../src/app/qml/pdfView/PdfView.qml:37
>  msgid "Contents"
>  msgstr ""
>  
> -#: ../src/app/qml/pdfView/PdfContentsPage.qml:32
> +#: ../src/app/qml/pdfView/PdfContentsPage.qml:38
>  msgid "Hide table of contents"
>  msgstr ""
>  
> @@ -319,6 +319,6 @@
>  msgid "Open"
>  msgstr ""
>  
> -#: /home/stefano/tmp/build-ch-imported-documents-name-Desktop-Default/po/com.ubuntu.docviewer.desktop.in.in.h:2
> +#: /home/letozaf/autopilot-tests/build-test-toc-Desktop-Default/po/com.ubuntu.docviewer.desktop.in.in.h:2
>  msgid "documents;viewer;pdf;reader;"
>  msgstr ""
> 
> === modified file 'src/app/qml/pdfView/PdfContentsPage.qml'
> --- src/app/qml/pdfView/PdfContentsPage.qml	2015-04-16 12:58:29 +0000
> +++ src/app/qml/pdfView/PdfContentsPage.qml	2015-05-04 20:16:06 +0000
> @@ -22,6 +22,12 @@
>  import "../upstreamComponents"
>  
>  Page {
> +    id: pdfContents
> +    objectName: "pdfcontents"
> +
> +    // this property will have to be removed when bug #1341671 will be fixed.
> +    property string testProperty: "for page name issue"
> +
>      // TRANSLATORS: "Contents" refers to the "Table of Contents" of a PDF document.
>      title: i18n.tr("Contents")
>  
> @@ -57,6 +63,7 @@
>  
>      ListView {
>          id: view
> +        objectName: "view"
>          anchors.fill: parent
>          clip: true
>  
> @@ -64,6 +71,7 @@
>  
>          delegate: ListItemWithActions {
>              id: delegate
> +            objectName: "delegate" + index
>  
>              width: parent.width
>              height: (model.level === 0) ? units.gu(7) : units.gu(6)
> @@ -73,6 +81,7 @@
>                                                        : Theme.palette.normal.background
>  
>              AbstractButton {
> +                objectName: "abstractbutton"
>                  anchors.fill: parent
>  
>                  onClicked: {
> @@ -91,6 +100,7 @@
>                  spacing: units.gu(1)
>  
>                  Label {
> +                    objectName: "content"
>                      Layout.fillWidth: true
>  
>                      text: model.title
> @@ -102,6 +112,7 @@
>                  }
>  
>                  Label {
> +                    objectName: "pageindex"
>                      text: model.pageIndex + 1
>                      font.weight: model.level == 0 ? Font.DemiBold : Font.Normal
>                      color: (model.level === 0) ? UbuntuColors.midAubergine
> 
> === modified file 'tests/autopilot/ubuntu_docviewer_app/__init__.py'
> --- tests/autopilot/ubuntu_docviewer_app/__init__.py	2015-04-14 15:37:06 +0000
> +++ tests/autopilot/ubuntu_docviewer_app/__init__.py	2015-05-04 20:16:06 +0000
> @@ -20,6 +20,7 @@
>  from autopilot import logging as autopilot_logging
>  logger = logging.getLogger(__name__)
>  
> +from autopilot.introspection import dbus
>  import ubuntuuitoolkit
>  
>  
> @@ -58,10 +59,20 @@
>          return self.wait_select_single(PdfView)
>  
>      @autopilot_logging.log_action(logger.info)
> +    def open_PdfContentsPage(self):
> +        """Open the PdfContents Page.
> +
> +        :return the PdfContents Page
> +
> +        """
> +        return self.wait_select_single(PdfContentsPage)
> +
> +    @autopilot_logging.log_action(logger.info)
>      def get_PdfViewGotoDialog(self):
>          """Return a dialog emulator"""
>          return self.wait_select_single(objectName="PdfViewGotoDialog")
>  
> +    @autopilot_logging.log_action(logger.info)
>      def go_to_page_from_dialog(self, page_no):
>          """ Go to page from get_PfdViewGotoDialog """
>          textfield = self.wait_select_single(
> @@ -70,6 +81,12 @@
>          go_button = self.wait_select_single("Button", objectName="GOButton")
>          self.pointing_device.click_object(go_button)
>  
> +    @autopilot_logging.log_action(logger.info)
> +    def click_go_to_page_button(self):
> +        """Click the go_to_page header button."""
> +        header = self.get_header()
> +        header.click_action_button('gotopage')
> +
>  
>  class Page(ubuntuuitoolkit.UbuntuUIToolkitCustomProxyObjectBase):
>  
> @@ -82,15 +99,92 @@
>          self.main_view = self.get_root_instance().select_single(MainView)
>  
>  
> -class PdfView(Page):
> +class PageWithBottomEdge(MainView):
> +    """
> +    An emulator class that makes it easy to interact with the bottom edge
> +    swipe page
> +    """
> +    def __init__(self, *args):
> +        super(PageWithBottomEdge, self).__init__(*args)
> +
> +    def reveal_bottom_edge_page(self):
> +        """Bring the bottom edge page to the screen"""
> +        self.bottomEdgePageLoaded.wait_for(True)
> +        try:
> +            action_item = self.wait_select_single(objectName='bottomEdgeTip')
> +            action_item.visible.wait_for(True)
> +            action_item.stretched.wait_for(True)
> +            start_x = (action_item.globalRect.x +
> +                       (action_item.globalRect.width * 0.5))
> +            start_y = (action_item.globalRect.y + 1)
> +            stop_y = start_y - (self.height * 0.5)
> +            self.pointing_device.drag(start_x, start_y,
> +                                      start_x, stop_y, rate=2)
> +            self.isReady.wait_for(True)
> +        except dbus.StateNotFoundError:
> +            logger.error('BottomEdge element not found.')
> +            raise
> +
> +
> +class PdfView(PageWithBottomEdge):
>      """Autopilot helper for PdfView page."""
>  
>      @autopilot_logging.log_action(logger.info)
>      def toggle_header_visibility(self):
>          """Show/hide page header by clicking on the center of main view"""
> -        self.pointing_device.click_object(self.main_view)
> -
> -    def click_go_to_page_button(self):
> -        """Click the go_to_page header button."""
> -        header = self.main_view.get_header()
> -        header.click_action_button('gotopage')
> +        self.pointing_device.click_object(self)
> +
> +    def get_currentpage_number(self):
> +        """return the value of the currentPage property"""
> +        logger.warn(self.currentPage)
> +        return self.currentPage
> +
> +
> +class PdfContentsPage(Page):
> +    """Autopilot helper for PdfContents page."""
> +
> +    @autopilot_logging.log_action(logger.info)
> +    def get_content_and_line_pageindex(self, labelText):
> +        content_line, page_no = self._get_listitem(labelText)
> +        return content_line, page_no
> +
> +    def _get_listitem(self, labelText):
> +        view_item = self.select_single(
> +            "QQuickListView", objectName="view")
> +        list_items_count = view_item.count
> +
> +        index = 0
> +        for index in range(list_items_count):
> +            while True:

this loop is a little concerning, as it will never end. I don't see why it's needed at all actually.

> +                try:
> +                    list_item = self.select_single(
> +                        "ListItemWithActions", objectName="delegate{}".
> +                        format(index))
> +                    while list_item.y > (view_item.contentY +

This too should have a control variable that will break the loop if needed.

> +                                         view_item.globalRect.height):
> +                            self.scroll_pdfcontentspage()
> +                    break
> +                except dbus.StateNotFoundError:
> +                    self.scroll_pdfcontentspage()
> +            label = list_item.select_single("Label", objectName="content")
> +            if label.text == labelText:
> +                page_no = list_item.select_single(
> +                    "Label", objectName="pageindex").text
> +                return label, page_no
> +                break
> +
> +    @autopilot_logging.log_action(logger.info)
> +    def click_content_line(self, content_line):
> +        self.pointing_device.click_object(content_line)
> +
> +    @autopilot_logging.log_action(logger.info)
> +    def scroll_pdfcontentspage(self):
> +        action_item = self.select_single("QQuickListView")
> +        start_x = (action_item.globalRect.x +
> +                   (action_item.globalRect.width * 0.5))
> +        start_y = (action_item.globalRect.y +
> +                   (action_item.height * 0.8))
> +        stop_y = start_y - (action_item.height * 0.7)
> +        self.pointing_device.drag(start_x, start_y,
> +                                  start_x, stop_y, rate=2)
> +        action_item.moving.wait_for(False)
> 
> === added file 'tests/autopilot/ubuntu_docviewer_app/files/serverguide.pdf'
> Binary files tests/autopilot/ubuntu_docviewer_app/files/serverguide.pdf	1970-01-01 00:00:00 +0000 and tests/autopilot/ubuntu_docviewer_app/files/serverguide.pdf	2015-05-04 20:16:06 +0000 differ
> === modified file 'tests/autopilot/ubuntu_docviewer_app/tests/test_docviewer.py'
> --- tests/autopilot/ubuntu_docviewer_app/tests/test_docviewer.py	2015-04-14 15:37:06 +0000
> +++ tests/autopilot/ubuntu_docviewer_app/tests/test_docviewer.py	2015-05-04 20:16:06 +0000
> @@ -60,7 +60,7 @@
>          self.launch_app()
>          pdf = self.app.main_view.open_PdfView()
>          pdf.toggle_header_visibility()
> -        pdf.click_go_to_page_button()
> +        self.app.main_view.click_go_to_page_button()
>          self.go_to_page_no(page_no)
>  
>          self.assertThat(
> 
> === added file 'tests/autopilot/ubuntu_docviewer_app/tests/test_toc.py'
> --- tests/autopilot/ubuntu_docviewer_app/tests/test_toc.py	1970-01-01 00:00:00 +0000
> +++ tests/autopilot/ubuntu_docviewer_app/tests/test_toc.py	2015-05-04 20:16:06 +0000
> @@ -0,0 +1,49 @@
> +# -*- Mode: Python; coding: utf-8; indent-tabs-mode: nil; tab-width: 4 -*-
> +# Copyright 2013 Canonical
> +#
> +# This program is free software: you can redistribute it and/or modify it
> +# under the terms of the GNU General Public License version 3, as published
> +# by the Free Software Foundation.
> +
> +"""Docviewer app autopilot tests."""
> +
> +from ubuntu_docviewer_app.tests import DocviewerAppTestCase
> +
> +import logging
> +logger = logging.getLogger(__name__)
> +
> +
> +class TestMainWindow(DocviewerAppTestCase):
> +
> +    """Tests the TOC features"""
> +    scenarios = [
> +        ('Chapter 1',
> +            {'content_label': 'Chapter 1. Introduction'
> +             }),
> +
> +        ('Chapter 3',
> +            {'content_label': 'Chapter 3. Package Management',
> +             })
> +    ]
> +
> +    def setUp(self):
> +        super(TestMainWindow, self).setUp()
> +
> +    def test_go_to_chapters_in_toc(self):
> +        """" Testing going to chapters from Table Of Contents     """
> +        self.filepath = 'ubuntu_docviewer_app/files/serverguide.pdf'

will this be used by other tests? If so, I would move it into setup or inside the class itself.

> +
> +        self.launch_app()
> +        pdf = self.app.main_view.open_PdfView()
> +        pdf.reveal_bottom_edge_page()
> +        contents_page = self.app.main_view.open_PdfContentsPage()
> +        content_line, page_no = contents_page.\
> +            get_content_and_line_pageindex(self.content_label)
> +        logger.warn(page_no)

why a warning? is this a debug line? Is it still needed?

> +        contents_page.click_content_line(content_line)
> +
> +        word_in_currentpage = pdf.get_currentpage_number().split()
> +        pdfview_currentpage = word_in_currentpage[1]
> +        logger.warn(pdfview_currentpage)

why a warning? is this a debug line? Is it still needed?

> +
> +        self.assertEquals(pdfview_currentpage, page_no)
> 


-- 
https://code.launchpad.net/~carla-sella/ubuntu-docviewer-app/test-toc/+merge/258082
Your team Ubuntu Document Viewer Developers is subscribed to branch lp:ubuntu-docviewer-app.