Now that the "Clear List" button is gone and multiple selection is allowed in the download manager, we should make it behave like other file list windows, allowing for keyboard and shortcut menu access to "Select All" - pressing Cmd/Ctrl+A should select all - "Select All" should be in the context menu, between separators, above the "remove from list" and "clear list" options This also requires modifying the browser overlay to map the Edit -> Select All menuItem on OSX to the download manager function. I've attached a first pass at the patch; doesn't seem to work yet. Any hints?

Comment on attachment 316371 [details] [diff] [review] WIP (UI shows up, command doesn't do anything) >+++ toolkit/mozapps/downloads/content/downloads.js >@@ -687,16 +707,17 @@ var gDownloadViewController = { > case "cmd_copyLocation": >+ case "cmd_selectAllDownload": > return true; There should be a section above this that just looks at the command without needing to look at a download. Would do the same as you have.. return true. >+++ toolkit/mozapps/downloads/content/downloads.xul >@@ -74,22 +76,24 @@ > <commandset id="generalCommands"> > <command id="cmd_findDownload" oncommand="setSearchboxFocus();"/> >+ <command id="cmd_selectAllDownload" oncommand="gDownloadsView.selectAll();)"/> > </commandset> > > <keyset id="downloadKeys"> > <key keycode="VK_ENTER" oncommand="doDefaultForSelected();"/> > <key keycode="VK_RETURN" oncommand="doDefaultForSelected();"/> > <key id="key_pauseResume" key=" " oncommand="performCommand('cmd_pauseResume');"/> >+ <key id="key_selectAllDownload" key="&selectAllCmd.key;" modifiers="accel" command="cmd_selectAllDownload"/> Both of these should use oncommand with performCommand('cmd_selectAllDownload') to dispatch through the download manager's custom perform command stuff. Not sure if that will help getting things to work. These comments just make it follow the rest of the download manager command pattern. Any ideas which things are being called when you hit cmd-a?

Gavin helped me on IRC a bit. This now works nicely on Vista, haven't tested on OSX or Linux. On OSX we want to make sure that the "Select All" menuItem from the Edit menu is properly mapped.

Addresses Mardak's first point, moving the switch for cmd_selectAllDownload to the upper section meant for actions that do not require a download object. Mardak: not sure your second comment is right, as I followed the pattern set by cmd_findDownload which, like cmd_selectAllDownload doesn't require a download object, and thus doesn't really need the custom command dispatch. Let's see what Shawn/Gavin say!

(Patch seems to work fine on OS X)

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9pre) Gecko/2008041601 SeaMonkey/2.0a1pre Ctrl-A works in Sm Download Manager => adding Sm-parity in whiteboard

Comment on attachment 316377 [details] [diff] [review] adds select all keyboard shortcut and context menuItem I'm actually doing homework right now and not looking at this patch at all ;) I'd also like a test for this, which won't be too hard to do. You'll want to copy what we do for this test: http://mxr.mozilla.org/seamonkey/source/toolkit/mozapps/downloads/tests/browser/browser_bug_411172.js With some differences however: 1) just test that the selectedItems property of the richlistbox is the same length as the number of downloads you add 2) Don't do the window.setTimeout stuff, but do it like this test: http://mxr.mozilla.org/seamonkey/source/toolkit/mozapps/downloads/tests/browser/browser_multiword_search.js You might be better off bribing Edward to do the test for you too. Who says beltzner can't code anyway? :) r=sdwilsh

Not sure how I can test for the right-click menu, but this seems like it should work as a test for the keyboard command event.

Comment on attachment 316427 [details] test for keyboard event Context menu would be a lot harder to test (although doable I think). I'm not gonna ask for that however. >function test_selectAllKeySelectsAll(aWin) >{ > // This also tests the ordering of the display > var doc = aWin.document; > > var dm = Cc["@mozilla.org/download-manager;1"]. > getService(Ci.nsIDownloadManager); > var db = dm.DBConnection; So, the testcase I linked to doesn't quite do this correctly (it's not wrong, but it's not safe). Please wrap the following in a try block > var stmt = db.createStatement("SELECT COUNT(*) FROM moz_downloads"); > stmt.executeStep(); > var richlistbox = doc.getElementById("downloadView"); > is(stmt.getInt32(0), richlistbox.children.length, > "The database and the number of downloads display matches"); And this should be in a finally block > stmt.reset(); with stmt.finalize() after it. > var len = DownloadData.length; > // Select all with keyboard > EventUtils.synthesizeKey("a", {metaKey: true}, aWin); > // Perform an action > EventUtils.synthesizeLey("VK_DELETE", {}, aWin); > stmt.executeStep(); > // Confirm the action happened on all > is(stmt.getInt32(0), 0, "All downloads were selected and removed"); > stmt.reset(); I'd rather not do an action here - that makes this test test more, so if it ever starts failing, it makes the developer have to look at more things that could be wrong. is(aWin.getElementById("downloadView").selectedItems.legnth, len) would be a better test over the above code. >function test() >{ > var dm = Cc["@mozilla.org/download-manager;1"]. > getService(Ci.nsIDownloadManager); > var db = dm.DBConnection; > > // First, we populate the database with some fake data > db.executeSimpleSQL("DELETE FROM moz_downloads"); Start a try block again > var rawStmt = db.createStatement( > "INSERT INTO moz_downloads (name, source, target, startTime, endTime, " + > "state, currBytes, maxBytes, preferredAction, autoResume) " + > "VALUES (:name, :source, :target, :startTime, :endTime, :state, " + > ":currBytes, :maxBytes, :preferredAction, :autoResume)"); > var stmt = Cc["@mozilla.org/storage/statement-wrapper;1"]. > createInstance(Ci.mozIStorageStatementWrapper) > stmt.initialize(rawStmt); > for each (var dl in DownloadData) { > for (var prop in dl) > stmt.params[prop] = dl[prop]; > > stmt.execute(); > } end try, start finally, add stmt.statement.reset() > stmt.statement.finalize(); From this point on, could you copy the code from http://mxr.mozilla.org/seamonkey/source/toolkit/mozapps/downloads/tests/browser/browser_multiword_search.js#85 with one difference - the body of the observer should be the body of the finishUp function you have. The window.setTimeout stuff is not reliable as sayrer has informed me, so I'm trying to move away from it (I just haven't had the time to update all of the DM tests yet).

Comment on attachment 316377 [details] [diff] [review] adds select all keyboard shortcut and context menuItem >Index: toolkit/mozapps/downloads/content/downloads.xul >+ <command id="cmd_selectAllDownload" oncommand="gDownloadsView.selectAll();"/> I think you can change this to oncommand="performCommand('cmd_selectAllDownload');". >Index: browser/base/content/downloadManagerOverlay.xul > window.addEventListener("load", function(event) { > var findMenuItem = document.getElementById("menu_find"); > findMenuItem.setAttribute("command", "cmd_findDownload"); > findMenuItem.setAttribute("key", "key_findDownload"); > }, false); >+// Map Edit -> Select All command to download manager's command >+ window.addEventListener("load", function(event) { >+ var selectAllMenuItem = document.getElementById("menu_selectAll"); >+ selectAllMenuItem.setAttribute("command", "cmd_selectAllDownload"); >+ selectAllMenuItem.setAttribute("key", "key_selectAllDownload"); >+ }, false); > ]]></script> You should merge these together (i.e. just put the three selectAllMenuItem lines right after the findMenuItem lines). >Index: browser/base/content/browser-menubar.inc I don't think this change is required, but I suppose it doesn't hurt.

Comment on attachment 316377 [details] [diff] [review] adds select all keyboard shortcut and context menuItem (In reply to comment #9) > >Index: browser/base/content/browser-menubar.inc > I don't think this change is required, but I suppose it doesn't hurt. It's needed for the getElementById below: >+// Map Edit -> Select All command to download manager's command >+ window.addEventListener("load", function(event) { >+ var selectAllMenuItem = document.getElementById("menu_selectAll"); >+ selectAllMenuItem.setAttribute("command", "cmd_selectAllDownload"); >+ selectAllMenuItem.setAttribute("key", "key_selectAllDownload"); >+ }, false); > ]]></script>

Add test to check for ctrl-a functionality based on beltzner's test and updated the menuitem test. Also reworked the patch to get rid of the performCommand stuff which is mainly used for a particular download. So instead the menu and key use the same command without performCommand. (In reply to comment #9) > > window.addEventListener("load", function(event) { .. > >+// Map Edit -> Select All command to download manager's command > >+ window.addEventListener("load", function(event) { > > ]]></script> > You should merge these together (i.e. just put the three selectAllMenuItem > lines right after the findMenuItem lines). Done.

Comment on attachment 316516 [details] [diff] [review] v2 >diff --git a/toolkit/mozapps/downloads/content/downloads.xul b/toolkit/mozapps/downloads/content/downloads.xul >+ <command id="cmd_selectAllDownloads" oncommand="gDownloadsView.selectAll();"/> performCommand('cmd_selectAllDownloads') please r=sdwilsh

(In reply to comment #12) > (From update of attachment 316516 [details] [diff] [review]) > >diff --git a/toolkit/mozapps/downloads/content/downloads.xul b/toolkit/mozapps/downloads/content/downloads.xul > >+ <command id="cmd_selectAllDownloads" oncommand="gDownloadsView.selectAll();"/> > performCommand('cmd_selectAllDownloads') please performCommand is supposed to be for a particular download object. And if no download is provided, it's going to do the command for all selected downloads.

ah, good catch. Disregard comment.

Comment on attachment 316516 [details] [diff] [review] v2 Add select all for use with delete now to help with the clear list. Comes with 2 testcases for keyboard select all and menu select all.

Comment on attachment 316516 [details] [diff] [review] v2 ui-r?mconnor for double checking the position of selectAll in the context menu <mconnor> select all is generally the last item <mconnor> lemme think about the UI

Er. ignore the "Clear *Current* List" thing..

Comment on attachment 316516 [details] [diff] [review] v2 Yeah, I think this works. a=me as well. bombs away!

Checking in browser/base/content/browser-menubar.inc; /cvsroot/mozilla/browser/base/content/browser-menubar.inc,v <-- browser-menubar.inc new revision: 1.159; previous revision: 1.158 done Checking in browser/base/content/downloadManagerOverlay.xul; /cvsroot/mozilla/browser/base/content/downloadManagerOverlay.xul,v <-- downloadManagerOverlay.xul new revision: 1.4; previous revision: 1.3 done Checking in toolkit/mozapps/downloads/content/downloads.js; /cvsroot/mozilla/toolkit/mozapps/downloads/content/downloads.js,v <-- downloads.js new revision: 1.148; previous revision: 1.147 done Checking in toolkit/mozapps/downloads/content/downloads.xul; /cvsroot/mozilla/toolkit/mozapps/downloads/content/downloads.xul,v <-- downloads.xul new revision: 1.50; previous revision: 1.49 done Checking in toolkit/mozapps/downloads/tests/browser/Makefile.in; /cvsroot/mozilla/toolkit/mozapps/downloads/tests/browser/Makefile.in,v <-- Makefile.in new revision: 1.25; previous revision: 1.24 done RCS file: /cvsroot/mozilla/toolkit/mozapps/downloads/tests/browser/browser_select_all.js,v done Checking in toolkit/mozapps/downloads/tests/browser/browser_select_all.js; /cvsroot/mozilla/toolkit/mozapps/downloads/tests/browser/browser_select_all.js,v <-- browser_select_all.js initial revision: 1.1 done Checking in toolkit/mozapps/downloads/tests/browser/browser_multi_select.js; /cvsroot/mozilla/toolkit/mozapps/downloads/tests/browser/browser_multi_select.js,v <-- browser_multi_select.js new revision: 1.3; previous revision: 1.2 done

beltzner writing a 11KB patch? What's the world coming to?

ugh, an* 11KB patch. :(

cvs commit -m 'Fix testcase for bug 429614 to check platform (cmd-a vs ctrl-a)' toolkit/mozapps/downloads/tests/browser/browser_select_all.js Checking in toolkit/mozapps/downloads/tests/browser/browser_select_all.js; /cvsroot/mozilla/toolkit/mozapps/downloads/tests/browser/browser_select_all.js,v <-- browser_select_all.js new revision: 1.2; previous revision: 1.1 done

Verified FIXED (both the context-menu item and the shortcut) using: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9pre) Gecko/2008041904 Minefield/3.0pre Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9pre) Gecko/2008041904 Minefield/3.0pre -and- Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008041907 Minefield/3.0pre

Is this only supposed to select visible items? It takes more than 15 seconds for my download manager to load fully, I have to wait for it to fully load, it won't carry on "selecting all". This intentional or should I make a new bug?

(In reply to comment #26) > Is this only supposed to select visible items? > > It takes more than 15 seconds for my download manager to load fully, I have to > wait for it to fully load, it won't carry on "selecting all". This intentional > or should I make a new bug? Please file a separate bug (and no, probably not intentional).

Yeah, the way this is written, it will only select items that the richlistbox knows about, and since we're batch updating that set of contents, it won't actually select all of the items. Please file a follow-up bug and cc me.