516884 - High-level UI to control major feature changes between TB2 and TB3

Status: RESOLVED FIXED

(Thunderbird :: Toolbars and Tabs, defect, P2)

Description

[David Ascher (:davida)]

Attachment: WIP v1

In TB3, we've made several changes which are sometimes shocking to long-time Thunderbird users, or which may not be optimal for some users.  This bug is about the idea of a UI in chrome that makes it really easy for people to a) find out about these changes on profile upgrade (especially from TB2, but possibly from earlier betas) what the changes are, and why they might or might not want to change them, and b) to undo the changes.

I'll attach my WIP patch, and a screenshot.

None of this is ready for review -- in particular the UI and the wording is very much a first cut, and I expect both will change a lot.

Comment 1

[David Ascher (:davida)]

Attachment: screenshot

Comment 2

[Ludovic Hirlimann [:Usul]]

(In reply to comment #1)
> Created an attachment (id=400940) [details]
> screenshot

Looks nice. How do you do the detection from 2.0 profile ?

Comment 3

[David Ascher (:davida)]

The current patch doesn't focus on profile upgrade.  But we already have code to detect 2->3 profile upgrade, and we could open this tab automatically in those cases.

Comment 4

[rsx11m]

Yes, that's good, also showing some easy explanations on benefits and possible reasons why a user may not want it. Some remarks on the current wording:

 - Synchronize Messages (Benefits): This should say "IMAP account" somewhere,
   and "This will also allow you to read your e-mails while not connected" or
   similar to have the "offline use" part covered. In the second part, adding
   "will download all your mail [by default]" could be added as the user can
   fine-tune the settings by switching it off for a folder or by message age.

 - Smart Folders (Benefits): maybe "[consolidated] view of all folders" to make
   more clear what it does? Also, is this applicable for single-account users?

 - New Toolbar (turn off): "This will also retain all your customizations for
   the main toolbar", one of the main points in the discussion about it. It's
   a bit confusing what "old" versus "new" exactly means. In essence, it's the
   same toolbar, the user is only asked to decide whether it should be reset
   (to add the new search item onto it) or keep its current layout (for the
   user to manually place the new search bar as desired).

Comment 5

[Joe Sabash [:JoeS1]]

I imagine your Synchronize Messages section should also clearly cover cases like:
http://groups.google.com/group/mozilla.support.thunderbird/msg/1adff2197faea528

Note that the poster is a very savvy user.

Comment 6

[David Ascher (:davida)]

Re: comment #5 -- that looks like a different bug...

Comment 7

[David Ascher (:davida)]

Attachment: patch v2

Ok, this is getting closer to reviewable.  Some notes on the code:

1) I've added support for persistence of the choices (in particular the toolbar changes), cause, well, duh.  I think philor will particularly appreciate the change to messenger.xul

2) The IMAP autosync UI is a bit more complex, because it's not always just "all on" or "all off".  

3) I think there are too many words.  So someone should do a serious editing pass.

4) I think it looks too boring.  Someone should make it pretty.

5) I haven't moved the strings to a locale file until we settle the words, to ease in testing the edits.

Let's start the code review while the html/css/dtd files get tweaked.

Comment 8

[David Ascher (:davida)]

Attachment: new screenshot

Comment 9

[Ludovic Hirlimann [:Usul]]

(In reply to comment #8)
> Created an attachment (id=401572) [details]
> new screenshot

Can we move auto-synch to be the last item, people might have more than one account and the list might end up hiding the other features.

Comment 10

[rsx11m]

Just wondering after encountering a profile which didn't have any toolbar customizations and therefore no mail-bar2 in localstore.rdf; what happens in this case, given that the default mail-bar2 definitions are no longer present anywhere and thus no old bar exists to recover the buttons from?

Comment 11

[David Ascher (:davida)]

rsx11m: good point. garrumph.  I guess we may have to ship some version of the old toolbar currentset somewhere.  Nice catch!

Comment 12

[Phil Ringnalda (:philor)]

Just so I don't lose this in yet another session-eating, half-done-review-killing Fx crash: you don't need to use an hbox and then kill its display yourself: unlike most of the uses of <data> in the tree, which seem to be things nobody is willing to touch just to move things that should be properties into a .properties file, if you use a <data> to hold the persist, you'll actually be using it in a reasonable way.

But you're right, I do love that bit of sneakiness :)

Comment 13

[David Ascher (:davida)]

Attachment: changes to the strings & trivial changes to the code

Given that string freeze is imminent, Bryan and I tweaked the layout, text, and unfortunately a teeeny bit the logic of the checkboxes.  Phil, let me know if you want me to apply whatever changes you have pending on this one.  (haven't deal with the <data> and no-upgrade paths yet).

Comment 14

[David Ascher (:davida)]

Attachment: updated screenshot

Comment 15

[Joe Sabash [:JoeS1]]

Attachment: EML version of the page

Maybe this will render differently when coming from chrome.
But I noticed the "New toolbar" and "smart folders" Benefits and Alternatives do not align according to your screenshot. (open the eml to see)
Also, a stark white background has been noted by some as "hard on the eyes"
I used #ffffee in the eml.

IMO this should be offered to users migrating from earlier versions of TB3 as well as TB2. Not sure if that option is included.

Comment 16

[David Ascher (:davida)]

I don't understand why bother w/ an eml version.  I attached a screenshot taken from chrome.

Comment 17

[Joe Sabash [:JoeS1]]

Well, maybe to show that "What you see" in the screenshot, might not be "what I get" in the final rendering.
I don't have the capability to apply the patch locally.
It might just be that chrome rendering of HTML Vs "in the message pane" is different enough to account for what I see.
Sorry if this was a distraction.

Comment 18

[Phil Ringnalda (:philor)]

That link to AMO? The one with two dozen query string param? What are we going to do instead of that, that isn't just begging to get broken in something that we've shipped, localized no less?

Comment 19

[David Ascher (:davida)]

So, one thing we could do is to use 

http://live.mozillamessaging.com/{locale}/header_link

and have that be a redirect we maintain on the server.

Works for you?

Comment 20

[Phil Ringnalda (:philor)]

Yep, but https:// please - encouraging people to install addons from an easily hijacked link that they should expect to redirect seems unhygienic.

Comment 21

[Phil Ringnalda (:philor)]

Attachment: After a denit round

Minus trailing whitespace, with newlines at the end of files, some random bits of spacing, and to not be totally nitty, using Cc and Ci inside FeatureConfigurator, since you had them defined already.

Comment 22

[Phil Ringnalda (:philor)]

I'm not entirely sure about the checkboxes: to me, while a button says "we're going to do this now" a checkbox says "once you decide to submit, it'll be like this." I seriously did scroll up and down looking for the submit button.

Comment 23

[David Ascher (:davida)]

Attachment: patch with DTDification

This version of the patch deals with:

1) moving all strings to a DTD
2) switching from an hbox to a data XUL element
3) adding the original TB2 defaultset for those non-upgrade paths (see Comment #10)
4) adding a parametrized pref which currently points at a URL that we're discussing in 518769 -- if that bug gets nixed, we can revisit this code to use a direct AMO link. 

This patch doesn't deal with philor's comment about checkboxes maybe feeling too form-like, and not immediate-action-like a button, pending discussion w/ bryan.

Note also: I still need to add localization notes to the DTD, but this will do for tonight.

Ludovic: we changed the layout to a columnar layout, so I don't think there's a problem that imap will push stuff below the fold too much -- it's also the most impactful change for some people (filling up disk space, etc.), so bryan felt it important to have it be the first.

Once I have the last two bits dealt with, I'll ask for a formal review.

Comment 24

[Philippe M. Chiasson (:gozer)]

(In reply to comment #19)
> So, one thing we could do is to use 
> 
> http://live.mozillamessaging.com/{locale}/header_link

https://live.mozillamessaging.com/thunderbird/header_link?locale=ab-CD

> and have that be a redirect we maintain on the server.
> 
> Works for you?

Certainly works for me.

Comment 25

[David Ascher (:davida)]

Attachment: patch for review

Ok, talked about it w/ bryan, and we all agree that the checkboxes are a bit confusing.  So I implemented columnar buttons like for autosync.  I'm going to assume that we're going w/ a live.mozillamessaging.com URL, but if that needs tweaking later, that's easy.

I've added localization nodes for all of the ones that seemed non-trivial, and in particular explaining some of the intent behind the wording for some of the labels.

There's one micro bug I found, which is that if people were in a non-all-folders mode, there's no real way for us to get back to that.  I can live with that given the value that this bug provides in general.

Comment 26

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 402911 [details] [diff] [review]
patch for review

I've already seen / worked this so I'm going to do a bit of web-dev code review.

As a CSS style nit you can use:
.button[invisible]
to test the existance of the invisible attr since you're just setting it to something and then removing it.  Instead of this way:
.button[invisible="true"]

Also:
+        for (let i = 0; i < bits.length; i++) {
+          let bit = bits[i];

for each(let bit in bits)

ui-r+ with something like those fixes

Comment 27

[Phil Ringnalda (:philor)]

(In reply to comment #26)
> for each(let bit in bits)

for ... in over arrays considered harmful: https://developer.mozilla.org/en/Core_JavaScript_1.5_Reference/Statements/for...in#Description

Comment 28

[Phil Ringnalda (:philor)]

cc:ing tb@l10n in hopes that if there's a l10n issue in the strings, someone will spot it faster and better than me :)

Comment 29

[Charles]

Hello David,

Looks promising, but there is nothing in the new Autosync UI screenshot about Sync-On-Demand...

It sounded like there was support for Bug 508276 (Provide UI for Sync On Demand preference) from the devs - so, is it just too much work to make it into TB3? I understand if that is the case, but I was hoping that since the *behavior* is already there (except for the per-folder capability), and it was just a UI issue, that it would make it into TB3.

Also, I thought my suggestion to:

1. make Sync-On-Demand the new *default* for new installs, and

2. honor existing settings for upgrades

made much more sense that defaulting to full offline mode.

Thanks for your hard work and consideration of our (users) ideas...

Comment 30

[Charles]

I just remembered an alternative I thought of back in May, but forgot to document it (and so of course forgot about completely) - that in fact makes more sense than either my 'Sync-On-Demand' bug or making full offline mode the default...

My primary concern has to do with the fact that a lot of my email consists of lots of tiny text messages, with a good number having large binary attachments. (Please, don't start arguing about how large attachments are evil - dealing with large binary attachments is absolutely necessary in the advertising world.)

So, instead of 'Sync-On-Demand' applying to all messages, have it apply *only* to *binary attachments*, and optionally, only binary attachments larger than a given size (I just modified Bug 508276 with this in mind, and added an updated screenshot).

When I thought of this, I posted a question on the doveoct mailing list, and Timo made it clear that it should be fairly easy to differentiate between binary attachments and plain body text (whether html or plain text) in every way necessary, so this should be doable. Of course, being that I'm not a programmer, I have no idea how difficult it would be, but from what Timo said it seems like it wouldn't be that difficult.

Anyway, here is the thread in question:

http://www.mail-archive.com/[e-mail]/msg18866.html

I hope you will take a few minutes to read it and see if this maybe makes more sense.

Thanks for all your hard work guys!

Comment 31

[Phil Ringnalda (:philor)]

Just so I don't forget, we need to deal with bug 509044 while setting/resetting the toolbar.

Comment 32

[David Ascher (:davida)]

Philor: while we're at it, we probably want to unearth the icon size, etc.

Charles: I think that some new UI to help people with fine grained control over when to download what would be a good thing to experiment with, but that's not what this bug is about. I very much don't want to complicate the options offered in this tab as that would make it less effective at its primary goal (to allow the undoing of some of the higher-impact changes in the 2->3 transition).  This new tab is not about adding any new configuration options, only providing a high-level way of managing existing options.    It strikes me that turning off autosync solves the critical problem for users with limited disk space or bandwidth.  What you're advocating for makes sense for some sets of users, but not a large enough proportion in my assessment to warrant complicating both the UI, and adding more back-end code, especially at this stage in TB3's release.

I'd be interested in working with you (and others) on the design of an add-on that provided control for the (many, many) different profiles of users regarding download/autosync/indexing features, but I won't have time to do that until after TB3 is out.  (Aside: let's do it on another bug, so this bug is easier to actually finish).

Comment 33

[Phil Ringnalda (:philor)]

Attachment: reviewed patch

Some CSS tidying, fixed the l10n note entity names, which were sometimes not the same as the entity and other times the same as some other entity, replaced double spaces between sentences since HTML's going to anyway, and added license blocks to the files that I didn't think could do without (not the skin CSS files, much as that would have amused me). Probably not perfect yet, since we should at least tweak the way we revert the toolbar, but I think this is good enough for landing pre-string-freeze.

Comment 34

[Phil Ringnalda (:philor)]

Comment on attachment 403190 [details] [diff] [review]
reviewed patch

Mark, could you take a quick look at the tab-type-adding bits? I'm not convinced I know enough about it to really review that part.

Comment 35

[Charles]

David: gotcha, and completely understand...

What will be available in TB3 is so far beyond TB2 that I really can't complain, even though being able to download just message body/headers without large attachments would to me be the end all/be all... ;)

Thanks for all you guys' hard work..

Comment 36

[Mark Banner (:standard8)]

Comment on attachment 403190 [details] [diff] [review]
reviewed patch

This doesn't actually work for the upgrade case, and breaks what's new as well - In openSpecialTabsOnStartup, currentApplicationVersion isn't defined.

Even if that is fixed, I'm not convinced it is right - I think your changes will make it so that the new tab is displayed only if the new version is 3.0.

So if someone updates from 2.0.0.x to 3.0.1 then they won't see the page.

I've not got time to look at fixing this tonight, but can do it tomorrow morning (my time) if you are needing to work on other things as well (hence leaving review-request open so I can find the bug).


Also, I'm not convinced about duplicating contentTab into chromeTab, as we're going to very quickly find ourselves with a job to keep both updated. Hence I think we should file a follow-up bug to investigate merging the two and having a openTab parameter to switch between the two.

Comment 37

[David Ascher (:davida)]

Attachment: [checked in] addressing upgrade issues

Good points all.  I refactored the upgrade detecting code a bit to yield more flexible semantics I think.  It seems to work for me.  From recollection of a conversation w/ clarkbw I believe we want the feature configurator to be the "second" tab, with the what's new tab being the third, so that we could even introduce it and point to it from the what's new tab content.

Comment 38

[Bryan Clark (DevTools PM) [:clarkbw]]

(In reply to comment #37)
> From recollection of a
> conversation w/ clarkbw I believe we want the feature configurator to be the
> "second" tab, with the what's new tab being the third, so that we could even
> introduce it and point to it from the what's new tab content.

Yes, that's the plan :)

Comment 39

[Mark Banner (:standard8)]

Comment on attachment 403428 [details] [diff] [review]
[checked in] addressing upgrade issues

Ok, this actually breaks when mstone == ignore in that it'll show the what's new page on every start up.

However, given the string impact I decided it is best to land for today's nightly with that broken and get some eyes on it.

I think the findbar is broken as well, though I haven't worked out why that is yet.

I'll take fixing both of these on in a follow-up patch.

Checked in: http://hg.mozilla.org/comm-central/rev/13a8ea0d02a9

Comment 40

[Mark Banner (:standard8)]

(In reply to comment #39)
> (From update of attachment 403428 [details] [diff] [review])
> Ok, this actually breaks when mstone == ignore in that it'll show the what's
> new page on every start up.

So I totally forgot about bloat and mozmill when I said that, so I ended up fixing it and checking it in as a bustage fix:

http://hg.mozilla.org/comm-central/rev/0a124a4beb0b

Comment 41

[Mark Banner (:standard8)]

Attachment: [checked in] Remove find bar on chrome tabs

I've just done some digging and it turns out that find bars want to use the "same type root docshell" for what they actually search within. That's fine for content browsers, but for chrome browsers it doesn't work as everything up to the window docshell is chrome.

So I think the easiest option for now is to disable the find bar - as we're not using chrome tabs for much, I don't think that will really matter.

Comment 42

[David Ascher (:davida)]

Comment on attachment 403475 [details] [diff] [review]
[checked in] Remove find bar on chrome tabs

I'm fine w/ removing the findbar from chrome tabs.  I'm not convinced by your analysis, though as in my experiments w/ the findbar and search results, I was able to find toplevel nodes, it's just that anonymous content nodes aren't walked by the find code.  But r=davida either way.

Comment 43

[Blake Winton (:bwinton) (:☕️)]

One other change that we've made is to set labelalign="end" on the toolbox by
default.  So we'll probably want to revert that when we revert the rest of the
toolbar changes.  (Just deleting the attribute should work.  If not, setting it
to "bottom" will.)

Comment 44

[Mark Banner (:standard8)]

Comment on attachment 403475 [details] [diff] [review]
[checked in] Remove find bar on chrome tabs

Checked in: http://hg.mozilla.org/comm-central/rev/ebfc3087b0b2

Comment 45

[Mark Banner (:standard8)]

Back to David for the toolbar change mentioned in comment 43.

Comment 46

[David Ascher (:davida)]

Attachment: labelalign patch, v1

Things are never simple.

The attached patch improves things, by restoring the labelalign property from TB2 profiles (unlikely to be set), as well as the iconsize (more likely).

Some remaining problems, not all of which are worth solving I suspect:

1) the persistence trick that we're using to uncover customization of the mail toolbar in the 3-pane window relied on the fact that we changed the ID of the toolbar.  That trick can't be used for either the toolbox itself (meaning we can't recover mode!="full"), or for the other toolbars (compose, addressbook, whatever else).  

The only way around that that I can think of would be to change the IDs of all toolboxen and toolbars, which doesn't seem worth the code churn, and the disruption.  The primary purpose of this button is to undo things for people who are not experienced customizers. 

2) We could loop over open windows (and should for 3panes at least), and tweak their toolbar's settings, and cause them to persist those.  Not ideal, but likely would help some users.

Other thoughts?

Comment 49

[u60234]

Comment on attachment 403428 [details] [diff] [review]
[checked in] addressing upgrade issues

>+<!-- LOCALIZATION NOTE (featureConfigurator.toolbar.original.status): The
>+word 'original' is chosen deliberately to be neutral, vs. 'old' which in
>+English at least had a negative connotation.-->
>+<!ENTITY featureConfigurator.toolbar.original.status "Using the original toolbar.">

<snip>

>+<!ENTITY featureConfigurator.toolbarAlternative.para "If you like the old toolbar configuration better, you don't want to re-customize it, or you don't use the message pane in your layout.">

Is "old toolbar configuration" a Freudian slip, or do we not need to be neutral here?

Comment 50

[David Ascher (:davida)]

A bad editing job for sure.  I'm not sure it's worth a new ID, but I'd like to fix the english string.

Comment 51

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 403850 [details] [diff] [review]
labelalign patch, v1

Let's start by getting this patch (or a derivative of it) checked in, and then we can figure out whether the rest of it is worth fixing/worth blocking on.

Thanks,
Blake.

Comment 52

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 403850 [details] [diff] [review]
labelalign patch, v1

seems good.  The customization I'm most worried about is the toolbar with the new changes we've presented.

Comment 53

[Phil Ringnalda (:philor)]

Comment on attachment 403850 [details] [diff] [review]
labelalign patch, v1

Need to replace that XXX comment with an ifdef, but other than that it should work.

Comment 54

[rsx11m]

BTW: Why are attachment 403428 [details] [diff] [review] and attachment 403475 [details] [diff] [review] marked
obsolete? If they were checked in (and they obviously are), they shouldn't be obsoleted by a follow-up patch.

Comment 55

[Joe Sabash [:JoeS1]]

Please see my comments in bug 522875 about screen size.

Comment 56

[Blake Winton (:bwinton) (:☕️)]

Attachment: [checked in] The previous patch, with fewer XXXs, and more ifdefs.

I also needed to set the iconsize attribute on the toolbox, to have the check box in the customization show up with the correct value.

And add the * in the jar.mn to get the featureConfigurator.js file preprocessed.

Thanks,
Blake.

Comment 57

[Phil Ringnalda (:philor)]

Comment on attachment 407032 [details] [diff] [review]
[checked in] The previous patch, with fewer XXXs, and more ifdefs.

http://hg.mozilla.org/comm-central/rev/8534aff23b0f and it's time to get out of this bug: if there are further tweaks, we can do them in separate bugs, if there are things still unaddressed scattered in comments here, they need to be moved out to their own bugs.

Comment 59

[Jochen De Smet]

I'm the reported of the bug that got marked as a dupe of this in the previous comment.  However in the new migration assistant I don't see anything about the "Global search and indexer", which was the particular feature that bug was about. So I'd request that my bug gets unduped as per comment #57, since it's not addressed yet.