Closed Bug 675392 Opened 9 years ago Closed 9 years ago

Remove code duplication for find keys and menu items by moving to utilityOverlay

Categories

(SeaMonkey :: UI Design, defect)

defect
Not set

Tracking

(seamonkey2.5 fixed)

RESOLVED FIXED
seamonkey2.5
Tracking Status
seamonkey2.5 --- fixed

People

(Reporter: iann_bugzilla, Assigned: iann_bugzilla)

Details

Attachments

(1 file, 4 obsolete files)

Attached patch move to utilityOverlay (obsolete) — Splinter Review
At the moment we have very similar code for the find keys and menu items in navigatorOverlay, mailWindowOverlay, messengercompose and editorOverlay.
The duplication could be removed by having a single version in utilityOverlay.
This patch:
* In editorOverlay.xul, messengercompose.xul and mailWindowOverlay.xul, removes find key elements, adjusts the related command and menuitem elements and adds keyset for overlay.
* In TB's editorOverlay.xul, adjusts the related command elements (label's were needless duplication).
* In navigatorOverlay.xul, removes find key elements, adjusts the related command (renaming ids from Browser:Find to cmd_find, etc) and menuitem elements and adds keyset for overlay.
* In editorOverlay.dtd, navigatorOverlay.dtd, messengercompose.dtd and messenger.dtd, removes the related entities that are no longer needed.
* Moves the VK_F19 key from editorOverlay.xul, browser/unix/platformNavigationBindings.xul and mailnews/unix/platformMailnewsOverlay.xul into common/unix/platformCommunicatorOverlay.xul (so editorOverlay.xul no longer needs to be pre-processed).
* In utilityOverlay.xul, creates a keyset findKeys with the find key elements in and creates menuitem overlays for find menuitems.
* In utilityOverlay.dtd, adds the related entities that are now needed.
Attachment #549531 - Flags: review?(neil)
Comment on attachment 549531 [details] [diff] [review]
move to utilityOverlay

Unfortunately this breaks mailnews, which wants to enable or disable the find command depending on whether there's a message loaded or not. Now for the previously shared keys it bites the bullet and updates them at all sorts of times (which is annoyingly slow in the case of cmd_delete, but there you go...) but for other keys it cheats and makes the <key> call goDoCommand directly. By making it a shared key you need to either a) ensure that the command is updated when a message loads or unloads or b) stick with goDoCommand.

>       <menuitem id="menu_find"
>+                label="&findCmd.label;"/>
Normally I wouldn't bother wrapping this, but I guess you can keep it...
Attachment #549531 - Flags: review?(neil) → review-
Attached patch move find to utilityOverlay (obsolete) — Splinter Review
Changes since last version:
* Renamed cmd_findAgain to cmd_findNext so that enablement works correctly for mail display windows.
Attachment #549531 - Attachment is obsolete: true
Attachment #549603 - Flags: review?(neil)
Changes since last version:
* Wrapped changed command element cmd_findNext.
* Unwrapped menu_find lines as suggested.
Attachment #549603 - Attachment is obsolete: true
Attachment #549603 - Flags: review?(neil)
Attachment #549605 - Flags: review?(neil)
(In reply to comment #2)
> * Renamed cmd_findAgain to cmd_findNext so that enablement works correctly
> for mail display windows.
Well, the good news is that I had noticed that the Find Again menuitem was incorrectly enabled in the mail window, and I had forgotten to mention it in the review, so thanks for fixing it anyway, but my issue is with the keys for Find*, which after the patch are only enabled once you open the menu...
Changes since last version:
* Use dispatcher to generate event each time message header pane is shown/hidden.
Attachment #549605 - Attachment is obsolete: true
Attachment #549605 - Flags: review?(neil)
Attachment #549641 - Flags: review?(neil)
Comment on attachment 549641 [details] [diff] [review]
move find to utilityOverlay with changed wrapping and dispatcher

I was surprised to find the key active in more situations than I expected, until I opened the menu and found it was "wrong" too, so not your fault.

> function ShowMessageHeaderPane()
> {
>+  document.commandDispatcher.updateCommands("mail-pane");
I have a slight preference to put this at the end of the function. Who knows, some day it may become an extension hook and it would be an idea to have the pane in the correct state when the update fires. Also, IMHO mail-pane is a bit vague, perhaps message-header-pane would be a better event name.
Attachment #549641 - Flags: review?(neil) → review+
Changes since last version:
* Renamed event to message-header-pane as suggested.
* Moved dispatcher to the end of each function.

Carrying forward r+
Attachment #549641 - Attachment is obsolete: true
Attachment #549685 - Flags: review+
Comment on attachment 549685 [details] [diff] [review]
move find to utilityOverlay with changed wrapping and renamed dispatcher event [Checked in: Comment 9]

Requesting r for TB affecting changes (editorOverlay.dtd and mail/components/compose/content/editorOverlay.xul)
Attachment #549685 - Flags: review?(mbanner)
Attachment #549685 - Flags: review?(mbanner) → review+
Comment on attachment 549685 [details] [diff] [review]
move find to utilityOverlay with changed wrapping and renamed dispatcher event [Checked in: Comment 9]

http://hg.mozilla.org/comm-central/rev/8b76ed94d36f
Attachment #549685 - Attachment description: move find to utilityOverlay with changed wrapping and renamed dispatcher event → move find to utilityOverlay with changed wrapping and renamed dispatcher event [Checked in: Comment 9]
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.5
You need to log in before you can comment on or make changes to this bug.