Should support Mozilla as default News client too, like Mail

RESOLVED FIXED

Status

MailNews Core
Simple MAPI
RESOLVED FIXED
17 years ago
10 years ago

People

(Reporter: Henrik Gemal, Assigned: Ian Neal)

Tracking

Trunk
x86
Windows 2000

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 10 obsolete attachments)

1.35 KB, patch
Details | Diff | Splinter Review
16.34 KB, patch
Ian Neal
: review+
Ian Neal
: superreview+
Details | Diff | Splinter Review
(Reporter)

Description

17 years ago
Mozilla is created as a available program for mailto: links. This is done by
adding Mozilla to the reg.edit.
The same should be done for news links. If we do it so, Mozilla could be choosen
as default news program inside Internet Explorer.

The reg entry should set set under:
HKEY_LOCAL_MACHINE\SOFTWARE\Clients\News

20010222
(Reporter)

Comment 1

17 years ago
It should be dead easy. Just add it inthe function updateMapi() function at:
http://lxr.mozilla.org/seamonkey/source/xpinstall/packager/windows/mail.jst#206

Updated

17 years ago
QA Contact: esther → stephend
(Reporter)

Updated

17 years ago
Component: Mail Back End → Simple MAPI

Comment 2

16 years ago
*** Bug 132667 has been marked as a duplicate of this bug. ***

Comment 3

16 years ago
adding self to cc list

Comment 4

16 years ago
*** Bug 159994 has been marked as a duplicate of this bug. ***
(Reporter)

Comment 5

16 years ago
is this Frontend or MAPI?

It should be plain copy of the Mail setting stuff...
*** Bug 177939 has been marked as a duplicate of this bug. ***

Comment 7

15 years ago
See corresponding discussion in Bug 155241.

Reporter said "Mozilla is created as a available program for mailto: links."
This appears to not be strictly correct.  If Mozilla sets itself as the default
mail program, it is invoked by apps that use MAPI to handle "mailto:" -- such
as Internet Explorer and MSOffice.  Other programs (Opera, SecureCRT) check
with the OS for the default "mailto" handler, which Mozilla does not set up.

Under Windows, the associations for mailto:, news:, secnews: and nntp: ought
to be set up by Mozilla when it calls itself the default.  Further,
the settings for 'default mailer' and 'default newsreader' should be separate.

Windows' default handlers for these protocols go to its url.dll, which must
look somewhere else for the actual handler (such as OutlookExp).  Maybe that
should be the point to hook into.

I don't know how these associations are handled under Unix or Mac systems.

Currently, Bug 155241 is listed as an 'installer' bug regarding the general
issues of protocols.  This bug is primarily the file-handling aspects of
associating Mozilla to news: (and mailto:).

I suggest this bug be expanded to cover how all the protocols (including
http:, ftp:, gopher:) hook into Windows.  It is not a Simple MAPI issue,
I don't think.  I also suggest Bug 155241 be kept as an installer bug,
regarding offering an option at install to prevent Mozilla from grabbing the
various protocols and file-extensions, as I suggested over there.

Comment 8

15 years ago
> Windows' default handlers for these protocols go to its url.dll, which must
> look somewhere else for the actual handler (such as OutlookExp).  Maybe that
> should be the point to hook into.

I believe on Windows url.dll looks in the registry, under key(s) for the
appropriate protocol, as listed in the reporter's description.

Comment 9

15 years ago
> I believe on Windows url.dll looks in the registry, under key(s) for the
> appropriate protocol, as listed in the reporter's description.

I have tested this assertion, and it appears to be incorrect.  As far as I can
determine, URL.DLL doesn't do much of anything.

I spent the weekend researching the Client and internet-protocol registry keys;
my results are at:
  http://members.toast.net/4pf/Protocol.html

This bug appears to be a duplicate of Bug 77846.  I am posting more commentary
over there.
*** Bug 204905 has been marked as a duplicate of this bug. ***
*** Bug 206756 has been marked as a duplicate of this bug. ***

Comment 12

15 years ago
Created attachment 123999 [details] [diff] [review]
Win32 registry patch - make Mozilla the default Mailto:/NNTP:/NEWS handler

After hacking around in my registry, this is a registry patch that should set
mozilla as the default application for Mail and News URLs. Tested locally and
seems to work.

Comment 13

15 years ago
Well, that's OK for a basic install.  You should add an additional set of keys 
for the snews: protocol.

Really, however, the installer needs to be writing those keys, some of them 
selectively, if for no other reason than to keep the path correct.  For proper 
integration into "Set Program Access & Defaults" it also needs to replicate all 
the key structure for
  HKEY_LOCAL_MACHINE\Software\Clients\Mail 
into
  HKEY_LOCAL_MACHINE\Software\Clients\News
Your .REG file doesn't handle the important parts of that tree.

In order for the SPA&D integration to work, Mozilla needs to add support for a 
-setDefaultNews switch, analogous to the -setDefaultMail switch.  The "set 
default" routines for both client types (Mail and News) need to copy the
   [protocol]\open\command   branch from   HKLM\Software\Clients\[clienttype]   
to   HKEY_CLASSES_ROOT.

Further, Mozilla needs to both add a UI for "Default news reader" and improve 
the UI "Default mail reader."  Rather than a checkbox, making Mozilla the 
default should be handled with a pushbutton, in the same way it handles "Make 
Mozilla the default browser."

And, once again, I believe that this bug is itself a dupe, of bug 77846.

Comment 14

15 years ago
I have not dealt with snews. I take it snews is an encrypted nntp? similiar to
the differences between, say, pop3 and pop3s.

The patch was meant as a guideline only and was meant to be incorporated into
the installer.

Comment 15

15 years ago
Should we dupe this to bug 202497, as that bug has a decent patch that is
further along than this one.

Comment 16

14 years ago
*** Bug 235960 has been marked as a duplicate of this bug. ***

Comment 17

14 years ago
I think maybe the summary of this bug needs to be edited.  I honestly tried
searching for this bug prior to posting my report, and I couldn't find it. 
There seem to be a lot of duplicates, and maybe editing the summary would make
this case easier to search for.

Comment 18

14 years ago
*** Bug 237126 has been marked as a duplicate of this bug. ***
Product: MailNews → Core

Comment 19

13 years ago
The backend got checked in as part of the aviary landing,
so this only needs a couple of XUL file changes.
(Assignee)

Comment 20

13 years ago
Created attachment 183601 [details] [diff] [review]
Patch v0.1

This patch:
* Adds options for making default application for News and RSS Feeds
* Does some tidy up of pref-mailnewsOverlay.js
Assignee: mscott → bugzilla
Status: NEW → ASSIGNED
Attachment #183601 - Flags: review?(mnyromyr)

Comment 21

13 years ago
Comment on attachment 183601 [details] [diff] [review]
Patch v0.1

Two things ahead:

1. When I open the preferences from the MailNews window (with no other Mozilla
windows open), I get this:

[Exception... "'[JavaScript Error: "prefWindow has no properties" {file:
"chrome://messenger-mapi/content/pref-mailnewsOverlay.js" line: 64}]' when
calling method: [nsIDOMEventListener::handleEvent]" nsresult: "0x80570021
(NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "<unknown>"  data:
yes]

The new checkboxes aren't in a valid state then.
After switching panels and returning (or opening preferences from a browser
window and switching to the MailNews panel), everything is working fine, incl.
correct entries in the registry (short of #2).

2. I can't deselect Mozilla as the default mail client (but I can for news and
RSS).
(This may well be a backend issue and be beyond the scope of this bug.)


>Index: mapi/resources/content/pref-mailnewsOverlay.xul
>===================================================================
[...]
>       _elementIDs.push("mailnewsEnableMapi");
[...]
>+      _elementIDs.push("mailnewsEnableNews");
>+      _elementIDs.push("mailnewsEnableFeed");

Array.push allows multiple parameters, eg. x.push(1,2,3);

>Index: mapi/resources/content/pref-mailnewsOverlay.js
>===================================================================
[...]
>+    mailnewsEnableMapi.setAttribute("checked", mapiRegistry.isDefaultMailClient ? "true" : "false");
>+    mailnewsEnableNews.setAttribute("checked", mapiRegistry.isDefaultNewsClient ? "true" : "false");
>+    mailnewsEnableFeed.setAttribute("checked", mapiRegistry.isDefaultFeedClient ? "true" : "false");

These mapiRegistry.isDefault*Client functions return a boolean, so there's no
need for the ?: operator in the first place. Furthermore, checkboxes have a
|checked| property, so there's no need for the setAttribute, either.
|mailnewsEnableFeed.checked = mapiRegistry.isDefaultFeedClient;| etc. should
suffice.
(Apart from that, instead of setting an attribute to its default setting, you'd
better remove it. This is done here by the property function.)

>+    mailnewsEnableMapi.setAttribute("disabled", "true");
>+    mailnewsEnableNews.setAttribute("disabled", "true");
>+    mailnewsEnableFeed.setAttribute("disabled", "true");

|disabled| is available as a property, too.

>+    mailnewsEnableMapi.setAttribute("checked", parent.mapiPref.isDefaultMailClient ? "true" : "false");
>+    mailnewsEnableNews.setAttribute("checked", parent.mapiPref.isDefaultNewsClient ? "true" : "false");
>+    mailnewsEnableFeed.setAttribute("checked", parent.mapiPref.isDefaultFeedClient ? "true" : "false");

See above.

>+function onEnable(pref, ele) {
>+  // save the state of the checkbox
>+  if ("mapiPref" in parent)
>+    parent.mapiPref[pref] = ele.checked;

The four extra characters of 'element' won't have hurt. ;-)
And please use the usual Mozilla prefix 'a' for function argument names.

>+function onOK() {
>+  var mapiRegistry = getMapiRegistry();
>+  if (mapiRegistry && ("mapiPref" in parent)) {

No extra () necessary.


Minussing just for 1. above.
Attachment #183601 - Flags: review?(mnyromyr) → review-
(Assignee)

Comment 22

13 years ago
Created attachment 183767 [details] [diff] [review]
Updated Patch v0.1b

Changes since v0.1a:
* Changed from setAttribute to setting property directly
* Changed from using ? "true" : "false"
* Moved from using onload listener to execute overlay code to using startupFunc
attribute so prefWindow exists when it is called.
* Replaced newObject with {}
Attachment #183601 - Attachment is obsolete: true
Attachment #183767 - Flags: review?(mnyromyr)

Comment 23

13 years ago
Comment on attachment 183767 [details] [diff] [review]
Updated Patch v0.1b

>Index: mapi/resources/content/pref-mailnewsOverlay.js
>===================================================================
>+    const prefbase = "system.windows.lock_ui.";

Constants should have prefix 'k'.

>+function onEnable(pref, aElement) {

Please use the 'a' prefix on all arguments.


r=me with that fixed on checkin.

(And a note to possible testers: make sure your previous default program stored
in the registry wasn't Mozilla also, because this mechanism restores the old
registry value - and if that had the same name, it looks as if nothing happens
when you deselect Mozilla as the default handler!)
Attachment #183767 - Flags: review?(mnyromyr) → review+
(Assignee)

Comment 24

13 years ago
Created attachment 183943 [details] [diff] [review]
Tweaked Patch v0.1c

Changes since v0.1b:
* pref -> apref and prefbase -> kprefbase as per reviewer's request

Carrying forward r= and requesting sr=
Attachment #183767 - Attachment is obsolete: true
Attachment #183943 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #183943 - Flags: review+
(Assignee)

Updated

13 years ago
Attachment #183943 - Flags: superreview?(neil.parkwaycc.co.uk)
(Assignee)

Comment 25

13 years ago
Created attachment 183950 [details] [diff] [review]
Further tweaked patch v0.1d

Changes since v0.1c:
* Changed from apref -> aPref
Attachment #183943 - Attachment is obsolete: true
Attachment #183950 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #183950 - Flags: review+

Comment 26

13 years ago
(In reply to comment #24)
> kprefbase as per reviewer's request

Actually, I meant kPrefBase or something like that... ;-)
(No need to create a new patch for that, just change it before checkin, if noone
objects.)

Comment 27

13 years ago
Comment on attachment 183950 [details] [diff] [review]
Further tweaked patch v0.1d

>+    <separator class="thin"/>
The separator doesn't belong here, it belongs in the overlay.

>+    <hbox align="start" id="mapi"/>
Might as well make this a vbox to match the overlay better.

>+        <checkbox id="mailnewsEnableFeed" label="&enableFeed.label;"
>+                  accesskey="&enableFeed.accesskey;" 
>+                  oncommand="onEnable('isDefaultFeedClient', event.target);"/>
We don't support feed: URLs... linked from the browser, let's not falsely
advertise.

The JS changes diffed badly so I'll look at them separately.
Attachment #183950 - Flags: superreview?(neil.parkwaycc.co.uk) → superreview-

Comment 28

13 years ago
>    const kprefbase = "system.windows.lock_ui.";
Should be kPrefBase

>  try {
>    mapiRegistry = Components.classes["@mozilla.org/mapiregistry;1"]
>                             .getService(Components.interfaces.nsIMapiRegistry);
You should use if ("@mozilla.org/mapiregistry;1" in Components.classes) as your
test. You could also return the component without using a temporary variable.

>    if (mapiRegistry.isDefaultMailClient != parent.mapiPref.isDefaultMailClient)
>      mapiRegistry.isDefaultMailClient = parent.mapiPref.isDefaultMailClient ;
Why the checking here, while not in mailnewsOverlayStartup?
(Assignee)

Comment 29

13 years ago
Created attachment 184056 [details] [diff] [review]
Reduced Patch v0.1e

Changes since v0.1d:
* Removed RSS Feed bits
* Changed to using kPrefBase

>    if (mapiRegistry.isDefaultMailClient !=
parent.mapiPref.isDefaultMailClient)
>      mapiRegistry.isDefaultMailClient = parent.mapiPref.isDefaultMailClient ;

Why the checking here, while not in mailnewsOverlayStartup?

This checking just makes sure no unneccesary calls to mapiRegistry are made if
the relevant default has not changed. Not sure how that can be done in
mailnewsOverlayStartup.
Attachment #183950 - Attachment is obsolete: true
Attachment #184056 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #184056 - Flags: review+

Comment 30

13 years ago
Comment on attachment 184056 [details] [diff] [review]
Reduced Patch v0.1e

>+    <vbox align="start" id="mapi"/>
Nit: you probably don't need the align="start" here.

(In reply to comment #29)
>This checking just makes sure no unneccesary calls to mapiRegistry are made if
>the relevant default has not changed.
Hmm... well, just checking still makes a call to mapiRegistry so it's not that
cheap. A couple of alternative ideas could have been a) also cache the original
values in the parent.mapiPref b) just set parent.isDefaultMail/NewsClient =
this.checked; in the command handler, and in the OK callback only write to the
mapiRegistry if the properties are set.
Attachment #184056 - Flags: superreview?(neil.parkwaycc.co.uk) → superreview+
(Assignee)

Comment 31

13 years ago
Created attachment 184269 [details] [diff] [review]
mapiReg Patch v0.1f

Changes since v0.1e:
* Caches mapiRegistry setting in parent.mapiReg
* Caches disabled status of checkbox
* Only writes mapiRegistry if it has been changed and is different to cached
copy
* Other general tidy up
Attachment #184056 - Attachment is obsolete: true
Attachment #184269 - Flags: superreview?(neil.parkwaycc.co.uk)

Comment 32

13 years ago
Comment on attachment 184269 [details] [diff] [review]
mapiReg Patch v0.1f

Sorry, those alternatives were meant to be exclusive...
The easiest way to fix up this patch is to ensure that mapiReg and mapiPref are
always created and populated (with false values if necessary) so that you won't
need any "in" tests.

>+      var isDefault;
Thanks for avoiding the strict strict JS warning ;-)

>-        mapiRegistry.isDefaultMailClient = parent.mapiPref.isDefaultMailClient ;
It would be nice if you could kill the space before the ; next time.
Attachment #184269 - Flags: superreview?(neil.parkwaycc.co.uk) → superreview-
(Assignee)

Comment 33

13 years ago
Created attachment 184432 [details] [diff] [review]
mapiStartup patch v0.1g

Changes since v0.1f:
* Adds new variable mapiStartup to use instead of testing for existence of
mapiReg in parent all the time.
* Removes extrananeous space from before the ; on one line.
Attachment #184269 - Attachment is obsolete: true
Attachment #184432 - Flags: superreview?(neil.parkwaycc.co.uk)
(Assignee)

Updated

13 years ago
Attachment #184432 - Flags: superreview?(neil.parkwaycc.co.uk)
(Assignee)

Comment 34

13 years ago
Created attachment 184449 [details] [diff] [review]
Tweaked patch v0.1h
Attachment #184432 - Attachment is obsolete: true
Attachment #184449 - Flags: superreview?(neil.parkwaycc.co.uk)
(Assignee)

Comment 35

13 years ago
Created attachment 184514 [details] [diff] [review]
Correct tweaked patch v0.1h

This time I've attached a diff from my updated tree.
Attachment #184449 - Attachment is obsolete: true
Attachment #184514 - Flags: superreview?(neil.parkwaycc.co.uk)

Comment 36

13 years ago
Comment on attachment 184514 [details] [diff] [review]
Correct tweaked patch v0.1h

>+    parent.mapiInitialized = false;
It seems to me as if this makes the subsequent four lines (below) unnecessary,
as we only need to try to initialize mapi once. Although, I'm now wondering why
we should bother with the callback if we can't initialize mapi...

>+  }
> 
>+  if (!parent.mapiInitialized)
>+  {
Attachment #184514 - Flags: superreview?(neil.parkwaycc.co.uk) → superreview+
(Assignee)

Updated

13 years ago
Attachment #184449 - Flags: superreview?(neil.parkwaycc.co.uk)
(Assignee)

Comment 37

13 years ago
Created attachment 184822 [details] [diff] [review]
Revised callback patch v0.1i

Changes since v0.1h:
* Moves registering of callback so it is only done if mapiRegistry exists
* Removes unneeded if in onOK function because of above change
* Removes use of mapiCallback and simplifies if statements in
mailnewsOverlayStartup function
Attachment #184514 - Attachment is obsolete: true
Attachment #184822 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #184822 - Flags: review?(mnyromyr)

Updated

13 years ago
Attachment #184822 - Flags: superreview?(neil.parkwaycc.co.uk) → superreview+

Comment 38

13 years ago
Comment on attachment 184822 [details] [diff] [review]
Revised callback patch v0.1i

Just one nit:

>Index: mapi/resources/content/pref-mailnewsOverlay.js
>===================================================================
>+function mailnewsOverlayStartup()
[...]
>+    var mapiRegistry = getMapiRegistry();
>+    if (mapiRegistry)
>+    {
[...]
>+      parent.mapiInitialized = true;
[...]
>     }
>     else
>     {
[...]
>+      parent.mapiInitialized = false;
>     }

I'm no friend of such constructions, especially if both values are quite
distant (measured in lines of source code). If you want make sure that
parent.mapiInitialized is of type Boolean, use this:

var mapiRegistry = getMapiRegistry();
parent.mapiInitialized = !!mapiRegistry;
if (mapiRegistry)
{
  [...]
}


But a IMO better possibility is to set
   parent.mapiRegistry = getMapiRegistry();
directly and use that instead of both mapiInitialized and the local
mapiRegistry variable.
(This will mean we have a mapiRegistry object hanging around as long as the
preference dialog is open, but I don't consider this a problem, though - it's
just a pref dialog...)

r=me with that fixed.
Attachment #184822 - Flags: review?(mnyromyr) → review+
(Assignee)

Comment 39

13 years ago
Created attachment 185356 [details] [diff] [review]
Tweaked initialize patch v0.1j

Changes since v0.1i:
* Changed how parent.mapiInitialized was set as per reviewer's request

Carrying forward r= and sr= and requesting a= for suite-only, fairly low risk
patch
Attachment #184822 - Attachment is obsolete: true
Attachment #185356 - Flags: superreview+
Attachment #185356 - Flags: review+
Attachment #185356 - Flags: approval1.8b3?

Updated

13 years ago
Attachment #185356 - Flags: approval1.8b3? → approval1.8b3+
(Assignee)

Comment 40

13 years ago
Comment on attachment 185356 [details] [diff] [review]
Tweaked initialize patch v0.1j

Checking in base/prefs/resources/content/pref-mailnews.xul;
new revision: 1.78; previous revision: 1.77
Checking in base/prefs/resources/locale/en-US/pref-mailnews.dtd;
new revision: 1.34; previous revision: 1.33
Checking in mapi/resources/content/pref-mailnewsOverlay.xul;
new revision: 1.13; previous revision: 1.12
Checking in mapi/resources/content/pref-mailnewsOverlay.js;
new revision: 1.11; previous revision: 1.10
Checking in mapi/resources/locale/en-US/pref-mailnewsOverlay.dtd;
new revision: 1.6; previous revision: 1.5
done
(Assignee)

Updated

13 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
Duplicate of this bug: 102152
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.