ubuntu-touch-coreapps-reviewers team mailing list archive

[Nicholas Skaggs <nskaggs@xxxxxxxxxx>]

Review: Needs Fixing

A couple tweaks suggested :-)

Diff comments:

> === modified file 'MonthView.qml'
> --- MonthView.qml	2015-02-18 19:27:20 +0000
> +++ MonthView.qml	2015-03-01 15:00:26 +0000
> @@ -50,6 +50,7 @@
>          ]
>  
>          contents: Label {
> +            objectName:"monthYearLabel"
>              fontSize: "x-large"
>              // TRANSLATORS: this is a time formatting string,
>              // see http://qt-project.org/doc/qt-5/qml-qtqml-date.html#details for valid expressions.
> 
> === modified file 'tests/autopilot/calendar_app/__init__.py'
> --- tests/autopilot/calendar_app/__init__.py	2015-02-26 18:26:10 +0000
> +++ tests/autopilot/calendar_app/__init__.py	2015-03-01 15:00:26 +0000
> @@ -386,6 +386,9 @@
>          month = self.get_current_month()
>          return month.currentMonth.datetime.strftime("%B")
>  
> +    def get_current_selected_day(self):
> +        return self.select_many('MonthComponentDateDelegate', isToday=True)

I realize you probably tried this, but this concerns me this is a select many. This should be a select_single . . .

> +
>  
>  class DayView(ubuntuuitoolkit.UbuntuUIToolkitCustomProxyObjectBase):
>  
> 
> === modified file 'tests/autopilot/calendar_app/tests/test_monthview.py'
> --- tests/autopilot/calendar_app/tests/test_monthview.py	2014-09-04 16:04:37 +0000
> +++ tests/autopilot/calendar_app/tests/test_monthview.py	2015-03-01 15:00:26 +0000
> @@ -96,3 +96,19 @@
>  
>      def test_monthview_go_to_today_prev_year(self):
>          self._go_to_today(-12)
> +
> +    def test_current_day_month_and_year_is_selected(self):
> +        """
> +        By default, the month view shows the current day, month and year.
> +        """
> +        now = datetime.now()
> +        expected_month_name_year = now.strftime("%B %Y")
> +
> +        self.assertThat(self.app.main_view.get_month_year(self.month_view),
> +                        Equals(expected_month_name_year))
> +
> +        expected_day = str(int(now.strftime("%d")))
> +        selected_day = self.month_view.get_current_selected_day()[0]

This is very tempting, but *if* the correct response is to indeed grab the [0] element, I think I would rather see that in the get_current_selected_day method. Doing it this way might get you in trouble because of ordering: http://people.canonical.com/~nskaggs/autopilot/guides/good_tests.html#do-not-depend-on-object-ordering

> +
> +        self.assertThat(selected_day.select_single('Label').text,
> +                        Equals(expected_day))
> 


-- 
https://code.launchpad.net/~carla-sella/ubuntu-calendar-app/current-day-month-year-selected/+merge/251387
Your team Ubuntu Calendar Developers is subscribed to branch lp:ubuntu-calendar-app.