Currently we repeat the device name as a tooltip instead of showing the last sync time. Is it available? Let's use it here.

I may be wrong but I don't think we have per-device last sync info in the clients collection, thoughts Mark?

I think we do know the "last modified" date, which is a good approximation - although it will be wrong when we send a command to that device, and possibly in the future if we allow devices to (eg) rename other devices. We could maybe consider adding a last-sync date to the client record, but that doesn't help Android or iOS - Richard and Nick, what do you think about having each client track its own "last sync" date and maintaining that in its server record? If we agree on that approach we might be able to prefer that new field but fallback to last-modified if it doesn't exist (in the hope that it soon will)?

I wonder what iOS using to show the last sync data on their synced tabs screen?

Redirecting this to Robin who can turn this into actionable bugs if we need parity with desktop or android.

(In reply to Stefan Arentz [:st3fan] from comment #4) > Redirecting this to Robin who can turn this into actionable bugs if we need > parity with desktop or android. Luckily iOS already listed a date called "Last synced" in the Synced Tabs view (cloud icon). I just wonder where the number is coming from (question for engineering)

(In reply to Ryan Feeley [:rfeeley] from comment #5) > (In reply to Stefan Arentz [:st3fan] from comment #4) > > Redirecting this to Robin who can turn this into actionable bugs if we need > > parity with desktop or android. > > Luckily iOS already listed a date called "Last synced" in the Synced Tabs > view (cloud icon). I just wonder where the number is coming from (question > for engineering) Android: For remote clients, we use the last modified date from the service. See https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/RemoteTabsExpandableListAdapter.java#172, which is just passing through the last modified field. For the local client, we use the server's response timestamp for when we're done. See https://dxr.mozilla.org/mozilla-central/source/mobile/android/services/src/main/java/org/mozilla/gecko/fxa/activities/FxAccountStatusFragment.java#650. IIRC, iOS is the same.

Thanks all - I think it's fine to go with last-modified for now even if it might be wrong in unlikely edge-cases. FTR, I think one such edge-case will be: (a) client-1 syncs and doesn't sync again. (b) one week later client-2 tries to send tab to client-1. (c) all clients will now see last-synced as the time of (b) rather than (a)

Comment on attachment 8748890 [details] [diff] [review] bug-1269348.patch Review of attachment 8748890 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/syncedtabs/TabListView.js @@ +204,5 @@ > * @param {Element} itemNode - Element to update. > */ > _updateClient(item, itemNode) { > itemNode.setAttribute("id", "item-" + item.id); > + let lastSync = new Date(item.lastModified * 1000); I think we should create a new function in browser-syncui - something like formatLastSyncDate(date) so we don't need to duplicate this cruft and remember to update it when we change to a relative time - you should be able to get at it via getChromeWindow().gSyncUI... But taking that further, I see no reason not to reuse the string here - I don't see how it could be considered a different context - it's a tooltip that shows when something was last synced - so maybe we should just have something like getLastSyncedTooltip(date) which returns the entire string?

Actually I'm not sure about factoring this function, maybe we need a different format string. Here's why: when a device hasn't synced for > 1 week, it's impossible to tell when it was synced for the last time since we only show the day and not the day of the month. Ryan do you have any thoughts on this?

(In reply to Edouard Oger [:eoger] from comment #10) > Actually I'm not sure about factoring this function, maybe we need a > different format string. Here's why: when a device hasn't synced for > 1 > week, it's impossible to tell when it was synced for the last time since we > only show the day and not the day of the month. But why wouldn't that be true if the *current* device hadn't synced for that long? Also, it looks to me like that patch you requested review on has an identical string? I could certainly agree that we may use a different format string depending on the |date| param (ie, so could still be in a single function that selects the format string based purely on the param), I just don't see why we would want a different string based on whether it is the remote device or the current device.

There is a big chance the current device will have synced in the same day (unless the network connectivity is off), but the probably that you don't sync on another device for more than 1 week is more significant I think. (The patch doesn't reflect this of course)

I agree there's a better chance the local device has synced more recently - but there's no guarantee - eg, a laptop that hasn't been opened for 2 weeks will have the same problem (last sync date for the current device could be misleading in that case) And coming from the other direction: there's still a good chance the remove devices *have* synced regularly and are syncing at the same rate as the current device. But in both cases, they are just chances - it still makes sense to me to have the same logic in both cases, even if we believe the current device hits one case more often than the other. ie, instead of designing such that we get an optimal outcome only if what we think is likely turns out to be true, we get an optimal outcome in all cases.

Can we stick with the current format if it's in the past 6 days but show "Last sync: {Month} {Day}" if it's before that. That should be fine for anyone syncing in the past year.

I factorized the date formatting code and created 2 paths for recent dates and dates > 6 days

Comment on attachment 8750428 [details] [diff] [review] bug-1269348.patch Review of attachment 8750428 [details] [diff] [review]: ----------------------------------------------------------------- ::: services/sync/modules/engines/tabs.js @@ +261,5 @@ > create: function (record) { > this._log.debug("Adding remote tabs from " + record.clientName); > + this._remoteClients[record.id] = Object.assign({}, record.cleartext, { > + lastModified: record.data.modified > + }); Thanks Edouard, This is looking great, but I think we can tweak it a little. I mentioned this in IRC but you might have missed it. What I think we should do is something like: * Use record.modified here, like is done below (they are equivalent, but we should be consistent). * Given record.modified is seconds, the existing code below looks (a) broken due to the "/ 1000", and (b) unused according to dxr. We should open a bug to remove that code with a needinfo? on rnewman to verify it truly is dead and add a comment to this effect referencing that bug here. * Having .modified in our "synced tabs" abstraction being seconds rather than ms will be confusing, so I think SyncedTabs.jsm should convert the timestamp to ms (which makes sense, as SyncedTabs.jsm is already the bridge between Sync abstractions and the rest of the browser). * TabListView can then use the .modified attribute directly without adjusting it.

Review commit: https://reviewboard.mozilla.org/r/52257/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/52257/

Comment on attachment 8751872 [details] MozReview Request: Bug 1269348 - Show last sync date tooltip on Synced Tabs sidebar device names. r=markh https://reviewboard.mozilla.org/r/52257/#review49299 Looks great, thanks - please add the new comment and land it! ::: services/sync/modules/engines/tabs.js:265 (Diff revision 1) > > create: function (record) { > this._log.debug("Adding remote tabs from " + record.clientName); > - this._remoteClients[record.id] = record.cleartext; > + this._remoteClients[record.id] = Object.assign({}, record.cleartext, { > + lastModified: record.modified > + }); can you please open a bug to remove the the code following this, then add a comment here indicating it appears broken, unused and referencing the bug#

Comment on attachment 8751872 [details] MozReview Request: Bug 1269348 - Show last sync date tooltip on Synced Tabs sidebar device names. r=markh Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52257/diff/1-2/

Comment on attachment 8751872 [details] MozReview Request: Bug 1269348 - Show last sync date tooltip on Synced Tabs sidebar device names. r=markh https://reviewboard.mozilla.org/r/52257/#review49662

Backed out in https://hg.mozilla.org/integration/fx-team/rev/d81b1964bb6b for https://treeherder.mozilla.org/logviewer.html#?job_id=9387531&repo=fx-team

Comment on attachment 8751872 [details] MozReview Request: Bug 1269348 - Show last sync date tooltip on Synced Tabs sidebar device names. r=markh Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52257/diff/2-3/

Trying to verify this implementation and have the following comments: 1. I'm not aware of any way to insert sync records, therefore the testing I was able to do was split into using Sync history and live sync scenarios in a rather limited time frame (past 2 days). 2. I cannot verify the change that contains the formatting difference between recent dates and dates > 6 days as specified in Comment15. (maybe this has something to do with the fact that the sync data which is older than 6 days is historic sync data?) 3. Using the current date time format is confusing when having syncs that are older than 1 week (scenario from Comment10) - this is covered by bug 1184265 (which should be applied to both sync and sync sidebar). I would suggest that the current bug/feature should not shipped without the relative date implementation from 1184265. 4. Why use tool-tip for "Last Sync"? There are three elements that we have in the sidebar: device name, last sync and the tabs. The device name and last sync should be paired when displayed and I don't see any reason not to use the Android model: see attached screenshot. If on a mobile device there is enough space to display both the "Device name" and "Last sync" I believe we should have enough space as well.

(In reply to Adrian Florinescu [:AdrianSV] from comment #27) > Created attachment 8759539 [details] > Sync Desktop vs Android > > Trying to verify this implementation and have the following comments: > > 1. I'm not aware of any way to insert sync records, therefore the testing I > was able to do was split into using Sync history and live sync scenarios in > a rather limited time frame (past 2 days). > 2. I cannot verify the change that contains the formatting difference > between recent dates and dates > 6 days as specified in Comment15. (maybe > this has something to do with the fact that the sync data which is older > than 6 days is historic sync data?) I don't quite understand the above 2 comments - (1) says you only tested over 2 days, but (2) says you expected to see a change that only kicks in after 6 days? > 3. Using the current date time format is confusing when having syncs that > are older than 1 week (scenario from Comment10) - this is covered by bug > 1184265 (which should be applied to both sync and sync sidebar). I would > suggest that the current bug/feature should not shipped without the relative > date implementation from 1184265. I understand what we have isn't ideal, but without this bug there is literally no way to see when a device last synced. I don't agree that is better than the (admittedly imperfect) implementation in this bug, especially given the way we show the last-sync for other devices is the same as how we show it for the current device. > 4. Why use tool-tip for "Last Sync"? There are three elements that we have > in the sidebar: device name, last sync and the tabs. The device name and > last sync should be paired when displayed and I don't see any reason not to > use the Android model: see attached screenshot. If on a mobile device there > is enough space to display both the "Device name" and "Last sync" I believe > we should have enough space as well. The sidebars all use tree controls on Desktop. It's not clear that it makes sense to try and integrate a collapsible tree control with nested children with the model used by Android. I agree it might make some sense in the future to look at not using a tree here and trying to come up with something similar to what Android does, but that's a much bigger effort.

What I meant in comment 1 and 2 was that I tested with a sync profile that had historic data - sync devices that were synced in the past (you can see in the attached screenshot) and only 2 days of syncs performed with the build that contained this feature and at the time I thought that this might be the reason for which I don't get the date change that was supposed to kick in after 6 days. I have 7 days data now and the change in date that is supposed to kick in after 6 days isn't there: the date remains in the same format: Friday 06:10 PM. If I were to relate only to the validity of the date shown, ignoring the fact that I cannot verify which Tuesday it shows me as last sync (got a Tuesday 4:14PM and a Tuesday 4:11PM in different weeks), I would conclude that the last sync date is correct. Referring to the rest of the issues that were brought up in 3. and 4. that I consider them to be the same importance as the "last sync date correctness": summing that up I conclude that we sometimes expect a bit too much from our users and it would be nice that the end-users don't receive a half-baked versions rather than having nothing. For this feature especially we need to take in consideration that most of the users are going to come experiencing the same usability as they were used to (or close enough) on mobile devices(IOS/Android) and nowadays an user line of though would be: "I don't necessarily need that feature, but when I get it, it better be close to what I expect".

(In reply to Adrian Florinescu [:AdrianSV] from comment #29) > What I meant in comment 1 and 2 was that I tested with a sync profile that > had historic data - sync devices that were synced in the past (you can see > in the attached screenshot) and only 2 days of syncs performed with the > build that contained this feature and at the time I thought that this might > be the reason for which I don't get the date change that was supposed to > kick in after 6 days. > I have 7 days data now and the change in date that is supposed to kick in > after 6 days isn't there: the date remains in the same format: Friday 06:10 > PM. Edouard, are you able to have a look at this? > Referring to the rest of the issues that were brought up in 3. and 4. that I > consider them to be the same importance as the "last sync date correctness": > summing that up I conclude that we sometimes expect a bit too much from our > users and it would be nice that the end-users don't receive a half-baked > versions rather than having nothing. I'm sympathetic to that and I agree it would be nice, but that's not the current reality - it was this or nothing. Our UX team decided this was better than nothing. > For this feature especially we need to > take in consideration that most of the users are going to come experiencing > the same usability as they were used to (or close enough) on mobile > devices(IOS/Android) and nowadays an user line of though would be: "I don't > necessarily need that feature, but when I get it, it better be close to what > I expect". I don't quite agree with that, or we would be arguing that the entire bookmarks sidebar is wrong as it doesn't mimic how Android shows bookmarks. It's reasonable in general for desktop and mobile to leverage their different screen sizes and also a simple reality that changing all sidebars to mimic Android is significant work that would come at the expense of other things that are considered more important. Regardless though, note that I'm not the decision maker for any of this stuff, I'm just trying to help you understand how the decisions were made by the people who are. I'd love to see bug 1186262 fixed which would allow us to fix 1/2 of your objections.

(In reply to Mark Hammond [:markh] from comment #30) > I don't quite agree with that, or we would be arguing that the entire > bookmarks sidebar is wrong as it doesn't mimic how Android shows bookmarks. > It's reasonable in general for desktop and mobile to leverage their > different screen sizes and also a simple reality that changing all sidebars > to mimic Android is significant work that would come at the expense of other > things that are considered more important. I'm not generally advocating for mimicking mobile interfaces, but in the current bug case it was easier exemplifying with the mobile model which has all the requisites when discussing about "last sync date" feature being intuitive, usable and user-friendly. > Regardless though, note that I'm not the decision maker for any of this > stuff, I'm just trying to help you understand how the decisions were made by > the people who are. I'd love to see bug 1186262 fixed which would allow us > to fix 1/2 of your objections. I'm aware of the fact that neither of us has any decision power over this, but I feel that it is well within our responsibility to provide enough information to the people that have that decision power. Fixing 1186262 would be really nice.

I could not reproduce the issue on my side

I have reproduced this bug with Nightly 49.0a1 (2016-05-02), on Windows 8.1 , 64 bit! This bug's fix is verified on Latest Aurora 49.0a2. Build ID : 20160720004018 User Agent : Mozilla/5.0 (Windows NT 6.3; WOW64; rv:49.0) Gecko/20100101 Firefox/49.0 [bugday-20160720]

Reproduced this issue in firefox aurora 48.0a2 (2016-05-02) with ubuntu 16.04 (64 bit) Verified as this issue fixed with latest firefox aurora 49.0a2 (Build ID: 20160722004032) Mozilla/5.0 (X11; Linux x86_64; rv:49.0) Gecko/20100101 Firefox/49.0 As it is also verified on windows (Comment 33), Marking it as verified!