Closed Bug 127423 Opened 22 years ago Closed 21 years ago

Offline prefs appear when Mail&Newsgroups aren't installed

Categories

(SeaMonkey :: MailNews: Backend, defect)

x86
All
defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: bugzilla, Assigned: sspitzer)

References

Details

Attachments

(1 file, 9 obsolete files)

if you dont install Mail&Newsgroups prefs still appear.
See Edit -> Prefs -> Offline & Disk Space

it talks about unsent messages and folders.
It should be moved to an overlay like the rest of the messenger stuff

20020222
Are you saying until we actually setup a mail and/or news account
don't have edit|prefs|offline&diskspace appear? Wouldn't the
same apply to edit|Mail&Newsgroups?

no... I'm saying that if you dont even install "Mail & Newsgroup" is very
confusing to have prefs talk about the offline use of messages.
@see bug 78923
i agree. but i have to ask what happened to the one single pref that prevented
us from deleting this panel from its top level status. as is the panel is 100%
useless unless you install mailnews (i didn't with this rc2 final)
*** Bug 164098 has been marked as a duplicate of this bug. ***
OS: Windows 2000 → All
Attached patch moves offline into mail & news (obsolete) — Splinter Review
This is a _preliminary_ patch.	It's largely Hakan Waara's patch from bug 78923
minus the bitrot and some changes that I considered unjustified.

The startup section is currently commented out (pending bug 82487) and needs to
be moved to some other section, I vote for advanced.  Any better ideas?

Comments?
Keywords: patch, review
Attachment #105051 - Flags: review?(diego)
Attachment #105051 - Flags: review?(diego) → review?(sspitzer)
Comment on attachment 105051 [details] [diff] [review]
moves offline into mail & news

>+            <treecell class="treecell-indent"
		       ^^^^^^^^^^^^^^^^^^^^^^^^ wrong

>+<!ENTITY offline.label "Offline &amp; Disk Space">
Should this be removed from preftree.dtd?
Attached patch neil's concerns corrected (obsolete) — Splinter Review
OK, I corrected both bugs, thanks for spotting them, please have a look at my
new version.  We still need to decide what to do with the (currently
uncommented and therefore invisible) startup settings and the help.
Attachment #105051 - Attachment is obsolete: true
Attachment #109087 - Flags: review?(neil)
Comment on attachment 109087 [details] [diff] [review]
neil's concerns corrected

>--- mailnews/jar.mn	Tue Oct  8 12:09:39 2002
>+++ mailnews/jar.mn	Sun Nov  3 19:34:57 2002
>@@ -99,6 +99,7 @@
>     content/messenger/mailPrefsOverlay.xul                                     (base/prefs/resources/content/mailPrefsOverlay.xul)
>     content/messenger/messenger.css                                            (base/resources/content/messenger.css)
>     content/messenger/messenger.xul                                            (base/resources/content/messenger.xul)
>+    content/messenger/pref-offline.xul                                         (base/prefs/resources/content/pref-offline.xul)
>     content/messenger/mail3PaneWindowVertLayout.xul                            (base/resources/content/mail3PaneWindowVertLayout.xul)
>     content/messenger/mailWindowExtrasOverlay.xul                              (base/resources/content/mailWindowExtrasOverlay.xul)
>     content/messenger/mailWindowOverlay.xul                                    (base/resources/content/mailWindowOverlay.xul)
>@@ -270,6 +271,7 @@
>     locale/en-US/messenger/pref-receipts.dtd                            (base/prefs/resources/locale/en-US/pref-receipts.dtd)
>     locale/en-US/messenger/pref-viewing_messages.dtd                    (base/prefs/resources/locale/en-US/pref-viewing_messages.dtd)
>     locale/en-US/messenger/aw-accname.dtd                               (base/prefs/resources/locale/en-US/aw-accname.dtd)
>+    locale/en-US/messenger/pref-offline.dtd                             (base/prefs/resources/locale/en-US/pref-offline.dtd)
>     locale/en-US/messenger/aw-accounttype.dtd                           (base/prefs/resources/locale/en-US/aw-accounttype.dtd)
>     locale/en-US/messenger/aw-done.dtd                                  (base/prefs/resources/locale/en-US/aw-done.dtd)
>     locale/en-US/messenger/aw-email.dtd                                 (base/prefs/resources/locale/en-US/aw-email.dtd)
I don't like where you've put these lines, IMHO they should be in the same
block as the other mailnews prefs files. Apart from that the patch works, so
now all you need is UI and module owner approval and super review :-)
Attachment #109087 - Flags: review?(neil)
Attachment #105051 - Flags: review?(sspitzer)
OK, I moved the entries to the proper place as suggested by neil.  Could you
give r= for this now?
Attachment #109087 - Attachment is obsolete: true
Attachment #110305 - Flags: superreview?(sspitzer)
Attachment #110305 - Flags: review?(neil)
Attachment #110305 - Flags: review?(neil) → review+
Diego,
I saw your comment in bug 82487
http://bugzilla.mozilla.org/show_bug.cgi?id=82487#c90

You definitly would need to get approval from ui & module owner.
And if this change does take place, we would also have to update
our Help Docs to. Ccing robinf.

Honestly i don't know if this will get approved anytime soon.
on comment I have is that in 4.x, the offline panel had something that wasn't
specific to mail:  the startup state pref UI.

are planning on adding that back?  

if not, I think this would be fine.

but even if we were to add that back, the pref UI shouldn't show up when mail is
not built.

diego's change is an improvement for browser only users, so we'll probably go
with that, and deal with this when (and if) the startup state pref UI lands.

cc ccarlen for his opinion.

Assignee: bienvenu → sspitzer
I tried the patch, and it appears that it doesn't work.

(pref changes aren't remembered.)
>on comment I have is that in 4.x, the offline panel had something that wasn't
>specific to mail:  the startup state pref UI.

This was actually in mozilla for a while as well, but got removed for some 
reason. Hence, why this was grouped as a global pref and not just grouped 
w/mailnews prefs.
should bug 78923 be reconsidered (reopened), then? unless, of course there are
near-future plans to bring back the startup state pref ui...
The startup state pref ui was removed because it didn't work properly.
I think it's very complicated necko bug, please see bug 82487 for more info.

So instead of getting more bugs on it not working, it was decided we remove
UI and put it back if bug 82487 ever got fixed. The bug to remove(hide) ui
is bug 132896.

guess another question would be if we ever got bug 82487 fixed
and we moved this UI from Prefs to under Mail/News, would we have
to move it back to it's original spot?


The startup prefs are currently commented out waiting for the fix to bug 82487.
 However that bug is currently dormant, the last patch is well over a year old.

We don't need to reopen bug 78923, my patch is adapted from there.

I have tried to update the help, but I did not completely understand how the
help system is supposed to work.  A helping hand would be appreciated.

Prefs are indeed not remembered.  I'll have another look, hints welcome.
Severity: normal → enhancement
An alternative approach would be to rename the "Offline & Disk Space" prefs to
"Startup" and then comment it out instead of removing it.  Startup prefs would
thus remain where they are and offline stuff would still go to MailNews where it
belongs.  The people in bug 82487 can then put them back in should they ever
feel like it and we only have to touch MailNews stuff.  I assume this would also
get approval more easily.

Opinions?
Oops, sorry that change was by mistake, reversing.
Severity: enhancement → normal
Comment on attachment 110305 [details] [diff] [review]
further updates as suggested by neil

denying, because the patch doesn't work, and of the "startup" prefs issue.
Attachment #110305 - Flags: superreview?(sspitzer) → superreview-
ideally, we'd have the start up pref UI in this section for browser-only, and
the mail specific UI would overlay on top of it.

but without the startup pref, we have the problem that we have nothing to put in
the panel, when mail/news isn't installed.

a work around could be to add some js to hide the treechild for this pref page
if getService("contract ID for some mail service") fails.  (if it failed, we'd
know mail wasn't installed.)

then, we could remove that hack once the startup pref work was complete.

the work to move the pref UI to an overlay (under mozilla/mailnews) would still
be worth doing, and would not need to be altered after the startup pref work was
done.
Attached patch now remembers settings (obsolete) — Splinter Review
+onload="parent.initPanel('chrome://communicator/content/pref/pref-offline.xul');"


should have been

+onload="parent.initPanel('chrome://messenger/content/pref-offline.xul');"

now the settings are remembered
Attachment #110305 - Attachment is obsolete: true
> ideally, we'd have the start up pref UI in this section for browser-only, and
> the mail specific UI would overlay on top of it.

I disagree.  It should not overlay, it belongs in the Mail & News section.  It
is not general enough to warrant putting it on its own and I would never expect
to find Mail & News prefs at this position.  I was quite surprised the first
time I noticed this.

> but without the startup pref, we have the problem that we have nothing to put
> in the panel, when mail/news isn't installed.

How about commenting it out then and putting it back once the startup issue is
fixed.  Or move it somewhere else then, but this would be up to the guys in the
startup bug to decide.

> a work around could be to add some js to hide the treechild for this pref page
> if getService("contract ID for some mail service") fails.  (if it failed, we'd
> know mail wasn't installed.)

Sounds difficult and hackish and I think it's unnecessary as explained above.

> then, we could remove that hack once the startup pref work was complete.

As I said above, what about commenting out this prefs panel until the startup
bug gets fixed.  Oh, we should also rename it to "Startup" then.

> the work to move the pref UI to an overlay (under mozilla/mailnews) would
> still be worth doing, and would not need to be altered after the startup pref
> work was done.

I prefer my solution ;-)

Shall I prepare a patch that implements this?
*** Bug 199472 has been marked as a duplicate of this bug. ***
Attached patch alternative approach (obsolete) — Splinter Review
Here is a new patch that implements the alternative approach I described above.
 The startup prefs remain in place, I moved just the rest under the Mail & News
section and commented the offline & Disk Space top level entry out, to be
revived or moved once bug 82487 gets fixed.  I like this solution even better
and it should be possible to get this into the tree without approval from
several different parties.
Attachment #112141 - Attachment is obsolete: true
Attached patch alternative approach 1.1 (obsolete) — Splinter Review
Ooops, one little thing slipped through, this version should be fine.
Attachment #122257 - Attachment is obsolete: true
Attachment #122258 - Flags: superreview?(sspitzer)
Attachment #122258 - Flags: review?(neil.parkwaycc.co.uk)
Comment on attachment 122258 [details] [diff] [review]
alternative approach 1.1

>    Contributor(s): Mohan Bhamidipati (mohanb@netscape.com)
>+                   Diego Biurrun     (diego@biurrun.de)
Removing 48 lines is not what I call a contribution ;-)

>+<?xul-overlay href="chrome://global/content/dialogOverlay.xul"?>
I don't think you need this.

>+<!DOCTYPE window SYSTEM "chrome://messenger/locale/pref-offline.dtd">
Should be DOCTYPE page to match <page

>+  ]]>
This belongs just before the </script>

+    enableField(aCheckbox, "offlineCompactFolderMin");
Has the code changed since you copied it? The old code has , false on the end.
Also can you make the indentation in this new file consistently 2 spaces and
not the mixture of 2, 3 or 4 spaces that you copied.

>+<!--LOCALIZATION NOTE (textStart): Don't translate "&brandShortName;".
>+    Place "&brandShortName;" in the phrase where the name of the application should
>+	 appear
>+-->
You don't need this becuase you don't use &brandShortName;
Attachment #122258 - Flags: review?(neil.parkwaycc.co.uk) → review-
Attached patch alternative approach 1.2 (obsolete) — Splinter Review
All of Neill's comments addressed, this version should be better.
Attachment #122258 - Attachment is obsolete: true
Attachment #122403 - Flags: superreview?(sspitzer)
Attachment #122403 - Flags: review?(neil.parkwaycc.co.uk)
Comment on attachment 122403 [details] [diff] [review]
alternative approach 1.2

I was unable to apply the patch. Only the first hunk succeeded, then three
hunks failed, finally complaining that the patch was malformed.

>-  function enableField(aCheckbox, aNodeID, setFocus) 
>+  function enableField(aCheckbox, aNodeID) 
I'm still worried that you're changing something here by mistake.
Attachment #122403 - Flags: review?(neil.parkwaycc.co.uk) → review-
Attached patch alternative approach 1.3 (obsolete) — Splinter Review
OK, this patch should apply cleanly.  Neil, thanks for reviewing thoroughly, I
was indeed doing that change by mistake, I had carried it over from a stale
version of the original file in my local tree.	This version (hopefully)
addresses all problems.
Attachment #122403 - Attachment is obsolete: true
Attachment #122742 - Flags: superreview?(sspitzer)
Attachment #122742 - Flags: review?(neil.parkwaycc.co.uk)
Comment on attachment 122742 [details] [diff] [review]
alternative approach 1.3

>-      oncommand="enableField(this,'offlineCompactFolderMin',true);"/> 
>+      oncommand="enableField(this,'offlineCompactFolderMin');"/>
I think this is your only mistake. Fix this and r=me
Attachment #122742 - Flags: review?(neil.parkwaycc.co.uk) → review+
Attached patch alternative approach 1.4 (obsolete) — Splinter Review
OK, I hope this is the last iteration, thanks for spotting my last glitch.
Attachment #122742 - Attachment is obsolete: true
Attachment #122762 - Flags: superreview?(sspitzer)
Attachment #122762 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #122762 - Flags: review?(neil.parkwaycc.co.uk) → review+
Attachment #122258 - Flags: superreview?(sspitzer)
Attachment #122403 - Flags: superreview?(sspitzer)
Attachment #122742 - Flags: superreview?(sspitzer)
OK, my last patch is reviewed and I think it is ready to go in.  Seth, could you
please superreview this and commit it if you think it's OK?  Thanks.
Two months have passed without a reaction..  Seth could you sr= this or could
somebody point me to some other person that could give sr= for this?
Blocks: 123569
Comment on attachment 122762 [details] [diff] [review]
alternative approach 1.4

sr?
Hmm, what are you trying to say?  Has the superreview system been completely
overturned?  Could somebody with CVS access please check this in then?
I think bugmail cut something off. I was just trying to send the "get review" once more. Most mozilla developers dont read bugmail but they read "review mail"...
Attachment #122762 - Flags: superreview?(sspitzer)
Same patch without bitrot.  The context in one file had changed slightly so the
patch did not apply cleanly anymore.  No changes otherwise.
Attachment #122762 - Attachment is obsolete: true
Attachment #128933 - Flags: superreview?(sspitzer)
Attachment #128933 - Flags: review?(neil.parkwaycc.co.uk)
Comment on attachment 128933 [details] [diff] [review]
unrotted version 1.4

transferring r=, trying a different sr
Attachment #128933 - Flags: superreview?(sspitzer)
Attachment #128933 - Flags: superreview?(scott)
Attachment #128933 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #128933 - Flags: review+
Attachment #128933 - Flags: superreview?(scott) → superreview+
Halleluja, I have sr=!  Thanks, Scott.  Could someone with CVS access please
check this in?  I can't and Seth Spitzer seems to have disappeared...
Fix checked in.
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Is there a new bug about putting liberating the network-level offline prefs
("startup" and "remember me" in a non-MailNews pref? Is that simply bug 82487? 
There is no bug about liberating the network-level offline prefs ("startup" and
"remember me") in a non-MailNews pref because they have never been moved to a
MailNews pref.  They have only been commented out.  If and when bug 82487 is
fixed they will reappear at the old location.
I don't understand why this was done in the first place, but if bug 82487 is
fixed, then what is the course of action? Create a new bug that says "move the
pref UI 'back'"?

And how is this done? Removal of the UI added here + uncommenting the UI that
was commented out to fix this bug?
Ben, I'm not entirely sure what is confusing you, so I'll try to give an
overview of the situation in the hopes of clearing things up.

In the prefs panel there was an entry called "Offline & Disk Space" that
contained some Mail & News and some other (commented out) prefs.  My patch moved
these entries under the "Mail & Newsgroups" entry where they belong.  Since the
prefs remaining in the original entry were commented out and the panel would
have been empty I commented out the whole entry. Now that bug 82487 is resolved
we could remove the comments, making the entry visible again. 
So should this be VERIFIED b/c you moved the prefs to mailnews?

Should a new bug be created asking you to bring the preferences back to where
they belong? (I never understood why we moved the prefs to begin with...) It
seems like SOME of the disk space items belonged in mailnews, but the offline
stuff sure didn't.
(In reply to comment #46)
> So should this be VERIFIED b/c you moved the prefs to mailnews?

Yes.

> Should a new bug be created asking you to bring the preferences back to where
> they belong? (I never understood why we moved the prefs to begin with...) It
> seems like SOME of the disk space items belonged in mailnews, but the offline
> stuff sure didn't.

All of the disk space items belong in mailnews.  Just look at the source, there
are two prefs related to disk space we are talking about here:

  mail.prompt_purge_threshhold
  mail.purge_threshhold

Without a doubt these are mail prefs, so they belong under the mailnews prefs
section and that is why they were moved.

The offline prefs reappearing there is another issue.  If you look at my patch
you will see that I did *not* move them there, they stayed in place (but were
commented out and thus invisible).

The start up prefs have being added there again was done by someone else.  The
commit message of the file indicates bug 221734, but the bug itself is
inconclusive and the patches attached there are not what was checked in. Some
mistakes have been made here.  I'll go reopen bug 221734 (which is really a dupe
of bug 82487, btw).
Blocks: 221734
VERIFIED: allplats, Mozilla 1.6f.
removed 221734 dependency.

The bug to restore the offline prefs to a non-MailNews portion of the Prefs UI
is bug 202529 
No longer blocks: 221734
Status: RESOLVED → VERIFIED
QA Contact: grylchan → benc
Blocks: 61689
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: