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

RESOLVED FIXED in Thunderbird 3.0b3

Status

Thunderbird
General
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: sid0, Assigned: sid0)

Tracking

(Blocks: 2 bugs)

Trunk
Thunderbird 3.0b3
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 5 obsolete attachments)

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?
(Assignee)

Updated

9 years ago
Depends on: 467117

Comment 1

9 years ago
(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?
(Assignee)

Comment 2

9 years ago
(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.
(Assignee)

Updated

9 years ago
Blocks: 468373
(Assignee)

Updated

9 years ago
Depends on: 470145
(Assignee)

Comment 3

9 years ago
Created attachment 375597 [details] [diff] [review]
WIP

Very incomplete. Nothing works right now. :)
Assignee: nobody → sid.bugzilla
Status: NEW → ASSIGNED
(Assignee)

Comment 4

9 years ago
Created attachment 376043 [details] [diff] [review]
WIP #2

More hooks added, but quite a bit is left.
Attachment #375597 - Attachment is obsolete: true
(Assignee)

Comment 5

9 years ago
Created attachment 376228 [details] [diff] [review]
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 ;) )
Attachment #376043 - Attachment is obsolete: true
(Assignee)

Comment 6

9 years ago
Created attachment 376230 [details] [diff] [review]
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.
Attachment #376228 - Attachment is obsolete: true
Attachment #376230 - Flags: ui-review?(clarkbw)
Attachment #376230 - Flags: review?(bienvenu)
(Assignee)

Comment 7

9 years ago
Oh, and right now the UI defaults to false. Once this patch lands, we can make a true/false decision in bug 468373.
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.
Attachment #376230 - Flags: ui-review?(clarkbw) → ui-review+

Comment 9

9 years ago
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

9 years ago
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?
Attachment #376230 - Flags: review?(bienvenu) → review+
Comment on attachment 376230 [details] [diff] [review]
patch, v1.01

I've tested on Mac, looks good.
Attachment #376230 - Flags: review+
This is a git patch, so it needs hg import or equivalent.
Keywords: checkin-needed

Comment 13

9 years ago
(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 :-)
Checked in: http://hg.mozilla.org/comm-central/rev/6410ed4e37b8
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.0b3
(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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Created attachment 377361 [details] [diff] [review]
always include SearchIntegration.js, and add bloat test prefs
Attachment #376230 - Attachment is obsolete: true
(Assignee)

Updated

9 years ago
Attachment #377361 - Attachment is patch: true
Attachment #377361 - Attachment mime type: application/octet-stream → text/plain
(Assignee)

Updated

9 years ago
Attachment #377361 - Flags: review?(bugzilla)
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.)
Created attachment 377452 [details] [diff] [review]
fix order of DIRS in mail/components/Makefile.in

Oops, build/ should always be at the end.
Attachment #377361 - Attachment is obsolete: true
Attachment #377452 - Flags: review?(bugzilla)
Attachment #377361 - Flags: review?(bugzilla)
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.
Attachment #377452 - Flags: review?(bugzilla) → review+
Pushed to c-c: http://hg.mozilla.org/comm-central/rev/16fea4d289f6
along with followup: http://hg.mozilla.org/comm-central/rev/2dfdbe90425f
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago9 years ago
Resolution: --- → FIXED
(In reply to comment #15)
> 
> - Mail Lk regressions on Linux (probably caused by Components.utils.import
> leaking on failure).

Filed bug 493216 about this.
Created attachment 378143 [details] [diff] [review]
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. :(
Attachment #377452 - Attachment is obsolete: true
Attachment #378143 - Flags: review?
(Assignee)

Updated

9 years ago
Attachment #377452 - Attachment is obsolete: false
(Assignee)

Updated

9 years ago
Attachment #378143 - Flags: review? → review?(bugzilla)

Comment 23

9 years ago
I applied the patch "fixed regression with ..." for my setup (where I normally compile with disable_vista_sdk_requrements) and it fixed the regression.
Attachment #378143 - Flags: review?(bugzilla) → review+
(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
You need to log in before you can comment on or make changes to this bug.