User-Agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1.5) Gecko/20091109 Ubuntu/9.10 (karmic) Firefox/3.5.5 Build Identifier: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.3a1pre) Gecko/20091204 Shredder/3.1a1pre Spun off Bug 521026. <commandsets> are updated only when the menu(s) are opened. Some <keyset> functionality is broken because of this; very few of the <key> elements use the "command" attribute, like they should. A probable solution is updating all <commandsets> when switching tabs. Then the <key> elements can properly use their "command" attribute. Reproducible: Always

Something like this, maybe? The patch in bug 521026 will conflict.

Ooo! Yes, this does seem like a more comprehensive solution. For a change like this we would likely want to add mozmill tests to make sure we are not regressing anything. https://developer.mozilla.org/en/Thunderbird/Thunderbird_MozMill_Testing I expect the test structure would want to go something like this: - Verify that things are correctly configured at startup. (3-pane "folder" tab) - Open a "message" tab, switch to it, make sure things are correctly configured. - Switch back to the "tab", make sure things are correctly configured. - Open a radically different tab type; probably a content tab. - Verify things are configured correctly. Because we want to check what is happening at startup, this would likely want its own subdirectory off of mozmill. When I say 'verify things are configured correctly', I am thinking we would just check that all of the commands' defined states match what the active (tab) controller says they should be. It is circular in some ways, but captures the state requirements well. And it certainly would be a great test if, in the future, we try and optimize things by not re-checking everything all the time. I am told that our mozmill test runner may run best with mozmill 1.2 right now rather than the latest and greatest...

I missed a lot of goDoCommand() calls. This update takes care of almost all of them. There are still a few questionable uses of oncommand="goDoCommand(...)" Here's the full list: http://hg.mozilla.org/comm-central/file/4b63c4badb42/mail/base/content/mailWindowOverlay.xul#l291 http://hg.mozilla.org/comm-central/file/4b63c4badb42/mail/base/content/mailWindowOverlay.xul#l300 http://hg.mozilla.org/comm-central/file/4b63c4badb42/mail/base/content/mailWindowOverlay.xul#l1708 The first two are calling event.stopPropagation(), and the last one selectively chooses which command to execute with a ternary expression. Not sure what to do about those? Line 1708 could probably be left as it is.

(In reply to comment #3) Sorry, ignore that. I'm just not thinking today! The original patch was fine, the update breaks everything; not supposed to patch "oncommand" attributes in any elements except <key>'s.

Updates the patch for comm-central, and fixes some missed <key> elements (for real, this time!) This also effectively backs out the patch in Bug 521026.

This is my first attempt at a MozMill test! Right now it only tests the F8 key (See Bug 521026). I'm not sure if this is the correct way to do this. :asuth, do you think you could provide some guidance on this test? I want to be sure I'm on the right track with this before I dive in too much further. Thanks!

Adds an additional event to one of the commandsets. Also has a workaround for some strangeness caused by the last patch; see the comments in the commandglue.js patch.

Comment on attachment 441204 [details] [diff] [review] Patch v3 Thank you for the patch; I won't be able to get to it for a few days because of trying to finish out some fixes for the 3.1 beta 2 freeze, but should be able to look at it after that and we should ideally be able to target it for the release candidate.

There is something missing here, unfortunately. After testing the patch more thoroughly this weekend, I came to the conclusion that the current menus expect different states for becoming "enabled" or "disabled". For example, the delete function (in the Edit menu, and associated to the Delete key [and Backspace on OS X]) expects one or more messages to be selected before it can be marked enabled. Switching tabs won't necessarily have an effect on which messages (or how many) are selected/unselected, but selection events in message trees will. I'm beginning to question whether this patch is enough to fix this. The real bug here is that some commands are disabled when they should be enabled, and vice versa. Adding more events that can update those commands is the direction I was heading. FWIW, fixing this will also fix some related bugs. I'm marking this bug as blocking those. Please correct me if I'm wrong! See also: Bug 544983 (not sure if that one is the same problem, since I haven't investigated it). The other two are definitely related here.

(In reply to comment #9) > FWIW, fixing this will also fix some related bugs. I'm marking this bug as > blocking those. Please correct me if I'm wrong! See also: Bug 544983 (not > sure if that one is the same problem, since I haven't investigated it). The > other two are definitely related here. I very much doubt this would fix bug 526288 - iirc that is more of a problem with the way Lightning does its command handlers (overriding Thunderbird's) than an issue with switching tabs. Indeed if your trying to solve this I would recommend doing it without Lightning installed (if you have it installed already).

At the time I filed this bug, the only event I was aware of that caused a problem with command handlers was switching tabs. It's not the only one. But you're right, this won't completely fix Bug 526288; it just needs this fixed too. Even with Lightning's commands updated properly, the Thunderbird commands won't be.

Ah, I missed that the test was a separate file. We actually already have various tests for the message pane in the mozmill test suite and they also have existing helpers. The helpers are here: http://mxr.mozilla.org/comm-central/source/mail/test/mozmill/shared-modules/test-folder-display-helpers.js#1498 The main test file for all things message pane display related: http://mxr.mozilla.org/comm-central/source/mail/test/mozmill/folder-display/test-message-pane-visibility.js It looks like you wrote your test with the IDE in mind, but you will need to run any tests from our command line mechanism using our support code as documented here: https://developer.mozilla.org/en/Thunderbird/Thunderbird_MozMill_Testing The most important thing in the test is to set up the appropriate situation for the test. The ideal situation is to write a test that fails without the changed functionality and passes with it. So in this case we'd want to create a situation where previously the status of the command failed to get updated (and failed to run), but now gets properly updated and properly executes. In the case where our use of goDoCommand has kludged around the underlying issue, if the status can help us detect a current fail and future success, that can work. I think when I made comment #2 suggesting a test I was probably much more informed about the situation than I am now... what are your thoughts about creating a test along those lines. I/we can provide input on how to accomplish such a thing if you agree it's the right strategy.

Comment on attachment 441204 [details] [diff] [review] Patch v3 (Clearing feedback request since I provided feedback, I think. Let me know if you would like more feedback!)

Jason, excellent work thus far. possible for you to update and finish off the patch?

Sorry, I won't have time for this. Should reassign to someone else.

(In reply to Jason Oster (:Parasyte) from comment #15) > Sorry, I won't have time for this. Should reassign to someone else.