Closed Bug 1124193 Opened 5 years ago Closed 5 years ago

Last synced date is wrong on first sync

Categories

(Firefox for Android :: Android Sync, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 39
Tracking Status
firefox35 --- unaffected
firefox36 --- wontfix
firefox37 --- affected
firefox38 --- fixed
firefox39 --- verified

People

(Reporter: TeoVermesan, Assigned: rricard, Mentored)

References

Details

(Whiteboard: [lang=java][good next bug])

Attachments

(5 files, 2 obsolete files)

Attached image 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.
Component: General → Android Sync
Product: Firefox for Android → Android Background Services
Version: Trunk → unspecified
Blocks: 1096669
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.
Mentor: nalexander, vivekb.balakrishnan
Whiteboard: [lang=java][good first bug]
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
Whiteboard: [lang=java][good first bug] → [lang=java][good second bug]
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.
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
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 !
Assignee: nobody → ricard.robin
Status: NEW → ASSIGNED
Attachment #8564558 - Flags: review?(nalexander)
Attached image Patch result
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.)
Attachment #8564558 - Flags: review?(nalexander) → feedback+
Ok, fixing this and sending you a new patch !
Attachment #8564558 - Attachment is obsolete: true
Attachment #8565005 - Flags: review?(nalexander)
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
Attachment #8565005 - Flags: review?(nalexander) → review+
Yup, IntelliJ always get me back the import *. I've changed my settings.
And I enabled the auto-import opt for unused imports
Attached patch 1124193.patchSplinter Review
Attachment #8565005 - Attachment is obsolete: true
Attachment #8565927 - Flags: review+
Flags: needinfo?(nalexander)
https://hg.mozilla.org/mozilla-central/rev/da5bf8c21c13
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38
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/ricard.robin@gmail.com-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
(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/ricard.robin@gmail.com-
> 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.
Flags: needinfo?(ricard.robin)
Seems weird, I'll look at it when possible
Flags: needinfo?(ricard.robin)
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.
Flags: needinfo?(teodora.vermesan)
Yes. The issue is still reproducing on latest Nightly(2015-02-21) on Nexus 4(Android 5.0.1)
Flags: needinfo?(teodora.vermesan)
I think I won't be able to help you with that, I'm asking for help. It really doesn't work on Nightly...
Flags: needinfo?(nalexander)
Nick,

Seems like Robin changes were reverted when Bug 1117829 landed.
https://hg.mozilla.org/mozilla-central/rev/4d10b6a2cd5f#l12.50
Flags: needinfo?(nalexander)
vivek: thanks.  NI to me to reland.
Status: RESOLVED → REOPENED
Flags: needinfo?(nalexander)
Resolution: FIXED → ---
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.
Flags: needinfo?(nalexander)
Attachment #8565927 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/837fb11b49fa
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Comment on attachment 8565927 [details] [diff] [review]
1124193.patch

Approving for uplift to aurora since it needs re-landing and seems low risk.
Attachment #8565927 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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"
Target Milestone: Firefox 38 → Firefox 39
Duplicate of this bug: 1148742
Product: Android Background Services → Firefox for Android
You need to log in before you can comment on or make changes to this bug.