1392536 - Tooltip for a device in the "send tab to device" menus should show the device's last sync time

Status: RESOLVED FIXED

(Firefox :: Sync, enhancement, P3)

Description

[Mark Hammond [:markh] [:mhammond]]

Currently the tooltip is the device name (ie, identical to the menu itself). It should probably be just the last-sync time like the sidebar and menu. This will help the user if there are duplicates.

Comment 1

[Henrik]

I've attempted at adding this functionality. If I understand the description correctly I need to add the sync time of each device to the "Send tab to Device" dropdown menu that appears when right-clicking a tab. Is this correct?

I poked around that code and found that Weave provides clients with a field called "serverLastModified". I couldn't find what this was about but my guess is that it is the number of seconds since epoch that the device was last synced. Is this true?

With the above in mind I think a viable solution would be to append something like "Synced MM/DD/YYYY" to the name of the device (so it shows up in the tab). Does this make sense or is there a better approach?

I've basically implemented this but would be great to get some feedback on it, especially i18n issues with date would be greatly appreciated!

Comment 2

[Henrik]

Attachment: What the test implementation looks like on my computer

Comment 3

[Henrik]

Attachment: my.patch

Comment 4

[Edouard Oger [:eoger]]

Thanks a lot for your contribution Anting :)

> I've attempted at adding this functionality. If I understand the description correctly I need to add the sync time of each device to the "Send tab to Device" dropdown menu that appears when right-clicking a tab. Is this correct?

I think that Mark was talking about this menu https://imgur.com/a/KKloC in his first message. We should have been more precise here, that's on us sorry.

> I poked around that code and found that Weave provides clients with a field called "serverLastModified". I couldn't find what this was about but my guess is that it is the number of seconds since epoch that the device was last synced. Is this true?

Good find! Indeed, this property is the one we want to use here.

> With the above in mind I think a viable solution would be to append something like "Synced MM/DD/YYYY" to the name of the device (so it shows up in the tab). Does this make sense or is there a better approach?

We probably want to use the same wording as the Synced Tabs sidebar: https://imgur.com/a/9khwR
As for now Firefox doesn't have a way to do relative dates (e.g. X minutes ago), it will change in the future weeks/months but for now we'll be fine with absolute dates.
gSync.formatLastSyncDate() in browser-sync.js will do everything for you!

Thanks again!

Comment 5

[Edouard Oger [:eoger]]

> Good find! Indeed, this property is the one we want to use here.

Actually I might be wrong here, since other clients can change a client record.
We might want to use the tabs collection instead to determine that information, what do you think Mark?

e.g. new Date(Weave.Service.engineManager.get("tabs")._store._remoteClients[clientId].lastModified * 1000)

Comment 6

[Edouard Oger [:eoger]]

OOps

Comment 7

[Mark Hammond [:markh] [:mhammond]]

(In reply to Edouard Oger [:eoger] from comment #5)
> > Good find! Indeed, this property is the one we want to use here.
> 
> Actually I might be wrong here, since other clients can change a client
> record.
> We might want to use the tabs collection instead to determine that
> information, what do you think Mark?
> 
> e.g. new
> Date(Weave.Service.engineManager.get("tabs")._store._remoteClients[clientId].
> lastModified * 1000)

The user might have disabled tab syncing :( You are probably correct, but I think it's fine for this bug to do what the menu and sidebar already does, and if necessary, we can adjust all places later.

ie, something like http://searchfox.org/mozilla-central/rev/7ba03cebf1dcf3b4c40c85a4efad42a5022cf1ec/browser/components/customizableui/CustomizableWidgets.jsm#436

The patch obviously has a few other issues, including embedding an English string. Ed, can you please detail the specific changes needed for anting?

Comment 8

[Edouard Oger [:eoger]]

Here's what you need to do Anting:

- In browser-pageActions.js:

  * You want the createDeviceNodeFn to take another argument named lastModified: http://searchfox.org/mozilla-central/rev/7ba03cebf1dcf3b4c40c85a4efad42a5022cf1ec/browser/base/content/browser-pageActions.js#833
    The createDeviceNodeFn function creates a DOM node for each device.

  * You want the tooltip text to be "Last Sync: [date]". But we also don't want tooltips for the "Send to All Devices" button.
    Fortunately, clientId will be falsy for that last case.
http://searchfox.org/mozilla-central/rev/7ba03cebf1dcf3b4c40c85a4efad42a5022cf1ec/browser/base/content/browser-pageActions.js#839-842
    gSync.formatLastSyncDate() will take care of creating that "Last Sync: [date]" string (implementation is here: http://searchfox.org/mozilla-central/rev/7ba03cebf1dcf3b4c40c85a4efad42a5022cf1ec/browser/base/content/browser-sync.js#601)


- In browser-sync.js

 * You want to pass client.lastModified as an argument here: http://searchfox.org/mozilla-central/source/browser/base/content/browser-sync.js#346

Don't hesitate to ping me if you're blocked somewhere!

Comment 9

[Henrik]

Thanks for the great pointers! I will try and implement Edouards bullet points during the weekend and come back with a new patch.

Comment 10

[Henrik]

Attachment: my.patch

Comment 11

[Henrik]

Attachment: SyncTooltip.png

Comment 12

[Henrik]

I've updated the patch and the screenshot with the new implementation. I wasn't sure what to do for tooltips that weren't clients so for those I just used the old behavior. If this is incorrect, please let me know! 

Thanks again for the great pointers Edouard, they were very helpful!

Comment 13

[Edouard Oger [:eoger]]

Comment on attachment 8916365 [details] [diff] [review]
my.patch

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

The patch looks great!
Could we also add tests? (http://searchfox.org/mozilla-central/rev/aa721cc82a56fc307e899263d325c81b73e38a28/browser/base/content/test/urlbar/browser_page_action_menu.js#454)

::: browser/base/content/browser-pageActions.js
@@ +842,5 @@
>          item.classList.add("subviewbutton-iconic");
>        }
> +
> +      // Only add Last Sync for clients
> +      if (clientId && lastModified) {

lastModified will always exist so no need to check for that.
Also, let's show a tooltip only if clientId is truthy.

::: browser/base/content/browser-sync.js
@@ +368,4 @@
>        separator.classList.add("sync-menuitem");
>        fragment.appendChild(separator);
>        const allDevicesLabel = this.fxaStrings.GetStringFromName("sendToAllDevices.menuitem");
> +      addTargetDevice("", allDevicesLabel, "", null);

I think we can omit the 4th argument for this and all the cases where you pass null.

Comment 14

[Henrik]

Attachment: my.patch

Comment 15

[Henrik]

I added some tests to the last patch I made and removed "lastModified" so that it only checks for clientId as well as keeping the original calls to addTargetDevice where the fourth argument doesn't matter. 

In the tests I was not sure if I should mock gSync.formatLastSyncDate in browser_page_action_menu.js or not, I saw that some other functions were mocked...

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a7b7044ecb7b
Add last sync date as tooltip to send to device tab

Comment 17

[Edouard Oger [:eoger]]

Great patch Anting, I have pushed your changes to the integration repository.
Thanks a lot for your contribution!

Comment 18

[Henrik]

It was a pleasure helping :)

Thanks a lot Edouard and Mark for the help!

Comment 19

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 20

[Sayed [:LiquidO2]]

I can see this tool tip showing last sync time in latest nightly 58.0a1 in 64bit Linux.

Build ID 	20171012105833
User Agent 	Mozilla/5.0 (X11; Linux x86_64; rv:58.0) Gecko/20100101 Firefox/58.0