467116 - Merge default client dialog and search integration dialog into an "OS Integration" dialog

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[Rain [:rain1] (inactive)]

Some things that have to be looked at:

- What should trigger the dialog? Something like a specified period of inactivity might be a good idea.
- Modality
- What about news and RSS?
- Should we allow third-party search integration providers to hook into the dialog?

Comment 1

[Magnus Melin [:mkmelin]]

(In reply to comment #0)
> - What should trigger the dialog? Something like a specified period of
> inactivity might be a good idea.

Why would that change from the current set up?

Comment 2

[Rain [:rain1] (inactive)]

(In reply to comment #1)
> (In reply to comment #0)
> > - What should trigger the dialog? Something like a specified period of
> > inactivity might be a good idea.
> 
> Why would that change from the current set up?

I believe that this came out of a discussion on IRC, and the reason this was suggested was because for a new user, the first run may be too early a time to judge whether he'd want to use Tb as the default client.

I guess that should be a separate bug, though, and that this should concentrate on the unification.

Comment 3

[Rain [:rain1] (inactive)]

Attachment: WIP

Very incomplete. Nothing works right now. :)

Comment 4

[Rain [:rain1] (inactive)]

Attachment: WIP #2

More hooks added, but quite a bit is left.

Comment 5

[Rain [:rain1] (inactive)]

Attachment: patch, v1

I've added UI at two places --
1. in the default client dialog (now called the "system integration" dialog)
2. in the advanced pane, as part of the system integration groupbox.

The UI will be
- hidden if the OS doesn't support it (XP, Linux)
- visible but disabled if the OS does support it but required components aren't running (Vista/7 with Windows Search off, a check like this should probably be added for OS X as well)
- active if the OS supports it and the components are running (Vista/7, OS X)

The code to disable search integration hadn't been exercised, so it had a bug, which I've fixed.

(I've also renamed showDefaultClientDialog to something more appropriate ;) )

Comment 6

[Rain [:rain1] (inactive)]

Attachment: patch, v1.01

I think this patch should be ready for review now. I've tested it and it behaves as expected on Vista, XP and Linux. I don't have a Mac, but it should behave similarly to Vista, as most of the code is common.

Comment 7

[Rain [:rain1] (inactive)]

Oh, and right now the UI defaults to false. Once this patch lands, we can make a true/false decision in bug 468373.

Comment 8

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 376230 [details] [diff] [review]
patch, v1.01

I haven't tried this since I still can't get my Vista build working but I believe we talked about it enough that I'm comfortable with it.

I think I've pretty much made up my mind on true in bug 468373.  But I'm ok with just getting the patch in here and then getting in another "default choice" patch over there.

Comment 9

[David :Bienvenu]

Comment on attachment 376230 [details] [diff] [review]
patch, v1.01

when I try to build this, I get an error in the jar process:

RuntimeError: file not found: content/systemIntegrationDialog.xul

Comment 10

[David :Bienvenu]

Comment on attachment 376230 [details] [diff] [review]
patch, v1.01

I've tried this on Windows and it looks OK - do you need me to try it on the Mac?

Comment 11

[David Humphrey (:humph)]

Comment on attachment 376230 [details] [diff] [review]
patch, v1.01

I've tested on Mac, looks good.

Comment 12

[Rain [:rain1] (inactive)]

This is a git patch, so it needs hg import or equivalent.

Comment 13

[David :Bienvenu]

(In reply to comment #12)
> This is a git patch, so it needs hg import or equivalent.

Yes, I figured that out and used qimport :-)

Comment 14

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/6410ed4e37b8

Comment 15

[Mark Banner (:standard8)]

(In reply to comment #14)
> Checked in: http://hg.mozilla.org/comm-central/rev/6410ed4e37b8

Backed out due to:

- Mail Lk regressions on Linux (probably caused by Components.utils.import leaking on failure).
- Need to provide disable preferences for the bloat tests.

Comment 16

[Rain [:rain1] (inactive)]

Attachment: always include SearchIntegration.js, and add bloat test prefs

Comment 17

[Rain [:rain1] (inactive)]

Comment on attachment 377361 [details] [diff] [review]
always include SearchIntegration.js, and add bloat test prefs

Standard8, can you review the changes? The interdiff plus <https://bugzilla.mozilla.org/attachment.cgi?id=377361&action=diff#a/mail/components/Makefile.in_sec1> look like they cover all the changes.

(The patch size reduction is because for some reason, when I renamed a file, the line endings got changed from UNIX to DOS -- I've fixed that in this patch.)

Comment 18

[Rain [:rain1] (inactive)]

Attachment: fix order of DIRS in mail/components/Makefile.in

Oops, build/ should always be at the end.

Comment 19

[Mark Banner (:standard8)]

Comment on attachment 377452 [details] [diff] [review]
fix order of DIRS in mail/components/Makefile.in

Ok, looks good, r=me for the interdiff. I haven't tested the leak numbers, so we'll have to watch those on checkin.

Comment 20

[Rain [:rain1] (inactive)]

Pushed to c-c: http://hg.mozilla.org/comm-central/rev/16fea4d289f6
along with followup: http://hg.mozilla.org/comm-central/rev/2dfdbe90425f

Comment 21

[Rain [:rain1] (inactive)]

(In reply to comment #15)
> 
> - Mail Lk regressions on Linux (probably caused by Components.utils.import
> leaking on failure).

Filed bug 493216 about this.

Comment 22

[Rain [:rain1] (inactive)]

Attachment: fix regression with disable vista sdk requirements build

We don't include WinSearchIntegration.js on such a build. The limitations of the python preprocessor limit us a bit here. :(

Comment 23

[Kent James (:rkent)]

I applied the patch "fixed regression with ..." for my setup (where I normally compile with disable_vista_sdk_requrements) and it fixed the regression.

Comment 24

[Rain [:rain1] (inactive)]

(In reply to comment #22)
> Created an attachment (id=378143) [details]
> fix regression with disable vista sdk requirements build
> 

Pushed to c-c: http://hg.mozilla.org/comm-central/rev/35fb29728f2a