1124193 - Last synced date is wrong on first sync

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Android Sync, defect)

Description

[Teodora Vermesan (:TeoVermesan)]

Attachment: Last_synced_date

Steps to reproduce:
1. With a clean profile, go to Menu -> Settings -> Sync -> Get Started
2. Perform Sync
3. Tap "Back to browsing"
4. Go to Menu -> Settings -> Sync

Actual result:
- Last synced date is Jan 1,1970

Expected result:
- Last synced date should be blank or something suggestive, since you have never performed sync.

Comment 1

[Nick Alexander :nalexander [he/him]]

Known issue, cosmetic only, but worth addressing.  I would make this a mentor bug except there's code clean-up needed here because there are *so many* places that we format Sync timestamps for display.

Comment 2

[Nick Alexander :nalexander [he/him]]

Actually, let's call this a mentor bug.  What needs to happen:

1) Move the getLastSyncedString out of TabsAccessor [1] entirely.  Move it into RemoteTabsExpandableListAdapter [2].

2) Update getLastSyncedString to handle really early times -- say before year 2000 or something -- and say something like "Never" or "--".

3) This will also need to happen in the "other" implementation of getLastSyncedString (don't ask why we have two -- but they can't be harmonized right now) at [3].

[1] https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/db/TabsAccessor.java#26

[2] https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/RemoteTabsExpandableListAdapter.java

[3] https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/fxa/activities/FxAccountStatusFragment.java#535

Comment 3

[swaroop.rao]

I can take this up, if nobody's working on it. I'll pull a fresh copy of the code base tonight and get a build ready.

Comment 4

[Vivek Balakrishnan[:vivek]]

Hi Swaroop,

Go for it. 
Nick has listed the required changes in his previous comment.
Do feel free to ping back in case if you need any help.

Happy coding

Comment 5

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://mail.mozilla.org/pipermail/mobile-firefox-dev/2015-February/001090.html

Comment 6

[Robin Ricard [:rricard]]

Ok, I think I'll take it (as advised by Nick in bug 1130872). That would be a good introduction to the "Java-side" of the codebase !

Comment 7

[Robin Ricard [:rricard]]

Attachment: Last synced date is wrong on first sync

Comment 8

[Robin Ricard [:rricard]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8c15c81cdbc2

Comment 9

[Robin Ricard [:rricard]]

Attachment: Patch result

Comment 10

[Robin Ricard [:rricard]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=847c84a7c09e (with more flags)

Comment 11

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8564558 [details] [diff] [review]
Last synced date is wrong on first sync

Review of attachment 8564558 [details] [diff] [review]:
-----------------------------------------------------------------

This is a really good patch.  I've asked for a lot of changes but it's clear you are a strong contributor and will have no trouble with them.  Ping me in #mobile so that we can talk about finding you more significant contributions.

Thanks!

::: mobile/android/base/RemoteTabsExpandableListAdapter.java
@@ +3,5 @@
>   * You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  package org.mozilla.gecko;
>  
> +import java.util.*;

Keep the explicit imports.  These .* imports are a pain.

@@ +32,5 @@
> +    static {
> +        NEVER_CALENDAR = GregorianCalendar.getInstance();
> +        NEVER_CALENDAR.set(2000, Calendar.JANUARY, 1, 0, 0, 0);
> +    }
> +    public static final Date NEVER_DATE = NEVER_CALENDAR.getTime();

Let's call this something more descriptive, like EARLIEST_VALID_SYNCED_DATE; and let's add a descriptive Javadoc, like:

/**
 * If a device claims to have synced before this date, we will assume it has never synced.
 */


Rather than exposing this NEVER_CALENDAR, fold it into the NEVER_DATE static initializer, like:

public static final Date EARLIEST_VALID_SYNCED_DATE;
static {
  final Calendar calendar = ...
  E_V_S_D = calendar.getTIme();
}

@@ +229,5 @@
> +     * @param time to format string for.
> +     * @return string describing time span
> +     */
> +    public String getLastSyncedString(Context context, long now, long time) {
> +        if(new Date(time).before(NEVER_DATE)) {

nit: space before paren, like:

if (new Date(time)...)

@@ +230,5 @@
> +     * @return string describing time span
> +     */
> +    public String getLastSyncedString(Context context, long now, long time) {
> +        if(new Date(time).before(NEVER_DATE)) {
> +            return "--";

Let's make this a new string, say "remote_tabs_never_synced".  I haven't a clue what other locales would consider appropriate, so let's give them a choice.  Add a "localization note" (there are lots of examples in the .dtd file) explaining where the string will be used.

::: mobile/android/base/fxa/activities/FxAccountStatusFragment.java
@@ +530,5 @@
>    }
>  
>    // This is a helper function similar to TabsAccessor.getLastSyncedString() to calculate relative "Last synced" time span.
>    private String getLastSyncedString(final long startTime) {
> +    if(new Date(startTime).before(RemoteTabsExpandableListAdapter.NEVER_DATE)) {

For historical reasons, we need to do this FxAccountStatusFragment one differently.  (The Sync code is developed separately, in https://github.com/mozilla-services/android-sync, and does not have access to all the Fennec code.  That includes not having access to RemoteTabsELA.)

Comment 12

[Robin Ricard [:rricard]]

Ok, fixing this and sending you a new patch !

Comment 13

[Robin Ricard [:rricard]]

Attachment: Last synced date is wrong on first sync

Comment 14

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8565005 [details] [diff] [review]
Last synced date is wrong on first sync

Review of attachment 8565005 [details] [diff] [review]:
-----------------------------------------------------------------

I have some minor nits, nothing major -- just trying to communicate intent and be consistent -- and then this is good to go.  Post an updated patch, carry-forward the r+ (you can set it manually), then NI me to land.  Thanks!  This was a very good interaction: strong first patch, almost perfect second patch, ready to land.  Bravo!

::: mobile/android/base/RemoteTabsExpandableListAdapter.java
@@ +3,5 @@
>   * You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  package org.mozilla.gecko;
>  
> +import java.util.*;

Ditto.

::: mobile/android/base/fxa/activities/FxAccountStatusFragment.java
@@ +3,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  package org.mozilla.gecko.fxa.activities;
>  
> +import java.util.*;

Again, we don't use these * imports (looks like your IDE might be using them).

@@ +8,4 @@
>  
>  import org.mozilla.gecko.AppConstants;
>  import org.mozilla.gecko.R;
> +import org.mozilla.gecko.RemoteTabsExpandableListAdapter;

This code doesn't have access to the this class, but it actually appears unused.  Remove it.

::: mobile/android/base/locales/en-US/android_strings.dtd
@@ +459,5 @@
>  <!ENTITY remote_tabs_panel_moved_desc "We\'ve moved your tabs from other devices into a panel on your home page that can be easily accessed every time you open a new tab.">
>  <!ENTITY remote_tabs_panel_moved_link "Take me to my new panel.">
>  
> +<!-- Localization note: Used when the sync has not happend yet, showed in place of a date -->
> +<!ENTITY remote_tabs_never_synced "Never Synced">

Let's move this beside [1] and follow the same format.  So either make this string:

"Last synced: never"

or if you prefer

"Never synced"

Or make this "never" and use the other string to interpolate it using the &formatS;

[1] https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/locales/en-US/android_strings.dtd#577

Comment 15

[Robin Ricard [:rricard]]

Yup, IntelliJ always get me back the import *. I've changed my settings.

Comment 16

[Robin Ricard [:rricard]]

And I enabled the auto-import opt for unused imports

Comment 17

[Robin Ricard [:rricard]]

Attachment: 1124193.patch

Comment 18

[Nick Alexander :nalexander [he/him]]

https://hg.mozilla.org/integration/fx-team/rev/da5bf8c21c13

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/da5bf8c21c13

Comment 20

[Teodora Vermesan (:TeoVermesan)]

Testing with:
Device: Nexus 5 (Android 5.0.1) and Nexus 4 (Android 4.4)
Builds:
1. try build: ftp://ftp.mozilla.org/pub/mobile/try-builds/[e-mail]-847c84a7c09e/try-android-api-11/
- Last synced date on first sync is "--": http://i.imgur.com/6kiDZ2D.png

2. latest Nightly build (2015-02-19)
- Last synced date on first sync is still Jan 1,1970

Comment 21

[Nick Alexander :nalexander [he/him]]

(In reply to Teodora Vermesan (:TeoVermesan) from comment #20)
> Testing with:
> Device: Nexus 5 (Android 5.0.1) and Nexus 4 (Android 4.4)
> Builds:
> 1. try build:
> ftp://ftp.mozilla.org/pub/mobile/try-builds/[e-mail]-
> 847c84a7c09e/try-android-api-11/
> - Last synced date on first sync is "--": http://i.imgur.com/6kiDZ2D.png
> 
> 2. latest Nightly build (2015-02-19)
> - Last synced date on first sync is still Jan 1,1970

rricard: can you see if this is the case?  I'm quite busy today and tomorrow.

Comment 22

[Robin Ricard [:rricard]]

Seems weird, I'll look at it when possible

Comment 23

[Robin Ricard [:rricard]]

It does effectively fail for me on the latest nightly on my Nexus 5. Can you check tomorrow if the problem still exist ? I'll take a look in this case.

Comment 24

[Teodora Vermesan (:TeoVermesan)]

Attachment: tmp_11378-Screenshot_2015-02-21-18-14-491053555603.png

Yes. The issue is still reproducing on latest Nightly(2015-02-21) on Nexus 4(Android 5.0.1)

Comment 25

[Robin Ricard [:rricard]]

I think I won't be able to help you with that, I'm asking for help. It really doesn't work on Nightly...

Comment 26

[Vivek Balakrishnan[:vivek]]

Nick,

Seems like Robin changes were reverted when Bug 1117829 landed.
https://hg.mozilla.org/mozilla-central/rev/4d10b6a2cd5f#l12.50

Comment 27

[Nick Alexander :nalexander [he/him]]

vivek: thanks.  NI to me to reland.

Comment 28

[Nick Alexander :nalexander [he/him]]

https://hg.mozilla.org/integration/fx-team/rev/837fb11b49fa

Comment 29

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8565927 [details] [diff] [review]
1124193.patch

Approval Request Comment
[Feature/regressing bug #]: I landed on top in Bug 1117829.
[User impact if declined]: issue isn't fixed -- bad synced date before first sync.
[Describe test coverage new/current, TreeHerder]: none.
[Risks and why]: very low.
[String/UUID change made/needed]: a new Android string but not a new entity, so none.

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/837fb11b49fa

Comment 31

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8565927 [details] [diff] [review]
1124193.patch

Approving for uplift to aurora since it needs re-landing and seems low risk.

Comment 32

[Teodora Vermesan (:TeoVermesan)]

Attachment: Screenshot_2015-03-04-14-35-09.png

Tested with:
Device: Nexus 4 (Android 4.4)
Build: Firefox for Android 39.0a1 (2015-03-04)
Last synced date on first sync is displayed as: "never"

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ce7ddf3f848c