731145 - Provide a checkbox for "default offline.autoDetect" option

Status: RESOLVED FIXED

(Thunderbird :: Preferences, defect)

Description

[Martin Stránský [:stransky] (ni? me)]

+++ This bug was initially created as a clone of Bug #391732 +++

I believe Thunderbird should provide a checkbox for it (in Offline Settings dialog), because the offline mode is more used than in Firefox, the online/offline status autodetection may be buggy (depends on wifi drivers and so) or the user may want to control it manually (bandwidth save for instance).

Comment 1

[Martin Stránský [:stransky] (ni? me)]

Btw. Why is the "File -> Offline -> Offline Settings..." disabled? It still works from "Edit -> Preferences -> Advanced -> Network & Disk Space".

Comment 2

[Martin Stránský [:stransky] (ni? me)]

Attachment: patch

I hope it's correct, it's my first GUI patch ;-) Mark, I guess you're a right person to review this one?

Comment 3

[Martin Stránský [:stransky] (ni? me)]

Attachment: how it looks like

Comment 4

[Magnus Melin [:mkmelin]]

Maybe the wording should be something like "Detect network state automatically"? I don't think it's clear what is automatic as is.

Please also add an accesskey, and the file header should be mpl2 now
http://www.mozilla.org/MPL/headers/

Comment 5

[Martin Stránský [:stransky] (ni? me)]

Attachment: v2

Thanks for the comments, there's the updated one.

Comment 6

[Mark Banner (:standard8)]

Comment on attachment 604366 [details] [diff] [review]
v2

Start off with ui/ux review, then we'll review the code once the experience is okayed.

Comment 7

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

Comment on attachment 604366 [details] [diff] [review]
v2

So, I've got some UI comments below, but while I was trying to find them a few things jumped out at me…

>+++ b/mail/components/preferences/offline.js
>@@ -0,0 +1,49 @@
>+# -*- Mode: Java; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*-
>+# ***** BEGIN LICENSE BLOCK *****
>+# Version: MPL 1.1/GPL 2.0/LGPL 2.1

(Just to save time in your next review, this should be the much shorter MPL2 header…)

>+++ b/mail/components/preferences/offline.xul
>@@ -43,26 +43,35 @@
> <prefwindow id="OfflineSettingsDialog" type="child"
>             xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
>+            onload="gOfflineDialog.dialogSetup();"
>             dlgbuttons="accept,cancel"
>             title="&offlineDialog.title;">
> 
>   <prefpane id="OfflineSettingsDialogPane">
>     <preferences id="OfflineSettingsPreferences">
>+      <preference id="offline.autoDetect"                    name="offline.autoDetect"    type="bool" 
>+                  onchange="gOfflineDialog.dialogSetup();"/>

Why are you calling this function twice?

>+++ b/mail/locales/en-US/chrome/messenger/preferences/offline.dtd
>@@ -1,10 +1,12 @@
>+<!ENTITY textAutoDetect "Detect network state automatically">

Since all the other strings here use "online state", I think this should be "Detect online state automatically".

As for the UI…

It seems to me that the "autoDetect" you've implemented kind of controls the rest of the screen, yet it's at the same level of indent as the other things.  (Check the "Manual proxy configuration" option in the Connection Settings dialog for an example of how I think it should look.)

But that kind of raises another problem.  It seems to me that I would want to be able to choose whether to download or send messages even if I had chosen to detect the network state automatically.  So those shouldn't be enabled or disabled.  And at that point, I think it might be clearer to just make it one of the options in the "When starting up" option list.  (Which should probably be renamed at that point, since one of the options won't be only for startup.)

So, given all that, I think I'm going to have to say ui-r-, but hopefully I haven't added too much work for you, and I promise that I'll ui-review your next version of this patch _much_ sooner!

Thanks,
Blake.

Comment 8

[Martin Stránský [:stransky] (ni? me)]

Attachment: v3 - image

Comment 9

[Martin Stránský [:stransky] (ni? me)]

Attachment: patch, v3

Ohh, I missed your reply, sorry for the delay. There's a new version which should address your comments. 

Thanks!

Comment 10

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

Comment on attachment 650883 [details] [diff] [review]
patch, v3

Perfect!  ui-r=me!

Thanks,
Blake.

Comment 11

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 650883 [details] [diff] [review]
patch, v3

Thanks! Mark, can you please look at it?

Comment 12

[Mark Banner (:standard8)]

Comment on attachment 650883 [details] [diff] [review]
patch, v3

I think you've missed handling the case where automatic detection is on but one of the non-always start up online options. With the patch, if I've got always start offline selected but then choose automatic, it'll still come up offline.

Not quite sure what to do here. I can't remember if automatic detection still works in that instance or not.

Comment 13

[Martin Stránský [:stransky] (ni? me)]

Ahh, it looks like we need some changes in offlineStartup.js

Comment 14

[Martin Stránský [:stransky] (ni? me)]

Hm, it looks more complicated than I expected. Because there's a start-up state (offline.startup_state) which can be:

0 = remember last state, 1 = ask me, 2 == online, 3 == offline, 4 = automatic

Note that the "3 == offline and 4 = automatic" states are used by seamonkey only.

And there's offline.autoDetect which controls whether the link state can be managed by system or not. 

Another online/offline state control is in Menu, File->Offline->Work Offline check box, which controls the recent online/offline state and is independent on the start-up state (and this state is not saved).

So I guess we need to provide a check-box for the offline.autoDetect (to enable link-control by system) and add the missing start-up states (offline, autodetection).

Comment 15

[Martin Stránský [:stransky] (ni? me)]

Attachment: v4 - image

Comment 16

[Martin Stránský [:stransky] (ni? me)]

Attachment: v4 patch

I believe this one is more accurate. It adds missing start-up states and allows to enable/disable automatic online state detection when thunderbird is running.

Comment 17

[Magnus Melin [:mkmelin]]

I'd suggest 
Automatic detection -> Use detected system online state

I'd hesitate to have UI for "always start up offline". The use case is odd, and the list is very long already. Ideally we should just be able to always just follow system state.

Comment 18

[Martin Stránský [:stransky] (ni? me)]

The "always start up offline" is useful when you're connected over some expensive connection (like GSM phone) and don't want to access the net just after the TB start.

Comment 19

[Magnus Melin [:mkmelin]]

But who's "always" connected that way? In such cases i'd still much prefer if it just asks me.

Comment 20

[Martin Stránský [:stransky] (ni? me)]

"always" does not have to mean "forever". It could be "for this 14 days when I'm traveling" for instance.

Comment 21

[Magnus Melin [:mkmelin]]

Sure, but i doubt people go into preferences for such things.

Comment 22

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

Comment on attachment 656405 [details] [diff] [review]
v4 patch

This seems like a step back from https://bugzilla.mozilla.org/attachment.cgi?id=650882  It's adding a lot of extra choices that I don't believe most people will want most of the time.  (Just because the back-end is complicated doesn't mean that the front-end needs to also be complicated.  ;)

Thanks,
Blake.

Comment 23

[Martin Stránský [:stransky] (ni? me)]

Okay. Do you have a problem with the "When running" part or with "When starting up"? Because I have no idea how to combine the two controls (start-up state and automatic link control) to the one.

Comment 24

[Magnus Melin [:mkmelin]]

I guess it's like https://bugzilla.mozilla.org/attachment.cgi?id=650882 - if system state is enabled, the startup options would be disabled and automatically use system state, why would you use system state only while running?

---
[x] Automatically follow detected system online state

Manual online state when starting up:
[o] Remember from last time
[ ] Ask me
[ ] Online

---

Comment 25

[Martin Stránský [:stransky] (ni? me)]

I'm not going to work on that anytime soon.

Comment 27

[Or Schiro]

Any chance that this will be picked up any time soon?

A checkbox would indeed be helpful for users to discover this useful option. 

Thanks

Comment 28

[:aceman]

Attachment: v5 path

I am actually in favor of the "manual offline" option so I have kept it. So this is mkmelin's proposal (automatic disabled all the manual options) + the one offline option. And the offline.autoDetect and offline.startup_state == 4 are tied in the UI. I am not sure if the offlineStartup.js changes are enough. Please try on Windows as there it is most possible the automatic detection is supposed to work.

Comment 29

[Or Schiro]

> Please try on Windows as there it is most possible the automatic detection is supposed to work.

Will it work on Linux?

Comment 30

[:aceman]

I don't know how the backend actually works. But it allows the options so we just offer them in the UI here.

Comment 31

[Magnus Melin [:mkmelin]]

The backend offline detection very recently got a lot of love in bug 939318. Should be mostly windows yet, but other platforms are coming.

Comment 32

[Mark Banner (:standard8)]

Comment on attachment 8503601 [details] [diff] [review]
v5 path

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

Looks reasonable, although I've not tested it/gone into depth.

Comment 33

[Josiah Bruner [:jsbruner]]

Comment on attachment 8503601 [details] [diff] [review]
v5 path

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

This looks fine, but I would drop the "system" word, it just adds clutter.

Comment 34

[Magnus Melin [:mkmelin]]

Comment on attachment 8503601 [details] [diff] [review]
v5 path

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

The manual options should get disabled when you choose automatic, no?

Comment 35

[:aceman]

(In reply to Magnus Melin from comment #34)
> The manual options should get disabled when you choose automatic, no?

It is behaving like that for me. If "automatically..." is checked, all the other 4 options are disabled. Is it not behaving like that for you?

But I see a small glitch in the patch. If we check "automatic" a hidden radio option is selected (the automatic value 4), so none of the visible ones. But when we uncheck the "automatic" one, one of the visible option should become selected. I'll fix that quickly.

Comment 36

[:aceman]

Attachment: v5.1 patch

Solved the nit from Josiah and added a default selection when "automatic" is unchecked.

Comment 37

[Magnus Melin [:mkmelin]]

Comment on attachment 8530898 [details] [diff] [review]
v5.1 patch

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

::: mail/components/preferences/offline.js
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +var gOfflineDialog = {
> +  dialogSetup: function ()

nit: no space before () I think

@@ +14,5 @@
> +      offlineStartupStatePref.value = startupRadio.querySelector("[_automatic]").value;
> +    } else {
> +      if (offlineStartupStatePref.value == startupRadio.querySelector("[_automatic]").value)
> +        offlineStartupStatePref.value = startupRadio.querySelector("[_default]").value;
> +    }

I don't know what the _automatic and _default is supposed to be good for? Seems to me it'd be clearer just using the values

Comment 38

[:aceman]

Attachment: v5.2 patch

That was just a fancy way to not hardcode the constants in the JS code but fetch the values via marking the options via attributes.
I have rewritten it if you don't like it.

Comment 39

[Magnus Melin [:mkmelin]]

Comment on attachment 8531716 [details] [diff] [review]
v5.2 patch

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

Yes I prefer using constants

::: mail/components/preferences/offline.xul
@@ +39,3 @@
>      <radiogroup id="whenStartingUp" class="indent" preference="offline.startup_state">
>        <radio value="0" label="&radioRememberPrevState.label;" accesskey="&radioRememberPrevState.accesskey;"/>
> +      <radio value="1" label="&radioAskState.label;"          accesskey="&radioAskState.accesskey;"/>

odd extra spaces in between here (and the ones below)
either wrap it or remove them

Comment 40

[:aceman]

(In reply to Magnus Melin from comment #39)
> Comment on attachment 8531716 [details] [diff] [review]
> v5.2 patch
> 
> Review of attachment 8531716 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Yes I prefer using constants
> 
> ::: mail/components/preferences/offline.xul
> @@ +39,3 @@
> >      <radiogroup id="whenStartingUp" class="indent" preference="offline.startup_state">
> >        <radio value="0" label="&radioRememberPrevState.label;" accesskey="&radioRememberPrevState.accesskey;"/>
> > +      <radio value="1" label="&radioAskState.label;"          accesskey="&radioAskState.accesskey;"/>
> 
> odd extra spaces in between here (and the ones below)
> either wrap it or remove them
The spaces are to align the accesskey attributes vertically. As is done in all the other radiogroups in that file. Is that an old pattern that is no longer accepted?

Comment 41

[:aceman]

Comment on attachment 8531716 [details] [diff] [review]
v5.2 patch

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

According to comment 14, the automatic value (4) is used by Seamonkey. Neil, please see if the change to mailnews/extensions/offline-startup/js/offlineStartup.js doesn't break anything there.

Comment 42

[:aceman]

(In reply to Martin Stránský from comment #1)
> Btw. Why is the "File -> Offline -> Offline Settings..." disabled?
This is a different dialog than "Edit -> Preferences -> Advanced -> Network & Disk Space". If you are in an IMAP account the menuitem gets enabled and upon clicking it it opens the Synchronization & Storage pane of account settings of that account. So if that is unexpected or is a misleading label, please file that as a new bug. Thanks.

Comment 43

[Magnus Melin [:mkmelin]]

(In reply to :aceman from comment #40)
> The spaces are to align the accesskey attributes vertically. As is done in
> all the other radiogroups in that file. Is that an old pattern that is no
> longer accepted?

Generally I think that should be avoided yes. Assuming the code ever changes, you'd easily have to align a lot of rows just to keep in sync. Besides, the line lengths here end up over 100ch.

Comment 44

[:aceman]

Attachment: v5.3 patch [checked in]

Comment 45

[[e-mail]]

Comment on attachment 8532146 [details] [diff] [review]
v5.3 patch [checked in]

>-    if (gOfflineStartupMode == kAlwaysOffline)
>+    if (gOfflineStartupMode == kAutomatic)
>+    {
>+      // Offline state should be managed automatically
>+      // so do nothing specific at startup.
>+    }
>+    else if (gOfflineStartupMode == kAlwaysOffline)
r=me because this change has no actual effect on the code, amirite?

Comment 46

[:aceman]

Yes, as there seems to be no fall-through "else" branch that would now not be reached with value of 4.

Comment 47

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/6025c959d8a7

Comment 48

[Ian Neal]

Comment on attachment 8532146 [details] [diff] [review]
v5.3 patch [checked in]

>+++ b/mail/locales/en-US/chrome/messenger/preferences/offline.dtd

>-<!ENTITY radioAlwaysOnline.label "Always start up online">
>+<!ENTITY radioAlwaysOnline.label "Online">
> <!ENTITY radioAlwaysOnline.accesskey "l">

As you have not changed the entity name, have you posted to m.d.l10n about this change?

Comment 49

[:aceman]

Oh, I didn't :(

Comment 50

[Kent James (:rkent)]

Wouldn't it be better to change the name in the patch?

Comment 51

[:aceman]

Yes, should I attach a fix here?

Comment 52

[Kent James (:rkent)]

Others are more familiar with l10n than I am, but my understanding is that changing the name triggers all of the automatic processes for translation, while a post to the newsgroup forces people to treat this as an exception. So I think a correction patch would be a better approach.

Comment 53

[Ian Neal]

It will depend on whether you can get approval to land a string change on what will be comm-beta in the next few days. Looking at mxr - http://mxr.mozilla.org/l10n-mozilla-aurora/search?string=radioAlwaysOnline.label - about 40% of them have picked it up (br, cy, en-GB, es-ES, fr, gd, hr, hu, hy-AM, it, lt, nb-NO, pl, pt-PT, ru, sk, sl, sv-SE, zh-CN, zh-TW, hsb and dsb; maybe fi; sq have both strings!), 40% haven't (be, bg, ca, cs, da, de, es-AR, et, eu, fy-NL, ga-IE, is, ja, ja-JP-mac, ko, nl, nn-NO, pt-BR, rm, tr) and 20% haven't got a recent localisation (before TB 31).
Note: this was just a rough google translate on the strings, but shouldn't be too far out.
I guess the important target is TB 38 for the next ESR.

Comment 54

[Kent James (:rkent)]

Oh I'm sorry, I did notice that this had landed TB 37. Then maybe we do need to handle it with a note to m.d.l10n

Comment 55

[:aceman]

Attachment: update string after change

I'd think we are fine if TB37 (that almost nobody uses) will just have a wrong translation (but the sense of the message is still understandable). We can still save TB38 by pushing this patch.

Comment 56

[Kent James (:rkent)]

Comment on attachment 8567388 [details] [diff] [review]
update string after change

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

Yes I think this is the safest thing to do. Might annoy people who caught the issue, but will help insure that TB 38 is correct.

Comment 57

[:aceman]

Thanks.

Comment 58

[Kent James (:rkent)]

Comment on attachment 8567388 [details] [diff] [review]
update string after change

Checked in https://hg.mozilla.org/comm-central/rev/a6a86951b08c

This patch landed in Thunderbird 38