121369 - Should support Mozilla as default News client too, like Mail

Status: RESOLVED FIXED

(MailNews Core :: Simple MAPI, defect)

Description

[Henrik Gemal]

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

Comment 1

[Henrik Gemal]

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

Comment 2

[Andrew Lin]

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

Comment 3

[Ian Pottinger]

adding self to cc list

Comment 4

[R.K.Aa.]

*** Bug 159994 has been marked as a duplicate of this bug. ***

Comment 5

[Henrik Gemal]

is this Frontend or MAPI?

It should be plain copy of the Mail setting stuff...

Comment 6

[Matthias Versen [:Matti]]

*** Bug 177939 has been marked as a duplicate of this bug. ***

Comment 7

[Mike Cowperthwaite]

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

[Davor Cubranic]

> 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

[Mike Cowperthwaite]

> 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.

Comment 10

[Matthias Versen [:Matti]]

*** Bug 204905 has been marked as a duplicate of this bug. ***

Comment 11

[Matthias Versen [:Matti]]

*** Bug 206756 has been marked as a duplicate of this bug. ***

Comment 12

[Dan Egli]

Attachment: 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

[Mike Cowperthwaite]

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

[Dan Egli]

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

[earthsound]

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

Comment 16

[Alfonso Martinez]

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

Comment 17

[William M. Quarles]

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

[Daniel Wang]

*** Bug 237126 has been marked as a duplicate of this bug. ***

Comment 19

[[e-mail]]

The backend got checked in as part of the aviary landing,
so this only needs a couple of XUL file changes.

Comment 20

[Ian Neal]

Attachment: Patch v0.1

This patch:
* Adds options for making default application for News and RSS Feeds
* Does some tidy up of pref-mailnewsOverlay.js

Comment 21

[Karsten Düsterloh]

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.

Comment 22

[Ian Neal]

Attachment: 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 {}

Comment 23

[Karsten Düsterloh]

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!)

Comment 24

[Ian Neal]

Attachment: Tweaked Patch v0.1c

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

Carrying forward r= and requesting sr=

Comment 25

[Ian Neal]

Attachment: Further tweaked patch v0.1d

Changes since v0.1c:
* Changed from apref -> aPref

Comment 26

[Karsten Düsterloh]

(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

[[e-mail]]

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.

Comment 28

[[e-mail]]

>    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?

Comment 29

[Ian Neal]

Attachment: 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.

Comment 30

[[e-mail]]

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.

Comment 31

[Ian Neal]

Attachment: 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

Comment 32

[[e-mail]]

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.

Comment 33

[Ian Neal]

Attachment: 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.

Comment 34

[Ian Neal]

Attachment: Tweaked patch v0.1h

Comment 35

[Ian Neal]

Attachment: Correct tweaked patch v0.1h

This time I've attached a diff from my updated tree.

Comment 36

[[e-mail]]

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)
>+  {

Comment 37

[Ian Neal]

Attachment: 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

Comment 38

[Karsten Düsterloh]

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.

Comment 39

[Ian Neal]

Attachment: 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

Comment 40

[Ian Neal]

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