Closed Bug 482489 Opened 12 years ago Closed 12 years ago

Feedback/Alert notifications need identity/connection information

Categories

(MailNews Core :: Backend, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.0b3

People

(Reporter: standard8, Assigned: standard8)

References

(Blocks 1 open bug)

Details

(Whiteboard: [b3ux][m3])

Attachments

(3 files)

At the moment the feedback/alert notifications only have the information that is in the error message, e.g. "Failed to connect to server xxx".

We should try and provide identity/connection/account information to allow some grouping/more information about where the feedback originated from.
Flags: blocking-thunderbird3+
Whiteboard: [b3ux]
Status: NEW → ASSIGNED
Whiteboard: [b3ux] → [b3ux][m2]
Whiteboard: [b3ux][m2] → [b3ux][m3]
Attached patch Backend supportSplinter Review
This builds on the new alerts system by replacing the nsIMsgWindow argument with an nsIMsgMailNewsUrl argument. This gives us the benefit that nsIMsgMailNewsUrl knows about the relevant nsIMsgIncomingServer as well as the nsIMsgWindow.

There should be a little bit of code size reduction in there as well as we're not getting the message window in multiple places, just where we need it.
Attachment #371304 - Flags: superreview?(bienvenu)
Attachment #371304 - Flags: review?(bienvenu)
Attached image Proposed UI
Proposed UI - effectively we will group errors/warnings underneath their account name - matching the account name in the folder pane.

This gives more context, especially in the case where a user may have multiple accounts on one server.

Not sure if we should prefix the grouping by "Account: " to give slightly more information as to what its actually referring to.
Attachment #371307 - Flags: ui-review?(clarkbw)
Attachment #371307 - Attachment description: UI patch → Proposed UI
Attached patch UI patchSplinter Review
there may be cases where there is no url - for example, we may get an alert notification from an idle imap connection.  Or we may be doing some activity that doesn't have a url, like compacting folders, and want to put up an alert. So we might want a flavor that takes an nsIMsgWindow as well. Or some way to poke the alert w/o having a url...but the patch looks OK at first look.
(In reply to comment #4)
> there may be cases where there is no url - for example, we may get an alert
> notification from an idle imap connection.  Or we may be doing some activity
> that doesn't have a url, like compacting folders, and want to put up an alert.
> So we might want a flavor that takes an nsIMsgWindow as well. Or some way to
> poke the alert w/o having a url...but the patch looks OK at first look.

Note the url is (as the nsIMsgWindow was) an optional argument. Unless I've missed something, all the cases in which we currently call AlertUser we are getting a message window from the url (the nsImapProtocol ones call GetMsgWindow which gets it from the running url).

I'm happy to add an nsIMsgWindow as an extra argument (or perhaps a different function would be clearer), but I don't see the need for it in the code at the moment.
Whiteboard: [b3ux][m3] → [b3ux][m3][needs review bienvenu,clarkbw]
Attachment #371307 - Flags: ui-review?(clarkbw) → ui-review+
Comment on attachment 371307 [details]
Proposed UI

looks pretty good so far
Comment on attachment 371309 [details] [diff] [review]
UI patch

Bryan's happy with the look of the UI, so time for reviews on this.
Attachment #371309 - Flags: review?(bienvenu)
Whiteboard: [b3ux][m3][needs review bienvenu,clarkbw] → [b3ux][m3][needs review bienvenu]
Attachment #371309 - Flags: review?(bienvenu) → review+
Attachment #371304 - Flags: superreview?(bienvenu)
Attachment #371304 - Flags: superreview+
Attachment #371304 - Flags: review?(bienvenu)
Attachment #371304 - Flags: review+
Right, I just meant that sometime in the future we may need the ability to do this without a nsIMsgMailNewsUrl - sometimes, when all else fails, we have ways of getting a msg window.
Whiteboard: [b3ux][m3][needs review bienvenu] → [b3ux][m3]
Both patches checked in: http://hg.mozilla.org/comm-central/rev/23163b1cbecf
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.