Open Bug 257942 (activitymgr) Opened 20 years ago Updated 6 months ago

activity manager tracker [meta]

Categories

(Thunderbird :: General, enhancement)

enhancement

Tracking

(Not tracked)

People

(Reporter: thephilips, Unassigned)

References

(Depends on 24 open bugs, Blocks 7 open bugs, )

Details

(Keywords: dev-doc-complete, meta, Whiteboard: [no l10n impact])

Attachments

(3 files, 22 obsolete files)

2.04 KB, patch
philor
: review+
davida
: ui-review+
Details | Diff | Splinter Review
41.30 KB, patch
philor
: review+
davida
: ui-review+
Details | Diff | Splinter Review
3.13 KB, patch
philor
: review+
davida
: ui-review+
Details | Diff | Splinter Review
User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.7) Gecko/20040628 Firefox/0.9.1
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.7) Gecko/20040628 Firefox/0.9.1

TB/Messenger for version 1.0 urgently needs Activity Log, which will record all
event on per account basis.

Events need logging:
 0. network problems
 1. pop3 auth errors.
 2. smtp sending problems
 3. received message.
 4. sent message.

This will improve support for multi-account systems which are configured for
some automatic tasks, e.g. junk mail auto deletion, sorting/filtering.

It can be organazed as a mbox under "Local Folders", where every log message
will be a message. Some text file will do too.

Problem as I see it:
1. Some activity messages go only to status bar. If you have several accounts -
and you are checking several accounts at the same time - you really cannot be
sure that some operation with some account didn't failed. And what is worse - if
some operation have failed - message box will be displayed, preventing
TB/Messanger from further operations, until attention from operator. (It is
quite annouying with unstable network connection to get bunch of dialog boxes
that TB cannot connect to servers - well, I know that network is down, but I
have no chance to ignore that event.)

P.S. Hopefully after introduction of activity log, it would be possible to make
all those annoying modal error message dialogs optional.

P.P.S. Is there any way to submit feature resuqeest not thru this dumbed down
bug form?

Reproducible: Always
Steps to Reproduce:
1.
2.
3.
http://www.mozilla.org/quality/mailnews/mail-troubleshoot.html#imap
Status: UNCONFIRMED → RESOLVED
Closed: 20 years ago
Resolution: --- → WORKSFORME
Well, I'm sorry but I'm talking about log inside of TB/Messenger.

What you have pointed to - is just a hack'around. It is for problem resolving - but not for normal end-
user. Well I'm not end-user - but still will like to see this as feature, not hack. Something to check did I 
had e.g. problems last thursday with my mail - friend didn't got my e-mail. Once again: not hack - 
feature.

I'm talking about a feature, comparable to The Bat! log. Check it out. 
(<http://www.ritlabs.com/en/products/thebat/>)

And as potential of future improvement - turning off message boxes on errors, since all errors will be 
logged. If one checks multiple accounts, it will be possible to make a nice message that during mail 
check errors were encountered. Not like now - if left unattended message box to my understanding will 
block further activities of TB/Messenger.

After all, it is marked not as bug - but as enhancement.

It is needed once per year - but after you have had a problem and you mail gone - it 
is too late to activate some cryptic logging - it will not help you to find out, which message was sent, 
which was not. Think about it. It can be done only automatically by TB/Messenger - debugging log will 
not help.

P.S. If you are lazy to check The Bat!, list of events I have mentioned in original post aprox. matches 
what The Bat! has.

P.P.S Or just post message that you believe that no-one need it, you do not care, etc - so I will know 
that we reached understanding.
Status: RESOLVED → UNCONFIRMED
Resolution: WORKSFORME → ---
*** Bug 275721 has been marked as a duplicate of this bug. ***
(In reply to comment #0)
> Events need logging:
>  0. network problems
>  1. pop3 auth errors.
>  2. smtp sending problems
>  3. received message.
>  4. sent message.
> 
> This will improve support for multi-account systems which are configured for
> some automatic tasks, e.g. junk mail auto deletion, sorting/filtering.

good detail.

example log screen from Bug 275721
https://bugzilla.mozilla.org/attachment.cgi?id=175135 
 

> It can be organazed as a mbox under "Local Folders", where every log message
> will be a message. Some text file will do too.

I question placement by default on the front end screen - suggest a view option.  

Suggest wrap around log in memory (don't want the performance impact of a disk log).


> Problem as I see it:
> 1. Some activity messages go only to status bar. If you have several accounts 

additional reasons:

1. TB is a billed as a so-called an end user product - as such, an end-user UI
log / debug tool which is simple to find and operate is most appropriate for a
polished, mass-market product (which presumably TB aspires to be)

2. debugging "after the fact" has inherent problems - questionable
reproducibility, time involved, lack of details about the initial incident, etc
- would therefore be much better to catch problems the first time, in real time

3. even if a log's level of detail is insufficient to determine the cause of an
issue - having a log would at least a) confirmat a problem existed and b)
provide some of the sequence of events that led to the issue (and directly
impacts the ability to reproduce the issue in the future)


> P.S. Hopefully after introduction of activity log, it would be possible to make
> all those annoying modal error message dialogs optional.

such as bug 123440  bug 223131  Bug 82999  Bug 245536  (Bug 219000)


> P.P.S. Is there any way to submit feature resuqeest not thru this dumbed down
> bug form?

nope - this is a good way to do it.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: [Feature Request] ThunderBird/Messenger need Activity Log → need connection / communication Activity Log
*** Bug 317397 has been marked as a duplicate of this bug. ***
*** Bug 307332 has been marked as a duplicate of this bug. ***
*** Bug 362956 has been marked as a duplicate of this bug. ***
> *** Bug 362956 has been marked as a duplicate of this bug. ***

Well, it's vaguely related, but I'm not thinking of a debugging log, but rather a GUI with the following functionality:
- status of the current connection
- progress bar for the downloading of the current message (with indication of size)
- progress bar for the downloading of all messages (with indication of size and number of messages)
- a cancel/skip button for the current message, and for the overall downloading
QA Contact: general
cc'ing Bryan to get some views on this.
Assignee: mscott → nobody
I think something like the new interactive status bar concept covers what this is bug is after. Please see the designs linked off the wiki page.
http://wiki.mozilla.org/Thunderbird:Interactive_Status_Bar

We don't currently have a bug tracking the implementation of the new status bar.
From comment #11
> I think something like the new interactive status bar concept
> covers what this is bug is after.

Idea seem nice, but it fails to address very important issue of preserving
history of Tb activity over some time frame.

There are some scenarios when you can't replace simple and dumb log with
some fancy interactive UI:
1. Batch sending of huge attachments (which take long time you do not want to sit and wait for to finish).
2. Debugging connectivity problems with particular servers.
3. Tracking of activity in multi-account environment.

The point of "Interactive status bar" is to give better visual feedback on what
Tb is doing (what is very important too). The point of the bug is to have some
activity log which reflects what was happening in Tb for some time. Many (if
not) all mail-sending problems pop-up only days after the problem had actually
occurred. That's why keeping history (log) for some time is important.

Up-shot is that the two features are obviously related. One can say that in proposed activity log, messages displayed in "Interactive status bar" about action results are preserved for later investigation.

But in no way interactive status bar is replacement for activity log - they rather complement each other.

P.S. You also have to keep in mind that messages in interactive status bar would have to disappear after some time. If Friday evening I'm leaving Tb to send some huge message to colleagues, chances are that on Monday morning, the status bar would have no info about my send job. It would be replaced by recent Tb activity. And Tb per definition does something all the time.
> But in no way interactive status bar is replacement for activity 
> log - they rather complement each other.

As improvement for "Interactive Status Bar", one can add button called "History" - to show the activity log. Right now the proposed "activity log" has no right place in Tb UI. But interactive status bar seem to be right place to add a button for such log (or more user-friendly "History").
(In reply to comment #13)
> As improvement for "Interactive Status Bar", one can add button called
> "History" - to show the activity log. Right now the proposed "activity log" has
> no right place in Tb UI. But interactive status bar seem to be right place to
> add a button for such log (or more user-friendly "History").

If you check out the other links from the Interactive Status Bar page I think you'll see something like what you're suggesting here.  Essentially using the Download Manager from Firefox for a status / activity / download history view that is searchable and actionable.

https://wiki.mozilla.org/Thunderbird:Activity_Manager
http://clarkbw.net/blog/2008/06/04/activity-is-the-new-download/
http://clarkbw.net/designs/interactive-status-bar/thunderbird-activity-manager.png
> If you check out the other links from the Interactive Status Bar 
> page I think you'll see something like what you're suggesting here.

Sorry that hadn't dug deeper into the links.

That looks very promising.

As soon as bug for "Activity Manager" would be open, I think it is OK to
set the bug to "DUP".

Or, since I do not see the Tb bug for "Activity Manager" (search for it yields this very bug on top), this one can be used for its implementation, since this is essentially what the bug was open for.
Ok, retitling this bug and setting the right priorities such that we can aim for it to get into TB3.  Hopefully someone can pick this up and start working on it soon so we can up the target to b2 or rc1.
Flags: wanted-thunderbird3+
Priority: -- → P2
Summary: need connection / communication Activity Log → interactive status bar and activity history manager
Target Milestone: --- → Thunderbird 3
Attached patch WIP (nothing useful yet) (obsolete) — Splinter Review
I'm attaching a patch which coarsely clones the infrastructure used by the download manager in toolkit/components/downloads and toolkit/mozapps/components/downloads/, as much of it seems directly applicable to this problem.

A few notes:

 - the XBL needs to be tweaked to support the different kinds of entries we want to show, rather than downloads
 - the activity.js file is all junk, and should probably be rewritten from scratch
 - currently the tools/activity menu is brought in as an overlay, to keep this patch completely independent from the main UI.

There is much to be done to figure out:

1) how customers (autosync code, addrbook code, lightning code, etc.) should indicate what activities are going on

2) how we dispatch activity-specific actions back to code in each area (sub-interfaces of nsIActivity?)

3) how we want to persist activities (a mozStorage seems possible but may be overkill), and how long we want to persist them (last N activities at most?)
The extension Mail Tweaks has a Status Bar Cache feature
http://mailtweak.mozdev.org/tweaks.html lists and describes the features.
The status cache sets a display time and quantity of cached items.

To me a flaw of it's implementation is a lack of filters to avoid caching hyper link hover actions. My opinion is only a link mouse click action should be cached. I get lots of e-commerce mail loaded with links that push connection status entries out through the overflow.

In reply to Comment #15, item 3).
Core code currently can work with the OS to run protocol logging. Would a pseudo-protocol fit this need and tap the stdout to a disk log file. Gotcha is lack of upper entry limit. As to a log viewer, what about reuse of the existing View Source window code.
(In reply to comment #17)
> 1) how customers (autosync code, addrbook code, lightning code, etc.) should
> indicate what activities are going on
> 
> 2) how we dispatch activity-specific actions back to code in each area
> (sub-interfaces of nsIActivity?)

I think getting a wiki document up for the proposed interfaces and interactions would be a good idea. We could then move it to devmo later (once implemented).

> 3) how we want to persist activities (a mozStorage seems possible but may be
> overkill), and how long we want to persist them (last N activities at most?)

JSON could be a simpler option here.
Attached patch updated WIP (obsolete) — Splinter Review
I'd forgotten some files, and the jar.mn format has changed slightly.
Attachment #341362 - Attachment is obsolete: true
Attached patch Demo with autosync activities. (obsolete) — Splinter Review
Just a demo, still work in progress. Activity.js needs to be cleaned-up, and organized. It works with auto-sync activities without grouping.
Attached file Activity Manager API (obsolete) —
Submitted to get community's opinion on the interface.
Attached file Comments on the interface idl (obsolete) —
I am posting some comments on the idl as a file rather than clutter the comments, as it is rather long. Thanks for working on this, Emre, it's important work that is really needed!
Comment on attachment 345638 [details]
Activity Manager API

My comments, feel free to take with a grain of salt:

>diff -r fe63791a49aa mail/components/activity/public/nsIActivity.idl

Like rkent said (and KaiRo from the newsgroups), I would prefer that this go into mailnews/, for the following reasons:
1. This seems extremely useful to me for testing.
2. I'm guessing core mailnews wants to use these interfaces. A lot.
3. SeaMonkey probably wants this too.

>+[scriptable, uuid(35ee2461-70db-4b3a-90d0-7a68c856e911)]
>+interface nsIActivityCancelable : nsISupports {
>+  nsresult cancel(in nsIActivity aActivity);
>+};
[ and the other ones ]
Any reasons these do not inherit from nsIActivity?

Also, these lack documentation, which makes it hard for me to judge what I would have problems with. Salient questions:
1. What is the aActivity parameter?
2. What happens if the activity failed cancellation for any reason? Is that possible?

>+/**
>+ * Represents an activity.
>+ */
>+[scriptable, uuid(C5719E10-6AF5-4466-BC33-5A47DB28A32F)]
>+interface nsIActivity : nsISupports {

Seems a little skimpy on interface documentation, but I've had problems with generic interface documentation comments myself, so I won't push too hard.

[ activity states]
>+  /**
>+   * Activity is completed.
>+   * Only nsIActivityEvent can be in this state.
>+   */
>+  const short ACTIVITY_COMPLETED = 1;

Why?
>+  /**
>+   * Activity failed due to error.
>+   * Only nsIActivityProcess can be in this state.
>+   */
>+  const short ACTIVITY_FAILED = 2;

Again, why? (x 5)

I notice that of the 7 states, 6 are specific to one type, with the other one being "Not started" (i.e., default). If something is neither a process nor an event, its only possible state according to this interface is to not have been started.

>+  /**
>+   * ID assigned by the activity manager.
>+   */
>+  readonly attribute unsigned long id;

This implies that the manager is the only thing capable of generating activities, therefore implying that people are not expected to provide their own implementations of nsIActivity. Is this true?

>+  /**
>+   * A brief description of the activity.
>+   */
>+  readonly attribute AString displayText;

Would this be localized text, or a handle to pass off to a bundle to get the localized text?

>+  /**
>+   * Initiator of the activity.
>+   */
>+  readonly attribute nsISupports initiator;

Any hints as to what the initiator is? For example, suppose I click on the Get Mail button and get new mail, what is the initiator? Is it the server that's getting the messages, the folder of said server that gets the messages, or the service that actually constructs and dispatchs the URI to load the messages?

>+  /**
>+   * The type of the activity.
>+   * possible values:
>+   *  unknown
>+   *  send
>+   *  sync
>+   *  copy
>+   *  move
>+   *  delete
>+   *  add_item
>+   *  remove_item
>+   *  update
>+   *  compact
>+   *  index
>+   *  undo
>+   */ 
>+  attribute AString type;

Is this list complete? What do the items mean? What do I do if I want to add something into the list?

>+  /**
>+   * Activity domain specific objects.
>+   * (i.e. a folder being copied to another location, source and target folders
>+   *  of move operation, etc...)
>+   */
>+  void addSubject(in nsIVariant aSubject);

The subjects of a move object are not commutative; which one is which? If I'm doing, say, the undo type as above, what subjects might it have?

>+  /**
>+   * The type of the activity context.
>+   * possible values:
>+   *  unknown
>+   *  account
>+   *  smtp
>+   *  calendar
>+   *  addressbook
>+   */ 
>+  attribute AString contextType;

Similar to the type question above... is this complete, etc.

>+  /**
>+   * The context of the activity.
>+   */
>+  attribute nsIVariant contextVal;

This is really vague. Judging from the contextType, these seems somewhat redundant with the initiator.

>+  /**
>+   * Should be called by the activity manager.
>+   */
>+  void setId(in unsigned long aID);

Why not just make id a regular attribute, then?

>+[scriptable, uuid(9DC7CA67-828D-4AFD-A5C6-3ECE091A98B8)]
>+interface nsIActivityProcess : nsIActivity {
>+ 
>+  /**
>+   * The percentage of activity completed.
>+   * If the max value is unknown it'll be -1 here.
>+   */
>+  readonly attribute long percentComplete;

Perhaps this wants to be a double or float?

>+  /**
>+   * Total amount of work units.
>+   */
>+  readonly attribute unsigned long totalWorkUnits;

I'm not sure if this comes up in practice, but it might be possible we don't know how many work units we have to do yet. In this case, what is the value of this attribute?

>+  /**
>+   * The time a transfer was started.
>+   */
>+  readonly attribute long long startTime;

Why not PRTime?

>+  /**
>+   * Indicates if the activity can be canceled by the user or not.  
>+   */
>+  readonly attribute nsIActivityCancelable cancelable;
[ and friends ]

As I mentioned above, I think this works better if the implementations QI the other interfaces as appropriate, so that one can check with an instanceof or QI check as necessary.

>+[scriptable, uuid(2B1B0D03-2820-4E37-8BF8-102AFDE4FC45)]
>+interface nsIActivityEvent : nsIActivity {
>+  
>+  /**
>+   * A brief text about the event status.
>+   */
>+  readonly attribute AString statusText;

[ Repeat l10n question here.]

>+  /**
>+   * The starting time of the activity.
>+   */
>+  readonly attribute long long startTime;

Same PRTime question; maybe this wants to move to nsIActivity?

>+  /**
>+   * The completion time of the activity.
>+   */
>+  readonly attribute long long completionTime;
>+  
>+  /**
>+   * Indicates if the activity can be undo after completed successfully. 
>+   */ 
>+  readonly attribute nsIActivityUndoable undoable;

Copy aforementioned comments to these two properties as appropriate.

>+[scriptable, uuid(abfcfa8a-c127-40dc-ac64-64b1ffc024ec)]
>+interface nsIActivityListener : nsISupports {
>+  void onStateChanged(in nsIActivity aActivity, in short aOldState);
>+  void onProgressChanged(in nsIActivity aActivity, in long aPercentComplete,
>+                         in AString aStatusText, in unsigned long aWorkUnitsCompleted, 
>+                         in unsigned long aTotalWorkUnits);
>+};

I'm not sure about the aStatusText parameter, but I'll defer that to others.

>diff -r fe63791a49aa mail/components/activity/public/nsIActivityManager.idl

Next file!

>+[scriptable, uuid(BBE42BCE-4122-45FB-B928-298EBD15B8CF)]
>+interface nsIActivityContextDisplayHelper : nsISupports {
>+  /**
>+   * Allows the activity originator to customize
>+   * context display text for activity groups.
>+   */ 
>+  AString getContextDisplayText(in nsIActivity aActivity);
>+};

L10n comment repeated. Also generic "who's implementing this?" comment.

>+[scriptable, uuid(8ed29f45-e0c4-4259-ad80-6ff177f87aa6)]
>+interface nsIActivityMgrListener : nsISupports {
>+  /**
>+   * Called _after_ activity manager adds an activity into
>+   * the managed list.
>+   */ 
>+  void onAddedActivity(in unsigned long aID, in nsIActivity aActivity);

Managed list = the list that would be in the dialog?

>+  /**
>+   * Called _after_ activity manager removes an activity from
>+   * the managed list.
>+   */ 
>+  void onRemovedActivity(in unsigned long aID);

No nsIActivity parameter? Also, who removes the activity?

>+[scriptable, uuid(237912f8-79e3-470b-9a4c-95761d5076ce)]
>+interface nsIActivityManager : nsISupports {
>+  /**
>+   * Shows each activity process as a standalone item.
>+   */
>+  const short VIEW_TYPE_STANDALONE = 1;
>+
>+  /**
>+   * Groups activity process' by their context.
>+   */
>+  const short VIEW_TYPE_GROUPBYCONTEXT = 2;

I think this would be clearer if I saw UI, but the comments and variable names don't quite seem to match.

>+  /**
>+   * Adds the given activity into the managed acitivities list.
>+   *
>+   * @param aActivity The activity that will be added.
>+   * 
>+   * @return Unique activity identifier.
>+   */
>+  unsigned long addActivity(in nsIActivity aActivity);

Will the activity manager be setting the ID itself or not? If it is, why do you need to return the value?

>+  /**
>+   * Retrieves all activities managed by the activity manager.  This can be one that
>+   * is in progress (process), or one that has completed (event) in the past and is stored in the
>+   * persistent store.
>+   *
>+   * @return A read-only list of activities managed by the activity manager.
>+   */
>+  void getActivities(out unsigned long length, [retval, array, size_is(length)] out nsIActivity aActivities);

Any particular order?

>+  /** 
>+   * The number of activities currently being processed.
>+   */
>+  readonly attribute long processCount;

"Being processed" == ? Those that are in progress?

>+  /**
>+   * Registers a view type (see above) per activity type.
>+   * Default view type is VIEW_TYPE_STANDALONE.
>+   */
>+  void registerActivityViewType(in AString aActivityType, in short aViewType);

The activity type is nsIActivity::activityType?

Also, would this persist? (repeat for next method).

>diff -r fe63791a49aa mail/components/activity/public/nsIActivityManagerUI.idl

Next file!

>+[scriptable, uuid(ae7853b0-2e1f-4dc1-89cd-f8bbfb745d4d)]
>+interface nsIActivityManagerUI : nsISupports {

This looks like a copy-paste job from the toolkit download manager. A question more for the SM folks as well: how well could they be used together?

Comments/questions in general:
* I'm not entirely certain who implements what, etc. The most key one here is nsIActivity.
* The inheritance of design from the download manager is obvious, but it doesn't feel as clean as the download manager IDLs.
* In particular, nsIActivity seems to be trying to like nsIDownload, floating agnostically above what actually has to happen, when it would rather be tied to the guts of the operations. I personally feel that a model like nsIMsgIncomingServer--with a generic utility implementation that does most of the interface and the specific subclasses do the rest--would be better; JS would need a utility module as well, of course.
* I'm not seeing the whole picture from the present documentation; it would be nice to have one of the documentation comments give some brief code as to what would happen in, say, downloading messages.
Does this give us a way of handling what are currently alerts that aren't associated with a particular activity?  For example, any IMAP url can cause the server to tell us to alert the user about something (typically, quota alerts). I'd like to be able to use non-alert UI to display the message from the server, with a minimum of fuss, on demand, when the IMAP server tells us. I don't want to have to make every possible thing that can cause an imap url to run to be part of an activity, if that makes sense. If that's just a matter of creating a very temporary activity, telling it about the quota message, and then destroying the activity, that would work.
Thanks for the comments and questions. I've put a wiki page together to cover your questions at 

https://wiki.mozilla.org/User:Emre/tb/activitymanagerInterface. 

Hopefully this will clear the air a little bit more, and provide some sort of documentation. 

Below are the answers to your questions that are, I believed, not directly addressed in the document:

Kent:

> re STATE_NOTSTARTED

Good idea, will do.

> I assume this is supposed to be localized. How do we hook into localization? 

Each extension should provide its own localization. I don't know how we can provide an uniform mechanism to do the localization for them. 

> Extensions are not normally formal XPCOM components. 

Since the initiator is for internal use, and it is optional, we don't care about its interface, or what it is. 

You are right about extensions that are not XPCOM components.  Although there are other ways for such extensions to associate the activity with themselves, I start to think that maybe we should change the type of the initiator to nsIVariant. Thoughts?

> It takes a lot of work to setup, and release in the C++ case, an array of variants. .. nsIObserverService
> void getActivities(out unsigned long length, [retval, array, size_is(length)] out nsIActivity aActivities);

My knowledge is limited in this area, and highly appreciated your help here. Only comment I have here is that the majority of the development is moving toward to JavaScript, and any trade-off we have to make should favor JavaScript IMHO.

Joshua:

>  What happens if the activity failed cancellation for any reason? Is that possible?

Yes it is possible. It stays on the window until the initiator removes it from Activity Manager.

> therefore implying that people are not expected to provide their own implementations of nsIActivity. Is this true?

They can provide their own implementations, but we encourage them to use the generic implementations, for simplicity.

> Why not just make id a regular attribute, then?

Since id is giving by Activity Manager during initialization, and shouldn't be changed by other parties during the life time of the activity, I tried to emphasis this fact that way.  I don't know any idiom in JavaScript to make it clean cut. I am open to suggestions.

>I'm not sure if this comes up in practice, but it might be possible we don't 
> know how many work units we have to do yet. In this case, what is the value of 
> this attribute?

This information comes handy when nsIActivityCancelable is called on the activity.

> Why not PRTime?

DownloadManager uses long long, and has the function converting this value into JavaScript date/time..

>+  readonly attribute long long startTime;
>Same PRTime question; maybe this wants to move to nsIActivity?

Definitely, we can move this into nsIActivity.

> Any particular order?

Lowest id first.

> readonly attribute long processCount;
> "Being processed" == ? Those that are in progress?

Number of nsIActivityProcess components in the list.

>This looks like a copy-paste job from the toolkit download manager. A question
>more for the SM folks as well: how well could they be used together?

Ideally, Download Manager should be the specialized version of the Activity Manager, and its functionality should be implemented
through Activity Manager interface. We can work with SM folks to achieve this goal.

David:

>Does this give us a way of handling what are currently alerts that aren't
>associated with a particular activity?

I think the new status-bar is the medium to communicate such alerts. Bryan?
Before I continue, let me state that I'm relating most of the interface design to that practiced by nsIMsgIncomingServer in particular.

(In reply to comment #27)
> > Why not just make id a regular attribute, then?
> 
> Since id is giving by Activity Manager during initialization, and shouldn't be
> changed by other parties during the life time of the activity, I tried to
> emphasis this fact that way.  I don't know any idiom in JavaScript to make it
> clean cut. I am open to suggestions.

http://mxr.mozilla.org/mozilla/source/mailnews/base/public/nsIMsgIncomingServer.idl#66

nsIMsgIncomingServer::key should be set only by the account manager. This seems to be a similar thing.
 
> > Why not PRTime?
> 
> DownloadManager uses long long, and has the function converting this value into
> JavaScript date/time..

Something that definitely needs to be documented (same with the others).

> Ideally, Download Manager should be the specialized version of the Activity
> Manager, and its functionality should be implemented
> through Activity Manager interface. We can work with SM folks to achieve this
> goal.

Have you talked about this with sdwilsh as well?
(In reply to comment #28)
> > Ideally, Download Manager should be the specialized version of the Activity
> > Manager, and its functionality should be implemented
> > through Activity Manager interface. We can work with SM folks to achieve this
> > goal.
> 
> Have you talked about this with sdwilsh as well?

I agree we should talk to the toolkit guys here, ideally I'd have expected what toolkit provides to have be the generic implementation (with potentially a download manager specialisation). This may be too late in the FF cycle, but we can at least plan with it in mind.
In attachment 344848 [details] [diff] [review], I don't see how it hooks into the autosync activities. What I'm currently trying to understand is the extent of hooks that we'll need throughout the mailnews code base.

Whilst I'm here, please don't define/use Cc/Ci globally etc in chrome code (components are ok though). Due to core's inconsistent implementation which causes problems with extensions and other applications, we are keeping away from them in comm-central.

(In reply to comment #27)
> >Does this give us a way of handling what are currently alerts that aren't
> >associated with a particular activity?
> 
> I think the new status-bar is the medium to communicate such alerts. Bryan?

Do we need to consider alerts linked to activities? E.g. smtp sending failed because your server isn't online. Even if its not implemented at the first stage, we should probably consider the connection between the two.
Attached patch m1 (obsolete) — Splinter Review
Work in progress. Some interface, UI and implementation improvements according to comments.
Attachment #345638 - Attachment is obsolete: true
Icons are place-holders and they are going to be changed in the following submissions. To replace the old icons, you need to delete the classic.jar file from dist/bin.
I've added some comments to the talk page of the wiki (https://wiki.mozilla.org/User_talk:Emre/tb/activitymanagerInterface) in an effort to keep noise in this bug down.
Blocks: 123440
Assignee: nobody → bugmil.ebirol
Emre asked me for some comments though IRC on a proposal to add a wrapper class
around the nsIVariant used to represent an aInitiator, to make it easier for
C++ callers to use. I'll give my opinion, but I'll emphasize that this is just
my personal reaction, and others may feel differently.

In order to use this, I have to try to understand the wrapper class, which is a
funny form of nsIVariant. But really all that you need to do is to use the
nsIVariant to pass a COM pointer - which is already supported in nsIVariant. I
don't think that the mental cost of the extra level of abstraction added by the
wrapper is worth the benefit. Even if the use of nsIVariant takes a few more
lines of code, I would prefer to see an example that uses nsIVariant directly
and copy that, rather than have to understand the wrapper.
Blocks: 469049
Blocks: 469050
Blocks: 469051
Blocks: 469052
Blocks: 469053
What Kent said makes sense. I am not going to introduce a template based nsIVariant wrapper unless we have a good reason to do so (i.e. need to use weak pointers to initiator and subjects). Using nsVariant component which is a generic support for nsIVariant interface would suffice for now.

Before submitting for official review, I am waiting for asuth's opinion.

I updated C++ sample and added an UML diagram for activity manager on https://wiki.mozilla.org/User:Emre/tb/activitymanagerInterface. 

I am going to update the documentation before review submission.
Attachment #347939 - Attachment is obsolete: true
Attached patch Milestone 1 - rev 1 (obsolete) — Splinter Review
All backend mechanism has been implemented with this patch. Some UI features such as facet view, search, clear list button, navigation, icon decisions are left to to MS2.
Attachment #352459 - Attachment is obsolete: true
Attachment #353650 - Flags: review?(bugzilla)
+  /**
+   * Activity is currently in progress.
+   */
+  const short STATE_INPROGESS = 0;

Typo, you meant STATE_INPROGRESS.

From looking at the interfaces, it looks quite easy to integrate calendar into the activity manager. I took a quick poke at it today and am successfully creating activities for add/delete/modify actions.

Please remove "calendar" from the generic context display helper, since I think lightning should rather be implementing a helper for that.


Just a question I have, are you planning to make the activity manager obsolete the nsITransaction stuff used for the undo/redo queue?
Attached patch Milestone 1 - rev 2 (obsolete) — Splinter Review
>Typo, you meant STATE_INPROGRESS.

this patch fixes the typo, thanks a lot for catching this at an early stage.

>Please remove "calendar" from the generic context display helper, since I think
>lightning should rather be implementing a helper for that.

done. Still keeping "Calendar" string in activity.properties for your use.

>Just a question I have, are you planning to make the activity manager obsolete
>the nsITransaction stuff used for the undo/redo queue?

Not sure yet. On the long run we need a better undo management solution. For mid term, I guess refactoring transaction manager would be enough for us to move some undo stuff on activities. I like to know what bienvenu thinks about that.

I moved the updated wiki page to https://wiki.mozilla.org/Thunderbird:Activity_Manager/Developer
Attachment #353650 - Attachment is obsolete: true
Attachment #353910 - Flags: review?(bugzilla)
Attachment #353650 - Flags: review?(bugzilla)
Comment on attachment 353910 [details] [diff] [review]
Milestone 1 - rev 2

Given the necessary size of patch, this is going to be a multi-stage review. Here's some simple things to begin with:

>+++ b/mail/components/activity/Makefile.in

>+include $(topsrcdir)/config/rules.mk
>+
>+

nit: no need for extra blank lines at the end of the file. Just a cr after rules.mk will do. Please check in the other files as well.

Whilst I'm on the blank lines subject, there's a lot of blank lines in the patch that have spaces on - these should just be empty blank lines.

>+/* Only focus buttons in the selected item*/

nit: space after item.

>+function ProcessRetryTest()
>+{}
>+ProcessRetryTest.prototype = {
>+  retry: function(aActivity) {
>+    let process = aActivity.QueryInterface(Ci.nsIActivityProcess);
>+    let initiator = process.initiator;
>+    
>+    initiator.retry(process);
>+    
>+    process.state = Ci.nsIActivityProcess.STATE_INPROGRESS;
>+    return Cr.NS_SUCCESS;

I don't know where you got this from, but NS_SUCCESS doesn't exist; NS_OK does.

>+////////////////////////////////////////////////////////////////////////////////
>+//// Utility Functions
>+
>+/**
>+ * Initiate building the activity list to have the processes followed by
>+ * events.
>+ *
>+ * @param aForceBuild
>+ *        Force the list to be built even if the search terms don't change
>+ */
>+function buildActivityList(aForceBuild)
>+{
>+  /* for future use

For future use when? Should this be another bug?

>+/**
>+ * Remove the currently shown activity from the activity list.
>+ */
>+function clearActivityList()
>+{
>+  /*
>+  // Clear the whole list if there's no search
>+  if (gSearchTerms == "") {
>+    gActivityManagerUI.cleanUp();
>+    return;
>+  }
>+  */

This either needs removing or enabling.

>+function processKeyEvent(event)
>+{
>+  try {
>+    //dump("GOT EVENT: " + event + '\n');
>+    switch (event.keyCode) {
>+      case event.DOM_VK_RIGHT:
>+        dump("event.target.tagName = " + event.target.tagName + '\n')
>+        dump("event.target.id = " + event.target.id + '\n')

Do we need all these dumps, if so, there should be some form of better logging here.

>+    <key id="key_close"   key="&cmd.close.commandKey;"  oncommand="closeWindow(true);"    modifiers="accel"/>
>+#ifdef XP_GNOME
>+    <key id="key_close2"  key="&cmd.close2Unix.commandKey;" oncommand="closeWindow(true);" modifiers="accel"/>
>+#else
>+    <key id="key_close2"  key="&cmd.close2.commandKey;" oncommand="closeWindow(true);" modifiers="accel"/>
>+#endif
>+    <key                  keycode="VK_ESCAPE"           oncommand="closeWindow(true);"/>

nit: Only one space is necessary between attributes. Please also wrap the longer lines if they need it.

>+    <!-- Deprecated
>+    <menuitem id="menuitem_open" default="true"

Do we still need it then, or can we just remove it?

>diff --git a/mail/components/activity/jar.mn b/mail/components/activity/jar.mn

>+* content/activity.js (content/activity.js)
>+* content/activity.css (content/activity.css)

I don't see any ifdefs in this file (or #) so it shouldn't need to be preprocessed.

>diff --git a/mail/components/activity/public/Makefile.in b/mail/components/activity/public/Makefile.in

>+# The Original Code is mozilla.org code.
>+#
>+# The Initial Developer of the Original Code is
>+# Netscape Communications Corporation.
>+# Portions created by the Initial Developer are Copyright (C) 1998
>+# the Initial Developer. All Rights Reserved.
>+#
>+# Contributor(s):

This needs filling in and the date updating.
When clicking on the activity manager, I'm getting errors such as:

JavaScript error: chrome://activity/content/activity.xul, line 1: buildContextMenu is not defined
JavaScript error: chrome://activity/content/activity.xul, line 1: onActivityDblClick is not defined

I got this when opening the activity manager window:

WARNING: Illegal character in window name Activity:Manager: file /Users/moztest/comm/main/src/mozilla/embedding/components/windowwatcher/src/nsWindowWatcher.cpp, line 1387

The list of actions is lost on shutdown - is that intentional? Would there be a way to save/log it if we wanted to for debug.

What causes the activity to pause (in this case I'm looking at imap folder syncing on a big unsynced folder)? It is unclear to me why it is paused, is it a mistake in the code or does it only download so many at a time - there are no restart or cancel buttons either, unlike the toolkit download manager - and no indication other than it has paused.
>JavaScript error: chrome://activity/content/activity.xul, line 1:
>buildContextMenu is not defined

Right-Click and Double-Click is not implemented yet. Part of 2nd phase.

>WARNING: Illegal character in window name Activity:Manager: file
>/Users/moztest/comm/main/src/mozilla/embedding/components/windowwatche
>/src/nsWindowWatcher.cpp,
>line 1387

Looks like a bug. Will fix that.

>The list of actions is lost on shutdown - is that intentional?

Yes. Part of 2nd phase.

>What causes the activity to pause (in this case I'm looking at imap folder
>syncing on a big unsynced folder)? It is unclear to me why it is paused, is it
>a mistake in the code or does it only download so many at a time 

Auto-Sync pauses when idle is back (when not-idle). This is the expected behavior.

>there are no restart or cancel buttons either, unlike the toolkit download
>manager - and no indication other than it has paused.

Currently, we don't allow the user to pause, cancel Auto-sync. This is intentional.
> >What causes the activity to pause (in this case I'm looking at imap folder
> >syncing on a big unsynced folder)? It is unclear to me why it is paused, is it
> >a mistake in the code or does it only download so many at a time 
> 
> Auto-Sync pauses when idle is back (when not-idle). This is the expected
> behavior.
> 
> >there are no restart or cancel buttons either, unlike the toolkit download
> >manager - and no indication other than it has paused.
> 
> Currently, we don't allow the user to pause, cancel Auto-sync. This is
> intentional.

This makes me think we should be giving this as part of the process status
message.  Something like:  "Paused.  Will resume when Thunderbird is idle" - Not sure that's the exact phrasing I'd use, but a start.
(In reply to comment #43)
> > Currently, we don't allow the user to pause, cancel Auto-sync. This is
> > intentional.
> 
> This makes me think we should be giving this as part of the process status
> message.  Something like:  "Paused.  Will resume when Thunderbird is idle" -
> Not sure that's the exact phrasing I'd use, but a start.

I agree with Bryan here - we need something that says this is a process that runs when xxx.

(In reply to comment #42)
> >JavaScript error: chrome://activity/content/activity.xul, line 1:
> >buildContextMenu is not defined
> 
> Right-Click and Double-Click is not implemented yet. Part of 2nd phase.

Please can you stub those functions in that case (just put an empty function with an XXX comment in). Otherwise when we land the first phase, we will get bugs filed on them.
Comment on attachment 353910 [details] [diff] [review]
Milestone 1 - rev 2

Some build related issues:

- Please put the idl and js source files straight in the mail/components/activity directory, keep the content files in mail/components/activity/content (this will keep the number of directories we have to enter during build to a minimum).
- Locale files should go into mail/locales/en-US/chrome/activity and be added to the mail/locales/jar.mn file.
Comment on attachment 353910 [details] [diff] [review]
Milestone 1 - rev 2

>--- /dev/null
>+++ b/mail/components/activity/Makefile.in
>@@ -0,0 +1,12 @@
>+DEPTH		= ../../..
>+topsrcdir	= @top_srcdir@
>+srcdir		= @srcdir@
>+VPATH		= @srcdir@
>+
>+include $(DEPTH)/config/autoconf.mk
>+
>+DIRS		= public src
>+
>+include $(topsrcdir)/config/rules.mk
>+
>+

General nit: files should end with a single newline character.


>diff --git a/mail/components/activity/content/activity.css b/mail/components/activity/content/activity.css

This file needs a license header.

>diff --git a/mail/components/activity/content/activity.js b/mail/components/activity/content/activity.js

>+ * Contributor(s):
...
>+ *   David Ascher <dascher@mozillamessaging.com>

I think you should be adding yourself to these.

>+let Cc = Components.classes;
>+let Ci = Components.interfaces;
>+let Cr = Components.results;
>+let Cu = Components.utils;

Please don't define these in chrome/content files. It makes it complicated for extensions.

>+Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>+Cu.import("resource://gre/modules/PluralForm.jsm");
>+
>+const nsIAM = Ci.nsIActivityManager;
>+const nsIAMU = Ci.nsIActivityManagerUI;
>+
>+let gActivityManager = Cc["@mozilla.org/activity-manager;1"].getService(nsIAM);
>+let gActivityManagerUI = Cc["@mozilla.org/activity-manager-ui;1"]
>+                         .getService(nsIAMU);

Once you remove the test code, gActivityManager is used once in startup and once in shutdown, gActivityManagerUI is only used once. I think these don't need to be globals - just get them when you need them.

>+let gActivityMgrListener = null;
>+let gActivitiesView = null;
>+let gSearchBox = null;
>+let gSearchTerms = [];
>+let gBuilder = 0;

These make me think that the functions in this file should be moved into their own object, and these items become member variables.

>+///////////////////////////////////////////////////////////////////////////////
>+//// interface implementation tests

I think the next patch needs all the debugging/test code removed please.

>+////////////////////////////////////////////////////////////////////////////////
>+//// An object to monitor nsActivityManager operations. This class acts as
>+//// binding layer between nsActivityManager and nsActivityManagerUI objects.
>+
>+function ActivityMgrListener()
>+{}
>+
>+ActivityMgrListener.prototype = {
>+
>+  onAddedActivity: function(aID, aActivity) {
>+    try {
>+      addActivityBinding(aID, aActivity);
>+    }
>+    catch(e) {
>+      dump(e + "\n");

What failures are these catching? Why are we not letting the error get to the error console?

>+/**
>+ * Creates the proper binding for the given activity
>+ */
>+function createActivityBinding(aActivity)
>+{
>+  let bindingName = aActivity.bindingName;
>+  let binding = document.createElement(bindingName);
>+  
>+  dump("++++++++ Binding name is: " + bindingName + "\n");

If we're keeping these (ditto for other dumps), then they should be converted to a logging mechanism.

>+/**
>+ * Adds a new binding to activity manager  window for the

nit: double space

>+  let isGroupByContext = (aActivity.groupingStyle ==
>+                          Ci.nsIActivity.GROUPING_STYLE_BYCONTEXT);
>+                        
>+  

nit: you only need one blank line, and you also have unncessary whitespace here

>+function removeActivityBinding(aID)
>+{
>+  // Note: document.getAnonymousNodes(gActivitiesView); didn't work
>+  //
>+  for (let i = 0; i < gActivitiesView.itemCount; i++) {
>+    let item = gActivitiesView.getItemAtIndex(i);
>+    if (!item) continue;

nit: continue; on the next line with a blank line after it

>+////////////////////////////////////////////////////////////////////////////////
>+//// Startup, Shutdown

I know you just copied these, but normally these are called things like onLoad xxxLoad etc and I'd prefer them to follow that convention here as it is not really startup and shutdown that these functions apply to.

>+function clearActivityList()
>+{
>+  /*
>+  // Clear the whole list if there's no search
>+  if (gSearchTerms == "") {
>+    gActivityManagerUI.cleanUp();
>+    return;
>+  }
>+  */

Is this future use or debug? (i.e. please add a comment if for future use).
Attachment #353910 - Flags: review?(bugzilla)
Comment on attachment 353910 [details] [diff] [review]
Milestone 1 - rev 2

>diff --git a/mail/components/activity/content/activity.xml b/mail/components/activity/content/activity.xml

>@@ -0,0 +1,896 @@
>+<?xml version="1.0"?>
>+
>+# -*- Mode: Java; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*-

These should both be 2 not 4 (please check these in other files as well).

>+<bindings id="activityBindings"
>+          xmlns="http://www.mozilla.org/xbl"
>+          xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
>+          xmlns:xbl="http://www.mozilla.org/xbl">
>+          
>+  <binding id="activity-group" extends="chrome://global/content/bindings/richlistbox.xml#richlistitem">
>+    <resources>
>+      <stylesheet src="chrome://messenger/skin/activity/activity.css"/>

This looks like a mixing of chrome:// meanings. i.e. you have chrome://activity/content and now you have chrome://messenger/skin/activity. Surely you want to use chrome://activity/ for both?

>+      <constructor>
>+       <![CDATA[
>+        this.logEnabled = false;
>+        this.log("+++++++++ in ctor of activitygroup\n");

This line is never (or the one in catch) is never going to get logged.

>+        try {
>+          // get the registered context display helper component for given
>+          // context type to format the text properly
>+          let helper = activityManager.getContextDisplayHelper(this.contextType);
>+          this.setAttribute('contextDisplayText',
>+                            helper.getContextDisplayText(this.contextType,
>+                                                         this.contextObj));
>+         }

nit: incorrect indentation.

>+        catch(e) {
>+          this.log("+++++++++ Error in ctor of activitygroup: " + e +"\n");
>+          this.setAttribute('contextDisplayText', "Undefined!");
>+        }

Again, this catch is going to hide errors - is it really necessary? (and the others in this file).

>+      <field name="contextType">
>+       "";
>+      </field>
>+      <field name="contextObj">
>+       null;
>+      </field>

You shouldn't need the ; on either of these (or the other field entries): https://developer.mozilla.org/en/XUL_Tutorial/Adding_Properties_to_XBL-defined_Elements#Fields

>+          this.text = {
>+            paused: "paused",
>+            canceled: "canceled",
>+            failed: "failed",
>+            waitingforinput: "waitingforinput",
>+            waitingforretry: "waitingforretry",
>+            yesterday: "yesterday",
>+            monthDate: "monthDate"
>+          };

You've got a mixture of label styles here. I'd prefer waitingForInput and waitingForRetry - especially as that is how .property files are normally done.

>+      <field name="logEnabled">
>+       false;
>+      </field>

So the only way the log can be enabled is via changing the code? If this logging is really that useful (and I'm not convinced it is) then we should be providing a hidden preference (probably not even in the .js file e.g. see signon.debug) to enable it - otherwise we should remove this option and allow the default for failures, and remove the success statements.

>+        <parameter name="visible"/>
>+        <body>
>+          <![CDATA[
>+            let item = document.getAnonymousElementByAttribute(this, "anonid",
>+                                                               anonid);
>+            item.setAttribute('hidden', !visible);

You don't need the intermediate item here.

>+            } else if (today - end < (24 * 60 * 60 * 1000)) {
>+              // activity finished after yesterday started, show yesterday
>+              dateTime = this.text.yesterday;
>+            } else if (today - end < (6 * 24 * 60 * 60 * 1000)) {

You should make the numbers in these if statements into constants.

>+        // We need to make this casting here in order to access to nsIActivityProcess
>+        // interface of the component from the the binding.
>+        // We deliberately propogate the exceptions to the caller.
>+        this._activity.QueryInterface(Components.interfaces.nsIActivityProcess);
>+        
>+        try {
>+          this.displayText = this.activity.displayText;

You're using this._activity and this.activity interchangeably/inconsistently. Its probably best to use this._activity internally and keep the activity property if you need to access it externally.

>+      <property name="paused">
...
>+      <property name="waitingforinput">
...
>+      <property name="waitingforretry">
>+        <getter>
>+        <![CDATA[
>+          return parseInt(this.getAttribute("state")) ==
>+            Components.interfaces.nsIActivityProcess.STATE_WAITINGFORRETRY;
>+        ]]>
>+        </getter>
>+      </property>

I can't see anywhere for the state attribute to be set for these properties. Should these not be using this._activity.state ?

>+      <property name="completionTime">
>+        <getter>
>+          <![CDATA[
>+            return this.getAttribute('completionTime');
>+          ]]>
>+        </getter>
>+        <setter>
>+          <![CDATA[
>+            this.setAttribute('completionTime', this.formatTime(val));
>+            this.setAttribute("completionTimeTip", this.formatTimeTip(val));

nit: inconsistent use of quote styles.

>+      <property name="canUndo">
>+        <getter>
>+        <![CDATA[
>+          try {
>+           return (this.activity.UndoHandler != null);
>+          }
>+          catch(e) {
>+            // In normal conditions, we shouldn't endup here. This can only
>+            // happen if the XBL is orphan - associated activity has been
>+            // deleted but XBL stays alive.
>+          }

This is actually a good example of why we should avoid try catch, although in this case it looks like you still need to keep it - The return statement should have a small u for undoHandler.

>+  <stringbundleset id="activitySet">
>+    <stringbundle id="brandStrings" src="chrome://branding/locale/brand.properties"/>
>+    <stringbundle id="activityStrings" src="chrome://activity/locale/activity.properties"/>
>+  </stringbundleset>

I don't see a use of brandStrings in this patch.


>+    <key id="key_removeFromList"  keycode="VK_DELETE" oncommand="performCommand('cmd_removeFromList');"/>
>+#ifdef XP_MACOSX
>+    <key id="key_removeFromList2" keycode="VK_BACK" oncommand="performCommand('cmd_removeFromList');"/>
>+#endif

nit: these longer lines need wrapping.

>+    <key id="key_close"   key="&cmd.close.commandKey;"  oncommand="closeWindow(true);"    modifiers="accel"/>
>+#ifdef XP_GNOME
>+    <key id="key_close2"  key="&cmd.close2Unix.commandKey;" oncommand="closeWindow(true);" modifiers="accel"/>
>+#else
>+    <key id="key_close2"  key="&cmd.close2.commandKey;" oncommand="closeWindow(true);" modifiers="accel"/>
>+#endif
>+    <key                  keycode="VK_ESCAPE"           oncommand="closeWindow(true);"/>

nit: only one space between attributes please. I don't think formatting these like this is buying us anything here.

>+    <key id="key_findActivity"
>+         key="&cmd.find.commandKey;"
>+         modifiers="accel"
>+         command="cmd_findActivity"/>
>+    <key id="key_findActivity2"
>+         key="&cmd.search.commandKey;"
>+         modifiers="accel"
>+         command="cmd_findActivity"/>
>+    <key id="key_selectAllActivities"
>+         key="&selectAllCmd.key;"
>+         modifiers="accel"
>+         command="cmd_selectAllActivities"/>

nit: I'd prefer to fit what is sensibly possible to a limit of 80 chars on one line. I know we're not consistent in mailnews, but lets get this patch in a format consistent with itself.

>diff --git a/mail/components/activity/content/activityOverlay.xul b/mail/components/activity/content/activityOverlay.xul
>new file mode 100644
>--- /dev/null
>+++ b/mail/components/activity/content/activityOverlay.xul
>@@ -0,0 +1,9 @@
>+<?xml version="1.0" encoding="UTF-8"?>
>+
>+<overlay id="activityOverlay"
>+         xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
>+  <key id="activityKey" key="4" modifiers="meta"/>
>+  <menupopup id="taskPopup">
>+    <menuitem label="Activity" key="activityKey" oncommand="Components.classes['@mozilla.org/activity-manager-ui;1'].getService(Components.interfaces.nsIActivityManagerUI).show(window);"/>
>+  </menupopup>
>+</overlay>

I think as the activity manager will be an integral part of Thunderbird, we can just drop this separate overlay and put the menuitem in the taskPopup directly and provide an appropriate function for it.

>diff --git a/mail/components/activity/locale/en-US/activity.dtd b/mail/components/activity/locale/en-US/activity.dtd
>diff --git a/mail/components/activity/locale/en-US/activity.properties b/mail/components/activity/locale/en-US/activity.properties

I'll review these in more detail on the next version of the patch.

>+# LOCALIZATION NOTE (paused): — is the "em dash" (long dash)
>+paused=Paused

I see no dash in Paused.

>diff --git a/mail/components/activity/public/nsIActivity.idl b/mail/components/activity/public/nsIActivity.idl
>+/**
>+ * See https://wiki.mozilla.org/Thunderbird:Activity_Manager/Developer for UML
>+ * diagram and sample codes.
>+ */

Separate note: we should move that to devmo.

>+/**
>+ * The handler to invoke when the recover button is pressed. Used by a Warning
>+ * to recover from the situation causing the warning. For instance, recovery
>+ * action for a "Over Quota Limit" warning, would be to cleanup some disk space,
>+ * and this operation can be implemented and set by the activity developer in
>+ * form of nsIActivityRecoveryHandler component.
>+ */
>+[scriptable, uuid(30E0A76F-880A-4093-8F3C-AF2239977A3D)]
>+interface nsIActivityRecoveryHandler : nsISupports {
>+  nsresult recover(in nsIActivityWarning aActivity);

Why return the result and not just throw it as per the normal xpcom processes. Is there a standard set of errors we're expecting? (if so, what are they?).

>+  /**
>+   * A text value to associate a facet type with the activity. If missing,
>+   * the activity shown in the 'Misc' section.
>+   */
>+  attribute AString facet;

The attribute can't be missing. Do you mean empty?


>+  /**
>+   * The handler to invoke when the retry button is pressed. If present
>+   * (non-null), the activity can be retryable and a  retry button will be

nit: double space.
Attachment #353910 - Flags: review-
Comment on attachment 353910 [details] [diff] [review]
Milestone 1 - rev 2

>diff --git a/mail/components/activity/public/nsIActivityManager.idl b/mail/components/activity/public/nsIActivityManager.idl

>+#include "nsISupports.idl"
>+#include "nsIActivity.idl"
>+
>+interface mozIStorageConnection;

nsIActivity can be an interface rather than an include here, then we don't have to regenerate this file if that one changes.

>+ * There are 3 different classifications of activity items which can be
>+ * displayed in the Activity Manager Window:
>+ *  o Process: Processes are transient in the display. They are not written to
>+ *             disk as they are always acting on some data that already exists
>+ *             locally or remotely. If a process has finished and needs to keep
>+ *             some state for the user (like last sync time) it can convert
>+ *             itself into an event.
>+ * o Event: Historical actions performed by the user and created by a process

nit: inconsistent indentation

>+  /**
>+   * Adds the given activity into the managed acitivities list.

nit: activities

>+interface nsIActivityManagerUI : nsISupports {
>+  /**
>+   * The reason that should be passed when the user requests to show the
>+   * activity manager's UI.
>+   */
>+  const short REASON_USER_INTERACTED = 0;
>+
>+  /**
>+   * The reason that should be passed to the show method when we are displaying
>+   * the UI because a new activity is being added to it.
>+   */
>+  const short REASON_NEW_ACTIVITY = 1;

Do we really need these with the activity manager?

>+    for (i = 0; i < this._listeners.length; i++) {
>+      if (this._listeners[i] == aListener) {
>+        this._listeners.splice(i,1);

nit: space after , please.

>+        case 'smtp':
>+          //TODO: get pretty name to display
>+          displayText = aContextObj;
>+          break;
>+        case 'addressbook':
>+          //TODO: get pretty name to display

General comment: Please change TODO to XXX - its more standard as a to do type comment.

>+    // notify all the listeners
>+    for each (let [, value] in Iterator(this._listeners)) {
>+      try {
>+        value.onAddedActivity(id, aActivity);
>+      }
>+      catch(e) {
>+        // ignore the exception

What's the exception normally? Should we be logging it so that extensions can find problems in their code?

>diff --git a/mail/themes/pinstripe/jar.mn b/mail/themes/pinstripe/jar.mn

There are no qute theme changes/additions in this patch.


>--- a/mail/themes/pinstripe/jar.mn
>+++ b/mail/themes/pinstripe/jar.mn
>@@ -25,6 +25,22 @@ classic.jar:
>   skin/classic/messenger/editContactOverlay.css                  (mail/editContactOverlay.css)
>   skin/classic/messenger/starred48.png                           (mail/starred48.png)
>   skin/classic/messenger/starIcons.png                           (mail/starIcons.png)
>+  skin/classic/messenger/activity/buttons.png                	 (mail/activity/buttons.png)  
>+  skin/classic/messenger/activity/defaultProcessIcon.png 	 (mail/activity/defaultProcessIcon.png)
>+  skin/classic/messenger/activity/defaultEventIcon.png 		 (mail/activity/defaultEventIcon.png)
>+  skin/classic/messenger/activity/defaultWarningIcon.png 	 (mail/activity/defaultWarningIcon.png)
>+  skin/classic/messenger/activity/activity.css               	 (mail/activity/activity.css)
>+  skin/classic/messenger/activity/unknownContentType.css     	 (mail/activity/unknownContentType.css)
>+  skin/classic/messenger/activity/undoIcon.png           	 (mail/activity/undoIcon.png)
>+  skin/classic/messenger/activity/syncMailIcon.png           	 (mail/activity/syncMailIcon.png)
>+  skin/classic/messenger/activity/sendMailIcon.png           	 (mail/activity/sendMailIcon.png)
>+  skin/classic/messenger/activity/removeItemIcon.png         	 (mail/activity/removeItemIcon.png)
>+  skin/classic/messenger/activity/addItemIcon.png            	 (mail/activity/addItemIcon.png)
>+  skin/classic/messenger/activity/moveMailIcon.png           	 (mail/activity/moveMailIcon.png)
>+  skin/classic/messenger/activity/copyMailIcon.png           	 (mail/activity/copyMailIcon.png)
>+  skin/classic/messenger/activity/deleteMailIcon.png         	 (mail/activity/deleteMailIcon.png)
>+  skin/classic/messenger/activity/compactMailIcon.png        	 (mail/activity/compactMailIcon.png)
>+  skin/classic/messenger/activity/indexMailIcon.png          	 (mail/activity/indexMailIcon.png)

Why are you overriding all of these? - we're not replacing anything here.

I'll look at the themes in a future updated patch.
This is on the UX changes we want for TB 3, therefore blocking.

Emre any updates? I'd like to get this in for B2 if possible.
Flags: blocking-thunderbird3+
Target Milestone: Thunderbird 3 → Thunderbird 3.0b2
Taking over from a driving perspective, and i can coordinate w/ emre.
Assignee: bugmil.ebirol → david.ascher
Whiteboard: [needs new patch]
Attached patch Milestone 1 - rev 3 (obsolete) — Splinter Review
>+* content/activity.js (content/activity.js)
>+* content/activity.css (content/activity.css)
>I don't see any ifdefs in this file (or #) so it >shouldn't need to be preprocessed.

Not sure what you mean.

> Please put the idl and js source files straight in the
> mail/components/activity directory, keep the content 
> files in mail/components/activity/content ...

Will do for next phase.

>I think the next patch needs all the debugging/test >code removed please.

Will do.

>+    try {
>+      addActivityBinding(aID, aActivity);
>+    }
>+    catch(e) {
>+      dump(e + "\n");
>What failures are these catching? Why are we not >letting the error get to the error console?

It catches the exception listeners might throw. Fail safety.

>If we're keeping these (ditto for other dumps), then >they should be converted to a logging mechanism.

Dump/Logs are my debugging mechanism. Will be removed
when I submit the final patch.

>+        catch(e) {
>+          this.log("+++++++++ Error in ctor of activitygroup: " + e +"\n");
>+          this.setAttribute('contextDisplayText', "Undefined!");
>+        }
> Again, this catch is going to hide errors - is it 
> really necessary? (and the others in this file). 

It sets a default value to contextDisplayText. It doesn't really hide the error. I open to suggestions.

>So the only way the log can be enabled is via changing 
>the code? If this logging is really that useful 
> (and I'm not convinced it is) 

I am using it as debugging mechanism. We don't need a real logging mechanism here really.

>nsIActivity can be an interface rather than an include >here, then we don't have
>to regenerate this file if that one changes.

IDL processor complaints when I do that.

>There are no qute theme changes/additions in this 
>patch.
---
> Why are you overriding all of these? - we're not 
> replacing anything here.

Not sure what is the right thing to do here. More explanation would help.

===

Other nits either are fixed, or left for the next phase.
Attachment #358228 - Flags: review?(bugzilla)
> >+* content/activity.js (content/activity.js)
> >+* content/activity.css (content/activity.css)
> >I don't see any ifdefs in this file (or #) so it >shouldn't need to be preprocessed.
> 
> Not sure what you mean.

The * means the file is going to be preprocessed. As you haven't got any ifdefs in the file it doesn't need preprocessing so you can drop the *.

> >+    try {
> >+      addActivityBinding(aID, aActivity);
> >+    }
> >+    catch(e) {
> >+      dump(e + "\n");
> >What failures are these catching? Why are we not >letting the error get to the error console?
> 
> It catches the exception listeners might throw. Fail safety.

We should either 1) document that you can't throw in your listener function or 2) propagate the error to the error console so that extension authors have a chance of knowing their extensions is doing something wrong.

> >If we're keeping these (ditto for other dumps), then >they should be converted to a logging mechanism.
> 
> Dump/Logs are my debugging mechanism. Will be removed
> when I submit the final patch.

I don't see how we're going to get to the final patch at the moment. I'd rather see it removed before I give r+.

> >+        catch(e) {
> >+          this.log("+++++++++ Error in ctor of activitygroup: " + e +"\n");
> >+          this.setAttribute('contextDisplayText', "Undefined!");
> >+        }
> > Again, this catch is going to hide errors - is it 
> > really necessary? (and the others in this file). 
> 
> It sets a default value to contextDisplayText. It doesn't really hide the
> error. I open to suggestions.

> >nsIActivity can be an interface rather than an include >here, then we don't have
> >to regenerate this file if that one changes.
> 
> IDL processor complaints when I do that.

ok, no problem leave it then.

> >There are no qute theme changes/additions in this 
> >patch.
> ---
> > Why are you overriding all of these? - we're not 
> > replacing anything here.
> 
> Not sure what is the right thing to do here. More explanation would help.

mail/themes/pinstripe is the mac theme, the qute and gnomestripe ones are the Windows/Linux themes. If we don't have appropriate theme updates for those then it'll fail to display properly on Windows/Linux. If you're not sure/can't test, then let me know and I'll see if I can come up with appropraite files.

Re the overriding - the '+' says you are overriding something that has already been put into the jar.mn file. So unless these images are in toolkit and you're changing them (and I don't think they are) then you can just drop the '+'.
Depends on: 475033
Attached patch Cleaned up patch (obsolete) — Splinter Review
Mark, I've taken emre's last patch, and cleaned up a few things, including some of the pending review items.  I've also done various minor cleanups:

 - switched to using log4moz logging everywhere, using the code in bug 475033
 - changed the signature of the progress setting to compute percentage centrally as opposed to peripherally
 - fixed a couple of bugs with the clearing of the list
 - removed the search field and context menu as they're not implemented yet
 - fixed attribution lines
 - fixed a bunch of whitespace nits
 - css tweaks

I have an updated combined patch for bugs 469052 & 469053 which I use to test, and which I suggest you look at at the same time (it helps me at least to see how it's used) -- i'll post it now as well.

I'm pretty sure I've forgotten something, like removing the logging default levels, and i don't have non-pinstripe themes yet, so I'm not expecting an r+ yet, but if you could give it a look that'd be helpful.
Attachment #342150 - Attachment is obsolete: true
Attachment #344848 - Attachment is obsolete: true
Attachment #353910 - Attachment is obsolete: true
Attachment #358228 - Attachment is obsolete: true
Attachment #358547 - Flags: review?(bugzilla)
Attachment #358228 - Flags: review?(bugzilla)
Blocks: 471208
Whiteboard: [needs new patch] → [needs review standard8]
Status: NEW → ASSIGNED
Comment on attachment 358547 [details] [diff] [review]
Cleaned up patch

I've just reviewed down to the end of the idl files, I'll finish the review tomorrow, but I thought there's probably a reasonable amount of work in the current set of comments. 

Issues from previous comments not addressed yet:

- (comment 41) WARNING: Illegal character in window name Activity:Manager
- (comment 45) idl and js source files (not content) straight into mail/components/activity, leave content in mail/components/activity/content.
- (comment 45) locale files should go somewhere under mail/locales/en-US
- (comment 47) emacs mode lines (Mode: Java; tab-width: 4; etc) should have tab-width and c-basic-offset set to 2 not 4.
- (comment 47) There is currently chrome://messenger/skin/activity/ and chrome://activity/content. We need to be consistent in both otherwise we're just going to confuse oursleves.

General Notes:

- The new menu item has now been added to mailWindowOverlay.xul but activityOverlay.xul still exists so we now have two menu items. I think we need to kill activityOverlay.xul.

diff --git a/mail/components/activity/content/activity.js b/mail/components/activity/content/activity.js

+let gActivityMgrListener = null;
+let gActivityManager = null;

gActivityManager is only used in startup and shutdown, we don't need a global for it.

+let gBuilder = 0;

This isn't used.

+function SetupActivityMgrLogger()
+{
+  gActivityLogger = Log4Moz.getConfiguredLogger("activity", Log4Moz.Level.Debug,
+                    Log4Moz.Level.Debug, Log4Moz.Level.Debug);

I guess these are the levels you mentioned that need changing.

+/**
+ * Returns the activity group binding matches with the context_type
+ * and context of the given activity, if any.

nit: "Returns the activity group binding *that* matches"

+function placeActivityBinding(aBinding)
+{
+  if (aBinding.isGroup || aBinding.isProcess)
+    gActivitiesView.insertBefore(aBinding, gActivitiesView.firstChild);
+  else {
+    let next = gActivitiesView.firstChild;
+    while (next && (next.isWarning || next.isProcess || next.isGroup))
+      next = next.nextSibling;
+    if (next === undefined)

I think that would be clearer as if (!next). Alternately swap the clauses around and make it if (next).

+function addActivityBinding(aID, aActivity)
+{
+  try {
+  gActivityLogger.info("Adding ActivityBinding: " + aID + ", " + aActivity)

I'd prefer not to have a try/catch here. We shouldn't generally be failing, if we do then we've probably
done something wrong. Also, the removeActivityBinding doesn't have it.

If you think we should keep it, then please re-indent the function.

+  gActivityLogger.info("focusing activitybinding")
+  actBinding.focus();

I'm concerned about this for when we display the activity manager. We'll be generating lots of focus events that we probably don't want.

+/**
+ * Update the disabled state of the clear list button based on whether or not
+ * there are items in the list that can potentially be removed.
+ */
+function updateClearListButton()
+{
+  let button = document.getElementById("clearListButton");
+  // The button is enabled if we have items in the list
+  button.disabled = !(gActivitiesView.itemCount);
+}

This doesn't work (or the call to it).

+function processKeyEvent(event)
+{
+  switch (event.keyCode) {
+    case event.DOM_VK_RIGHT:
+      if (event.target.tagName == 'richlistbox') {
+        // we need to find out which item in this richlistbox is
+        // selected
+        let richlistbox = event.target.selectedItem.processes; // get the richlistbox
+        if (richlistbox.tagName == 'xul:richlistbox') {
+          richlistbox.focus();
+          richlistbox.selectItem(richlistbox.getItemAtIndex(0));
+        }

I'm not quite sure what this is intending to do, but it doesn't work and generates:

JavaScript error: chrome://activity/content/activity.js, line 340: richlistbox is undefined

+      }
+      break;
+    case event.DOM_VK_LEFT:
+      if (event.target.tagName == 'activity-group') {
+        // we need to find out which item in this richlistbox is
+        // selected
+        var parent = event.target.parentNode;
+        if (parent.tagName == 'richlistbox') {
+          event.target.processes.clearSelection();
+          parent.selectItem(event.target);
+          parent.focus();
+        }
+      }

I'm not quite sure what this does either, maybe because I've not got an activity group item.

diff --git a/mail/components/activity/content/activity.xml b/mail/components/activity/content/activity.xml

+        try {
+          this.displayText = this._activity.displayText;
+          // make sure that binding reflects the latest state of the process
+          this.onStateChanged(this._activity.state,
+                              Components.interfaces.nsIActivityProcess.STATE_NOTSTARTED);
+          this.onProgressChanged(this._activity, this._activity.percentComplete,
+                                 this._activity.lastStatusText,
+                                 this._activity.workUnitComplete,
+                                 this._activity.totalWorkUnits);
+         }
+         catch(e) {
+           this.displayText = 'Invalid Process!';

This isn't very helpful to developers. We should at least be letting the error get through to the error console. 

+  <binding id="activity-event" extends="chrome://activity/content/activity.xml#activity-base">
...
+        try {
+          this.displayText = this._activity.displayText;
+          this.statusText =  this._activity.statusText;
+          this.completionTime = this._activity.completionTime;
+          this.setVisibility("button_undo", this._activity.undoHandler);
+        }
+        catch(e) {
+          this.displayText = 'Invalid Event!'; // XXX l10n?
+          this.completionTime = Date.now();
+          this.statusText =  'unknown'; // XXX l10n?

Again, we should be reporting the actual error somewhere (please also fix the other constructors in this file).

+  <!-- Use this commandset for command which do not depened on focus or selection -->

nit: depend

+  <commandset id="generalCommands">
+    <command id="cmd_findActivity" oncommand="setSearchboxFocus();"/>
+    <command id="cmd_selectAllActivities" oncommand="gActivitiesView.selectAll();"/>
+    <command id="cmd_clearList" oncommand="clearActivityList();"/>
+    <!-- Debugging only
+    <command id="cmd_testAddActivityProcess" oncommand="test_addNewActivityProcess();"/>
+    <command id="cmd_testAddActivityEvent" oncommand="test_addNewActivityEvent();"/>
+    <command id="cmd_testAddActivityWarning" oncommand="test_addNewActivityWarning();"/>
+    <command id="cmd_testRemoveFirstActivity" oncommand="test_RemoveFirstActivity();"/>
+    -->

I think we can drop these as well now.

+  <keyset id="activityKeys">
+    <key keycode="VK_ENTER" oncommand="doDefaultForSelected();"/>
+    <key keycode="VK_RETURN" oncommand="doDefaultForSelected();"/>

When I try and use these keys, I get:

JavaScript error: chrome://activity/content/activity.xul, line 1: doDefaultForSelected is not defined

+    <key id="key_pauseResume" key=" " oncommand="performCommand('cmd_pauseResume');"/>
+    <key id="key_removeFromList"  keycode="VK_DELETE" oncommand="performCommand('cmd_removeFromList');"/>
+#ifdef XP_MACOSX
+    <key id="key_removeFromList2" keycode="VK_BACK" oncommand="performCommand('cmd_removeFromList');"/>
+#endif

When I try and use these keys, I get:

JavaScript error:  chrome://activity/content/activity.xul, line 1: performCommand is not defined

+    <key id="key_findActivity" key="&cmd.find.commandKey;" modifiers="accel" command="cmd_findActivity"/>
+    <key id="key_findActivity2" key="&cmd.search.commandKey;" modifiers="accel" command="cmd_findActivity"/>

These don't work either:

JavaScript error: chrome://activity/content/activity.xul, line 1: setSearchboxFocus is not defined

diff --git a/mail/components/activity/jar.mn b/mail/components/activity/jar.mn

+activity.jar:
+% content activity %content/
+% locale  activity   en-US   %locale/en-US/

I'm not sure that having its own jar file is a good idea. We could easily get a lot of files that we have to individually open on startup. I suggest we either re-use messenger.jar, or make our own mail.jar.

diff --git a/mail/components/activity/locale/en-US/activity.dtd b/mail/components/activity/locale/en-US/activity.dtd

+<!ENTITY cmd.recover.label                "Recover">
+<!ENTITY cmd.recover.accesskey            "V">

nit: should be a small 'v' - generally the case of the accesskey should match that of the key in the label.

+<!ENTITY cmd.close.commandKey             "w">
+<!ENTITY cmd.close2.commandKey            "j">
+<!ENTITY cmd.close2Unix.commandKey        "y">
+<!ENTITY cmd.clearList.label              "Clear List">
+<!ENTITY cmd.clearList.tooltip            "Removes completed, canceled, and failed items from the list">
+<!ENTITY cmd.clearList.accesskey          "C">
+<!ENTITY cmd.find.commandKey              "f">
+<!ENTITY cmd.search.commandKey            "k">

nit: please make the commandKey all small letters, i.e. commandkey.

diff --git a/mail/components/activity/locale/en-US/activity.properties b/mail/components/activity/locale/en-US/activity.properties

+# Status Text
+paused=Paused. Will resume when Thunderbird is idle.

We shouldn't be using "Thunderbird" here, we should be using brandShortName. Its a bit of a pain, but we do do it, e.g. http://mxr.mozilla.org/comm-central/source/mail/locales/en-US/chrome/messenger/messenger.properties#107
Thanks, will take a whack at these today.
Whiteboard: [needs review standard8] → [needs more review standard8][needs new patch]
Attached patch revision 5 (obsolete) — Splinter Review
Revised patch which addresses all of the comments, I believe.  Two exceptions:


+function processKeyEvent(event)
+{
+  switch (event.keyCode) {
+    case event.DOM_VK_RIGHT:
+      if (event.target.tagName == 'richlistbox') {
+        // we need to find out which item in this richlistbox is
+        // selected
+        let richlistbox = event.target.selectedItem.processes; // get the
richlistbox
+        if (richlistbox.tagName == 'xul:richlistbox') {
+          richlistbox.focus();
+          richlistbox.selectItem(richlistbox.getItemAtIndex(0));
+        }

> I'm not quite sure what this is intending to do, but it doesn't 
> work and generates:

> JavaScript error: chrome://activity/content/activity.js, line 340: 
>  richlistbox is undefined


I can't repro.  What STR are you using?  (I explain what it's about below)

==

+      }
+      break;
+    case event.DOM_VK_LEFT:
+      if (event.target.tagName == 'activity-group') {
+        // we need to find out which item in this richlistbox is
+        // selected
+        var parent = event.target.parentNode;
+        if (parent.tagName == 'richlistbox') {
+          event.target.processes.clearSelection();
+          parent.selectItem(event.target);
+          parent.focus();
+        }
+      }

> I'm not quite sure what this does either, maybe because I've not got an
> activity group item.

Right, so both of these are keybindings to let you navigate the "tree" of activities, so that if you have e.g.

  [activity group
    [process 1]
    [process 2]
  ]
  [warning 1]*
  [event 1]
  [event 2]

and warning 1 is the selected thing, then up arrow will select the activity group.  at that point, right arrow will select "in" and select process 1, then down arrow will move to process 2, etc.

It's all to make it possible to do keyboard navigation of these nested elements i the richlistbox.  Does that help?

(to get a group activity, see the autosync patch).
Attachment #345677 - Attachment is obsolete: true
Attachment #358547 - Attachment is obsolete: true
Attachment #359215 - Flags: review?(bugzilla)
Attachment #358547 - Flags: review?(bugzilla)
Sorry for this stupid and little bit offtopic question, but what is the best way to apply this patch? I've applied this patch with "patch -p1 < patch1.diff" and it applies without any error, but without this png images. And if I try to apply the part with the png images alone, than I get an error, the patch contains only garbage(?).
What did I wrong? Thanks.
Comment on attachment 359215 [details] [diff] [review]
revision 5

(In reply to comment #57)
> +  switch (event.keyCode) {
> +    case event.DOM_VK_RIGHT:
> +      if (event.target.tagName == 'richlistbox') {
> +        // we need to find out which item in this richlistbox is
> +        // selected
> +        let richlistbox = event.target.selectedItem.processes; // get the
> richlistbox
> +        if (richlistbox.tagName == 'xul:richlistbox') {
> +          richlistbox.focus();
> +          richlistbox.selectItem(richlistbox.getItemAtIndex(0));
> +        }
> 
> > I'm not quite sure what this is intending to do, but it doesn't 
> > work and generates:
> 
> > JavaScript error: chrome://activity/content/activity.js, line 340: 
> >  richlistbox is undefined
> 
> I can't repro.  What STR are you using?  (I explain what it's about below)

Select a non-grouped item (e.g. use the mail movement patch), and press the right key.

> +    case event.DOM_VK_LEFT:
> +      if (event.target.tagName == 'activity-group') {
> +        // we need to find out which item in this richlistbox is
> +        // selected
> +        var parent = event.target.parentNode;
> +        if (parent.tagName == 'richlistbox') {
> +          event.target.processes.clearSelection();
> +          parent.selectItem(event.target);
> +          parent.focus();
> +        }
> +      }
> 
> > I'm not quite sure what this does either, maybe because I've not got an
> > activity group item.
> 
> Right, so both of these are keybindings to let you navigate the "tree" of
> activities, so that if you have e.g.

> (to get a group activity, see the autosync patch).

I couldn't get a group activity stable long enough to try this. Maybe I needed bigger messages. I did see one effect though - with lots of items in the list, and it doing some syncing, it was jumping up and down the display as it added new items and things.

I've also noticed the Clear button doesn't get disabled, as the code suggests it should be.

diff --git a/mail/components/activity/Makefile.in b/mail/components/activity/Makefile.in
+
+include $(topsrcdir)/config/rules.mk
+

nit: no blank needed here.

diff --git a/mail/components/activity/nsActivity.js b/mail/components/activity/nsActivity.js
+//// Base class for nsActivityProcess and nsActivityEvent objects
+
+function nsActivity()
+{
+  try {
+    this._initLogging();
+    this._id = -1;
+    this._listeners = [];
+    this._displayText = 'An Activity';
+    this._initiator = null;
+    this._contextType = 'unknown';
+    this._context = 'undefined';
+    this._subjects = [];
+    this._iconClass = "";
+    this._bindingName = "";
+    this._groupingStyle = Ci.nsIActivity.GROUPING_STYLE_BYCONTEXT;
+    this._facet = "";
+  } catch (e) {
+    dump("Exception: " + e + "\n");
+    throw(e);
+  }

This try/catch doesn't do anything useful, it just dumps the output which will get dumped again due to the throw(e) - at least on debug builds, release builds it will make it to the error console if not caught. Let's just drop it.

+  addListener: function(aListener) {
+    this.log.debug("Number of listeners (before addListener) for activity #" +
+             this._id + " is " + this._listeners.length + " -- " + aListener);
+    
+    this._listeners.push(aListener);
+    
+    this.log.debug("Number of listeners (after addListener) for activity #" +
+             this._id + " is " + this._listeners.length);

I think logging the number of listeners before and after is a bit excessive. Generally I think we could just log the fact that we've added a listener for activity n (and even then its only really useful for checking that we're balancing our add and removes).

+  removeListener: function(aListener) {

Same comment applies to this function.

+  get bindingName() {
+    return this._bindingName;
+  },
+  
+  set bindingName(val) {
+    this._bindingName = val;
+  },

bindingName, iconClass, groupingStyle, facet, displayText, initiator, contextType, contextObj (ok, so basically all the attributes on the nsIActivity interface) don't need to have explicit getters/setters. For js the only thing it gives us is another function to write and process.

We can just replace these by:

  bindingName: "",

(i.e. initialise the attribute with the value currently in the constructor and then drop the constructor initialisation). Of course, you'll also need to replace this._bindingName with this.bindingName.

+function nsActivityProcess()
+{
+  try {
+    nsActivity.call(this);
+    this._bindingName = "activity-process";
+    this._groupingStyle = Ci.nsIActivity.GROUPING_STYLE_BYCONTEXT;
+  } catch (e) {
+    dump("Exception: " + e + "\n");
+    throw(e);
+  }

Again, try/catch shouldn't be necessary here.

+  //// nsIActivityProcess
+  

I've noticed there's a lot of whitespace issues with this patch. Please check that it passes the whitespace issues mentioned when you pass it through http://beaufour.dk/jst-review/

+  _percentComplete: -1,
+  _lastStatusText: "",
+  _workUnitComplete: 0,
+  _totalWorkUnits: 0,
+  _startTime: Date.now(),
+  _cancelHandler: null,
+  _pauseHandler: null,
+  _retryHandler: null,
+  _state: Ci.nsIActivityProcess.STATE_INPROGRESS,
+  

Again, I think that attributes on nsIActivityProcess can just be accessed directly rather than with explicit get/set functions (this only applies where get/set just get/set the value, i.e. the state attribute needs to have its own functions because of the extra processes).

Same comment applies to nsActivityEvent and nsActivityWarning.

+  set state(val) {
+    try {
+      if (val == this._state)
+        return;
+      
+      // test validity of the new state
+      //
+      if (this._state == Ci.nsIActivityProcess.STATE_INPROGRESS &&
+          !(val == Ci.nsIActivityProcess.STATE_COMPLETED ||
+            val == Ci.nsIActivityProcess.STATE_CANCELED ||
+            val == Ci.nsIActivityProcess.STATE_WAITINGFORRETRY ||
+            val == Ci.nsIActivityProcess.STATE_WAITINGFORINPUT ||
+            val == Ci.nsIActivityProcess.STATE_PAUSED)) {
+        throw Cr.NS_ERROR_ILLEGAL_VALUE;
+      }

The nsIActivityProcess documentation for state, should mention that when setting the state, the expection may be thrown (use @exception NS_ERROR_ILLEGAL_VALUE <description as to why>).

+      this.log.debug("Number of listeners for onStateChanged: " +
+               this._listeners.length);
+      
+      // let the listeners know about the change
+      for each (let [, value] in Iterator(this._listeners)) {
+        this.log.debug("Notifying onStateChanged listeners");

I don't think we need to know how many listeners are being notified. Also, we only need to know once that we're notifying our listeners.

+        try {
+          value.onStateChanged(this, oldState);
+        }
+        catch(e) {
+          this.log.error("Exception thrown by onStateChanged listener: "+ e);
+        }
+      }
+    } catch (e) {
+      this.log.error("Other Exception thrown in state setter: "+ e);
+    }

That's not right, we nicely check to see if we're allowed to set the state and throw an exception, but don't let the caller know about it? That's not going to be easy to understand for development. I suggest we just drop the outer try/catch.

+  set cancelHandler(val) {
+    this._cancelHandler = val;
+    
+    // let the listeners know about the change
+    for each (let [, value] in Iterator(this._listeners)) {
+      this.log.debug("Notifying onHandlerChanged listeners");

Again, if we really want to long this, we only need to do it once. Please apply to the other *Handler functions in this file as well.

+  init: function(aDisplayText, aInitiator, aStatusText, aStartTime, aCompletionTime) {
+    try {
+      this._displayText = aDisplayText;
+      this._initiator = aInitiator;
+      this._statusText = aStatusText;
+      this._startTime = aStartTime;
+      this._completionTime = aCompletionTime;
+    } catch (e) {
+      dump("Exception: " + e + "\n")
+      throw(e);
+    }

No try/catch again please (ditto in nsActivityWarning).

diff --git a/mail/components/activity/nsActivityManager.js b/mail/components/activity/nsActivityManager.js

+  getContextDisplayText: function(aContextType, aContextObj) {
+    let displayText = "";
+    try {
+      let bundleService = Cc["@mozilla.org/intl/stringbundle;1"]
+                            .getService(Ci.nsIStringBundleService)
+                            .createBundle("chrome://activity/locale/activity.properties");

This should be chrome://messenger/locale/activity.properties

+      try {
+        displayText = bundleService.GetStringFromName(aContextType);
+      }
+      catch(e) {
+        displayText = bundleService.GetStringFromName("unknown");
+      }
+      switch(aContextType) {
+        case 'account':
+          // when the context type is |account|,
+          // expected context object type is nsIMsgIncomingServer
+          if (aContextObj instanceof Ci.nsIMsgIncomingServer)
+            displayText = displayText + ": " + aContextObj.prettyName;

This is wrong, especially for right-to-left locales. We should be special casing account, defining it as

Account: %s

in the properties file (with appropraite localisation note), and then using formatStringFromName.

+          break;
+        case 'smtp':
+          //XXX: get pretty name to display
+          displayText = aContextObj;
+          break;
+        case 'addressbook':
+          //XXX: get pretty name to display
+          displayText = aContextObj;

Are the pretty names not from the string bundle?

+          break;
+        default:
+          displayText = aContextObj;
+          break;
+      }
+    }
+    catch(e) {
+      displayText = aContextObj;
+    }

I'm not sure what this is trying to catch, but I don't think we need a try/catch here - if items are wrong, then it needs to be picked up at the development/testing stage.

+  addActivity: function (aActivity) {
+  try {
+    this.log.info("adding Activity");
+    // get the next valid id for this activity
+    let id = this.nextId;
+    aActivity.setId(id);
+    
+    // add activity into the activities table
+    this._activities[id] = aActivity;
+    // notify all the listeners
+    for each (let [, value] in Iterator(this._listeners)) {
+      //try {
+        value.onAddedActivity(id, aActivity);
+      //}
+      //catch(e) {
+        // ignore the exception 
+      //}
+    }
+    return id;
+  } catch (e) {
+    this.log.error("Exception: " + e);
+  }

If there's an issue add/removing items, we should be propagating it back to the caller. Note, I would support uncommenting the try /catch for the listener notification but with an additional log entry there.

+  
+  addListener: function(aListener) {
+    this.log.info("addListener\n");
+    this._listeners.push(aListener);
+  },
+  
+  removeListener: function(aListener) {
+    for (i = 0; i < this._listeners.length; i++) {
+      if (this._listeners[i] == aListener)
+        this._listeners.splice(i, 1);
+    }
+  },

Need to log on removeListener otherwise you'll make me thing we're leaking memory by holding onto references.

diff --git a/mail/components/activity/nsActivityManagerUI.js b/mail/components/activity/nsActivityManagerUI.js

+  //// nsIActivityManagerUI
+
+  show: function show(aWindowContext, aID, aReason)
+  {

nit: { should be on the end of the previous line.

+    var params = Cc["@mozilla.org/array;1"].createInstance(Ci.nsIMutableArray);
+
+    // Pass in the reason as well
+    let reason = Cc["@mozilla.org/supports-PRInt16;1"].
+                 createInstance(Ci.nsISupportsPRInt16);
+    reason.data = aReason;
+    params.appendElement(reason, false);
+
+    var ww = Cc["@mozilla.org/embedcomp/window-watcher;1"].
+             getService(Ci.nsIWindowWatcher);
+    ww.openWindow(parent,
+                  ACTIVITY_MANAGER_URL,
+                  "ActivityManager",
+                  "chrome,dialog=no,resizable",
+                  params);

Are we going to be automatically displaying the activity manager UI? If not, then I don't think we need this. Currently the argument isn't used.

+  getAttention: function getAttention()
+  {
+    if (!this.visible)
+      throw Cr.NS_ERROR_UNEXPECTED;
+
+    var prefs = Cc["@mozilla.org/preferences-service;1"].
+                getService(Ci.nsIPrefBranch);
+    // This preference may not be set, so defaulting to two.
+    let flashCount = 2;
+    try {
+      flashCount = prefs.getIntPref(PREF_FLASH_COUNT);
+    } catch (e) { }
+
+    var win = this.recentWindow.QueryInterface(Ci.nsIDOMChromeWindow);
+    win.getAttentionWithCycleCount(flashCount);
+  },

Again, if we're not automatically bringing it up, I'm not sure we need this.

diff --git a/mail/components/activity/content/activity.xml b/mail/components/activity/content/activity.xml

+         <xul:richlistbox class="activitygroupbox" id="activityGroupView"
+                        seltype="multiple" flex="1" anonid="activityGroupView"/>

I just noticed this is wrong. We shouldn't be setting an id - because we'll be getting multiple ids.

diff --git a/mail/themes/pinstripe/mail/activity/activity.css b/mail/themes/pinstripe/mail/activity/activity.css

+#activityGroupView {
+  -moz-appearance: none;
+  margin: 0;
+  padding: 0;
+  border-width: 0;
+}
+
+#activityView {
+  -moz-appearance: none;
+  margin: 0;
+  padding: 0;
+  border-width: 0;
+}

I'm not sure why we're scoping these to ids. For the first one we also have the activitygroupbox class which is also setting up styles on the same thing. Kinda confusing. Would it be worth making 

Also, we could just make them into one class and apply it in two places I think.

+#search {
+  -moz-box-pack: end;
+  /*-moz-padding-end: 4px;*/

If we don't need this, lets remove it.

+/*
+activity-process:not([selected="true"]):nth-child(odd) {
+  background-color: -moz-oddtreerow;
+}
+*/
+activity-event[selected="true"] {
+}
+
+activity-group .contextDisplayText {
+}
+
+activity-process .displayText {
+}

Why do we need these if they are empty?
Attachment #359215 - Flags: review?(bugzilla) → review-
Attached patch rev 6 (obsolete) — Splinter Review
All review comments dealt with. 

In addition, as discussed on IRC, I simplified the contextDisplayText stuff to not require the separate component registration business.  Activities now just have a contextDisplayText, and they can figure out what that should be, localized, decentralized, etc.

Also as discussed on IRC, I'm fine w/ landing this w/ the menu commented out for now, and to surface it when we have full themes and some users landed.
Attachment #359215 - Attachment is obsolete: true
Attachment #359649 - Flags: review?(bugzilla)
Comment on attachment 359649 [details] [diff] [review]
rev 6

As mentioned on irc. contextDisplayText doesn't seem to be easily useable.

In activity.js Startup() and Shutdown() you want the . on the start of the line before getService, rather than the end of the previous line.

We need to drop the unknown/account/smtp/calendar/addressbook strings from activity.properties if we're not using them in this patch.

Otherwise I think we're almost there for getting this in so we can build it up as we implement users.
Attachment #359649 - Flags: review?(bugzilla) → review-
Attached patch [checked in] rev 7 (obsolete) — Splinter Review
Thanks for catching the contextDisplayText bug, that was non-trivial.

I'll attach a revised bug on 469053 that shows usage.
Attachment #359649 - Attachment is obsolete: true
Attachment #359921 - Flags: review?(bugzilla)
Comment on attachment 359921 [details] [diff] [review]
[checked in] rev 7

This looks good for a first take. I'll land it in the morning as we discussed and we can build on it from there.

Note to observers: will be landing initially with no menu option and a mac theme only. More to follow.
Attachment #359921 - Flags: review?(bugzilla) → review+
I noticed that the string files of the new activity manager as well as the Pinstripe theme file lacks the licensing header. Shouldn't that be added there as well?
Whiteboard: [needs more review standard8][needs new patch] → [needs more review standard8][needs new patch] [has l10n impact]
(In reply to comment #66)
> I noticed that the string files of the new activity manager as well as the
> Pinstripe theme file lacks the licensing header. Shouldn't that be added there
> as well?

The pinstripe theme file, from the looks of it we should be adding a header. AFAIK we don't generally add license headers onto locale files.
Attachment #359921 - Attachment description: rev 7 → [checked in] rev 7
Comment on attachment 359921 [details] [diff] [review]
[checked in] rev 7

Checked in with license header added in themes/pinstripe:

http://hg.mozilla.org/comm-central/rev/0c91c794aa55
Whiteboard: [needs more review standard8][needs new patch] [has l10n impact] → [has l10n impact][initial part landed][needs Win & Linux themes]
(In reply to comment #68)
> (From update of attachment 359921 [details] [diff] [review])
> Checked in with license header added in themes/pinstripe:
> 
> http://hg.mozilla.org/comm-central/rev/0c91c794aa55

and the images: http://hg.mozilla.org/comm-central/rev/f542811f5f33
I've also added the new files to packages-static: http://hg.mozilla.org/comm-central/rev/813b0777c3c8

FTR if anyone wants to open the window for testing/dev (bearing in mind there are currently no users, no Linux/Window themes), it can be done by entering the following in the error console:

Components.classes['@mozilla.org/activity-manager-ui;1'].getService(Components.interfaces.nsIActivityManagerUI).show(window);
Depends on: 476487
Note: created blocking bug 476487 where we can track the work on the interactive status bar specifics, as this bug is unwieldy.
A few l10n comments to activity.properties:

> paused=Paused.
Is the dot intended to be there?

> autoRetry=Trying again in %S seconds
Do we have only the plural form?

In activity.dtd:
What is meant by "Recover" in Activitymanager?
Depends on: 476695
Depends on: 476696
Depends on: 476698
Depends on: 204132
Depends on: 476700
    This bug is unwieldy, but I don't think i can close it in favor of a pure
    tracker because of the large number of people watching it.  So I'm going to
    morph it.

    Things still to do now have their own bugs, blocking this one.

    * pinstripe theme work, & qute & gnomestripe themes needed [bug 476695]
    * smarter status bar that displays informative but non-interrupting
    notifications [bug 476487]
    * poptarts for notifications that require action [bug 476696]
    * client code to report on message & folder copies, moves, deletes [bug 469052]
    * client code to report on autosync status [bug 469053]
    * client code to report on asynchronous sending [bug 476698]
    * client code to report IMAP ALERT messages [bug 204132]
    * un-comment activity manager menu item when some client code lands [bug
    476700]
Alias: activitymgr
Summary: interactive status bar and activity history manager → activity manager tracker
(In reply to comment #72)
> A few l10n comments to activity.properties:
> 
> > paused=Paused.
> Is the dot intended to be there?
> 
> > autoRetry=Trying again in %S seconds
> Do we have only the plural form?
> 
> In activity.dtd:
> What is meant by "Recover" in Activitymanager?

Maybe you have a better chance (hopefully) to get an answer to your question if you add the information that you are the german Thunderbird localizer and you need this information to make a good german localization...
(In reply to comment #72)
> A few l10n comments to activity.properties:
> 
> > paused=Paused.
> Is the dot intended to be there?

Nope, I'm changing the string in the normal l10n way.

> > autoRetry=Trying again in %S seconds
> Do we have only the plural form?

This string isn't actually used at the moment, I'm going to propose we drop it until we're ready for it (at which point we'll do the plural form).

> In activity.dtd:
> What is meant by "Recover" in Activitymanager?

I think http://mxr.mozilla.org/comm-central/source/mail/components/activity/nsIActivity.idl#65 gives a pretty good explanation here.
Here's a few additional fixes:

- Drop the . from paused.
- Drop the seconds/hours/minutes/days strings. These aren't used at the moment, and if they are we need to make them more flexible in plural terms (e.g. see bug 476836).
- Drop cannotPause - I can't see a use for this in the short future, if we can't pause an activity then we just don't implement the nsIActivityPauseHandler interface. If we can pause it sometimes, then maybe we will need this, but I think we should just drop it till we need it.
- Drop autoRetry - we don't use this at the moment. The plural forms probably aren't strictly right, so again, lets drop it for now.
- Documentation in nsIActivityManager is wrong - we don't just have mac themes, and we changed how the contextType code worked before the last version of the patch.
Attachment #360707 - Flags: review?(philringnalda)
Blocks: 476836
Attachment #360707 - Flags: review?(philringnalda) → review+
Comment on attachment 360707 [details] [diff] [review]
[checked in] L10n and documentation fixes

>+   * icons are declared in |mail/themes/<themename>/mail/activity/activity.css|
>+   * file

"+   * files.", no? Claiming that's a singular file would mean you would need a "the" and then you'd be over 80 chars, and heading down a nightmarish path.
Comment on attachment 360707 [details] [diff] [review]
[checked in] L10n and documentation fixes

Checked in with comment fixed: http://hg.mozilla.org/comm-central/rev/fbbe5c1cc559
Attachment #360707 - Attachment description: L10n and documentation fixes → [checked in] L10n and documentation fixes
What is the ETA for this bug? 

Tomorrow at 23:59 PST is our string freeze and it would be great if this blocking bug could be fixed by that time as mentioned in https://wiki.mozilla.org/Thunderbird/Driver_Meetings/2009-02-03
Depends on: 477862
moving some bugs from depends on to blocked by.
Blocks: 204132, 476698
No longer depends on: 204132, 476698
Comment on attachment 359921 [details] [diff] [review]
[checked in] rev 7

I know that this was checked in already, but I found some issues with this patch that should be corrected:

>--- /dev/null
>+++ b/mail/components/activity/Makefile.in
>
>+# The Original Code is mozilla.org code.
>+#
>+# The Initial Developer of the Original Code is
>+# Mozilla Foundation.
>+# Portions created by the Initial Developer are Copyright (C) 2009
>+# the Initial Developer. All Rights Reserved.
>+#
>+# Contributor(s):
>+#  Emre Birol <emrebirol@gmail.com>
>+#  David Ascher <dascher@mozillamessaging.com>

This is wrong here and in the other new files:

- The original Code is not mozilla.org code, but 
  Mozilla Messaging code
- The initial developer is not the Mozilla Foundation 
  (or Netscape Communications as used elsewhere) but 
  the responsible developer (either Emre or David)
- Only the second developer should be listed as a 
  contributor here

>+++ b/mail/components/activity/content/activity.js
>
>+ * The Original Code is Mozilla.org Code.
>+ *
>+ * The Initial Developer of the Original Code is
>+ * Netscape Communications Corporation.
>+ * Portions created by the Initial Developer are Copyright (C) 2001
>+ * the Initial Developer. All Rights Reserved.
>+ *
>+ * Contributor(s):
>+ *   Blake Ross <blakeross@telocity.com> (Original Author of download mgr)
>+ *   Ben Goodger <ben@bengoodger.com> (v2.0)
>+ *   Dan Mosedale <dmose@mozilla.org>
>+ *   Fredrik Holmqvist <thesuckiestemail@yahoo.se>
>+ *   Josh Aas <josh@mozilla.com>
>+ *   Shawn Wilsher <me@shawnwilsher.com> (v3.0)
>+ *   Edward Lee <edward.lee@engineering.uiuc.edu>
>+ *   David Ascher <dascher@mozillamessaging.com> (activity manager version)
>+ *   Emre Birol <emrebirol@gmail.com>

I'm no code attribution expert, but this looks wrong to me. The code doesn't look like you were copy-pasting stuff from A to B. Therefore I think you should not copy the whole attribution header here.

>+++ b/mail/components/activity/content/activity.xml

Same here.

>+++ b/mail/components/activity/content/activity.xul

Same here.

>+++ b/mail/components/activity/content/activityBinding.css
>
>+ * The Original Code is mozilla.org code.
>+ *
>+ * The Initial Developer of the Original Code is Mozilla Messaging, Inc.
>+ * Portions created by the Initial Developer are Copyright (C) 2008
>+ * the Initial Developer. All Rights Reserved.
>+ *
>+ * Contributor(s):
>+ *  Emre Birol  <ebirol@gmail.com> (Original Author)
>+ *  David Ascher <dascher@mozillamessaging.com>

See above. The original code comes from MoMo. Either Emre or David is the initial dev and the other is the contributor.

>+++ b/mail/components/activity/nsActivity.js

See above.

>+++ b/mail/components/activity/nsActivityManager.js

See above.

>+++ b/mail/components/activity/nsActivityManagerUI.js

See above.

>+++ b/mail/components/activity/nsIActivity.idl
>
>+ * The Original Code is mozilla.org code.
>+ *
>+ * The Initial Developer of the Original Code is
>+ * Mozilla Messaging
>+ * Portions created by the Initial Developer are Copyright (C) 2008
>+ * the Initial Developer. All Rights Reserved.
>+ *
>+ * Contributor(s):
>+ *   David Ascher <dascher@mozillamessaging.com>
>+ *   Emre Birol <emrebirol@gmail.com>

See above.

>+++ b/mail/components/activity/nsIActivityManager.idl

See above.

>+++ b/mail/components/activity/nsIActivityManagerUI.idl
>
>+ * The Original Code is mozilla.org code.

Mozilla Messaging

>+ * The Initial Developer of the Original Code is
>+ * Shawn Wilsher <me@shawnwilsher.com>.
>+ * David Ascher <dascher@mozillamessaging.com>

There can only be one initial developer. The other is a contributor.

>+++ b/mail/themes/pinstripe/mail/activity/activity.css

This file contains a number of rules that regress my patch from bug 474807.

>+#search {
>+  -moz-box-pack: end;
>+  padding-top: 4px;
>+  padding-right: 4px;

Please use "-moz-padding-end: 4px;"

>+activity-process button {
>+  -moz-appearance: none;
>+  min-height: 16px;
>+  min-width: 16px;
>+  max-height: 16px;
>+  max-width: 16px;
>+  padding: 0;
>+  margin: 0 1px 0 1px;

Please use "margin: 0 1px;" instead to make it more apparent that the left- and right-hand-margin is the same.

>+activity-event button {
>+  -moz-appearance: none;
>+  min-height: 16px;
>+  min-width: 16px;
>+  max-height: 16px;
>+  max-width: 16px;
>+  padding: 0;
>+  margin: 0 1px 0 1px;

Please use "margin: 0 1px;" instead to make it more apparent that the left- and right-hand-margin is the same.

>+activity-warning button {
>+  -moz-appearance: none;
>+  min-height: 16px;
>+  min-width: 16px;
>+  max-height: 16px;
>+  max-width: 16px;
>+  padding: 0;
>+  margin: 0 1px 0 1px;

Please use "margin: 0 1px;" instead to make it more apparent that the left- and right-hand-margin is the same.

>+.activitygroupbox activity-event,
>+.activitygroupbox activity-warning,
>+.activitygroupbox activity-process {
>+  padding-left: 2em;

Please use "-moz-padding-start: 2em;"

>+activity-group button {
>+  -moz-appearance: none;
>+  min-height: 16px;
>+  min-width: 16px;
>+  max-height: 16px;
>+  max-width: 16px;
>+  padding: 0;
>+  margin: 0 1px 0 1px;

Please use "margin: 0 1px;" instead to make it more apparent that the left- and right-hand-margin is the same.

BTW given that all those four buttons (process, event, warning, group) have exactly the same styling, why didn't you just do this?

|activity-process button,
|activity-event button,
|activity-warning button,
|activity-group button {
|  -moz-appearance: none;
|  min-height: 16px;
|  min-width: 16px;
|  max-height: 16px;
|  max-width: 16px;
|  padding: 0;
|  margin: 0 1px;
|}
Re all the comments about license headers - in all cases (maybe mail/components/activity/Makefile.in could be the only one exception) this code was initially copied from the toolkit download manager. Therefore they should retain their current headers because they haven't been written from scratch but copied and reworked from other code.
If I have some entries in the activity manager window (e.g. after moving mails from one folder to another), this entries are lost If I restart Thunderbird. The activity manager window is always empty after restarting Thunderbird. Is this a wanted behaviour?
Whiteboard: [has l10n impact][initial part landed][needs Win & Linux themes] → [no l10n impact][initial part landed][needs Win & Linux themes]
(In reply to comment #82)
> Re all the comments about license headers - in all cases (maybe
> mail/components/activity/Makefile.in could be the only one exception) this 
> code was initially copied from the toolkit download manager. Therefore they 
> should retain their current headers because they haven't been written from 
> scratch but copied and reworked from other code.

As I said, I'm not an attribution expert, and I don't partcularly care either way. I just found it strange reading attributions to Netscape in a 2009 patch.

What about the issues I raised regarding the css stuff in activity.css? 
Davida, since you're the patch author, could you please respond either here 
in the bug or by committing an updated patch that fixes the issues I raised 
in comment 81.
Sure, I'm happy to do a revised patch w/ better margins for RTL.
Phil, if you can get this reviewed before freeze, that'd be good, as it's a blocker for enabling the activity manager UI.
Attachment #359921 - Attachment is obsolete: true
Attachment #360707 - Attachment is obsolete: true
Attachment #362777 - Flags: ui-review?(clarkbw)
Attachment #362777 - Flags: review?(philringnalda)
Windows skin. Again, mirrors download manager for now.
Attachment #362792 - Flags: ui-review?(clarkbw)
Attachment #362792 - Flags: review?(bienvenu)
This brings the mac theme in line w/ the others, w.r.t. using stripes to differentiate events, which can blend together otherwise.  (a feature from the download manager theme which had been removed, mistakenly as it turns out)
Attachment #362793 - Flags: ui-review?(clarkbw)
Attachment #362793 - Flags: review?(clarkbw)
Lunchtime quick-scan of gnomestripe:

whitespace is off in jar.mn

that license header is bogus - you don't appear to have the "it's bogus where I stole it" excuse since downloads.css doesn't appear to have one, but copyright Netscape 2008 is utterly wrong even if you did copy it from somewhere.

several instances of margin: with four values, which is an rtl red flag: you can only use margin if the left and right values are the same, and if they are the same you should be using either three values, or in these cases two.

one instance of padding-left when I expect you mean padding-start
Comment on attachment 362777 [details] [diff] [review]
Gnomestripe skin for activity manager.  Inspired by download manager gnomestripe.

saw this work in progress so a quick review!

A couple places had margin: 0 1px 0 1px; (TRBL) which could be better collapsed into margin: 0px 1px;

Saw a padding-left entry in there and thought you might want that replaced with -moz-padding-start instead, not sure as I didn't test it.
Attachment #362777 - Flags: ui-review?(clarkbw) → ui-review+
Comment on attachment 362793 [details] [diff] [review]
pinstripe patch to use odd/even stripes for events, as for the other themes

looks good
Attachment #362793 - Flags: ui-review?(clarkbw)
Attachment #362793 - Flags: ui-review+
Attachment #362793 - Flags: review?(clarkbw)
Attachment #362793 - Flags: review+
Attachment #362792 - Flags: ui-review?(clarkbw) → ui-review+
Comment on attachment 362792 [details] [diff] [review]
Qute skin for activity manager (windows)

saw this in progress, looks good.
Attached patch denitted gnomestrip patch (obsolete) — Splinter Review
carrying forward ui-r.  nits addressed.  I'll file new versions for pinstripe & qute as well w/ similar issues fixed there.
Attachment #362777 - Attachment is obsolete: true
Attachment #362796 - Flags: ui-review+
Attachment #362796 - Flags: review?(philringnalda)
Attachment #362777 - Flags: review?(philringnalda)
Attached patch denitted pinstripe patch (obsolete) — Splinter Review
better license header, and RTL-safe css version of previous patch.
carrying forward ui-r
Attachment #362793 - Attachment is obsolete: true
Attachment #362797 - Flags: ui-review+
Attachment #362797 - Flags: review?(philringnalda)
Comment on attachment 362792 [details] [diff] [review]
Qute skin for activity manager (windows)

looks better...
Attachment #362792 - Flags: review?(bienvenu) → review+
Attached patch better license header (obsolete) — Splinter Review
even better license header as i understand things.
Attachment #362796 - Attachment is obsolete: true
Attachment #362798 - Flags: ui-review+
Attachment #362798 - Flags: review?(philringnalda)
Attachment #362796 - Flags: review?(philringnalda)
here ya go, everything should be copacetic for lawyers and RTL alike.
Attachment #362792 - Attachment is obsolete: true
Attachment #362799 - Flags: ui-review+
Attachment #362799 - Flags: review?
Attachment #362799 - Flags: review? → review?(philringnalda)
Whiteboard: [no l10n impact][initial part landed][needs Win & Linux themes] → [no l10n impact][needs theme reviews philor]
Nope, you were misled and that's actually not better - note the lack of any "The original code is Mozilla Corporation code" in the tree. As http://www.mozilla.org/MPL/boilerplate-1.1/ says,

[[[
The "The Original Code is" line should be filled in with a short description of the code's function. If uninspired, use "mozilla.org code.".
]]]

so either "Thunderbird Activity Manager CSS" or "mozilla.org code" is correct, but "MoMo code" is not: it's supposed to be either a description or a cop-out, not an assertion of ownership.

You'll need to talk to your employer about copyright on your work-for-hire, but I sort of suspect that since MoCo employees don't retain copyright on their work, but instead say that MoCo is the initial developer, that you had it right in the first place with MoMo as the intial developer, the hint from the boilerplate being:

[[[
The "Initial Developer" is the copyright holder, which may be your employer rather than you. In this case, add yourself as a Contributor.
]]]
ok, thanks.  I guess I'll do yet another revision of the three patches.
license header fix, carrying ui-r.
Attachment #362797 - Attachment is obsolete: true
Attachment #362808 - Flags: ui-review+
Attachment #362808 - Flags: review?(philringnalda)
Attachment #362797 - Flags: review?(philringnalda)
license header fix since last rev, carrying ui-r.
Attachment #362798 - Attachment is obsolete: true
Attachment #362810 - Flags: ui-review+
Attachment #362810 - Flags: review?(philringnalda)
Attachment #362798 - Flags: review?(philringnalda)
qute patch.
Attachment #362799 - Attachment is obsolete: true
Attachment #362812 - Flags: ui-review+
Attachment #362812 - Flags: review?(philringnalda)
Attachment #362799 - Flags: review?(philringnalda)
Comment on attachment 362808 [details] [diff] [review]
pinstripe patch. [checked in]

r=me and http://hg.mozilla.org/comm-central/rev/c2366d3e92c7

(with the nit that the license-grovelling script needs things just so, I believe including having no indent for the initial developer and no blank line after him/it, but I decided it wouldn't be *that* funny to say you had to attach yet another round with that change)
Attachment #362808 - Attachment description: pinstripe patch. → pinstripe patch. [checked in]
Attachment #362808 - Flags: review?(philringnalda) → review+
Comment on attachment 362812 [details] [diff] [review]
qute theme fix w/ even more license header fixes [checked in]

http://hg.mozilla.org/comm-central/rev/9f3825e2dec5
Attachment #362812 - Attachment description: qute theme fix w/ even more license header fixes → qute theme fix w/ even more license header fixes [checked in]
Attachment #362812 - Flags: review?(philringnalda) → review+
Attachment #362810 - Attachment description: gnomestripe patch → gnomestripe patch [checked in]
Attachment #362810 - Flags: review?(philringnalda) → review+
Whiteboard: [no l10n impact][needs theme reviews philor] → [no l10n impact]
We're done with what was targeted for b2.  Moving to b3, until I can triage the dependent bugs and figure out which ones will land by b3.  [my initial focus will be on poptarts & super status bar, as well as what we learn from seeing the activity manager in the wild].
Target Milestone: Thunderbird 3.0b2 → Thunderbird 3.0b3
Depends on: 479857
No longer blocks: 123440
Depends on: 479489
I triaged all the dependencies and made sure that the blocker list is correct.  Removing blocker from this list as trackers shouldn't be blockers.
Flags: blocking-thunderbird3+ → blocking-thunderbird3-
Blocks: 488711
Blocks: 366431
I believe bugs should be opened for both User Documentation and Litmus.  To date I do not know what the new item in the Tools Menu is supposed to show me, it's always blank when I open the display window.
(In reply to comment #110)
> I believe bugs should be opened for both User Documentation and Litmus.  To
> date I do not know what the new item in the Tools Menu is supposed to show me,
> it's always blank when I open the display window.

killjay, is this still the case for you?
(In reply to comment #111)
> (In reply to comment #110)
>> I do not know what the new item in the Tools Menu is supposed to show me,
>> it's always blank when I open the display window.
> 
> killjay, is this still the case for you?

Yes.  When it was initially proposed I thought it would catch News activity.  That does not seem to be the case. It's unclear to me what is should be doing.

What has come to mind is the AM have a link to a MoMo web page (Like the AOM links to AMO) so Users can learn what the feature is about. This is proposed since We have no real built in Help.
Depends on: 480268
Depends on: 512995
Depends on: 512992
Depends on: 515916
Depends on: 516356
Depends on: 516523
Depends on: 517451
Assignee: david.ascher → nobody
Depends on: 519747
Depends on: 519750
Depends on: 524314
Depends on: 527228
Depends on: 528608
We've ported the developer docs from the wiki to MDC: Activity Manager overview (http://developer.mozilla.org/en/Thunderbird/Activity_Manager) Activity Manager interfaces (http://developer.mozilla.org/en/Thunderbird) and Activity Manager examples (http://developer.mozilla.org/en/Extensions/Thunderbird/HowTos/Activity_Manager). I've opened another bug (bug # 529656) and changed the "dev-doc-needed" keyword to "dev-doc-complete".
Depends on: 533641
Depends on: 545499
should depend on bug 545041
should depend on bug 540091
Depends on: 545041
my 2-cents - TB3 has an "Activity Manager" that I am assuming has nothing to do with this "bug" - am I right?

I may well be wrong but then consider how the current "Activity Manager" has virtually nothing to do with what thus "bug" set out from the beginning.
"Activity Manager" in TB3 can, IMO, at best be considered a premature BETA release preview of "things to come".

If it indeed is a BETA-thingy, please, some kind soul point me to the correct place where to offer beta-tester feedback on bugs, errors, and just "reporting  missing useful stuff that would make it worth anything" for the good of the final result.

;-) - really, all said with a smile...

cheers...
Vitus
Status: ASSIGNED → NEW
Depends on: 560552
Depends on: 564999
Depends on: 571030
Depends on: 540091
Depends on: 579166
Depends on: 579168
Depends on: 479484
Keywords: meta
Depends on: 614759
Depends on: 659753
Depends on: 668719
No longer blocks: 472483
Depends on: 788446
Depends on: 510254, 545477
Depends on: 909089
Summary: activity manager tracker → activity manager tracker [meta]
Depends on: 807909
Depends on: 493397
Depends on: 718384
No longer blocks: 469050
Depends on: 469050
Depends on: 484407
Depends on: 1116757
Depends on: 1211261
Blocks: 1562508
Depends on: 1569218
Priority: P2 → --
Target Milestone: Thunderbird 3.0b3 → ---
Depends on: 1695465
Depends on: 1737391
Severity: normal → S3
Depends on: 1842021
You need to log in before you can comment on or make changes to this bug.