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)
Tracking
(Not tracked)
VERIFIED
FIXED
People
(Reporter: bugzilla, Assigned: sspitzer)
References
Details
Attachments
(1 file, 9 obsolete files)
13.95 KB,
patch
|
neil
:
review+
mscott
:
superreview+
|
Details | Diff | Splinter Review |
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?
Reporter | ||
Comment 2•22 years ago
|
||
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)
Comment 4•22 years ago
|
||
*** Bug 164098 has been marked as a duplicate of this bug. ***
Updated•22 years ago
|
OS: Windows 2000 → All
Comment 5•22 years ago
|
||
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?
Updated•22 years ago
|
Updated•22 years ago
|
Attachment #105051 -
Flags: review?(diego)
Updated•22 years ago
|
Attachment #105051 -
Flags: review?(diego) → review?(sspitzer)
Comment 6•22 years ago
|
||
Comment on attachment 105051 [details] [diff] [review] moves offline into mail & news >+ <treecell class="treecell-indent" ^^^^^^^^^^^^^^^^^^^^^^^^ wrong >+<!ENTITY offline.label "Offline & Disk Space"> Should this be removed from preftree.dtd?
Comment 7•22 years ago
|
||
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
Updated•22 years ago
|
Attachment #109087 -
Flags: review?(neil)
Comment 8•22 years ago
|
||
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)
Updated•22 years ago
|
Attachment #105051 -
Flags: review?(sspitzer)
Comment 9•22 years ago
|
||
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
Updated•22 years ago
|
Attachment #110305 -
Flags: superreview?(sspitzer)
Attachment #110305 -
Flags: review?(neil)
Updated•22 years ago
|
Attachment #110305 -
Flags: review?(neil) → review+
Comment 10•22 years ago
|
||
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.
Assignee | ||
Comment 11•22 years ago
|
||
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
Assignee | ||
Comment 12•22 years ago
|
||
I tried the patch, and it appears that it doesn't work. (pref changes aren't remembered.)
Comment 13•22 years ago
|
||
>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.
Comment 14•22 years ago
|
||
should bug 78923 be reconsidered (reopened), then? unless, of course there are near-future plans to bring back the startup state pref ui...
Comment 15•22 years ago
|
||
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.
Comment 16•22 years ago
|
||
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?
Comment 17•22 years ago
|
||
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
Comment 18•22 years ago
|
||
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?
Comment 19•22 years ago
|
||
Oops, sorry that change was by mistake, reversing.
Severity: enhancement → normal
Assignee | ||
Comment 20•22 years ago
|
||
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-
Assignee | ||
Comment 21•22 years ago
|
||
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.
Comment 22•22 years ago
|
||
+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
Comment 23•22 years ago
|
||
> 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?
Comment 24•21 years ago
|
||
*** Bug 199472 has been marked as a duplicate of this bug. ***
Comment 25•21 years ago
|
||
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
Comment 26•21 years ago
|
||
Ooops, one little thing slipped through, this version should be fine.
Attachment #122257 -
Attachment is obsolete: true
Updated•21 years ago
|
Attachment #122258 -
Flags: superreview?(sspitzer)
Attachment #122258 -
Flags: review?(neil.parkwaycc.co.uk)
Comment 27•21 years ago
|
||
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-
Comment 28•21 years ago
|
||
All of Neill's comments addressed, this version should be better.
Attachment #122258 -
Attachment is obsolete: true
Updated•21 years ago
|
Attachment #122403 -
Flags: superreview?(sspitzer)
Attachment #122403 -
Flags: review?(neil.parkwaycc.co.uk)
Comment 29•21 years ago
|
||
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-
Comment 30•21 years ago
|
||
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
Updated•21 years ago
|
Attachment #122742 -
Flags: superreview?(sspitzer)
Attachment #122742 -
Flags: review?(neil.parkwaycc.co.uk)
Comment 31•21 years ago
|
||
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+
Comment 32•21 years ago
|
||
OK, I hope this is the last iteration, thanks for spotting my last glitch.
Attachment #122742 -
Attachment is obsolete: true
Updated•21 years ago
|
Attachment #122762 -
Flags: superreview?(sspitzer)
Attachment #122762 -
Flags: review?(neil.parkwaycc.co.uk)
Updated•21 years ago
|
Attachment #122762 -
Flags: review?(neil.parkwaycc.co.uk) → review+
Assignee | ||
Updated•21 years ago
|
Attachment #122258 -
Flags: superreview?(sspitzer)
Assignee | ||
Updated•21 years ago
|
Attachment #122403 -
Flags: superreview?(sspitzer)
Assignee | ||
Updated•21 years ago
|
Attachment #122742 -
Flags: superreview?(sspitzer)
Comment 33•21 years ago
|
||
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.
Comment 34•21 years ago
|
||
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?
Reporter | ||
Comment 35•21 years ago
|
||
Comment on attachment 122762 [details] [diff] [review] alternative approach 1.4 sr?
Comment 36•21 years ago
|
||
Hmm, what are you trying to say? Has the superreview system been completely overturned? Could somebody with CVS access please check this in then?
Reporter | ||
Comment 37•21 years ago
|
||
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"...
Updated•21 years ago
|
Attachment #122762 -
Flags: superreview?(sspitzer)
Comment 38•21 years ago
|
||
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
Updated•21 years ago
|
Attachment #128933 -
Flags: superreview?(sspitzer)
Attachment #128933 -
Flags: review?(neil.parkwaycc.co.uk)
Comment 39•21 years ago
|
||
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+
Updated•21 years ago
|
Attachment #128933 -
Flags: superreview?(scott) → superreview+
Comment 40•21 years ago
|
||
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...
Comment 41•21 years ago
|
||
Fix checked in.
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Comment 42•21 years ago
|
||
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?
Comment 43•21 years ago
|
||
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.
Comment 44•20 years ago
|
||
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?
Comment 45•20 years ago
|
||
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.
Comment 46•20 years ago
|
||
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.
Comment 47•20 years ago
|
||
(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).
Comment 48•20 years ago
|
||
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
Updated•20 years ago
|
Product: Browser → Seamonkey
You need to log in
before you can comment on or make changes to this bug.
Description
•