Closed Bug 954054 Opened 10 years ago Closed 10 years ago

Basic Jump List support on Windows 7

Categories

(Instantbird Graveyard :: Other, enhancement)

x86
Windows 7
enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: benediktp, Assigned: clokep)

References

Details

(Whiteboard: [0.3-wanted-beta])

Attachments

(5 files, 9 obsolete files)

*** Original post on bio 618 at 2010-12-11 23:20:00 UTC ***

Windows 7 has a feature called jump-lists that are a sort of transformed context menu on taskbar items and should include for example shortcuts to often used actions.
I wonder if we should include a list of open conversations as well or at least a list of the those having unread messages.
Blocks: 954055
Whiteboard: [0.3-nice-to-have]
*** Original post on bio 618 as attmnt 536 at 2011-02-24 15:14:00 UTC ***

There is a Pidgin plug-in that handles this well I think: http://code.google.com/p/pidgin-win7/

It shows common tasks (New IM / Join Chat), set status as well as links to settings (unnecessary I think).
*** Original post on bio 618 by Paul [sabret00the] <sabret00the AT yahoo.co.uk> at 2011-02-24 17:20:07 UTC ***

It's most certainly interesting and I largely agree with the implementation. But I'd say lose the status, as we'd have that as a right click option on the system tray icon and instead do as per Firefox have done by having Frequent.
*** Original post on bio 618 at 2011-02-24 23:20:44 UTC ***

(In reply to comment #2)
> It's most certainly interesting and I largely agree with the implementation.
> But I'd say lose the status, as we'd have that as a right click option on the
> system tray icon and instead do as per Firefox have done by having Frequent.

Is the system tray icon helpful in any way on Win7 if we already have always-visible information available from the dock icon?
*** Original post on bio 618 by Paul [sabret00the] <sabret00the AT yahoo.co.uk> at 2011-02-24 23:28:40 UTC ***

(In reply to comment #3)
> (In reply to comment #2)
> > It's most certainly interesting and I largely agree with the implementation.
> > But I'd say lose the status, as we'd have that as a right click option on the
> > system tray icon and instead do as per Firefox have done by having Frequent.
> 
> Is the system tray icon helpful in any way on Win7 if we already have
> always-visible information available from the dock icon?

Very much so, but due to the lack of features implemented thus far, we're not able to take full advantage of it's uses.
*** Original post on bio 618 at 2011-03-17 18:58:05 UTC ***

(In reply to comment #3)
> (In reply to comment #2)
> > It's most certainly interesting and I largely agree with the implementation.
> > But I'd say lose the status, as we'd have that as a right click option on the
> > system tray icon and instead do as per Firefox have done by having Frequent.
> Is the system tray icon helpful in any way on Win7 if we already have
> always-visible information available from the dock icon?

Use of the system tray icon on Windows 7 is to be avoided according to Microsoft's directions (although they still use it in Outlook, Office Communicator, etc. :-P)

Personally I tend to think it's unnecessary for Instantbird and the functions can be placed in the jump lists.

I disagree w/ Paul that the status changing is unnecessary since it'll be w/ the system tray icon...which I fully plan on not using. Unfortunately I'm not sure what else to put in there except that and options dialogs, but is that really "jump list worth"? (I think we can all agree that "Join chat" needs to be there?)

I'd like to put frequent contacts there, but we really have no way to get that without a better logger.
*** Original post on bio 618 by Paul [sabret00the] <sabret00the AT yahoo.co.uk> at 2011-03-17 19:20:53 UTC ***

(In reply to comment #5)
I think we have to look at it in a way where every other IM makes use of the system tray. No matter what, ease of transition is a key factor for the release of Instantbird.

On top of that, not everyone requires the application open on the taskbar constantly and as such they should be able to maintain the program without it cluttering the taskbar. 

I personally value using the system tray icon even when I have the application open on the taskbar in order to see at-a-glance if it's a new notification or a new message that I'm being notified of. At any given time the priority of those two interchanges and thus I can ignore one while checking the other.

I think it's highly important for the versatility of the alerts tab/panel/popup that users can at-a-glance tell if they're being notified of messages, emails, requests, etc no matter what. And I know we can produce with taskbar integration. However do the rest of our platforms enable such? XP still have a larger market-share than Windows 7.

Back on topic, status would most certainly suit some users there more than others, so as long as make sure users have some control over the content. We could have a pref for it somewhere.
*** Original post on bio 618 at 2011-03-17 22:59:31 UTC ***

(In reply to comment #6)
> (In reply to comment #5)
> I think we have to look at it in a way where every other IM makes use of the
> system tray. No matter what, ease of transition is a key factor for the release
> of Instantbird.
> 
> On top of that, not everyone requires the application open on the taskbar
> constantly and as such they should be able to maintain the program without it
> cluttering the taskbar. 
Sorry, I didn't mean to imply that we should not have a system tray icon. I meant that having a system tray icon is not a valid reason for not having some of the actions in the jump list.

> I personally value using the system tray icon even when I have the application
> open on the taskbar in order to see at-a-glance if it's a new notification or a
> new message that I'm being notified of. At any given time the priority of those
> two interchanges and thus I can ignore one while checking the other.
I think that we should be able to show the status in the icon in the taskbar as well, but I'm not positive about that. (And it would be a different issue than this anyway.)

> I think it's highly important for the versatility of the alerts tab/panel/popup
> that users can at-a-glance tell if they're being notified of messages, emails,
> requests, etc no matter what. And I know we can produce with taskbar
> integration.
That has nothing to do with this bug, this is about jump lists only.

> However do the rest of our platforms enable such? XP still have a
> larger market-share than Windows 7.
Perhaps, but the gap is closing very quickly. I'm not arguing against a status tray, but this bug is NOT about that, so let's not debate that on here.

> Back on topic, status would most certainly suit some users there more than
> others, so as long as make sure users have some control over the content. We
> could have a pref for it somewhere.
Perhaps, it should be easy enough to do. I don't see why you're so against it though, I think this would be a relatively common thing to do.

Anyway, looking over the documentation this should be easy to make the list...but I'm not sure how easy it is to integrate that list with our app, it seems it only supports "links" and "shortcuts", so we may have to implement some command line handlers as well.
*** Original post on bio 618 by Paul [sabret00the] <sabret00the AT yahoo.co.uk> at 2011-03-18 00:50:27 UTC ***

We need to define what we want really. I would suggest:

Tasks
- New Conversation
- View Alerts

Frequent 
- Frequent 1
- Frequent 2
- Frequent 3

Status
- Available
- Away
- Invisible

That seems to be enough for me. Alerts can come after depends on whether that even gets it's own tab, I know flo is still heavily in favour of an area on the buddy list. Frequent would show the people you most frequently convert with and enable you to quick open a chat and then status is self-explanatory.
Attached image Skype Jump List
*** Original post on bio 618 as attmnt 560 at 2011-03-18 01:31:00 UTC ***

I figured the Skype jump list might be applicable for ideas too, although it's pretty much just status information.
*** Original post on bio 618 at 2011-03-18 13:44:07 UTC ***

A few clarifications:

 - we all agree that we need to have a systray icon for at least Windows and GTK. This is however not relevant at all for this bug (but is relevant in bug 953598 (bio 151)... which unfortunately already contains several irrelevant comments :().

 - about jump lists, there are 2 different things here:

1. What we can technically do now for Instantbird 0.3. I think we should focus strictly on this part in this bug.

2. What we could do in an ideal world with Win7's jump list feature. This can lead to interesting discussions, but it's better to keep that separate as it's not something we are likely to finish for 0.3 (I believe for example that it would be interesting to show the most contacted contacts and the most often used status (including the status messages used with the status type) but that those are not things that we can currently get easily. We need a better log back-end before, and it's unlikely to happen before 0.4). If anybody believes now is the right time to start discussing this, that person is welcomed to file a bug and mark it dependent on this one.
Attached file Sample extension (obsolete) —
*** Original post on bio 618 as attmnt 567 at 2011-03-20 14:56:00 UTC ***

I've started work on this and was able the two command line parameters I know of to work as "Tasks" (-jsconsole and -preferences were the only two I know of).

Flo: Do you have any ideas of the best way to go about adding more? We could add a status command line parameter that takes the new status. Also a join chat one would be necessary.

I'm ignoring the "Frequent"/"Recent"/"Favorite" concept of buddies right now since we don't even have any way of getting that information. I only wish to handle "Tasks" in this bug.
Assignee: nobody → clokep
Status: NEW → ASSIGNED
Attached file Sample extension v1.1 (obsolete) —
*** Original post on bio 618 as attmnt 568 at 2011-03-20 21:43:00 UTC ***

I messed up the packaging of my extension the first time (sorry about that), sabret00the let me know on IRC, but I just got around to check what was wrong.  This one should work.
Comment on attachment 8352309 [details]
Sample extension

*** Original change on bio 618 attmnt 567 at 2011-03-20 21:43:03 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352309 - Attachment is obsolete: true
Depends on: 954170
Blocks: 954171
*** Original post on bio 618 at 2011-03-22 10:26:53 UTC ***

Changing summary to match the goal of immediately possible changes

(In reply to comment #10) 
> 1. What we can technically do now for Instantbird 0.3. I think we should focus
> strictly on this part in this bug.

Extended jump list support is covered by bug 954171 (bio 737) now.
No longer blocks: 954171
Summary: Use Jump-Lists on Windows 7 → Basic Jump List support on Windows 7
Blocks: 954171
No longer depends on: 954170
*** Original post on bio 618 at 2011-03-22 10:38:42 UTC ***

(In reply to comment #12)
> Created an attachment (id=568) [details]
> Sample extension v1.1
> 
> I messed up the packaging of my extension the first time (sorry about that),
> sabret00the let me know on IRC, but I just got around to check what was wrong. 
> This one should work.

I'm seeing a problem with -P and -no-remote which fails to run the command as intended (asks for the profile to use).
The same problem with Firefox is reported at https://bugzilla.mozilla.org/show_bug.cgi?id=526697 .
Depends on: 954184
No longer depends on: 954184
*** Original post on bio 618 as attmnt 623 at 2011-05-18 08:16:00 UTC ***

I added a status command line handler and changed something on the jump list code.

Known issues:
- it doesn't show the Contacts list when starting with one of those flags but you can set the current status when Instantbird is running already by chosing an item from the list.
- custom icons would be nice but it seems they have to be icons built into the application (since it is only possible to chose an iconIndex and not an image directly). If this is included one day, we should add appropriate icons.
- it doesn't clean/remove the list (call |deleteActiveList()| on the |nsIJumpListBuilder|) when being disabled or uninstalled but I guess this can easily be done by adding a |bootstrap.js| file which would register our components manually + implement the |shutdown()|/|uninstall()| methods as required.
Attached image Nice icons
*** Original post on bio 618 as attmnt 627 at 2011-05-19 22:42:00 UTC ***

I'm working on this as a patch now, still needs a bit of work. Here's a preview though of how the UI looks (which is done).
Attached patch v1.0 (obsolete) — Splinter Review
*** Original post on bio 618 as attmnt 628 at 2011-05-20 00:04:00 UTC ***

This is a working version, it uses a component (unlike what was discussed on IRC today). I'm going too see if I can whip up a JS module version. I have no idea what this will do on Linux or Mac (if it'll break anything, etc.)

flo and Mic let me know what you think of this. It'll probably need another revision, but it's close.  The three binary images are simple ico versions of the png images with the same name.
Attachment #8352371 - Flags: review?(benediktp)
Attached patch JSM v1.0 (obsolete) — Splinter Review
*** Original post on bio 618 as attmnt 629 at 2011-05-20 00:27:00 UTC ***

This is a JSM version (was a lot easier to convert than I thought!)

Also, both this version and the previous version fix the three issues that Mic mentioned (it uses the custom icons, it shows the buddy list if started with one of the flags, and it clears the jump list before recreating it every time (which is a necessary step for it to change btw)).
Attachment #8352372 - Flags: review?
Comment on attachment 8352371 [details] [diff] [review]
v1.0

*** Original change on bio 618 attmnt 628 at 2011-05-20 10:45:55 UTC ***

We're going w/ the JSM version, which Mic is adding a few things to currently.
Attachment #8352371 - Attachment is obsolete: true
Attachment #8352371 - Flags: review?(benediktp)
Comment on attachment 8352310 [details]
Sample extension v1.1

*** Original change on bio 618 attmnt 568 at 2011-05-20 10:47:58 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352310 - Attachment is obsolete: true
*** Original post on bio 618 at 2011-05-21 13:52:19 UTC ***

> diff --git a/instantbird/components/ibCommandLineHandler.manifest b/instantbird/components/ibCommandLineHandler.manifest
> +# Status command line handler (for Jump Lists)

The command line handler itself is independent from the jumplist, 
  should be possible to use it without jumplists.

> diff --git a/instantbird/components/ibStatusCommandLineHandler.js b/instantbird/components/ibStatusCommandLineHandler.js
> +function StatusCLH() { }
> +StatusCLH.prototype = {
> +  classDescription: "Instantbird Status Commandline Handler Component",
> +  classID:          Components.ID("{9da72063-b727-488d-9b3f-cc12e854ab33}"),
> +  contractID:       "@instantbird.org/status/clh;1",
> +  QueryInterface:   XPCOMUtils.generateQI([Ci.nsICommandLineHandler]),
> +
> +  /** nsICommandLineHandler **/
> +  handle: function (cmdLine) {
> +    let statusParam = cmdLine.handleFlagWithParam("status", false);
> +    if (statusParam) {
> +      statusParam = statusParam.toLowerCase();
> +
> +      const imIStatusInfo = Components.interfaces.imIStatusInfo;
> +
> +      let requestedStatus;
> +      if (statusParam == "available")
> +        requestedStatus = imIStatusInfo.STATUS_AVAILABLE;
> +      else if (statusParam == "away")
> +        requestedStatus = imIStatusInfo.STATUS_AWAY;
> +      else if (statusParam == "offline")
> +        requestedStatus = imIStatusInfo.STATUS_OFFLINE;
> +      else
> +        throw "Invalid status parameter: " + statusParam;

I know it's my code but anyways: |handleFlagWithParam| removes the flag and its 
 parameter, even in the case that we don't know what to do with it (ie throwing).
 |findFlag| and other methods of command line handlers would allow to only 
 remove the flag and the parameter if we really handled it ourselves. Another command 
 line handler still might know what to do with it (extensibility of the status-flag).
 See: https://developer.mozilla.org/en/nsICommandLine

> +      // We're unsetting the status message here at the moment.
> +      Services.core.setStatus(requestedStatus, "");

Should we keep the old message?

> +      cmdLine.preventDefault = false;

I'm not exactly sure what this does.

> diff --git a/instantbird/modules/ibCore.jsm b/instantbird/modules/ibCore.jsm
> +Cu.import("resource:///modules/ibWinJumpList.jsm");

Only include it if we're running on Windows.
Whiteboard: [0.3-nice-to-have] → [0.3-wanted-beta]
Attached patch v2.0 (obsolete) — Splinter Review
*** Original post on bio 618 as attmnt 648 at 2011-05-24 02:21:00 UTC ***

Fixes Mic's review comments. I'm not sure exactly where this code is. It works for the Jump List status messages, but I'm not sure the module is 100% done yet (i.e. I'm throwing it back onto your end of the court Mic. :))

I'd set a feedback ? flag for flo if I could. ;)
Attachment #8352391 - Flags: review?(benediktp)
Comment on attachment 8352372 [details] [diff] [review]
JSM v1.0

*** Original change on bio 618 attmnt 629 at 2011-05-24 02:21:20 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352372 - Attachment is obsolete: true
Attachment #8352372 - Flags: review?
*** Original post on bio 618 at 2011-05-24 08:03:46 UTC ***

Comment on attachment 8352391 [details] [diff] [review] (bio-attmnt 648)
v2.0

>diff --git a/instantbird/app/Makefile.in b/instantbird/app/Makefile.in

> DEFINES += \
>   -DINSTANTBIRD_ICO=\"$(DIST)/branding/instantbird.ico\" \
>+  -DSTATUS_AVAILABLE_ICO=\"$(topsrcdir)/instantbird/themes/available-16.ico\" \
>+  -DSTATUS_AWAY_ICO=\"$(topsrcdir)/instantbird/themes/away-16.ico\" \
>+  -DSTATUS_OFFLINE_ICO=\"$(topsrcdir)/instantbird/themes/offline-16.ico\" \

I would prefer if you could add only one define for the path to the icons...

>diff --git a/instantbird/app/splash.rc b/instantbird/app/splash.rc

>@@ -48,6 +50,9 @@
> 
> // Program icon.
> IDI_APPICON  ICON  INSTANTBIRD_ICO
>+2  ICON  STATUS_AVAILABLE_ICO
>+3  ICON  STATUS_AWAY_ICO
>+4  ICON  STATUS_OFFLINE_ICO

... and list here the actual file names after using the defined variable.

For example, -DSTATUS_ICON_DIR=$(topsrcdir)/instantbird/themes in the Makefile and

2  ICON  "STATUS_ICON_DIR/available-16.ico" in splash.rc

I'm not completely sure that works/if there's something special to do with the quotes.

The other solution (a bit ugly) would be to include the path directly in the file, as there are already several paths for the cursors.
"../themes/available-16.ico" should work.

>diff --git a/instantbird/components/Makefile.in b/instantbird/components/Makefile.in

> EXTRA_COMPONENTS = \
>-	ibCommandLineHandler.manifest \
>+	ibStatusCommandLineHandler.js ibCommandLineHandler.manifest \

I think I would prefer a separate manifest file for the new component. (Manifest files are cheap as they are linked together at build time for packaged builds.)

>diff --git a/instantbird/components/ibStatusCommandLineHandler.js b/instantbird/components/ibStatusCommandLineHandler.js

>+      // We're unsetting the status message here at the moment.
Really?
>+      Services.core.setStatus(requestedStatus,
>+                              Services.core.currentStatusMessage);

>+      // Don't prevent the default action (i.e. Instantbird loading).
>+      cmdLine.preventDefault = false;

So, if I understood correctly, this code can be executed in 2 different contexts:
1. Instantbird is already running, to change the status. In this case, do you really want the default action (open/focus the buddy list) to be performed?
2. Instantbird isn't running yet. If your command line handler is executed before the core is started, Services.core.setStatus will most likely throw NS_ERROR_NOT_INITIALIZED.

>diff --git a/instantbird/modules/ibWinJumpList.jsm b/instantbird/modules/ibWinJumpList.jsm
>new file mode 100644
>--- /dev/null
>+++ b/instantbird/modules/ibWinJumpList.jsm
>@@ -0,0 +1,181 @@

>+  // Default jumplist entries for changing the status
>+  jumplistEntries: [
>+    { type: "shortcut",
>+      id: "status_available",
>+      label: "Available",
>+      description: "Set status to available.",
>+      parameter: "-status available",
>+      iconIndex: 1
>+    },
>+    { type: "shortcut",
>+      id: "status_away",
>+      label: "Away",
>+      description: "Set status to away.",
>+      parameter: "-status away",
>+      iconIndex: 2
>+    },
>+    { type: "shortcut",
>+      id: "status_offline",
>+      label: "Offline",
>+      description: "Set status to offline.",
>+      parameter: "-status offline",
>+      iconIndex: 3
>+    }],
Nit: ], on a separate line.
label and description should be localized, shouldn't they?

>+  init: function() {
>+    let winTaskbar = Cc["@mozilla.org/windows-taskbar;1"]
>+                        .getService(Ci.nsIWinTaskbar);
>+    this.winJumpListBuilder = winTaskbar.createJumpListBuilder();
>+
>+    // Return early if we can't use the jumplist service
>+    if (!this.winJumpListBuilder || !this.winJumpListBuilder.available)
>+      return;
>+
>+    // Remove the current jump list so it can be replaced
>+    this.uninit();
This is a one-line, misnamed ("uninit" suggests its called at shutdown, which doesn't seem to be the case, "reset" may be better) method called only once. Inline it? :)
Or maybe it's supposed to be called by add-on developers wanting to replace completely the default implementation?

>+    let item;
Any reason for this to be defined outside of the for loop?

>+    let items = Cc["@mozilla.org/array;1"].createInstance(Ci.nsIMutableArray);
>+
>+    for (let i = 0; i < this.jumplistEntries.length; i++) {
>+      let currentItem = this.jumplistEntries[i];

for each (let currentItem in this.jumplistEntries) {

>+      if(currentItem.type == "separator")
Nit: missing space between 'if' and '('

>+        item = this._getSeparatorItem();
>+      else if (currentItem.type == "shortcut") {
>+        item = this._getHandlerAppItem(currentItem.label,
>+                                       currentItem.description,
>+                                       currentItem.parameter,
>+                                       currentItem.iconIndex);
>+      }
>+      else if (currentItem.type == "link")
>+        item = this._getLinkItem(currentItem.uri, currentItem.uriTitle);
>+      else
>+        throw "Unknown jumplist item type: " + currentItem.type;

Are there plans to use the "separator" and "link" types soon? If not, this code can be simplified a lot.
*** Original post on bio 618 at 2011-05-26 00:12:02 UTC ***

Thunderbird is planning to auto-extract from png sprites the images it needs and convert them to ico, this might be something we want instead of keeping multiple copies around, see https://bugzilla.mozilla.org/show_bug.cgi?id=659788
Attached patch v2.1 (obsolete) — Splinter Review
*** Original post on bio 618 as attmnt 659 at 2011-05-27 01:40:00 UTC ***

> >+      Services.core.setStatus(requestedStatus,
> >+                              Services.core.currentStatusMessage);
> 
> >+      // Don't prevent the default action (i.e. Instantbird loading).
> >+      cmdLine.preventDefault = false;
> 
> So, if I understood correctly, this code can be executed in 2 different
> contexts:
> 1. Instantbird is already running, to change the status. In this case, do you
> really want the default action (open/focus the buddy list) to be performed?
> 2. Instantbird isn't running yet. If your command line handler is executed
> before the core is started, Services.core.setStatus will most likely throw
> NS_ERROR_NOT_INITIALIZED.

1. We now prevent the default if it's not the first launch.
2. It's been working fine when I execute Instantbird with it, but I'm not sure if there's something I can do to ensure the core is loaded?


> >+  init: function() {
> >+    let winTaskbar = Cc["@mozilla.org/windows-taskbar;1"]
> >+                        .getService(Ci.nsIWinTaskbar);
> >+    this.winJumpListBuilder = winTaskbar.createJumpListBuilder();
> >+
> >+    // Return early if we can't use the jumplist service
> >+    if (!this.winJumpListBuilder || !this.winJumpListBuilder.available)
> >+      return;
> >+
> >+    // Remove the current jump list so it can be replaced
> >+    this.uninit();
> This is a one-line, misnamed ("uninit" suggests its called at shutdown, which
> doesn't seem to be the case, "reset" may be better) method called only once.
> Inline it? :)
> Or maybe it's supposed to be called by add-on developers wanting to replace
> completely the default implementation?

Yes, this is meant so an add-on developer can clear the whole list. Renamed to reset! :)

> >diff --git a/instantbird/modules/ibWinJumpList.jsm b/instantbird/modules/ibWinJumpList.jsm
> >new file mode 100644
> >--- /dev/null
> >+++ b/instantbird/modules/ibWinJumpList.jsm
> >@@ -0,0 +1,181 @@
> 
> label and description should be localized, shouldn't they?

Yes, we haven't handle localization yet.

> Are there plans to use the "separator" and "link" types soon? If not, this code
> can be simplified a lot.
I'm not sure exactly when / if they'll be used. Maybe Mic has some plans for them? It can be removed probably. Also something else I noticed about this patch, it doesn't let you add other lists, it only lets you add the one Tasks list.

To do still:
Localization
Comment on attachment 8352391 [details] [diff] [review]
v2.0

*** Original change on bio 618 attmnt 648 at 2011-05-27 01:40:37 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352391 - Attachment is obsolete: true
Attachment #8352391 - Flags: review?(benediktp)
*** Original post on bio 618 at 2011-05-27 08:55:17 UTC ***

Comment on attachment 8352402 [details] [diff] [review] (bio-attmnt 659)
v2.1

>diff --git a/instantbird/app/Makefile.in b/instantbird/app/Makefile.in
>--- a/instantbird/app/Makefile.in
>+++ b/instantbird/app/Makefile.in
>@@ -14,18 +14,19 @@
> # The Original Code is mozilla.org code.
> #
> # The Initial Developer of the Original Code is
> # Netscape Communications.
> # Portions created by the Initial Developer are Copyright (C) 2002
> # the Initial Developer. All Rights Reserved.
> #
> # Contributor(s):
>-#  Brian Ryner <bryner@brianryner.com>
>-#  Florian Quèze <florian@instantbird.org>
>+#   Brian Ryner <bryner@brianryner.com>
>+#   Florian Quèze <florian@instantbird.org>
>+#   Patrick Cloke <clokep@gmail.com>
> #
> # Alternatively, the contents of this file may be used under the terms of
> # either the GNU General Public License Version 2 or later (the "GPL"), or
> # the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
> # in which case the provisions of the GPL or the LGPL are applicable instead
> # of those above. If you wish to allow use of your version of this file only
> # under the terms of either the GPL or the LGPL, and not to allow others to
> # use your version of this file under the terms of the MPL, indicate your

So on this file you are taking credit for fixing the indentation of the contributor list? ;)

(In reply to comment #24)
> Created an attachment (id=659) [details]
> v2.1
[...]
> > 2. Instantbird isn't running yet. If your command line handler is executed
> > before the core is started, Services.core.setStatus will most likely throw
> > NS_ERROR_NOT_INITIALIZED.
> 
> 1. We now prevent the default if it's not the first launch.
> 2. It's been working fine when I execute Instantbird with it, but I'm not sure
> if there's something I can do to ensure the core is loaded?

Then the command line handler in ibCommandLineHandler.js is executed before yours.
I don't know how the order is decided (if it's stable, if you've just been lucky, ...). But if it "just works", maybe it doesn't matter to know... :)
*** Original post on bio 618 at 2011-05-27 10:25:02 UTC ***

I'd like to keep the methods to insert a separator and links into the list, so extensions can use them if they like.


> (In reply to comment #24)
> > Created an attachment (id=659) [details] [details]
> > v2.1
> [...]
> > > 2. Instantbird isn't running yet. If your command line handler is executed
> > > before the core is started, Services.core.setStatus will most likely throw
> > > NS_ERROR_NOT_INITIALIZED.
> > 
> > 1. We now prevent the default if it's not the first launch.
> > 2. It's been working fine when I execute Instantbird with it, but I'm not sure
> > if there's something I can do to ensure the core is loaded?
> 
> Then the command line handler in ibCommandLineHandler.js is executed before
> yours.
> I don't know how the order is decided (if it's stable, if you've just been
> lucky, ...). But if it "just works", maybe it doesn't matter to know... :)

First: the diff for the file ibCommandLineHandler.manifest is missing from the latest patch.

It looks like the order is decided by the entry name that the components are registered with the category manager. See [1].
We're registering the status command line handler as "m-status" while the other is registered as "b-n" (which is quite high priority) [2].


[1] https://developer.mozilla.org/en/nsICommandLineHandler
[2] http://lxr.instantbird.org/instantbird/source/instantbird/components/ibCommandLineHandler.manifest#3
*** Original post on bio 618 at 2011-05-27 11:59:01 UTC ***

(In reply to comment #26)

> It looks like the order is decided by the entry name that the components are
> registered with the category manager. See [1].
> We're registering the status command line handler as "m-status" while the other
> is registered as "b-n" (which is quite high priority) [2].
> 
> 
> [1] https://developer.mozilla.org/en/nsICommandLineHandler

Ok, thanks for the good explanation! :)
Attached patch v3.0 With Localization (obsolete) — Splinter Review
*** Original post on bio 618 as attmnt 666 at 2011-05-30 21:08:00 UTC ***

This removes the shortcut type and adds localized jump list entries.
Attachment #8352409 - Flags: review?(florian)
Comment on attachment 8352402 [details] [diff] [review]
v2.1

*** Original change on bio 618 attmnt 659 at 2011-05-30 21:08:10 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352402 - Attachment is obsolete: true
Comment on attachment 8352409 [details] [diff] [review]
v3.0 With Localization

*** Original change on bio 618 attmnt 666 at 2011-05-30 22:10:21 UTC ***

We discussed this over IRC while I was reviewing the patch, so I won't give detailed comments here. The requested changes that I remember are:

* Safety checks to not fail when not on Win7, similar to http://mxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserGlue.js#371

* Some "description" properties should be getters.

* Mic suggested that a 'set' method would be a nice API addition to pair with the 'reset' method that's already there.

* I suspect we should not touch the value of preventDefault if we are not setting it to true.

* tab indent in jar.mn

* Periods at the end of sentences in comments.
Attachment #8352409 - Flags: review?(florian) → review-
Attached patch v3.1 (obsolete) — Splinter Review
*** Original post on bio 618 as attmnt 667 at 2011-05-30 22:22:00 UTC ***

Review given over IRC, will be attaching an interdiff of some sort momentarily.
Attachment #8352410 - Flags: review?(florian)
Comment on attachment 8352409 [details] [diff] [review]
v3.0 With Localization

*** Original change on bio 618 attmnt 666 at 2011-05-30 22:22:12 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352409 - Attachment is obsolete: true
Attached patch Interdiff v3.0 to v3.1 (obsolete) — Splinter Review
*** Original post on bio 618 as attmnt 668 at 2011-05-30 22:23:00 UTC ***

Here's some sort of a diff of the patches, it's kind of sloppy, but it shows the differences.
Attached patch v3.2Splinter Review
*** Original post on bio 618 as attmnt 669 at 2011-05-30 23:41:00 UTC ***

This fixes a couple of nits:
Changing cmdLine.preventDefault = line to only touch preventDefault in certain situations
helpInfo : <-- no space between helpInfo and ':'
Fixing the init/set split so that it works and makes sense

Sorry for the absurd amount of reviews this is taking! :(
Attachment #8352412 - Flags: review?(florian)
Comment on attachment 8352410 [details] [diff] [review]
v3.1

*** Original change on bio 618 attmnt 667 at 2011-05-30 23:41:09 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352410 - Attachment is obsolete: true
Attachment #8352410 - Flags: review?(florian)
Comment on attachment 8352411 [details] [diff] [review]
Interdiff v3.0 to v3.1

*** Original change on bio 618 attmnt 668 at 2011-05-30 23:41:09 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352411 - Attachment is obsolete: true
Comment on attachment 8352412 [details] [diff] [review]
v3.2

*** Original change on bio 618 attmnt 669 at 2011-05-31 00:38:52 UTC ***

We discussed another few details on IRC (broken copyright year in license headers, forgotten package-manifest.in changes, ...) that I edited before committing, but overall, it looks great!
Attachment #8352412 - Flags: review?(florian) → review+
*** Original post on bio 618 at 2011-05-31 00:39:46 UTC ***

https://hg.instantbird.org/instantbird/rev/92c5329d2fd7
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 0.3b1
*** Original post on bio 618 at 2011-05-31 04:53:37 UTC ***

Comment on attachment 8352412 [details] [diff] [review] (bio-attmnt 669)
v3.2

It's 'a tad bit late' for this but I found some things that don't look like they should :(

>diff --git a/instantbird/locales/en-US/chrome/instantbird/winjumplist.properties b/instantbird/locales/en-US/chrome/instantbird/winjumplist.properties
>new file mode 100644
>--- /dev/null
>+++ b/instantbird/locales/en-US/chrome/instantbird/winjumplist.properties
>+available.tooltip=Set status to available.
>+away.tooltip=Set status to away
>+offline.tooltip=Set status to offline

There are missing dots at the end of the sentences.

>diff --git a/instantbird/modules/ibWinJumpList.jsm b/instantbird/modules/ibWinJumpList.jsm
>new file mode 100644
>--- /dev/null
>+++ b/instantbird/modules/ibWinJumpList.jsm
>+      else if (currentItem.type == "link")
>+        item = this._getLinkItem(currentItem.uri, currentItem.uriTitle);

This should be gone as well.
*** Original post on bio 618 at 2011-05-31 22:51:05 UTC ***

(In reply to comment #35)
> (From update of attachment 8352412 [details] [diff] [review] (bio-attmnt 669) [details])
> It's 'a tad bit late' for this but I found some things that don't look like
> they should :(

Addressed in https://hg.instantbird.org/instantbird/rev/f03505da64ec.
Blocks: 954274
You need to log in before you can comment on or make changes to this bug.