gallery-app

Merge lp:~artmello/gallery-app/gallery-app-remove_photo_from_album into lp:gallery-app

gallery-app-remove_photo_from_album
Merge into trunk

Proposed by Arthur Mello on 2014-06-11

Status:

Merged

Approved by:

Bill Filler on 2014-06-18

Approved revision:

1002

Merged at revision:

998

Proposed branch:

lp:~artmello/gallery-app/gallery-app-remove_photo_from_album

Merge into:

lp:gallery-app

Diff against target:

383 lines (+242/-9)

6 files modified

rc/qml/MediaViewer/MediaViewer.qml (+49/-5)
src/qml/qml-media-collection-model.cpp (+18/-0)
src/qml/qml-media-collection-model.h (+1/-0)
tests/autopilot/gallery_app/emulators/album_view.py (+94/-3)
tests/autopilot/gallery_app/emulators/gallery_utils.py (+3/-1)
tests/autopilot/gallery_app/tests/test_album_view.py (+77/-0)

To merge this branch:

bzr merge lp:~artmello/gallery-app/gallery-app-remove_photo_from_album

High

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Bill Filler (community)			Approve on 2014-06-18
PS Jenkins bot	continuous-integration		Approve on 2014-06-17
Tiago Salem Herrmann (community)		2014-06-11	Approve on 2014-06-16
Chris Gagnon (community)	autopilot tests review	2014-06-11	Approve on 2014-06-13
Review via email: mp+222739@code.launchpad.net

Commit message

Add option to remove from album or delete from gallery when viewing photos from an album

Description of the change

Add option to remove from album or delete from gallery when viewing photos from an album

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-06-11:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/gallery-app-ci/865/rebuild

review: Approve (continuous-integration)

Revision history for this message

Chris Gagnon (chris.gagnon) wrote on 2014-06-11:

This only tests one button of the 3 new buttons. The other 2 buttons should be tested. (make sure on the remove from album and delete test that you verify the file is removed from the file system.)

The one button test you have is pretty solid and would not be a flaky test, but a couple of small changes can make it easier to maintain and troubleshoot.

def get_remove_* should be made private to _get_remove, then consumed by a click_* method. something like click_remove_from_album_button(). You can check the button becoming visible and being not shown as part of the same function

@autopilot_logging.log_action(logger.info) should be used as a method decorator on the click_* method(s) see example:
method http://bazaar.launchpad.net/~elopio/ubuntu-ui-toolkit/intermediate_flickables/revision/976/tests/autopilot/ubuntuuitoolkit/emulators.py

238 + photo = self.album_view.get_first_photo()
239 + self.main_view.close_toolbar()
240 + self.click_item(photo)

This should also be a method click_first_photo() in album_view with the @autopilot_logging.log_action(logger.info) decorator

so you can use self.album_view.click_first_photo() or better self.album_view.click_photo(path or photo name) instead of using 3 lines. That way if the toolbar goes away we won't have to delete a line from alot of tests, just the 1 line.

review: Needs Fixing

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-06-12:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/gallery-app-ci/868/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Chris Gagnon (chris.gagnon) wrote on 2014-06-12:

There are some mixed tab/spaces which cause errors and other pep8 issues

  1 album_view.py|10 col 1| F401 'os' imported but unused
  2 album_view.py|141 col 80| E501 line too long (83 > 79 characters)
  3 album_view.py|149 col 80| E501 line too long (84 > 79 characters)
  4 album_view.py|155 col 80| E501 line too long (84 > 79 characters)
  5 album_view.py|157 col 80| E501 line too long (82 > 79 characters)
  6 album_view.py|180 col 5| E101 indentation contains mixed spaces and tabs
  7 album_view.py|180 col 5| W191 indentation contains tabs
  8 album_view.py|185 col 5| E101 indentation contains mixed spaces and tabs
  9 album_view.py|185 col 5| W191 indentation contains tabs
10 album_view.py|190 col 5| E101 indentation contains mixed spaces and tabs
11 album_view.py|190 col 5| W191 indentation contains tabs
12 album_view.py|195 col 5| E101 indentation contains mixed spaces and tabs
13 album_view.py|195 col 5| W191 indentation contains tabs
14 album_view.py|200 col 5| E101 indentation contains mixed spaces and tabs
15 album_view.py|200 col 5| W191 indentation contains tabs
16 album_view.py|205 col 5| E101 indentation contains mixed spaces and tabs
17 album_view.py|205 col 5| W191 indentation contains tabs
18 album_view.py|215 col 80| E501 line too long (87 > 79 characters)

tests/test_album_view.py:11:1: F401 'Is' imported but unused
tests/test_album_view.py:129:13: E128 continuation line under-indented for visual indent
tests/test_album_view.py:149:13: E128 continuation line under-indented for visual indent
tests/test_album_view.py:171:13: E128 continuation line under-indented for visual indent

I've also left an inline code comment for an additional assert

review: Needs Fixing

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-06-12:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/gallery-app-ci/869/rebuild

review: Approve (continuous-integration)

Revision history for this message

Arthur Mello (artmello) wrote on 2014-06-13:

Are there any related MPs required for this MP to build/function as expected? Please list: No
Is your branch in sync with latest trunk (e.g. bzr pull lp:trunk -> no changes): Yes
Did you perform an exploratory manual test run of your code change and any related functionality on device or emulator?: Yes
Did you successfully run all tests found in your component's Test Plan (https://wiki.ubuntu.com/Process/Merges/TestPlan/gallery-app) on device or emulator? Yes
If you changed the UI, was the change specified/approved by design? No UI changes.
If you changed the packaging (debian), did you subscribe a core-dev to this MP? No packaging changes.

Revision history for this message

Chris Gagnon (chris.gagnon) wrote on 2014-06-13:

There is one minor pep8 error introduced by a deleted line.

photo_viewer.py:89:5: E301 expected 1 blank line, found 0
gallery_utils.py|13 col 1| E302 expected 2 blank lines, found 1

Sorry I didn't catch it in the first review

review: Needs Fixing

Revision history for this message

Chris Gagnon (chris.gagnon) wrote on 2014-06-13:

LGTM thanks for fixing those :D

review: Approve (autopilot tests review)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-06-13:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/gallery-app-ci/870/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Tiago Salem Herrmann (tiagosh) wrote on 2014-06-13:

looks good to me.

-- Checklist --

Did you perform an exploratory manual test run of the code change and any related functionality on device or emulator?
Yes

Did CI run pass? If not, please explain why.
Yes

Have you checked that submitter has accurately filled out the submitter checklist and has taken no shortcut?
Yes

review: Approve

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-06-13:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/gallery-app-ci/871/rebuild

review: Approve (continuous-integration)

Revision history for this message

Tiago Salem Herrmann (tiagosh) on 2014-06-16:

review: Approve

Revision history for this message

Bill Filler (bfiller) wrote on 2014-06-16:

found an issue:
- add two photos to an album
- delete the second photo from the album with "remove from album and delete"
- the first photo is shown after the delete but no toolbar is visible and user gets stuck as no way to get back to the album view

review: Needs Fixing

Revision history for this message

Bill Filler (bfiller) wrote on 2014-06-16:

Same thing happens if you select "remove from album" (w/o delete). issue seems to be when removing leaves only a single picture left in album

lp:~artmello/gallery-app/gallery-app-remove_photo_from_album updated on 2014-06-17

1002. By Arthur Mello on 2014-06-17: Remove wrong signal since that cause issues when removing last photo of an album

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-06-17:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/gallery-app-ci/874/rebuild

review: Approve (continuous-integration)

Revision history for this message

Bill Filler (bfiller) wrote on 2014-06-18:

tested, works
Did you perform an exploratory manual test run of the code change and any related functionality on device or emulator?
yes

Did you successfully run all tests found in your component's Test Plan (https://wiki.ubuntu.com/Process/Merges/TestPlan/<package-name>) on device or emulator?
yes

Did CI run pass? If not, please explain why.
yes

Have you checked that submitter has accurately filled out the submitter checklist and has taken no shortcut?
yes

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk

Subscribers

People subscribed via source and target branches

to all changes:

Arthur Mello

Ubuntu Phablet Team

 === modified file 'rc/qml/MediaViewer/MediaViewer.qml'
 --- rc/qml/MediaViewer/MediaViewer.qml	2014-05-20 15:32:39 +0000
 +++ rc/qml/MediaViewer/MediaViewer.qml	2014-06-17 03:24:34 +0000
@@ -65,8 +65,8 @@
      // tooolbar actions for the full view
      property Item tools: media ? (media.type === MediaSource.Photo ?
--                                            d.photoToolbar : d.videoToolbar)
--                                         : null
++                                      d.photoToolbar : d.videoToolbar)
++                               : null
      /*!
      */
@@ -257,7 +257,7 @@
                          sharePanel.userAccountId = facebook.id;
                          sharePanel.visible = true;
                          viewerWrapper.tools.opened = false;
--                         _lastPopover = null;
++                        _lastPopover = null;
+                     }
+                 }
+             }
@@ -311,6 +311,48 @@
+             }
+         }
++        Component {
++            id: removeFromAlbumDialog
++            Dialog {
++                id: dialogue
++                objectName: "removePhotoFromAlbumDialog"
++                title: i18n.tr("Remove a photo from album")
++
++                function finishRemove() {
++                    if (model.count === 0)
++                        galleryPhotoViewer.closeRequested();
++                }
++
++                Button {
++                    objectName: "removeFromAlbumButton"
++                    text: i18n.tr("Remove from Album")
++                    color: Gallery.HIGHLIGHT_BUTTON_COLOR
++                    onClicked: {
++                        PopupUtils.close(dialogue)
++                        viewerWrapper.model.removeMediaFromAlbum(album, galleryPhotoViewer.media);
++                        dialogue.finishRemove();
++                    }
++                }
++
++                Button {
++                    objectName: "removeFromAlbumAndDeleteButton"
++                    text: i18n.tr("Remove from Album and Delete")
++                    onClicked: {
++                        PopupUtils.close(dialogue)
++                        viewerWrapper.model.destroyMedia(galleryPhotoViewer.media, true);
++                        dialogue.finishRemove();
++                    }
++                }
++
++                Button {
++                    objectName: "removeFromAlbumCancelButton"
++                    text: i18n.tr("Cancel")
++                    onClicked: PopupUtils.close(dialogue)
++                }
++            }
++
++        }
++
          onCloseRequested: viewerWrapper.closeRequested()
          onEditRequested: viewerWrapper.editRequested(media)
+     }
@@ -488,7 +530,10 @@
                      text: i18n.tr("Delete")
                      iconSource: "../../img/delete.png"
                      onTriggered: {
--                        PopupUtils.open(deleteDialog, null);
++                        if (album)
++                            PopupUtils.open(removeFromAlbumDialog, null);
++                        else
++                            PopupUtils.open(deleteDialog, null);
+                     }
+                 }
                  text: i18n.tr("Delete")
@@ -592,5 +637,4 @@
+             }
+         }
+     }
--
+ }
 === modified file 'src/qml/qml-media-collection-model.cpp'
 --- src/qml/qml-media-collection-model.cpp	2014-03-21 18:13:28 +0000
 +++ src/qml/qml-media-collection-model.cpp	2014-06-17 03:24:34 +0000
@@ -112,6 +112,24 @@
+ }
  /*!
++ * \brief QmlMediaCollectionModel::removeMediaFromAlbum
++ * \param valbum
++ * \param vmedia
++ */
++void QmlMediaCollectionModel::removeMediaFromAlbum(QVariant valbum, QVariant vmedia)
++{
++    Album* album = VariantToObject<Album*>(valbum);
++    if (album == NULL)
++        return;
++
++    MediaSource* media = VariantToObject<MediaSource*>(vmedia);
++    if (media == NULL)
++        return;
++
++    album->detach(media);
++}
++
++/*!
   * \brief QmlMediaCollectionModel::monitored
   * \return
   */
 === modified file 'src/qml/qml-media-collection-model.h'
 --- src/qml/qml-media-collection-model.h	2014-03-21 18:13:28 +0000
 +++ src/qml/qml-media-collection-model.h	2014-06-17 03:24:34 +0000
@@ -48,6 +48,7 @@
      Q_INVOKABLE QVariant createAlbumFromSelected();
      Q_INVOKABLE void destroySelectedMedia();
      Q_INVOKABLE void destroyMedia(QVariant vmedia, bool destroy_backing);
++    Q_INVOKABLE void removeMediaFromAlbum(QVariant valbum, QVariant vmedia);
      bool monitored() const;
      void setMonitored(bool monitor);
 === modified file 'tests/autopilot/gallery_app/emulators/album_view.py'
 --- tests/autopilot/gallery_app/emulators/album_view.py	2014-06-10 19:00:23 +0000
 +++ tests/autopilot/gallery_app/emulators/album_view.py	2014-06-17 03:24:34 +0000
@@ -5,13 +5,21 @@
  # under the terms of the GNU General Public License version 3, as published
  # by the Free Software Foundation.
--from testtools.matchers import GreaterThan, LessThan
++import logging
++import re
++
++from testtools.matchers import GreaterThan, LessThan, Equals, Is
++from autopilot.matchers import Eventually
++
++from autopilot import logging as autopilot_logging
  from gallery_app.emulators.gallery_utils import(
      GalleryUtils,
      GalleryAppException,
+ )
++logger = logging.getLogger(__name__)
++
  class AlbumView(GalleryUtils):
      """An emulator class that makes it easy to interact with the gallery app"""
@@ -41,8 +49,7 @@
      def get_album_photo_view(self):
          """Returns the photo view of the album viewer"""
--        view = self.get_album_view()
--        return view.select_single("PopupPhotoViewer")
++        return self.app.select_single("PopupPhotoViewer")
      def get_spread_view(self):
          """Returns the inner spread view to access the pages"""
@@ -126,3 +133,87 @@
          :param page_number: The starting page number you are swiping from
          '''
          self._swipe_setup(page_number, 'right')
++
++    def _get_remove_from_album_dialog(self):
++        """Returns the photo viewer remove from album dialog."""
++        return self.app.wait_select_single("Dialog",
++                                           objectName=
++                                           "removePhotoFromAlbumDialog")
++
++    def _remove_from_album_dialog_shown(self):
++        dialog = self.app.select_many("Dialog",
++                                      objectName="removePhotoFromAlbumDialog")
++        return len(dialog) >= 1
++
++    def _get_remove_from_album_popover_remove_item(self):
++        """Returns remove button of the remove from album popover."""
++        return self.app.select_single("Button",
++                                      objectName="removeFromAlbumButton",
++                                      visible=True)
++
++    def _get_remove_from_album_popover_delete_item(self):
++        """Returns delete button of the remove from album popover."""
++        return self.app.select_single("Button",
++                                      objectName=
++                                      "removeFromAlbumAndDeleteButton",
++                                      visible=True)
++
++    def _get_remove_from_album_popover_cancel_item(self):
++        """Returns cancel button of the remove from album popover."""
++        return self.app.select_single("Button",
++                                      objectName="removeFromAlbumCancelButton",
++                                      visible=True)
++
++    def _ensure_remove_from_album_dialog_is_open(self):
++        """Ensure that the remove from album dialog is fully opened."""
++        remove_dialog = self._get_remove_from_album_dialog()
++        self.assertThat(remove_dialog.visible, Eventually(Equals(True)))
++        self.assertThat(remove_dialog.opacity, Eventually(Equals(1)))
++
++    def _ensure_remove_from_album_dialog_is_close(self):
++        """Ensure that the remove from album dialog is fully closed."""
++        self.assertThat(self._remove_from_album_dialog_shown,
++                        Eventually(Is(False)))
++
++    @autopilot_logging.log_action(logger.info)
++    def click_remove_from_album_remove_button(self):
++        """Click on the remove from album button of the remove dialog."""
++        self._ensure_remove_from_album_dialog_is_open()
++
++        remove_item = self._get_remove_from_album_popover_remove_item()
++        self.pointing_device.click_object(remove_item)
++
++        self._ensure_remove_from_album_dialog_is_close()
++
++    @autopilot_logging.log_action(logger.info)
++    def click_remove_from_album_delete_button(self):
++        """Click on the remove and delete of the remove dialog."""
++        self._ensure_remove_from_album_dialog_is_open()
++
++        delete_item = self._get_remove_from_album_popover_delete_item()
++        self.pointing_device.click_object(delete_item)
++
++        self._ensure_remove_from_album_dialog_is_close()
++
++    @autopilot_logging.log_action(logger.info)
++    def click_remove_from_album_cancel_button(self):
++        """Click on the cancel of the remove dialog."""
++        self._ensure_remove_from_album_dialog_is_open()
++
++        cancel_item = self._get_remove_from_album_popover_cancel_item()
++        self.pointing_device.click_object(cancel_item)
++
++        self._ensure_remove_from_album_dialog_is_close()
++
++    @autopilot_logging.log_action(logger.info)
++    def click_first_photo(self):
++        """Click on the first photo of the album and return the path of it."""
++        photo = self.get_first_photo()
++        images = photo.select_many('QQuickImage')
++        path = ''
++        for i in images:
++            if str(i.source).startswith('image://gallery-standard/'):
++                path = re.sub('^image://gallery-standard/', '',
++                              i.source).split('?')[0]
++        self.pointing_device.click_object(photo)
++        return path
 === modified file 'tests/autopilot/gallery_app/emulators/gallery_utils.py'
 --- tests/autopilot/gallery_app/emulators/gallery_utils.py	2014-05-01 15:27:13 +0000
 +++ tests/autopilot/gallery_app/emulators/gallery_utils.py	2014-06-17 03:24:34 +0000
@@ -6,6 +6,8 @@
  # by the Free Software Foundation.
  import ubuntuuitoolkit.emulators
++from autopilot.testcase import AutopilotTestCase
++
  from time import sleep
@@ -13,7 +15,7 @@
      pass
--class GalleryUtils(object):
++class GalleryUtils(AutopilotTestCase):
      """An emulator class that makes it easy to interact with
         general components of the gallery app."""
 === modified file 'tests/autopilot/gallery_app/tests/test_album_view.py'
 --- tests/autopilot/gallery_app/tests/test_album_view.py	2014-05-22 17:06:01 +0000
 +++ tests/autopilot/gallery_app/tests/test_album_view.py	2014-06-17 03:24:34 +0000
@@ -14,9 +14,11 @@
  from gallery_app.emulators.album_view import AlbumView
  from gallery_app.emulators.albums_view import AlbumsView
  from gallery_app.emulators.media_selector import MediaSelector
++from gallery_app.emulators.photo_viewer import PhotoViewer
  from gallery_app.emulators import album_editor
  from gallery_app.tests import GalleryTestCase
++import os
  from time import sleep
  from unittest import skip
@@ -36,6 +38,10 @@
      def media_selector(self):
          return MediaSelector(self.app)
++    @property
++    def photo_viewer(self):
++        return PhotoViewer(self.app)
++
      def setUp(self):
          self.ARGS = []
          super(TestAlbumView, self).setUp()
@@ -105,6 +111,77 @@
              lambda: self.album_view.number_of_photos(),
              Eventually(Equals(num_photos_start + 1)))
++    def test_remove_photo_from_album(self):
++        self.main_view.close_toolbar()
++        self.open_first_album()
++        num_photos_start = self.album_view.number_of_photos()
++        self.assertThat(num_photos_start, Equals(1))
++
++        path = self.album_view.click_first_photo()
++
++        self.assertThat(lambda: os.path.exists(path),
++                        Eventually(Equals(True)))
++
++        photo_view = self.album_view.get_album_photo_view()
++        self.assertThat(photo_view.visible, Eventually(Equals(True)))
++
++        self.main_view.open_toolbar().click_button("deleteButton")
++        self.album_view.click_remove_from_album_remove_button()
++
++        self.assertThat(lambda: self.album_view.number_of_photos(),
++                        Eventually(Equals(num_photos_start - 1)))
++
++        self.assertThat(lambda: os.path.exists(path),
++                        Eventually(Equals(True)))
++
++    def test_remove_photo_from_album_and_delete(self):
++        self.main_view.close_toolbar()
++        self.open_first_album()
++        num_photos_start = self.album_view.number_of_photos()
++        self.assertThat(num_photos_start, Equals(1))
++
++        path = self.album_view.click_first_photo()
++
++        self.assertThat(lambda: os.path.exists(path),
++                        Eventually(Equals(True)))
++
++        photo_view = self.album_view.get_album_photo_view()
++        self.assertThat(photo_view.visible, Eventually(Equals(True)))
++
++        self.main_view.open_toolbar().click_button("deleteButton")
++        self.album_view.click_remove_from_album_delete_button()
++
++        self.assertThat(lambda: self.album_view.number_of_photos(),
++                        Eventually(Equals(num_photos_start - 1)))
++
++        self.assertThat(lambda: os.path.exists(path),
++                        Eventually(Equals(False)))
++
++    def test_cancel_remove_photo_from_album(self):
++        self.main_view.close_toolbar()
++        self.open_first_album()
++        num_photos_start = self.album_view.number_of_photos()
++        self.assertThat(num_photos_start, Equals(1))
++
++        path = self.album_view.click_first_photo()
++
++        self.assertThat(lambda: os.path.exists(path),
++                        Eventually(Equals(True)))
++
++        photo_view = self.album_view.get_album_photo_view()
++        self.assertThat(photo_view.visible, Eventually(Equals(True)))
++
++        self.main_view.open_toolbar().click_button("deleteButton")
++        self.album_view.click_remove_from_album_cancel_button()
++
++        self.main_view.open_toolbar().click_button("backButton")
++
++        self.assertThat(lambda: self.album_view.number_of_photos(),
++                        Eventually(Equals(num_photos_start)))
++
++        self.assertThat(lambda: os.path.exists(path),
++                        Eventually(Equals(True)))
++
      def test_add_photo_to_new_album(self):
          self.main_view.open_toolbar().click_button("addButton")
          self.ui_update()