Closed Bug 841480 Opened 7 years ago Closed 7 years ago

Change - Add Sync to Settings charm pane and move sync prefs to a new Sync panel

Categories

(Tracking Graveyard :: Metro Operations, defect, P1)

x86
Windows 8.1
defect

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: ywang, Assigned: bbondy)

References

Details

(Whiteboard: feature=change c=settings_pane_options_and_about u=metro_firefox_user p=3 status=verified)

Attachments

(6 files, 6 obsolete files)

53.40 KB, image/png
Details
37.02 KB, image/png
Details
46.08 KB, image/png
Details
12.44 KB, patch
bbondy
: review+
Details | Diff | Splinter Review
8.23 KB, patch
mbrubeck
: review+
Details | Diff | Splinter Review
1.27 MB, image/png
Details
Deeper flyout is acceptable in Settings, but not encouraged for content like Options. I walked through many Options flyout on Windows 8. None of them contains deeper flyouts.

That being said, my viewpoint is
Sync should be leveled up to entry point, since it will definitely contain deeper flyouts.

From Asa:
"I believe that we'll eventually have an "Account" or similar flyout hooked up to that top level in the Settings flyout that will be wired up to our next generation Sync stuff and perhaps that's where other profile actions, like clear private data and import from other browsers, should live. If so, maybe we create that space now and put our profile related features there."
Product: Firefox → Firefox for Metro
Whiteboard: [metro-mvp?]
Blocks: 831963
No longer blocks: metrov1triage
Whiteboard: [metro-mvp?] → feature=change
Blocks: metrov1it3
No longer blocks: 831963
Priority: -- → P1
Blocks: 831963
Whiteboard: feature=change → feature=change c=Settings_pane_options_and_about u=metro_firefox_user p=0
Whiteboard: feature=change c=Settings_pane_options_and_about u=metro_firefox_user p=0 → feature=change c=settings_pane_options_and_about u=metro_firefox_user p=0
Assignee: nobody → netzen
Whiteboard: feature=change c=settings_pane_options_and_about u=metro_firefox_user p=0 → feature=change c=settings_pane_options_and_about u=metro_firefox_user p=2
Flags: needinfo?(ywang)
Status: NEW → ASSIGNED
Brian, I've been reworking layout of Option over in bug 841511. I've pushed sync down to the bottom to get it out of the way. I'll post a patch there, that work might conflict with your browser.xul work here without it.
Let's add "Sync" after "Options" for now, since "Options" will be the mostly used entry point.
- Options
- Sync
- About
- Help(online)
- Permissions


"Account" is usually the top entry point in Metro apps. Once we are ready to integrate Metro with the next generation Sync, we will change "Sync" to "Account", and promote it on top of "Options".
Flags: needinfo?(ywang)
Attached patch Patch v1 - Widget changes (obsolete) — Splinter Review
Attachment #718578 - Flags: review?(jmathies)
Attachment #718579 - Flags: review?(mbrubeck)
Yuan I wanted to pass by some design decisions through you.  Please let me know if you have any objections.
- I put sync where you said after Options.
- The item just says "Sync" in the Windows settings panel.
- When clicked it brings up the setup pairing screen where it gives you 3 sets of numbers to put into another browser.
- After being successfully setup the item disappears from the Windows settings panel.
- The items in Firefox Options remain so you can disconnect your sync settings if you want to.

Mainly the decisions were made because the Windows settings panel can only have single text links only and no other labels or buttons.
Flags: needinfo?(ywang)
Comment on attachment 718578 [details] [diff] [review]
Patch v1 - Widget changes

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

::: widget/windows/winrt/nsWinMetroUtils.cpp
@@ +314,5 @@
> +  NS_ENSURE_ARG_POINTER(aOutIndex);
> +  if (!sSettingsArray)
> +    return NS_ERROR_UNEXPECTED;
> +  if (aIndex > sSettingsArray->Length())
> +    return NS_ERROR_UNEXPECTED;

I'd suggest return NS_ERROR_ILLEGAL_VALUE for index out of bounds errors.
Attachment #718578 - Flags: review?(jmathies) → review+
Comment on attachment 718579 [details] [diff] [review]
Patch v1 - WeaveGlue fixes so sync setup will work again

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

::: browser/metro/base/content/sync.js
@@ +31,4 @@
>      Services.obs.addObserver(function onReady() {
>        Services.obs.removeObserver(onReady, "weave:service:ready");
> +      self._init();
> +    }, "weave:service:ready", false);

I just landed a fix for this in bug 840643, so you can drop this change.  Sorry for the duplicated work.

@@ +123,5 @@
>        onComplete: function onComplete(aCredentials) {
>          self.jpake = null;
>  
> +        if (self._progressBar) {
> +          self._progressBar.mode = "determined";

Rather than putting if statements everywhere, let's just make WeaveGlue._progressBar into a lazy getter so it's guaranteed to work.
Attachment #718579 - Flags: review?(mbrubeck) → review+
Attached patch Patch v1 - Widget changes (obsolete) — Splinter Review
Applied widget nit
Attachment #718578 - Attachment is obsolete: true
Attachment #718614 - Flags: review+
Comment on attachment 718580 [details] [diff] [review]
Patch v1 - Promote sync to settings panel w/ ability to add/remove items

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

::: browser/metro/base/content/browser-ui.js
@@ +1704,5 @@
> +    switch (aTopic) {
> +      case "metro-settings-entry-selected":
> +        let entry = this._entries[parseInt(aData, 10)];
> +        if (entry)
> +          entry.onselected();

There's a possibility of a race condition here.  If we display the Settings charm, then we add or remove an entry, and then the user clicks on one of the already-displayed entries, we'll receive the wrong index.

Perhaps we should match by label rather than by index.  Or we could force a refresh of the Settings pane if it's open when an entry changes.  I think it's worth doing something here, since Sync setup/connection happens asynchronously it seems likely that some users will hit this bug.

@@ +1708,5 @@
> +          entry.onselected();
> +      break;
> +#ifdef MOZ_SERVICES_SYNC
> +      case "weave:service:setup-complete":
> +        this.removeEntryAt(this._syncSettingIndex);

Just to be safe, we should check 'if ("_syncSettingIndex" in this)' before calling removeEntryAt, and we should "delete this._syncSettingIndex;" afterwards.
Attachment #718580 - Flags: review?(mbrubeck) → review-
(In reply to Brian R. Bondy [:bbondy] from comment #6)
> Yuan I wanted to pass by some design decisions through you.  Please let me
> know if you have any objections.
> - I put sync where you said after Options.
> - The item just says "Sync" in the Windows settings panel.
> - When clicked it brings up the setup pairing screen where it gives you 3
> sets of numbers to put into another browser.
> - After being successfully setup the item disappears from the Windows
> settings panel.
> - The items in Firefox Options remain so you can disconnect your sync
> settings if you want to.
> 
> Mainly the decisions were made because the Windows settings panel can only
> have single text links only and no other labels or buttons.

Thanks for the comment, Brian.

I understand the limits of texts and links only on settings panel. And you are right, the item should just say "Sync". 

We need to move everything related to Sync out of Options flyout if we are going to promote Sync. 

That means we need a flyout page that is titled "Sync". It opens after the users tap on "Sync" from setting panel. At this stage, on this flyout page, the Sync UI can remain the same as the current Sync UI in options flyout. (a separate bug has been filed for using Win8 UI elements for Sync)

Note: After the users have set up Sync, the "Sync" entry point should still appear on setting panel. Tapping on it should open that "Sync" flyout page that shows the option to pair a device, and other syncing status.
 
Let me know if you have any question. Feel free to attach screenshot and ask for info you need.
Flags: needinfo?(ywang)
Summary: Change - Promote Sync to the top level flyout → Change - Promote Sync to the top level flyout AND create a new sync flyout
Whiteboard: feature=change c=settings_pane_options_and_about u=metro_firefox_user p=2 → feature=change c=settings_pane_options_and_about u=metro_firefox_user p=3
Attachment #718579 - Attachment is obsolete: true
Attachment #718580 - Attachment is obsolete: true
Attachment #718614 - Attachment is obsolete: true
Well that's a completely different task then what I understood this bug to be and spent the day doing. OK.

I don't think I understand the "deeper flyout" comment in Comment 0 because it's my understanding that having a sync flyout panel would make it at the same depth level. It's just moved from one flyout into another.
To avoid more unneeded work blocking on Comment 12 to understand the deeper flyout comment in case I've misunderstood some part of this work.
Flags: needinfo?(ywang)
Summary: Change - Promote Sync to the top level flyout AND create a new sync flyout → Change - Add Sync to Settings pane and move sync prefs to a new Sync panel
I think the work Bryan did is good enough for v1. We can re-visit more sync work in v2. Let's take this and push the rest off.

Brian, let's revert this back to what you signed up for and land that. We can do more later.
Based on Asa's comment, (In reply to Brian R. Bondy [:bbondy] from comment #12)
> Well that's a completely different task then what I understood this bug to
> be and spent the day doing. OK.
> 
> I don't think I understand the "deeper flyout" comment in Comment 0 because
> it's my understanding that having a sync flyout panel would make it at the
> same depth level. It's just moved from one flyout into another.

Brian, sorry about the confusion.
In my proposal, this bug was supposed to be dependent on another Bug 845468. 
You are correct. If the new layout will not get implemented, this Sync flyout panel is going to be at the same depth level as before.

I still think Sync related options should eventually move out of the options flyout, even though, some flows, such as "pairing a device" will be on the same depth level. Options are supposed to be a flat sheet that contains simple controls. Sync contains too much page transitions. It's better to create its own flyout to handle them.

I'm fine with landing what you have implemented for v1, if there is no time to revisit and complete Bug 845468.
Flags: needinfo?(ywang)
Since the end game result is bug 845468 and since I have unfinished review comments to implement, I'll just do the new flyout panel. I think it will be only slightly more work than the unfinished review comments anyway.
Attached patch Patch v1 - Sync pane (obsolete) — Splinter Review
The restyling will be done in a different bug.
Attachment #718779 - Flags: review?(mbrubeck)
Attached image Settings charm Pane
Attachment #718781 - Flags: ui-review?(ywang)
This is changing in another bug, so this is just for the context of this bug.
Attachment #718782 - Flags: ui-review?(ywang)
Ditto this is just the temporary look until the other bug is finished
Attachment #718784 - Flags: ui-review?(ywang)
Attachment #718781 - Flags: ui-review?(ywang) → ui-review+
Comment on attachment 718784 [details]
Sync panel connected w/ details expanded

Looks good to me.

2 small suggestions
Since now Sync has it own flyout, I think we can remove the Details toggle button. Just display all the information by default.

And the bold title "Sync" can be removed, as there is a header "Sync" already. 

I know those changes are out of scope in this bug, but it'll be great to have them fixed together :) I will give Review+ anyway.
Attachment #718784 - Flags: ui-review?(ywang) → ui-review+
Attachment #718782 - Flags: ui-review?(ywang) → ui-review+
(In reply to Brian R. Bondy [:bbondy] from comment #19)
> Created attachment 718782 [details]
> Sync pane - not connected

This really needs a description of what sync is in it. Maybe that's in the other bug?
Attachment #718779 - Flags: review?(mbrubeck) → review+
I'll land this since it's reviewed and do a followup for the title removing and details button removal.
Actually I have to wait because my patch doesn't apply to m-i because of jimm's patch, so I'll rebase it and test on m-c once that gets merged.  Thanks for the late night review by the way Matt!  I'll post Yuan's couple changes into his bug, just finishing them up.
Rebased, carrying forward r+.
Attachment #718779 - Attachment is obsolete: true
Attachment #718999 - Flags: review+
Attachment #718807 - Attachment is obsolete: true
Attachment #718807 - Flags: review?(mbrubeck)
Attachment #719000 - Flags: review?(mbrubeck)
Summary: Change - Add Sync to Settings pane and move sync prefs to a new Sync panel → Change - Add Sync to Settings charm pane and move sync prefs to a new Sync panel
Comment on attachment 719000 [details] [diff] [review]
Patch v2 - Remove bold title, remove details button, expand all - Rebased to m-c tip

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

::: browser/metro/locales/en-US/chrome/sync.dtd
@@ -8,4 @@
>  <!ENTITY sync.notconnected          "Not connected">
>  <!ENTITY sync.connect               "Connect">
>  <!ENTITY sync.connected             "Connected">
>  <!ENTITY sync.details               "Details">

I think you can remove sync.details too.
Attachment #719000 - Flags: review?(mbrubeck) → review+
Component: General → Metro Operations
Flags: ui-review+
Product: Firefox for Metro → Tracking
Target Milestone: Firefox 22 → ---
Version: Trunk → ---
Flags: needinfo?(jbecerra)
Tested on 2013-02-28 with a nightly build from http://hg.mozilla.org/mozilla-central/rev/c65d59d33aa8
- The Sync panel looks like third attachment with the revisions from the description in the patch in attachment 5 [details].
- It has a main Sync panel heading, followed with four entries: Connected, Last sync, This device, Account and their respective elements (button or text field).
Status: RESOLVED → VERIFIED
Flags: needinfo?(jbecerra)
Whiteboard: feature=change c=settings_pane_options_and_about u=metro_firefox_user p=3 → feature=change c=settings_pane_options_and_about u=metro_firefox_user p=3 status=verified
Depends on: 847477
Attached image sync.png
User Agent: Mozilla/5.0 (Windows NT 6.2; Win64; x64; rv:26.0) Gecko/20100101 Firefox/26.0
Build ID: 20130812030209
Built from http://hg.mozilla.org/mozilla-central/rev/87c1796bc46c

WFM
Tested on windows 8 using latest nightly for iteration-11. I saw the screen attached here.
OS: Windows 8 Metro → Windows 8.1
Product: Tracking → Tracking Graveyard
You need to log in before you can comment on or make changes to this bug.