Closed Bug 663695 Opened 9 years ago Closed 2 years ago

Implement feature for reordering attachments during composition: Functionality, UI, and keyboard shortcuts

Categories

(Thunderbird :: Message Compose Window, enhancement)

enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 58.0

People

(Reporter: bugzilla2007, Assigned: bugzilla2007)

References

(Depends on 1 open bug, Blocks 3 open bugs)

Details

(4 keywords)

Attachments

(3 files, 27 obsolete files)

942 bytes, text/plain
bwinton
: ui-review+
Details
15.59 KB, image/png
Details
69.83 KB, patch
aceman
: review+
Paenglab
: ui-review+
Paenglab
: feedback+
Details | Diff | Splinter Review
+++ This bug was initially created as a clone of Bug #229224 +++

Bug 229224 implements the functionality of reordering attachments during composition, made accessible via drag & drop for mouse users only.

This bug adds keyboard accessibility for reordering attachments.

STR

0 After bug 229224 is fixed...
1 Compose message, add 3 or more attachments
2 Select one or more attachments and try to move them up/down within the list of attachments using keyboard only

Current Result:
- reordering of attachments is not keyboard accessible

Expected Result:
- reordering of attachments must be keyboard accessible

Suggested Implementation:

A: Shortcut keys
For selected and focussed attachment(s) only(!):

Note that the following shortcut keys will only apply when
- there is more than 1 attachment
- focus is in attachment list and
- one or more attachments are selected

A1: [Alt]+[Cursor up] to move attachment(s) up within the list
    [Alt]+[Cursor down] to move attachment(s) down within the list

- A1 is the most important functionality, as it allows for simple, intuitive and and efficient reordering of attachments via keyboard (a must for disabled users; greater speed and precision than drag&drop for all users).
- We cannot use any other modifier keys as they are taken:
Ctrl is for selection (at least on Win)
Shift is for selection (at least on Win).
So [Alt] is the only option, and it's also somewhat intuitive if you think of
alt-eration of the position.
- [Alt]+Cursor keys is not used for anything else in the composer window

A2: [Alt]+[Pos1] to move attachment(s) to the beginning of the list
    [Alt]+[End] to move attachment(s) to the end of the list

- A2 would be the logical extension of A1 to fast-track moving attachments to the beginning or end of the list.
- intuitive, as Pos1 and End will move focus (and selection) to the beginning/end of the list; combined with [Alt], it's moving list items to the same positions.

I'll do the context menu later in a separate comment.
Next steps: request UI-review for A1 and A2 from bwinton
For details, pls read comment 1
Attachment #538763 - Flags: ui-review?(bwinton)
Comment on attachment 538763 [details]
Request UI-review to add keyboard access for reordering attachments in composition

By [Pos1], I'm assuming you mean [Home].  Given that, this seems like reasonable behaviour to me.  ui-r=me.
Attachment #538763 - Flags: ui-review?(bwinton) → ui-review+
Oh, and the Alt key won't work on Mac (Alt-Up is Home and Alt-Down is End), so you'll need to come up with different keys there.  "Things" uses Cmd-Up and Cmd-Down.  "iTunes" doesn't offer the capability at all.

Later,
Blake.
Blake, thanks for fast UI-review!

I need help with the keys for the MAC, anyone?
I looked at "Mac OS X keyboard shortcuts" [1], but it's confusing without knowledge and testing on a MAC.
From comment 4, I would suggest the following:

A1: to move attachment(s) up/down within the list
- Win etc. [Alt]+[Cursor up/down]
- MAC OS   [Cmd]+[Cursor up/down] (as in "Things" application; according to comment 4, cannot use Alt+Up/Down because they mean Home/End)

A2: to move attachment(s) to the beginning/end of the list
- Win etc. [Alt]+[Home/End]
- MAC OS   [Cmd]+[Alt]+[Cursor up/down] (because Alt+Up/Down is Home/End, + Modifier Ctrl for moving items)

According to official "Mac OS X keyboard shortcuts" [1], "Option-Command-Up Arrow/Down arrow" (i.e. Alt+Cmd+Up/Down) is not used for anything else.
[1] http://support.apple.com/kb/HT1343

Questions for both A1 and A2 shortcuts for the MAC as suggested above:
- Any conflicts with existing TB-MAC shortcuts in the attachment list?
- Any conflicts with existing MAC OS shortcuts?
- Does this feel right on the MAC?

More questions for message composition on TB-MAC
With the 2nd out of 3 attachments selected and focussed,...
- What does Cmd+Down arrow do on a single selected+focussed attachment?
- How do you open a single selected+focussed attachment on the MAC?
- What does Option-up/down (Alt+up/down) do?
(In reply to comment #5)
> A2: to move attachment(s) to the beginning/end of the list
> - Win etc. [Alt]+[Home/End]
> - MAC OS   [Cmd]+[Alt]+[Cursor up/down] (because Alt+Up/Down is Home/End, +
> Modifier Ctrl for moving items)

Typo fix: 
...because Alt+Up/Down is Home/End, + Modifier *Cmd* for moving items (as Ctrl and Cmd are two different keys on the MAC?)
Ludo, who can help me with shortcuts on the MAC (comment 5)?
Whiteboard: [has ui-review+ for Win&Unix]
(In reply to Thomas D. from comment #5)

> Questions for both A1 and A2 shortcuts for the MAC as suggested above:
> - Any conflicts with existing TB-MAC shortcuts in the attachment list?

Not that I've noticed - but I don't attach on my daily email habbits.

> - Any conflicts with existing MAC OS shortcuts?

Not when the focus is at the right place. If the focus is on the editor then this cmd up and down will scroll that window. So It's a matter of focus.

> - Does this feel right on the MAC?

Doesn't feel wrong.

> More questions for message composition on TB-MAC
> With the 2nd out of 3 attachments selected and focussed,...
> - What does Cmd+Down arrow do on a single selected+focussed attachment?

Nothing

> - How do you open a single selected+focussed attachment on the MAC?

Enter ?

> - What does Option-up/down (Alt+up/down) do?

Nothing.
Keywords: access
(In reply to Ludovic Hirlimann [:Usul] from comment #8)
> (In reply to Thomas D. from comment #5)
> 
> > Questions for both A1 and A2 shortcuts for the MAC as suggested above:
> > - Any conflicts with existing TB-MAC shortcuts in the attachment list?
> 
> Not that I've noticed - but I don't attach on my daily email habbits.
> [snip]

So from Ludo's feedback, it looks as if what I propose in comment 5 (sic), A1 and A2, would actually work for the MAC.

The only surprise is that with the 2nd out of 3 attachments selected and focussed, Ludo claims Option+up/down (=Alt+up/down) does *not* do anything, whereas from comment 4 I would expect them to move focus and selection to the first/last attachment (and hence, I suggested *Cmd*+Option+up/down to move selected *attachment* to the beginning or end of the list). But that's something for Jim to check if he has done all the keybindings as expected for the MAC.

Next step: request UI-review for keys for reordering attachments on the *MAC* (we already have ui-review+ for Win+Unix from comment 2)
Blake, 3 minutes to ui-review these keys for the MAC, adding to existing ui-review+

The windows/linux keys already have ui-review+ from Blake's Bug 663695 Comment 2.
Adding on top of that, this is to request ui-review+ for the suggested keys on *MAC*, starting out from Blake's Bug 663695 Comment 4.
Attachment #580025 - Flags: ui-review?(bwinton)
Comment on attachment 580025 [details]
Initial/partial UX specification.txt: Implement keyboard shortcuts for reordering attachments in composition (incl. MAC)

This seems reasonable for Mac.

Thanks,
Blake.
Attachment #580025 - Flags: ui-review?(bwinton) → ui-review+
Whiteboard: [has ui-review+ for Win&Unix] → [has ui-review+]
Attachment #538763 - Attachment is obsolete: true
Sometimes I feel like surprising others, even myself :)

Here's a fully functional patch which implements the core part of the new feature (see caveats below).

The ux-efficiency of this is just awesome, and the keyboard shortcuts have ui-r+ already from Blake.
For added WOW, this works 100% correctly and predictable on all kinds of selections: Single, multiple, adjacent, non-adjacent, mixed. You never know what users might want to do, let's have some candy for advanced users. That said, plain vanilla also works nicely. Enjoy!

WIP. Caveats:
* This patch applies to flat Windows installation of TB, not Trunk, not MAC.
* MAC needs their own shortcuts, as described in full UX specification (attachment 580025 [details]).
* We might want to add some more cool keyboard shortcuts while we are here, e.g. Alt+End/Alt+Home on Windows/Linux (see attachment 580025 [details]).
* Obviously, this would benefit from some more UI to be discoverable, e.g. (context) menus. I was even dreaming of small buttons which only appear when attachments are selected.
Assignee: nobody → bugzilla2007
Status: NEW → ASSIGNED
Attachment #8915985 - Flags: feedback?(jorgk)
Attachment #580025 - Attachment description: Request ui-review to add keyboard access for reordering attachments in composition (now incl. MAC).txt → Full UX specification.txt: Implement keyboard shortcuts for reordering attachments in composition (incl. MAC)
Attachment #580025 - Attachment filename: quest ui-review to add keyboard access for reordering attachments in composition (now incl. MAC).txt → UX doc - keyboard access for reordering attachments in composition (incl. MAC).txt
Richard, FYI ^^^

Note to self:
While experimenting with this, I had an initial version which would collapse non-adjacent selected attachments into a block. Which might also be useful for the advanced user.

So for selected attachments (on Windows), in additon to...
> Alt+Cursor down - move down (see patch)
> Alt+Cursor up - move up (see patch)
> Alt+Home - move to top of list
> Alt+End - move to end of list

... we could also have:
> Alt+Cursor left  - bundle-to-top
>                    (collapse non-adjacent selected attachments into a block starting from topmost selected attachment)
> Alt+Cursor right - bundle-to-bottom
>                    (collapse non-adjacent selected attachments into a block ending in last selected attachments)
Attachment #8915985 - Attachment description: Patch V.1: WIP / Proof of concept patch (caveat: for flat Windows install only) → Patch V.1: WIP / Proof of concept patch (caveat: for flat Windows install only) - Minimal implementation of Alt+Cursor Up/Down to reorder selected attachments
Caveat: Patch applies to flat Windows install of TB (not trunk, not MAC)

Improvements:
- streamlined code
- improved command names
- introduce versatile 'direction' parameter for MoveSelectedAttachments() function
- implement Alt+Home to move selected attachments to the top of the list
- implement Alt+End to move selected attachments to the end of the list

Next steps:
- Implement separate keyboard shortcuts for MAC
- ux-discovery (UI)
Attachment #8915985 - Attachment is obsolete: true
Attachment #8915985 - Flags: feedback?(jorgk)
Attachment #8916209 - Flags: feedback?(jorgk)
Clean up a bit (stale comment in js; respect 80 chars line length in xul file).
Attachment #8916209 - Attachment is obsolete: true
Attachment #8916209 - Flags: feedback?(jorgk)
Attachment #8916217 - Flags: feedback?(jorgk)
Hmmm, unfortunely Alt+Home doesn't handle some cases correctly yet:
If you find any other cases of weird selections where things end up in an unexpected order, please let me know.

# = selected

a#
b#
x
c#
d#

Alt+Home

Actual result
a#
c#
d#
b#
x

Expected result
a#
b#
c#
d#
x
(In reply to Thomas D. from comment #16)
> Hmmm, unfortunely Alt+Home doesn't handle some cases correctly yet:

Fixed.

> If you find any other cases of weird selections where things end up in an
> unexpected order, please let me know.

Dito.
Attachment #8916217 - Attachment is obsolete: true
Attachment #8916217 - Flags: feedback?(jorgk)
Attachment #8916227 - Flags: feedback?(jorgk)
Maybe command names should use the plural 'Attachments'?

cmd_moveAttachmentUp vs.
cmd_moveAttachmentsUp

etc.
Summary: Add keyboard accessibility for reordering attachments during composition (context menu, shortcut keys) → Implement functionality for reordering attachments during composition, incl. keyboard access (context menu, shortcut keys)
This is fun!

Enjoy a clean, fresh, and innovative Panel UI for the new feature.
The cool part being that when the panel is open, focus will remain where the real action is happening, in the attachment bucket, so those nifty keyboard shortcuts for moving attachments around will still work. And the panel conveniently advertises the shortcuts, too...

Works like a charme.
But still partly WIP/proof of concept, and still applicable to flat Win install only.

Richard, please have a look at this.
I had to temporarily hardcode style="height: 1px;" into the separators because it didn't work in the main style sheet, strangely. Also hardcoded: Panel strings with shortcut keys (toolbarbutton labels). We'd want the keyboard shortcut information to be automatically generated and right-aligned, like on normal menus.
This needs your CSS/UI experience. Feel free to choose different XUL controls if you're not happy with toolbarbutton. However, I'm quite happy with the current "light" layout, so maybe we can keep that. Or maybe you have other ideas. I definitely want to keep the panel with the anchored arrow.

We could try to use "contextmenu == true" in the openPopup method of panel, then try to use menuitems, which might simplify adding the shortcut strings on screen. I have deliberately not yet created strings for that until we know how exactly we want to do this.

I think Richard and I can polish this some more before we start to entertain Jörg for code review. But if there's anything odd in code apart from the deliberate omissions, please let me know.
Attachment #8916227 - Attachment is obsolete: true
Attachment #8916227 - Flags: feedback?(jorgk)
Attachment #8916443 - Flags: feedback?(richard.marti)
Attached image Screenshot 1: Reorder Attachments Panel (obsolete) —
This is how it looks! :)
Attached patch 663695_reorder_attachments.patch (obsolete) — Splinter Review
Patch that applies on c-c
... and DONE!!! (95%)

Now ready for consumption, smarter than ever!

Improvements with this patch:
* Applies to Trunk
* MAC shortcut keys added (Cmd+Up/Down, Cmd+Alt+Up/Down, per attachment 580025 [details])
* Move attachments only to the edges of the list; no rotation
* Fold non-coherent selections as they reach the edges of the list
* Keep 'Reorder Attachments' Panel open while attachment list or the panel have focus, so user can select other attachments without losing the panel
* Correct, dynamic tooltip display using slick CSS ::after selector as in latest FF (thanks Richard for the hint!)
* Even lighter panel UI (removed groupbox per Richard's advice, use menuseparators)
* Improved command enabling/disabling, now always correct and up to date
* Reorder Attachments... command added to attachment list context menu
* Fixed and improved the movement algorithm
* Various helper functions
* Tame some overlong XUL code lines in the neighbourhood

Todo:
* Richard, can you please check and duplicate the CSS for other themes?
* Jörg and/or Aceman, if you've got time, could we start looking at the code?
* Optionally, implement Alt+Cursor left/right for folding non-coherent selections in place, i.e. convenient grouping of non-adjacent attachments

Known issues:
* The panel does not close if you use any controls from menu bar or main toolbar, which is a pre-existing bug because the focus is not removed from attachment list (for comparison, see the correct behaviour when you click on Contacts Side bar).
Attachment #8916443 - Attachment is obsolete: true
Attachment #8916608 - Attachment is obsolete: true
Attachment #8916443 - Flags: feedback?(richard.marti)
Attachment #8916945 - Flags: feedback?(richard.marti)
Attachment #8916945 - Flags: feedback?(jorgk)
Attachment #8916945 - Flags: feedback?(acelists)
Summary: Implement functionality for reordering attachments during composition, incl. keyboard access (context menu, shortcut keys) → Implement feature for reordering attachments during composition: Functionality, UI, and keyboard shortcuts
Here's the new and lighter panel with pretty keyboard shortcuts, as of the latest patch. Also note command enabling/disabling.
Attachment #8916489 - Attachment is obsolete: true
(In reply to Thomas D. from comment #22)
> Created attachment 8916945 [details] [diff] [review]
> ...
> Todo:
> * Richard, can you please check and duplicate the CSS for other themes?

And of course, we need to have real icon images for the buttons, which is Richard's domain... :)
If we like the current icons, I think they should be relatively easy to do as SVG because we already have a similar SVG for our dropdown arrows...
Comment on attachment 8916945 [details] [diff] [review]
Patch V.4 (WIP, for Trunk): (Improved UI & behaviour) Implement full functionality, UI, and shortcut keys to reorder selected attachments

>+function AttachmentsSelectionIsBlock(where) {
> ...
>+
>+  let indexFirstSelAttachment =
>+    bucketList.getIndexOfItem(selItems[0]);
>+  let indexLastSelAttachment =
>+    bucketList.getIndexOfItem(selItems[selectedCount - 1]);
>+  let isBlock = ((indexFirstSelAttachment) ==
>+                 (indexLastSelAttachment + 1 - selectedCount));
>+
>+    switch (where) {
>+    case "top":
>+      // True if selection is a coherent block at the top of the list.
>+      return (indexLastSelAttachment == (selectedCount - 1));
>+    case "end":
>+      // True if selection is a coherent block at the end of the list.
>+      return (indexLastSelAttachment == (AttachmentsCount() - 1)) && isBlock;
>+    default:
>+      // True if selection is a coherent block.
>+      return ((indexLastSelAttachment + 1 - selectedCount) ==
>+               indexFirstSelAttachment);
>+    }
>+}

Oh, I didn't finish polishing this. Here's the streamlined code for next iteration:

    switch (where) {
    case "top":
      // True if selection is a coherent block at the top of the list.
      return (indexFirstSelAttachment == 0) && isBlock;
    case "end":
      // True if selection is a coherent block at the end of the list.
      return (indexLastSelAttachment == (AttachmentsCount() - 1)) && isBlock;
    default:
      // True if selection is a coherent block.
      return isBlock;
(In reply to Thomas D. from comment #22)
> Known issues:
> * The panel does not close if you use any controls from menu bar or main
> toolbar, which is a pre-existing bug because the focus is not removed from
> attachment list (for comparison, see the correct behaviour when you click on
> Contacts Side bar).

Which maybe isn't a bug, but a feature:
Main menu should not remove focus from attachment list because there are commands in Edit Menu which only work with focused attachments. I'm not sure about the main composition toolbar, but if we had a Delete button in customization, maybe it shouldn't be disabled either. It doesn't hurt much that the panel is a bit sticky in those situations, and closing the panel is never further away than your ESC key or clicking anywhere in the main window, contacts side bar, or headers.
(In reply to Thomas D. from comment #26)
> and closing the panel is
> never further away than your ESC key or clicking anywhere in the main
> window, contacts side bar, or headers.

Or so I thought (wrt ESC to close the panel), and it was working in some previous iteration, but not now... :|
And if somebody would kindly explain to me why simple event.preventDefault() in onpopuphiding of reorderAttachmentsPanel does NOT work to prevent closing of the panel, and instead gets you into all sorts of undefined UI states, I'll be very glad. Spent several hours trying to figure that out (incl. event.stopPropagation() etc.) until I decided to give up and come the other way round, to set noautohide="true" and then hidePopup() in onBlur of the attachmentBucket if focus is anywhere else. I'd bet that it's a bug, but maybe someone can enlighten me.
The original plan was to just check in onpopuphiding of the panel if the focus was now in attachmentBucket, and in that case, cancel the event so that the panel remains open. Which would probably also solve our ESC problem because I assume it's noautohide="true" which now prevents ESC.
For perfection, if we can't get the panel to close when using commands from menu or toolbar (which seems acceptable), we should update the moveAttachment* commands when attachments get added or removed for immediate, correct enabling/disabling of panel buttons. Renaming closes the panel, ah well.
(In reply to Thomas D. from comment #22)
> Created attachment 8916945 [details] [diff] [review]
> ...
> * Correct, dynamic tooltip display using slick CSS ::after selector as in
> latest FF (thanks Richard for the hint!)

Typo: That's dynamic *Shortcut Keys display* on the panel buttons, not tooltips...
Comment on attachment 8916945 [details] [diff] [review]
Patch V.4 (WIP, for Trunk): (Improved UI & behaviour) Implement full functionality, UI, and shortcut keys to reorder selected attachments

Review of attachment 8916945 [details] [diff] [review]:
-----------------------------------------------------------------

Just some drive-by comments, forgive me if those questions are already answered, I haven't read through the whole bug.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +40,5 @@
>  var mozISpellCheckingEngine = Components.interfaces.mozISpellCheckingEngine;
>  
>  var sDictCount = 0;
>  
> +var $ = function(aId) {

What's that funky thing?

::: mail/components/compose/content/messengercompose.xul
@@ +351,5 @@
> +                 label="&moveAttachmentTopPanelBtn.label;"
> +                 accesskey="&moveAttachmentTopPanelBtn.accesskey;"
> +                 key="key_moveAttachmentTop"
> +                 command="cmd_moveAttachmentTop"
> +                 image="data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAABAAAAAQCAIAAACQkWg2AAAABnRSTlMA/wD/AP83WBt9AAAA

Maybe a little unorthodox encoding the images here. We're actually moving to SVG for most images these days, but Richard knows this better.
Comment on attachment 8916945 [details] [diff] [review]
Patch V.4 (WIP, for Trunk): (Improved UI & behaviour) Implement full functionality, UI, and shortcut keys to reorder selected attachments

Good starting point. I'll upload a version with more CSS and SVG icons.
Attachment #8916945 - Flags: feedback?(richard.marti) → feedback+
Improved version UI wise.

I'm using the same colors as FX uses for Photon, but it's easy to use our own colors.

I removed the separators and added some margin to separate the buttons. I reduced the panel padding a bit but not completely like FX.

Thomas, on MDN it's written that when noautohide="true" is used a close button should be added. But yeah, also without it, the panel should one time close when I click somewhere of the composer. For testing the styles it was very handy it never closed. :)
Hmmmm, midair collision - 30 minutes ago, Richard uploaded a patch where he complemented V.4 of my patch with more CSS and SVG icons.
I've spent day and night programming to roll this out, so I'll do that anyway...

*** Patch status ***

* I'd now consider this patch feature-complete (not finished).

*** Caveats ***

* So we need to merge this patch with Richard's additions of attachment 8917887 [details] [diff] [review] for improved CSS and SVG icons. There are three new icons on two new buttons in my patch.
* Until then, hard-coded icons just for illustration purposes, obviously.

*** Improvements shipped with this patch ***

* New feature: 'Sort selection' (minimalistic, smart, dynamic toggle button, one for all); neatly implemented with dynamic data-* attributes in the XUL file for ease of maintenance and clean code.
* New feature: 'Move together' (Alt+Cursor-Left for grouping non-adjacent attachments in the list with the topmost selected item; when you add that missing attachment at the end of the list and you know exactly where in the list it needs to go).
* Easter egg: Alt+Cursor-Right for 'Move together' (group at last selected item); it just works for completeness sake...
* ESC now closes the panel for the most typical situation (focus on attachment bucket)
* Improved command handlers; the 'Reorder Attachments' panel is now available whenever focus is on attachment list, to allow easier access, esp. for sorting.
* New and improved helper functions; camel-style naming (we need to bite the bullet, I'm not the first to introduce modern style in this file)
* Improved, smart behaviour for various discontinuous selection edge cases
* Corrected, improved, and condensed multi-purpose movement algorithm (e.g., we're no longer moving items which are already in the correct location)
* Ubiquitous annotations so that we know what we're doing
* Peripheral Bug-Fixes and workarounds:
- After deleting items, we retain/restore an intelligent, visible, predictable keyboard focus (invisible, jumping focus before)
- After deleting items, command updaters (e.g. Rename, Remove, Reorder) now disabling correctly because I forced update of selectedIndex; there's a bug here that after removing items, listbox.selectedItem does not get released (still holding the deleted attachment), and listbox.selectedCount is incorrect (returning 1 although no attachment selected, which in turn confuses command controllers); interestingly, selectedIndex is already at -1 so we're not even changing it but still it forces everything into order.
- Single-left-clicking on whitespace of attachments listbox with file items must deselect items (as in Windows Explorer), not jump out with "Attach File" dialog. I doubt many users know this trick anyway. So it's now double-click for the trick, and single-click for platform-consistent deselection behaviour.
* All access keys and shortcut keys on the panel now triggering correctly; I'll forever fail to understand the non-logic of when they trigger or not. We'll want to double-check for interference with other UI elements which are not always on composition screen, e.g. attachment reminder, and a few others.
* The entire functionality still nicely packed into a single, small, and light-weight panel which is at your fingertips when you need it, and gets out of your way when you don't. Btw, with focus on attachment list, you can get the panel with Ctrl+# (did I do that for MAC?).
* For maximum ux-efficiency, the attachment-moving keyboard shortcuts work both with and without the panel. Consider the panel as your helpful guide (more like a interactive tooltip) until you'll know and love those keyboard shortcuts by heart, which is easy because they are so generic...
Attachment #8916945 - Attachment is obsolete: true
Attachment #8916957 - Attachment is obsolete: true
Attachment #8916945 - Flags: feedback?(jorgk)
Attachment #8916945 - Flags: feedback?(acelists)
Attachment #8917919 - Flags: feedback?(richard.marti)
Attachment #8917919 - Flags: feedback?(jorgk)
Attachment #8917919 - Attachment description: Patch V.4 (WIP): (Add 'Sort Selection', 'Move together' features, improved behaviour & code, peripheral bugfixes) Implement 'Reorder Attachments' feature: Functionality, UI, shortcut keys → Patch V.5 (WIP; to merge with 4.1): (Add 'Sort Selection', 'Move together' features, improved behaviour & code, peripheral bugfixes) Implement 'Reorder Attachments' feature: Functionality, UI, shortcut keys
(In reply to Thomas D. from comment #34)
> Created attachment 8917919 [details] [diff] [review]
> ...
> * The entire functionality still nicely packed into a single, small, and
> light-weight panel which is at your fingertips when you need it, and gets
> out of your way when you don't. Btw, with focus on attachment list, you can
> get the panel with Ctrl+# (did I do that for MAC?).

Typo: That's *ALT+#* on Win/Linux, and CMD+# on MAC. MAC users, am I stealing a shortcut which should do something else?

> * For maximum ux-efficiency, the attachment-moving keyboard shortcuts work
> both with and without the panel. Consider the panel as your helpful guide
> (more like a interactive tooltip) until you'll know and love those keyboard
> shortcuts by heart, which is easy because they are so generic...

I've missed this feature for a long time!!!
It's so odd having to remove all of your attachments if you want to swap/add one at the top of the list, or to change order.
With this patch, user now in full control of attachments order at any time.
Attached patch Patch v5.1 with CSS and SVG (obsolete) — Splinter Review
V5 with CSS and SVG. Quickly added a icon for "Move together". The sort button has still it's data images as this is way too complicated. Please don't do such things. One icon is enough and only change the text. On a main UI, okay but not in this covert UI.
Attachment #8917887 - Attachment is obsolete: true
Comment on attachment 8917919 [details] [diff] [review]
Patch V.5 (WIP; to merge with 4.1): (Add 'Sort Selection', 'Move together' features, improved behaviour & code, peripheral bugfixes) Implement 'Reorder Attachments' feature: Functionality, UI, shortcut keys

I'm waiting until the sort button is done simpler.
Attachment #8917919 - Flags: feedback?(richard.marti)
(In reply to Richard Marti (:Paenglab) from comment #36)
> Created attachment 8917941 [details] [diff] [review]
> Patch v5.1 with CSS and SVG

Thanks for updating the patch!!! :) :)
I'll look into it later, too tired after too much of marathon coding, plus my test message takes 13 seconds to load even though it's totally small in size. That's a lot of waiting time for hundred's of test rounds to eliminate bugs...
One small bug remained, spelling error, on letter missing in a variable for a last-minute correction, so dynamic sort button is currently broken.

> V5 with CSS and SVG. Quickly added a icon for "Move together". The sort
> button has still it's data images as this is way too complicated. Please
> don't do such things. One icon is enough and only change the text. On a main
> UI, okay but not in this covert UI.

Covert perfection, hehe...

But seriously, what's complicated about having two alternating images for one button? The code to change the img src is already working. We shouldn't show an A-Z picture when the button says Z-A, that's unprofessional and just confusing. Oh, and I sent you the original SVGs... did you get them?
I remember that we already did buttons with alternating images in the last century when IE4 was NEW, and FF probably not yet existing... :o)

Btw, what is better: changing image source in javascript as I did in the patch or just changing some custom attributes in code and use CSS to swap the images (I guess that works, after what I've seen with the keyboard shortcuts...)?
Awesome!

Here's a mini fix for a last-minute variable spelling mistake in my patch v.5 which unfortunately broke most of the dynamic of the new sort button.
Now everything should work (haven't tested Richard's variant of my patch yet, just corrected the nit in the patch).
Attachment #8917941 - Attachment is obsolete: true
Attachment #8917960 - Attachment description: Patch V.5.2 (WIP; with SVG/CSS): (nitfix variable which breaks dynamic sort button) Implement 'Reorder Attachments' feature → Patch V.5.2 (WIP; with SVG/CSS): (nitfix variable which broke the dynamic sort button) Implement 'Reorder Attachments' feature
Comment on attachment 8917960 [details] [diff] [review]
Patch V.5.2 (WIP; with SVG/CSS): (nitfix variable which broke the dynamic sort button) Implement 'Reorder Attachments' feature

This is not based on my patch -> No SVG and my CSS in it.

You can nitfix my patch yourself.
(In reply to Thomas D. from comment #38)
> (In reply to Richard Marti (:Paenglab) from comment #36)
> > Created attachment 8917941 [details] [diff] [review]
> > Patch v5.1 with CSS and SVG
> > V5 with CSS and SVG. Quickly added a icon for "Move together". The sort
> > button has still it's data images as this is way too complicated. Please
> > don't do such things. One icon is enough and only change the text. On a main
> > UI, okay but not in this covert UI.
> 
> Covert perfection, hehe...

But a lot of files which have to be maintained for a minor function.

> But seriously, what's complicated about having two alternating images for
> one button? The code to change the img src is already working. We shouldn't
> show an A-Z picture when the button says Z-A, that's unprofessional and just
> confusing. Oh, and I sent you the original SVGs... did you get them?
> I remember that we already did buttons with alternating images in the last
> century when IE4 was NEW, and FF probably not yet existing... :o)
> 
> Btw, what is better: changing image source in javascript as I did in the
> patch or just changing some custom attributes in code and use CSS to swap
> the images (I guess that works, after what I've seen with the keyboard
> shortcuts...)?

Changing attributes.
Attached image abstract sort icon (obsolete) —
Quick hack of a abstract sort icon where the direction doesn't matter.
Here's the right patch with Richard's updates and my nitfix to make the sort button work correctly, sorry for the confusion. I'm really brain-fried now after too much of coding...
Attachment #8917960 - Attachment is obsolete: true
(In reply to Richard Marti (:Paenglab) from comment #41)
> (In reply to Thomas D. from comment #38)
> > (In reply to Richard Marti (:Paenglab) from comment #36)
> > > Created attachment 8917941 [details] [diff] [review]
> > > Patch v5.1 with CSS and SVG
> > > V5 with CSS and SVG. Quickly added a icon for "Move together". The sort
> > > button has still it's data images as this is way too complicated. Please
> > > don't do such things. One icon is enough and only change the text. On a main
> > > UI, okay but not in this covert UI.
> > 
> > Covert perfection, hehe...
> 
> But a lot of files which have to be maintained for a minor function.

It's exactly 1 more file to have alternating images on Sort button, come on...
And what's the "maintenance burden" anyway, once they are in the system?

It might be a minor function in the big scheme of things, but it's a painful gap for those who need it, including myself. I don't know how many times I've written applications and had to start all over again with attachments when there was something wrong with the 1st one in the list.

I'd also expect a bit more welcoming reaction, not criticism when a volunteer (like you) provides a nicely polished feature after dozens of hours of work to eliminate bugs in the feature during production.

I'm surprised that we've come so low as to reject a truly shining small feature for the very fact that it's polished. Are you suggesting that we have buttons without icons so that we have 7 image files less to maintain?
I take that as a joke...
For the record, here's how it looks with my Patch V.5, attachment 8917919 [details] [diff] [review].
That's before Richard's makeover with SVGs/CSS.
Hooray!

This patch is now feature-complete and includes all SVG icons.
As far as I can see, everything works exactly as it should.
I hope that I didn't lose or add anything along the way, it has become a big thing now, and it's night time once again...

I've been working with Richard behind the scenes to find the best icons and Richard crafted the respective SVGs and saw to it that they get applied correctly via CSS. Great teamwork!

We went for modern and minimalist design.

Enjoy!
Attachment #8917919 - Attachment is obsolete: true
Attachment #8917965 - Attachment is obsolete: true
Attachment #8917966 - Attachment is obsolete: true
Attachment #8917985 - Attachment is obsolete: true
Attachment #8917919 - Flags: feedback?(jorgk)
Attachment #8918694 - Flags: ui-review?(richard.marti)
Attachment #8918694 - Flags: review?(jorgk)
... and this is how it looks now!
Comment on attachment 8918694 [details] [diff] [review]
Patch V.5.5: (complete with all SVGs/CSS) Implement 'Reorder Attachments' feature

Review of attachment 8918694 [details] [diff] [review]:
-----------------------------------------------------------------

Removing the review flags because this patch doesn't build correctly as it fail's on packaging move-together-up.svg which doesn't exist.

I know now why your patches removes empty lines. In the patch hunks, the unchanged empty lines have a white space. It seems, your editor automatically removes such spaces. Please, can you configure your editor to not do this in patch files?

Below the review of the CSS files.

::: mail/themes/linux/mail/compose/messengercompose.css
@@ +838,5 @@
> +  list-style-image: url("chrome://messenger/skin/icons/move-bottom.svg");
> +}
> +
> +#btn_moveAttachmentBundleUp {
> +  list-style-image: url("chrome://messenger/skin/icons/move-together-up.svg");

The correct file name is move-together.svg.

::: mail/themes/osx/mail/compose/messengercompose.css
@@ +1234,5 @@
> +  list-style-image: url("chrome://messenger/skin/icons/move-bottom.svg");
> +}
> +
> +#btn_moveAttachmentBundleUp {
> +  list-style-image: url("chrome://messenger/skin/icons/move-together-up.svg");

Again, move-together.svg.

::: mail/themes/shared/jar.inc.mn
@@ +41,5 @@
>    skin/classic/messenger/icons/junk.svg                       (../shared/mail/icons/junk.svg)
>    skin/classic/messenger/icons/mark.svg                       (../shared/mail/icons/mark.svg)
> +  skin/classic/messenger/icons/move-bottom.svg                (../shared/mail/icons/move-bottom.svg)
> +  skin/classic/messenger/icons/move-down.svg                  (../shared/mail/icons/move-down.svg)
> +  skin/classic/messenger/icons/move-together-up.svg           (../shared/mail/icons/move-together-up.svg)

Also here, move-together.svg

And the entry for sort.svg is completely missing which ends in not packaging the icon and none is shown in the UI.

::: mail/themes/shared/mail/messenger.css
@@ +132,5 @@
> +}
> +
> +.panelButton:not([disabled]):-moz-any([open],:hover:active) {
> +  background-color: var(--arrowpanel-dimmed-further);
> +  box-shadow: 0 1px 0 hsla(210,4%,10%,.03) inset;

Nit. When you're on fixing the patch, please can you add spaces after the commas?

::: mail/themes/windows/mail/compose/messengercompose.css
@@ +1031,5 @@
> +#btn_moveAttachmentBundleUp {
> +  list-style-image: url("chrome://messenger/skin/icons/move-together.svg");
> +}
> +
> +#btn_sortAttachmentsToggle[data-sortdirection] {

The attribute isn't needed here.

@@ +1036,5 @@
> +  list-style-image: url("chrome://messenger/skin/icons/sort.svg");
> +}
> +
> +#btn_sortAttachmentsToggle[data-sortdirection=descending] > .toolbarbutton-icon {
> +  transform: rotate(180deg);

Instead of rotate, please use scaleY(-1). Then the icon will only be flipped instead of calculating the rotation. This should be faster.

Please, also add this sort rules to linux and osx. Without this the two platforms will never see a icon for the sort entry.
Attachment #8918694 - Flags: ui-review?(richard.marti)
Attachment #8918694 - Flags: review?(jorgk)
So I have morphed and condensed the movement algorithms quite radically once again... Would be good to start finalizing this by way of code review...

* Radically simplified movement algorithms
* Fixed image inclusion issues mentioned in Richard's comment 48.
* Removed some unnecessary checks in functions, because they use other functions which have checked already.
* Shrinked Jim's nodelist conversion nicely as times have moved on:
> let selItems = Array.prototype.slice.call(bucketList.selectedItems);
let selItems = [...bucketList.selectedItems];
* Added missing function comments, completed/improved existing comments
* More minor nitfixes/improvements
Attachment #8918694 - Attachment is obsolete: true
Attachment #8919356 - Flags: ui-review?(richard.marti)
Attachment #8919356 - Flags: review?(jorgk)
Attachment #8919356 - Attachment description: Patch V.5.5: (nitfixes, condensed algorithm, full function comments) Implement 'Reorder Attachments' feature → Patch V.6: (nitfixes, condensed algorithm, full function comments) Implement 'Reorder Attachments' feature
Nits found and fixed.
Otherwise same as described in comment 49.
Attachment #8919356 - Attachment is obsolete: true
Attachment #8919356 - Flags: ui-review?(richard.marti)
Attachment #8919356 - Flags: review?(jorgk)
Attachment #8919365 - Flags: ui-review?(richard.marti)
Attachment #8919365 - Flags: review?(jorgk)
(In reply to Richard Marti (:Paenglab) from comment #48)
> I know now why your patches removes empty lines. In the patch hunks, the
> unchanged empty lines have a white space. It seems, your editor
> automatically removes such spaces.

Thanks a lot, yes, that was the reason for the unintentional removal of blank lines.

> Please, can you configure your editor to
> not do this in patch files?

I'm using Notepad++ and I do appreciate "Trim trailing and save" Macro feature, which I assigned to Ctrl+S.
It would be great to exclude patch files from that trick, but I wouldn't know how to configure that.
Even to adjust the original macro to skip lines with just a single space from trimming, it's rare for that to occur wrongly.
Any ideas?
Flags: needinfo?(richard.marti)
Flags: needinfo?(jorgk)
I don't use Notepad++ and can't say how you can configure it. I can only say, never use Ctrl+S (is this together with save?) on a patch. You can do this on every source file you want, but never on a patch. On my editor it also automatically strips the white spaces at the end of a line when I save a file, but I was able to configure do not do this on patch files.
Flags: needinfo?(richard.marti)
(In reply to Thomas D. from comment #51)
> Any ideas?
Nope. I don't strip trailing spaces by default (and also rarely edit patch files).
Flags: needinfo?(jorgk)
Nit: Another undeclared variable, but it still works. Fixed for next iteration.

Another question:

Alt+# already works to get "Reorder Attachments" panel when focus is on attachmentBucket.
I would like the same shortcut to also work when focus is NOT on attachmentBucket initially (as there's no reason why it shouldn't work, and it's unlikely for anything or anyone to use that shortcut for something else), as long as there is more than 1 attachment (that's another nit in the current controller, fixed for next iteration).

Unfortunately, attachment pane controller will only activate the key when attachmentBucket is focused.
How to activate the same command/key combo in main composition window?
Is there a general controller for composition where this could be hooked into?
Comment on attachment 8919365 [details] [diff] [review]
Patch V.6.1: (nitfixes: let, blank lines) Implement 'Reorder Attachments' feature

Patch doesn't work. Please upload the one with the variable fix.
Attachment #8919365 - Flags: ui-review?(richard.marti)
It worked on my flat installation, so probably compiling is stricter. Sorry. Thanks for trying this out!
Attachment #8919365 - Attachment is obsolete: true
Attachment #8919365 - Flags: review?(jorgk)
Attachment #8919407 - Flags: ui-review?(richard.marti)
Comment on attachment 8919407 [details] [diff] [review]
Patch V.6.2: (nitfixes, let) Implement 'Reorder Attachments' feature

Looks good now. Only one thing I've seen now: the panel closes when I click in the message- or the header area but not when I click in the toolbar or menubar. This should also close the panel.
Attachment #8919407 - Flags: ui-review?(richard.marti) → ui-review+
(In reply to Thomas D. from comment #56)
> It worked on my flat installation.

So how can we help you set up a proper build system? That would be a good time investment for all parties ;)
You should really never ever edit raw patch files. Just let hg produce what it needs.
(In reply to Richard Marti (:Paenglab) from comment #57)
> Comment on attachment 8919407 [details] [diff] [review]
>
> Looks good now. Only one thing I've seen now: the panel closes when I click
> in the message- or the header area but not when I click in the toolbar or
> menubar. This should also close the panel.

(In reply to Thomas D. from comment #22)
> Known issues:
> * The panel does not close if you use any controls from menu bar or main
> toolbar, which is a pre-existing bug because the focus is not removed from
> attachment list (for comparison, see the correct behaviour when you click on
> Contacts Side bar).

Contrary to my comment 26, I'm now convinced that this is a bug which my patch doesn't cause and so I can't fix it.
Put focus and select something in attachment pane without opening my panel, then click on menu bar or main toolbar.
Now you've got double focus (visually), happy guessing. If focus (not selection) was correctly removed from attachment list, my panel would close correctly. So there's nothing I can do.
Again, check windows explorer for the correct behviour, focus must be removed, selection remains e.g. when you click on the dropdown arrow of Properties button in windows explorer with a selected file. You'll still be working on the selection, but it won't have focus while those other controls have focus. The principle of keyboard focus is to be unique...
(In reply to Thomas D. from comment #59)
> You'll still be
> working on the selection, but it won't have focus while those other controls
> have focus. The principle of keyboard focus is to be unique...

So list selection without focus must have grey background.
(In reply to Magnus Melin from comment #58)
> (In reply to Thomas D. from comment #56)
> > It worked on my flat installation.
> 
> So how can we help you set up a proper build system? That would be a good
> time investment for all parties ;)

Thanks :) I don't usually fix things which would require building, except for marginal things like including new image files which can be done without building again. My flat installation allows for fast testing of the changes I implement without waiting for building. I have tortoisehg with comm-central, and I recently upgraded my production system to use tortoisehg for managing my flat build as well. Plus I have configured notepad++ with a simple keyboard shortcut to trigger a windiff comparison of my left and right current notepad++ files, i.e. daily.workbench (my flat install) file diff against comm-central file, which allows me to rapidly transfer changes from workbench into comm-central with a bit of caution against #ifdefs. I'm suspecting that I'd spend even more unpaid time with building without much gain on my side. That said, yeah, maybe one day...

> You should really never ever edit raw patch files. Just let hg produce what
> it needs.

I never edit raw patch files, so yes, I just let tortoise-hg produce what it needs.
I just wanted to verify the patch file in n++ by looking at it, maybe I could have changed a spelling error in the patch (bad idea, sure), otherwise it was Save-All or Ctrl+S muscle reflex which triggered my "Trim trailing and Save" macro at the wrong spot. Only after the mishap did I try editing the patch file RenameSelectedAttachment because empty lines kept changing by themselves until Richard uncovered the trailing space trim problem.
Unfortunately I'm not into python or notepad++ internals although I guess it would be relatively easy to exclude .patch files from that macro.
What I'd like to know from Magnus is why my panel correctly stays open when attachments are deleted, but goes away when just renaming one, and it's still open when the renaming dialogue appears, but only closes when the renaming gets executed. Except for the prompt itself, I don't see much difference in code between renaming and removing. The panel is designed to close only when focus exits from attachment pane, which does not seem to happen when the rename prompt comes on screen, so why after that?
Flags: needinfo?(mkmelin+mozilla)
(In reply to Thomas D. from comment #62)
> What I'd like to know from Magnus is why my panel correctly stays open when
> attachments are deleted, but goes away when just renaming one, and it's
> still open when the renaming dialogue appears, but only closes when the
> renaming gets executed. Except for the prompt itself, I don't see much
> difference in code between renaming and removing. The panel is designed to
> close only when focus exits from attachment pane, which does not seem to
> happen when the rename prompt comes on screen, so why after that?

More annoyingly, the panel actually closes after renaming is totally done, as all of the following console.logs show that the panel is still open:

>   let item = bucket.getSelectedItem(0);
>   let attachmentName = {value: item.attachment.name};
>   if (Services.prompt
>               .prompt(window,
>                       getComposeBundle().getString("renameAttachmentTitle"),
>                       getComposeBundle().getString("renameAttachmentMessage"),
>                       attachmentName,
>                       null,
>                       {value: 0}))
>   {
>     if (attachmentName.value == "")
>       return; // name was not filled, bail out
> console.log ('after-rename-dlg panel.state=' + $("reorderAttachmentsPanel").state);
>     let originalName = item.attachment.name;
>     item.attachment.name = attachmentName.value;
> console.log ('after-item.attachment.name panel.state=' + $("reorderAttachmentsPanel").state);
> 
>     item.setAttribute("name", attachmentName.value);
> console.log ('after-item.setattribute-name panel.state=' + $("reorderAttachmentsPanel").state);
> 
>     gContentChanged = true;
> 
>     let event = document.createEvent("CustomEvent");
>     event.initCustomEvent("attachment-renamed", true, true, originalName);
> console.log ('after-event.initCustomEvent panel.state=' + $("reorderAttachmentsPanel").state);
> 
>     item.dispatchEvent(event);
> console.log ('after-item.dispatchEvent panel.state=' + $("reorderAttachmentsPanel").state);
> 
>   }
> }

So if the panel is still open at this point when renaming is totally done, what would cause it to close? Maybe keyup event from pressing enter on rename dialogue ends up on the panel?
(In reply to Thomas D. from comment #63)

> So if the panel is still open at this point when renaming is totally done,
> what would cause it to close? Maybe keyup event from pressing enter on
> rename dialogue ends up on the panel?

It doesn't seem to be keyup either:
I added this to the panel...
> onkeyup="alert('keyup on panel');"
...and it didn't fire after renaming.

So it's a mystery to me how the panel gets closed although focus never seems to leave attachmentBucket, which is the exit condition for panel.
(In reply to Thomas D. from comment #64)
> (In reply to Thomas D. from comment #63)
> 
> > So if the panel is still open at this point when renaming is totally done,
> > what would cause it to close? Maybe keyup event from pressing enter on
> > rename dialogue ends up on the panel?
> 
> It doesn't seem to be keyup either:
> I added this to the panel...
> > onkeyup="alert('keyup on panel');"
> ...and it didn't fire after renaming.
> 
> So it's a mystery to me how the panel gets closed although focus never seems
> to leave attachmentBucket, which is the exit condition for panel.

In fact, and surprisingly, attachmentBucket receives a onBlur event (which I coded to close the panel) NOT when the rename dialogue is shown (that would make sense, isn't it) or executed, but when the entire rename routine is finished. Why?
(In reply to Richard Marti (:Paenglab) from comment #48)

> I know now why your patches removes empty lines. In the patch hunks, the
> unchanged empty lines have a white space. It seems, your editor
> automatically removes such spaces. Please, can you configure your editor to
> not do this in patch files?

FTR if others are facing the same challenge:

I succeeded to configure Notepad++ to do the following:
Ctrl+S on current document
--> If file extension is .patch: Save normally.
--> Any other file extension: Trim trailing whitespace and save.
Save command from menu or 'Save All' will still work as before.

With nppExec Plugin, I'm using a nppExec script (saved via nppExec's execute dialogue).
Then nppExec Advanced options to add it as a menuitem.
Then use notepad++ keyboard shortcut configurator to assign Ctrl+S keyboard shortcut to plugin menu item.

> NPP_CONSOLE 0
> echo Trim trailing whitespace (not for .patch) and save
> IF $(EXT_PART) == .patch GOTO normalsave
> :trimsave
> echo Trim trailing whitespace...
> npp_sendmsg NPPM_MENUCOMMAND 0 42024
> :normalsave
> echo Save...
> npp_save

I found the message number by looking at the file shortcuts.xml in notepad++ application folder

> <NotepadPlus>
>     <InternalCommands />
>     <Macros>
>         <Macro name="Trim Trailing and save" Ctrl="no" Alt="yes" Shift="yes" Key="83">
>             <Action type="2" message="0" wParam="42024" lParam="0" sParam="" />
>             <Action type="2" message="0" wParam="41006" lParam="0" sParam="" />
>         </Macro>
>     </Macros>
>     <UserDefinedCommands>
> ...
(In reply to Thomas D. from comment #65)
> In fact, and surprisingly, attachmentBucket receives a onBlur event (which I
> coded to close the panel) NOT when the rename dialogue is shown (that would
> make sense, isn't it) or executed, but when the entire rename routine is
> finished. Why?

Anyway, we might never find out why, so here's the workaround to fix our problem:
Now the Reorder panel will remain open when Rename Attachment command is used.
Attachment #8919407 - Attachment is obsolete: true
Attachment #8919679 - Flags: review?(jorgk)
I've rebased the patch (didn't apply due to bug 1404695). I also restored two empty lines which were removed for no apparent reason (which creates extra noise, like a pink left side in the review view). The one in front of |var sDictCount = 0| was surely desired.

Please answer comment #31:
> +var $ = function(aId) {
What's that funky thing?

Finally, Aceman is better at doing reviews like this one. I tried it briefly and it seemed to work :-)
Attachment #8919679 - Attachment is obsolete: true
Attachment #8919679 - Flags: review?(jorgk)
Attachment #8919861 - Flags: review?(acelists)
Attachment #8919861 - Flags: feedback+
(In reply to Jorg K (GMT+2) from comment #31)
> > +var $ = function(aId) {
> 
> What's that funky thing?

I hope there's a hidden smile somewhere in that question, because otherwise it could be mistaken as an arrogant criticism of a smart coding tweak whose obvious purpose is to make our life easier, with more compact and readable code. But since you're insisting, here's my explanation FTR, filed as bug 1409963 - let's discuss it there :)

(In reply to Thomas D. from bug 1409963 comment #0)

> $() as a shorthand for document.getElementById()
> 
> 1) An omni-present function, now with a unique and minimalist face
> 2) No loss of informational value; it's actually more salient hence easier
> to spot than before.
> 3) Simplify production; save developer time and focus
> 4) Smaller code-base size
> 5) More readable, dense/compact code
> 6) Coding style: avoid unnecessary variables, extra lines and code
> complexity; less memory footprint
> 7) No maintenance concerns.
> 
> I can only say from my own experience when writing up my patches on bug
> 663695 (e.g. attachment 8919861 [details] [diff] [review]) that using $() is
> completely fun, a no-brainer time-saver which makes for nicer and simpler
> code.
> Please look at my code there: it's actually a lot easier to spot where
> elements get initiated because of the unique face of the new $() function.
> 
> I hope that explains the "funky thing".
> I'm hereby proposing to make this helper function available globally in our
> codebase (comm-central).
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Sorry, accidental mis-click on touchpad
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Status: REOPENED → ASSIGNED
(In reply to Jorg K (GMT+2) from comment #31)

> ::: mail/components/compose/content/messengercompose.xul
> @@ +351,5 @@
> > +                 label="&moveAttachmentTopPanelBtn.label;"
> > +                 accesskey="&moveAttachmentTopPanelBtn.accesskey;"
> > +                 key="key_moveAttachmentTop"
> > +                 command="cmd_moveAttachmentTop"
> > +                 image="data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAABAAAAAQCAIAAACQkWg2AAAABnRSTlMA/wD/AP83WBt9AAAA
> 
> Maybe a little unorthodox encoding the images here. We're actually moving to
> SVG for most images these days, but Richard knows this better.

I find it a bit unfortunate (funky?) that you seem to believe and create the public impression here that I could want encoded inline images to go live; I'm not a newbie... And I had clearly marked that as part of the WIP/proof of concept nature of the patch, but then, as you mentioned and apologized for, unfortunately you don't have time to read previous comments on bugs...:

(In reply to Thomas D. from comment #24)
> And of course, we need to have real icon images for the buttons, which is
> Richard's domain... :)
> If we like the current icons, I think they should be relatively easy to do
> as SVG because we already have a similar SVG for our dropdown arrows...
Attachment #8919861 - Attachment description: Patch V.6.3: (keep panel open upon Rename) Implement 'Reorder Attachments' feature (rebased) → Patch V.6.3: (Thomas' V.6.2, rebased) Implement 'Reorder Attachments' feature
Attachment #8919861 - Attachment description: Patch V.6.3: (Thomas' V.6.2, rebased) Implement 'Reorder Attachments' feature → Patch V.6.3b: (Thomas' V.6.3, rebased) Implement 'Reorder Attachments' feature
(In reply to Thomas D. from comment #69)
> (In reply to Jorg K (GMT+2) from comment #31)
> > > +var $ = function(aId) {
> > 
> > What's that funky thing?

I'd avoid using this here too as it's not in line with the coding style, and would be out of place if the code is moved/refactored later at some point.
Flags: needinfo?(mkmelin+mozilla)
(In reply to Jorg K (GMT+2) from comment #68)
> Created attachment 8919861 [details] [diff] [review]
> I've rebased the patch (didn't apply due to bug 1404695).

Thanks for rebasing! :-)
Bug 1404695 landed 13 hours before my patch.

> I also restored
> two empty lines which were removed for no apparent reason

Interesting! So Jörg's first comment about this non-trivial patch in 6th iteration which implements a complete new feature is that there's "extra noise" from two removed blank lines... Must be a focus issue on Jörg's side; unfortunately hard to fix; I've tried in vain... :/

It would really be helpful Jörg if you could at least cursorily browse through the history of a bug so that you get the full picture. As explained above, there was an accident when my editor trimmed trailing spaces in a patch file, and as explained above, I've now configured Notepad++ to never do that again.
Then, again as explained before, I went through every blank line altered by that accident and fwiw, I deliberately decided to remove those two empty lines which you now restored.

> (which creates extra noise, like a pink left side in the review view).

Please add a smiley for such statements, otherwise people can take them serious... ;-)

> The one in front of |var sDictCount = 0| was surely desired.

Not sure what's special about |var sDictCount = 0| compared to preceding spell-related variables that it needs its own line?

> Please answer comment #31:
> > +var $ = function(aId) {
> What's that funky thing?
>
> Finally, Aceman is better at doing reviews like this one. I tried it briefly
> and it seemed to work :-)

Spät kommt der Smiley, doch er kommt...
It "seemed" to work or what you tried actually worked well?

Yeah, I would have appreciated Jörg's perfectionist review on this, but I'll gladly review this with Aceman, because I'll feel better when we start with a minimum of appreciation and then proceed to the real issues rather than starting from the "extra noise" of two pink blank lines...
Magnus, any ideas on my other question?

(In reply to Thomas D. from comment #63)
> (In reply to Thomas D. from comment #62)
> > What I'd like to know from Magnus is why my panel correctly stays open when
> > attachments are deleted, but goes away when just renaming one, and it's
> > still open when the renaming dialogue appears, but only closes when the
> > renaming gets executed. Except for the prompt itself, I don't see much
> > difference in code between renaming and removing. The panel is designed to
> > close only when focus exits from attachment pane, which does not seem to
> > happen when the rename prompt comes on screen, so why after that?
> 
> More annoyingly, the panel actually closes after renaming is totally done,
> as all of the following console.logs show that the panel is still open:
> > ...
> >     item.dispatchEvent(event);
> > console.log ('after-item.dispatchEvent panel.state=' + $("reorderAttachmentsPanel").state);
> > 
> >   }
> > }
> 
> So if the panel is still open at this point when renaming is totally done,
> what would cause it to close?

More specifically, what causes onBlur (loss of focus) event on attachmentBucket *after* the entire renaming function is done?
Both before and after renaming, and even when the renaming dialogue pops up, focus is always on the bucket.
Flags: needinfo?(mkmelin+mozilla)
(In reply to Thomas D. from comment #73)
> Not sure what's special about |var sDictCount = 0| compared to preceding
> spell-related variables that it needs its own line?
It separates the short-hand variables from something else:
var nsIMsgCompDeliverMode = Components.interfaces.nsIMsgCompDeliverMode;
(six more of these)
var nsISupportsString = Components.interfaces.nsISupportsString;
var mozISpellCheckingEngine = Components.interfaces.mozISpellCheckingEngine;

var sDictCount = 0;
(In reply to Magnus Melin from comment #72)
> (In reply to Thomas D. from comment #69)
> > (In reply to Jorg K (GMT+2) from comment #31)
> > > > +var $ = function(aId) {
> > > 
> > > What's that funky thing?
> 
> I'd avoid using this here too as it's not in line with the coding style, and
> would be out of place if the code is moved/refactored later at some point.

That is why I'm proposing to add it to the coding style in bug 1409963, with a number of good reasons.
Either way, it should be trivial to just use a simple string search and replace any occurrences.
So for a uniform coding style, we could just replace all occurences of document.getElementById() with $() in the entire codebase.

I don't see the problem even of accepting both ways. Whatever you do to document.getElementById() in your refactoring process, will automatically apply to $(), too. Then if you don't want two styles, just replace all occurences of the other style.
But let's move that discussion to bug 1409963.
Well we should aim for consistent code so it's not necessary to do string replacements when moving code (which make the diffs hard to read). 

As for your question, I don't know off hand and I doubt I time to investigate soon, sorry.
Flags: needinfo?(mkmelin+mozilla)
(In reply to Jorg K (GMT+2) from comment #75)
> (six more of these)
> var nsISupportsString = Components.interfaces.nsISupportsString;
> var mozISpellCheckingEngine = Components.interfaces.mozISpellCheckingEngine;
> 
> var sDictCount = 0;

Good point, I see.
(In reply to Thomas D. (currently busy elsewhere) from comment #44)
> I'd also expect a bit more welcoming reaction, not criticism when a
> volunteer (like you) provides a nicely polished feature after dozens of
> hours of work to eliminate bugs in the feature during production.

We can't write praise in every comment :) Please consider the fact you get fast replies and even css/svg work done to supplement your patch as the welcoming reaction.
 
> I'm surprised that we've come so low as to reject a truly shining small
> feature for the very fact that it's polished. Are you suggesting that we
> have buttons without icons so that we have 7 image files less to maintain?
> I take that as a joke...

Well, the trend seems to be to remove icons, there is a bug to remove them from the app menu, I think current Monterail theme also removes them from the main menu. Not that I agree with all the "simplification".
But here I'm also surprised this feature needs 6 new svg icons with symbols, that I would expect to already be somewhere in the tree. And this is a popup menu, not a button. You also do not see many icons in the message context menu.

(In reply to Thomas D. (currently busy elsewhere) from comment #73)
> Interesting! So Jörg's first comment about this non-trivial patch in 6th
> iteration which implements a complete new feature is that there's "extra
> noise" from two removed blank lines... Must be a focus issue on Jörg's side;
> unfortunately hard to fix; I've tried in vain... :/

> Yeah, I would have appreciated Jörg's perfectionist review on this, but I'll
> gladly review this with Aceman, because I'll feel better when we start with
> a minimum of appreciation and then proceed to the real issues rather than
> starting from the "extra noise" of two pink blank lines...

Jorg has chosen to not do the full review himself so he just picked up something small he noticed. It may improve your patch even when he doesn't read the whole patch to praise the new feature.

Please do not needlessly attack the people that still try to help you.
Back to bulky.
Attachment #8919861 - Attachment is obsolete: true
Attachment #8919861 - Flags: review?(acelists)
Attachment #8920325 - Flags: review?(acelists)
Comment on attachment 8920325 [details] [diff] [review]
Patch V.6.4: (- smart $(id), + document.getElementById(id)) Implement 'Reorder Attachments' feature

Review of attachment 8920325 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch. I played with the feature now. I consider this an interesting new feature as AFAIK Outlook doesn't have it (at least as of 2010), and dragging attachments there causes them to duplicate in the attachment list. So this looks like an unique feature (similar to the attachment notification bar) and thanks Thomas for working on it. Nice work with all the various ordering options this gives.

While playing in the attachment list, I noticed some pre-existing ugliness:
1. when you just click an attachment, its icon jumps around (it is probably hidden for a moment)
2. when the attachment has no icon (I tried a .class file), the name takes its space, so then the other attachment names (with icons) are not aligned
Maybe you want to look at these in the next bug, now that you have experience in the area:)

And here comes the lengthy review:)
I give r+ but I'd like to see the new version with the nits fixed.
And the last thing missing to perfection is a test:) But I'll do that for you.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +170,5 @@
> + * @return string  pretty, human-readable shortcut key string from the <key>
> + */
> +function getPrettyKey(aKeyId) {
> +  let aKeyEl = document.getElementById(aKeyId);
> +  return ShortcutUtils.prettifyShortcut(aKeyEl);

Won't this fit without the temp variable?

@@ +956,5 @@
> +      isEnabled: function() {
> +        if (attachmentsCount() == 0) {
> +          let reorderAttachmentsPanel =
> +            document.getElementById("reorderAttachmentsPanel");
> +          if (reorderAttachmentsPanel.state=="open") {

Spaces around == .

@@ +1400,5 @@
> +  goUpdateCommand("cmd_moveAttachmentBundleUp");
> +  goUpdateCommand("cmd_moveAttachmentBundleDown");
> +  goUpdateCommand("cmd_moveAttachmentTop");
> +  goUpdateCommand("cmd_moveAttachmentBottom");
> +  goUpdateCommand("cmd_sortAttachmentsToggle");

It seems this block of updates is repeated multiple times, maybe you could make a helper function for them.
But do you really need to update them so often? Is it not enough to update them when the menu is opened or when the attachment list is focused/clicked? Even the keys only work when inside the list.

@@ +2338,5 @@
>   * Called when number of attachments changes.
>   */
>  function AttachmentsChanged() {
>    manageAttachmentNotification(true);
> +  updateAttachmentItems();

Why here?

@@ +4464,5 @@
> + * Returns a sorted, "non-live" array of selected attachment list items.
> + *
> + * @param ascending  true (default): sort return array ascending
> + *                   false         : sort return array descending
> + * @return array     a sorted, "non-live" array of selected listitem elements in

I think the type if argument is denoted by braces, like
@return {array}

@@ +4467,5 @@
> + *                   false         : sort return array descending
> + * @return array     a sorted, "non-live" array of selected listitem elements in
> + *                   attachmentBucket listbox; [] if no attachments selected
> + */
> +function attachmentsSelectedItemsGetSortedArray(ascending = true)

aAscending

@@ +4479,5 @@
> +  let selItems = [...bucketList.selectedItems];
> +  selItems.sort(function(a, b) {
> +    return ascending ?
> +      bucketList.getIndexOfItem(a) - bucketList.getIndexOfItem(b)
> +    : bucketList.getIndexOfItem(b) - bucketList.getIndexOfItem(a);

How does this work? Why does it sort by the position in the list, not by the name of the file? Or is the description not accurate?

@@ +4499,5 @@
> + *                  false: The selected attachment items are NOT a coherent block
> + *                         (at the list edge if/as specified by 'where' parameter),
> + *                         or no attachments selected.
> + */
> +function attachmentsSelectionIsBlock(where)

aWhere, maybe aPosition would be nicer name.

@@ +4502,5 @@
> + */
> +function attachmentsSelectionIsBlock(where)
> +{
> +  let selectedCount = MessageGetNumSelectedAttachments();
> +  if (!selectedCount)

(selectedCount < 1)? Or what if count == 1? Is that a block?

@@ +4514,5 @@
> +    bucketList.getIndexOfItem(selItems[selectedCount - 1]);
> +  let isBlock = ((indexFirstSelAttachment) ==
> +                 (indexLastSelAttachment + 1 - selectedCount));
> +
> +    switch (where) {

Wrong indent.

@@ +4602,5 @@
>      child.attachment = null;
>    }
>  
>    if (removedAttachments.length > 0) {
> +

Added empty line.

@@ +4604,5 @@
>  
>    if (removedAttachments.length > 0) {
> +
> +    // Bug workaround: Force update of selectedCount and selectedItem.
> +    bucket.selectedIndex = -1;

That's bad that this happens. Can we fix it inside of "attachmentlist-base" binding? I see it inherits from "listbox-base". Does that one also have the problem? Or can we actually do this before removing the items? It seems setting .selectedIndex to -1 is documented as deselecting all items. So instead of a workaround, we can do it intentionally. You can also call .clearSelection().

@@ +4676,5 @@
>      }
>  
> +    // Bug workaround: Force update of selectedCount and selectedItem, both wrong
> +    // after item removal, to avoid confusion for listening command controllers.
> +    bucket.selectedIndex = -1;

The same.

@@ +4683,5 @@
> +    bucket.currentIndex = (focusIndex < bucket.itemCount) ?   // If possible,
> +                          focusIndex   // restore focus at original position;
> +                        : ( (bucket.itemCount > 0) ? // else: if attachments exist,
> +                            (bucket.itemCount - 1)   // focus last item;
> +                          : -1)                      // else: nothing to focus.

I'm not sure I like these vertical comments.

@@ +4741,5 @@
> + *                   "bundleDown": Move attachments together (downwards).
> + *                   "toggleSort": Sort attachments alphabetically (toggle).
> + *                   "showPanel" (default) : Show 'Reorder Attachments' Panel.
> + */
> +function moveSelectedAttachments(direction = "showPanel")

aDirection

@@ +4751,5 @@
> +
> +  // Ensure focus on bucket when we're coming from 'Reorder Attachments' panel.
> +  bucket.focus();
> +
> +  if (direction == "showPanel") {

Does this "command" need to be inside this function? It is not a movement operation.

@@ +4754,5 @@
> +
> +  if (direction == "showPanel") {
> +    // Open 'Reorder Attachments' panel (with focus on bucket). Done.
> +    document.getElementById("reorderAttachmentsPanel")
> +      .openPopup(bucket,"after_start", 15,0,true);

Spaces after commas.

@@ +4798,5 @@
> +                         checkItem
> +                         // Downwards: Insert block items *after* checkItem,
> +                         // i.e. *before* nextSibling.nextSibling of block,
> +                         // which works according to spec even if that's null.
> +                       : checkItem.nextSibling;

Ugh, the split expression :)

@@ +4806,5 @@
> +            }
> +          }
> +          // Else if checkItem doesn't exist, the block is already at the edge
> +          // of the list, so we can't move it in the intended direction.
> +          blockItems=[]; // Either way, we're done with the current block.

blockItems.length = 0 is apparently a better way to truncate an already existing array.

@@ +4824,5 @@
> +                       (bucket.itemCount - 1)
> +                     : (upwards ? bucket.getIndexOfItem(selItems[0].previousSibling)
> +                       : bucket.getIndexOfItem(selItems[selItems.length - 1].nextSibling)
> +                       )
> +                     );

Ugh...

@@ +4832,5 @@
> +    case "bottom":
> +    case "bundleUp":
> +    case "bundleDown":
> +      // Bundle selected attachments to top/bottom of the list or upwards/downwards.
> +

Intentional blank line?

@@ +4838,5 @@
> +      // Downwards: Reverse order of selItems so we can use the same algorithm.
> +      if (!upwards)
> +        selItems.reverse();
> +
> +      if (["top","bottom"].includes(direction)) {

Space after comma.

@@ +4919,5 @@
> +      } // next selItem
> +
> +      // Now let's sort our selItems according to sortDirection.
> +      selItems.sort(
> +          function(a, b) {

Bad indent?

@@ +4924,5 @@
> +            let name1 = a.attachment.name.toLocaleLowerCase();
> +            let name2 = b.attachment.name.toLocaleLowerCase();
> +            if (sortDirection == "ascending") {
> +              return (name1 < name2) ? -1
> +                  : ((name1 > name2) ? 1

We use to do a.attachment.name.toLowerCase().localeCompare(b.attachment.name.toLowerCase()) to do locale sorting. I'm not sure how toLocaleLowerCase() differs from toLowerCase() so you can keep it if you want, but do not use < and >, use .localeCompare().

@@ +4926,5 @@
> +            if (sortDirection == "ascending") {
> +              return (name1 < name2) ? -1
> +                  : ((name1 > name2) ? 1
> +                    : 0)
> +            } else { // sortDirection == "descending"

You can drop this branch, just call selItems.reverse() after the sort in case sortDirection == "descending".

@@ +4956,5 @@
> +  goUpdateCommand("cmd_moveAttachmentBundleUp");
> +  goUpdateCommand("cmd_moveAttachmentBundleDown");
> +  goUpdateCommand("cmd_moveAttachmentTop");
> +  goUpdateCommand("cmd_moveAttachmentBottom");
> +  goUpdateCommand("cmd_sortAttachmentsToggle");

The helper again.

@@ +4984,5 @@
> +
> +  // Check for ascending.
> +  ascending = selItems1.every(function(item, index) {
> +    return (item.attachment.name.toLocaleLowerCase() <
> +            selItems[index + 1].attachment.name.toLocaleLowerCase());

.localeCompare()

@@ +4994,5 @@
> +
> +  // If not ascending, maybe it's descending.
> +  descending = selItems1.every(function(item, index) {
> +    return (item.attachment.name.toLocaleLowerCase() >
> +            selItems[index + 1].attachment.name.toLocaleLowerCase());

.localeCompare()

@@ +5014,5 @@
> + * @param aDirection "ascending" : return true only if selected items are sorted
> + *                                 ascending.
> + *                   "descending": return true only if selected items are sorted
> + *                                 descending.
> + * @return boolean  true : if selected items are sorted ascending OR descending;

{boolean}

@@ +5024,5 @@
> +function attachmentsSelectionHasSortOrder(aDirection) {
> +  let sortOrder = attachmentsSelectionGetSortOrder();
> +
> +  if (!aDirection)
> +    return (sortOrder == "ascending" || sortOrder == "descending");

What does this do? The description does not mention the option to not specify aDirection.

@@ +5064,5 @@
> +function attachmentBucketOnBlur() {
> +  // Ensure that reorderAttachmentsPanel remains open while we're focused
> +  // on attachmentBucket or the panel, otherwise hide it.
> +  let attachmentBucket = document.getElementById("attachmentBucket");
> +  if (attachmentBucket.getAttribute("data-ignorenextblur")=="true") {

Spaces around == .

@@ +5077,5 @@
> +}
> +
> +function attachmentBucketOnKeyUp(aEvent) {
> +  // When ESC is pressed, close reorderAttachmentsPanel.
> +  if (aEvent.key=="Escape") {

Can you try aEvent.keyCode == aEvent.DOM_VK_ESCAPE ?

::: mail/components/compose/content/messengercompose.xul
@@ +344,5 @@
>  
>  <keyset id="baseMenuKeyset"/>
>  
>  <keyset id="editorKeys"/>
> +<panel id="reorderAttachmentsPanel"

The doorhanger panel is interesting UI which we don't use elsewhere. But it looks nice and I don't know where you would put the commands otherwise. Maybe in the normal attachment context menu. Did you intentionally separate the reorder commands out into a different menu/panel? Do the UI is uncommond, but if it passed UI review... :)

@@ +353,5 @@
> +       onpopupshowing="reorderAttachmentsPanelOnPopupShowing();"
> +       onpopupshown="reorderAttachmentsPanelOnPopupShown();"
> +       consumeoutsideclicks="false"
> +       noautohide="true">
> +  <description class="panelTitle">&reorderAttachmentsPanel.label;</description>

Maybe a separator line would be nice here.

@@ +360,5 @@
> +                 label="&moveAttachmentTopPanelBtn.label;"
> +                 accesskey="&moveAttachmentTopPanelBtn.accesskey;"
> +                 key="key_moveAttachmentTop"
> +                 command="cmd_moveAttachmentTop"/>
> +  <toolbarbutton id="btn_moveAttachmentUp"

Interesting that these are toolbarbuttons. The doorhanger panel is a strange beast.

@@ +1136,3 @@
>                            onselect="updateAttachmentItems();"
> +                          ondragstart="nsDragAndDrop.startDrag(event, attachmentBucketDNDObserver);"
> +                          onblur="attachmentBucketOnBlur();"/>

This works when clicking in recipients field or the msg body, but not when you click menus or context menu on the toolbars. Is that intentional or can we do something about it?

::: mail/locales/en-US/chrome/messenger/messengercompose/messengercompose.dtd
@@ +86,5 @@
>  <!ENTITY renameAttachmentCmd.label "Rename Attachment…">
>  <!ENTITY renameAttachmentCmd.accesskey "e">
> +<!ENTITY reorderAttachmentsCmd.label "Reorder Attachments…">
> +<!ENTITY reorderAttachmentsCmd.accesskey "s">
> +<!ENTITY reorderAttachmentsCmd.key "#">

Does this key work? I couldn't invoke it.

@@ +94,5 @@
> +<!ENTITY moveAttachmentUpPanelBtn.accesskey "U">
> +<!ENTITY moveAttachmentBundleUpPanelBtn.label "Move together">
> +<!ENTITY moveAttachmentBundleUpPanelBtn.accesskey "v">
> +<!ENTITY moveAttachmentDownPanelBtn.label "Move Down">
> +<!ENTITY moveAttachmentDownPanelBtn.accesskey "D">

How are the accesskeys invoked? I could't make them work when the reorder menu was open.

@@ +100,5 @@
> +<!ENTITY moveAttachmentBottomPanelBtn.accesskey "E">
> +<!ENTITY sortAttachmentsTogglePanelBtn.AZ.label "Sort Selection: A - Z">
> +<!ENTITY sortAttachmentsTogglePanelBtn.AZ.accesskey "o">
> +<!ENTITY sortAttachmentsTogglePanelBtn.ZA.label "Sort Selection: Z - A">
> +<!ENTITY sortAttachmentsTogglePanelBtn.ZA.accesskey "o">

This could use a comment that both accesskeys are intentionally the same.

::: mail/themes/linux/mail/compose/messengercompose.css
@@ +846,5 @@
> +#btn_sortAttachmentsToggle {
> +  list-style-image: url("chrome://messenger/skin/icons/sort.svg");
> +}
> +
> +#btn_sortAttachmentsToggle[data-sortdirection=descending] > .toolbarbutton-icon {

Doesn't this need to be ="descending" ? Other css rules use quotes.
Attachment #8920325 - Flags: review?(acelists) → review+
Can't believe that there's still so much to do... 8|

This patch addresses :aceman's review of comment 81.

I also fixed an error in the sorting algorithm, which so far was not handling {multiple attachments with same name} and {lowercase vs. uppercase} correctly, plus performance improvements and locale sensitivity for that algorithm.

The new cmd_moveAttachment* commands and the sort command, quite many, don't really belong to edit menu, so I relocated them to have their own separate space, which makes everything much more organized.

-----------------

@Richard, can you please test all MAC keyboard shortcuts found on the panel, and Cmd+# to open the Reorder Attachments Panel.
They should work with or without panel, when attachmentBucket has focus.

I added 2 which were missing on MAC:
Cmd+Alt+Arrow Left --> Bundle Up
Cmd+Alt+Arrow Right --> Bundle Down

I double-checked with official MAC keyboard shortcuts documentation and couldn't find any Problems; in fact, they make sense.
https://support.apple.com/en-us/HT201236

-----------------

(In reply to :aceman from comment #81)

Thanks a lot Aceman for this rapid and detailed review! :)

> Comment on attachment 8920325 [details] [diff] [review]
> Patch V.6.4: (- smart $(id), + document.getElementById(id)) Implement
> 'Reorder Attachments' feature
>
> Review of attachment 8920325 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> Thanks for the patch. I played with the feature now. I consider this an
> interesting new feature as AFAIK Outlook doesn't have it
> ... and thanks Thomas for working on it. Nice work
> with all the various ordering options this gives.

Thanks a lot for the considerate appreciation! :-))

> While playing in the attachment list, I noticed some pre-existing ugliness:
> 1. when you just click an attachment, its icon jumps around (it is probably
> hidden for a moment)

It doesn't for me on Windows, so I think this is specific to your OS.

> 2. when the attachment has no icon (I tried a .class file), the name takes
> its space, so then the other attachment names (with icons) are not aligned

Again, not on Windows.

> Maybe you want to look at these in the next bug, now that you have
> experience in the area:)

Given that I don't see them on Windows, I can't.

> And here comes the lengthy review:)
> I give r+ but I'd like to see the new version with the nits fixed.
> And the last thing missing to perfection is a test:) But I'll do that for
> you.

Thanks a lot. It's intelligent division of labour.

> @@ +1400,5 @@
> > +  goUpdateCommand("cmd_moveAttachmentBundleUp");
> > +  goUpdateCommand("cmd_moveAttachmentBundleDown");
> > +  goUpdateCommand("cmd_moveAttachmentTop");
> > +  goUpdateCommand("cmd_moveAttachmentBottom");
> > +  goUpdateCommand("cmd_sortAttachmentsToggle");
>
> It seems this block of updates is repeated multiple times, maybe you could
> make a helper function for them.
> But do you really need to update them so often? Is it not enough to update
> them when the menu is opened

Yes, indeed. Thanks.

> @@ +2338,5 @@
> >   * Called when number of attachments changes.
> >   */
> >  function AttachmentsChanged() {
> >    manageAttachmentNotification(true);
> > +  updateAttachmentItems();
>
> Why here?

When attachments get removed or added while the panel is open (which is conveniently possible, and it makes sense), it can change the absolute and relative position of selected attachments in the list, then we must update movement commands accordingly. Also, removing deselects, so delete command must also be updated, and all the rest of attachment commands. I fixed another existing bug here, as mentioned above.

> @@ +4464,5 @@
> > + * Returns a sorted, "non-live" array of selected attachment list items.
> > + *
> > + * @param ascending  true (default): sort return array ascending
> > + *                   false         : sort return array descending
> > + * @return array     a sorted, "non-live" array of selected listitem elements in
>
> I think the type if argument is denoted by braces, like
> @return {array}

Ok. Is there any documentation about this?

> @@ +4467,5 @@
> > + *                   false         : sort return array descending
> > + * @return array     a sorted, "non-live" array of selected listitem elements in
> > + *                   attachmentBucket listbox; [] if no attachments selected
> > + */
> > +function attachmentsSelectedItemsGetSortedArray(ascending = true)
>
> aAscending

Of course. (Is there any documentation about this?)

> @@ +4479,5 @@
> > +  let selItems = [...bucketList.selectedItems];
> > +  selItems.sort(function(a, b) {
> > +    return ascending ?
> > +      bucketList.getIndexOfItem(a) - bucketList.getIndexOfItem(b)
> > +    : bucketList.getIndexOfItem(b) - bucketList.getIndexOfItem(a);
>
> How does this work? Why does it sort by the position in the list, not by the
> name of the file? Or is the description not accurate?

Ask Jim ;-) ... I clarified function description. We need to sort selected items by their index because listBox.selectedItems doesn't do that for us; instead, e.g. whichever item you selected first will be the first in .selectedItems nodeList, which isn't helpful.
I'm not so sure about the "non-live" part, but I guess it could cause confusion if we're working on selectedItems and then their content changes whilst we're still on it.

> @@ +4499,5 @@
> > + *                  false: The selected attachment items are NOT a coherent block
> > + *                         (at the list edge if/as specified by 'where' parameter),
> > + *                         or no attachments selected.
> > + */
> > +function attachmentsSelectionIsBlock(where)
>
> aWhere, maybe aPosition would be nicer name.

True. I chose aListPosition.

> @@ +4502,5 @@
> > + */
> > +function attachmentsSelectionIsBlock(where)
> > +{
> > +  let selectedCount = MessageGetNumSelectedAttachments();
> > +  if (!selectedCount)
>
> (selectedCount < 1)? Or what if count == 1? Is that a block?

Yes, 1 item is a block (because there are no gaps between selected items), as defined in function comment. That's important for the purpose of this function.

> @@ +4604,5 @@
> >
> >    if (removedAttachments.length > 0) {
> > +
> > +    // Bug workaround: Force update of selectedCount and selectedItem.
> > +    bucket.selectedIndex = -1;
>
> That's bad that this happens. Can we fix it inside of "attachmentlist-base"
> binding? I see it inherits from "listbox-base". Does that one also have the
> problem?

Let's leave that to another bug.

> Or can we actually do this before removing the items?

No, there's no point in deselecting before we have acted on selection.

> It seems setting .selectedIndex to -1 is documented as deselecting all items.
> So instead of a workaround, we can do it intentionally.

Yes, when selected Items are removed, it should automatically deselect because there's no selection any more. Or when there's no selection any more, selectedCount and selectedItem and selectedItems should automatically update.

> You can also call
> .clearSelection().

Ok, why not. It's more readable, but otherwise it's the same ;)
Btw, there seems to be another bug that removeAllAttachments() never gets used.

> @@ +4683,5 @@
> > +    bucket.currentIndex = (focusIndex < bucket.itemCount) ?   // If possible,
> > +                          focusIndex   // restore focus at original position;
> > +                        : ( (bucket.itemCount > 0) ? // else: if attachments exist,
> > +                            (bucket.itemCount - 1)   // focus last item;
> > +                          : -1)                      // else: nothing to focus.
>
> I'm not sure I like these vertical comments.

I do! I think it's much more readable compared to splitting up the nested ternary operators with inline comments. Both operators and comments are nicely readable as it is.

> @@ +4751,5 @@
> > +
> > +  // Ensure focus on bucket when we're coming from 'Reorder Attachments' panel.
> > +  bucket.focus();
> > +
> > +  if (direction == "showPanel") {
>
> Does this "command" need to be inside this function? It is not a movement
> operation.

I was asking myself the same question and decided for yes. Reason: I wanted to allow moveSelectedAttachments() without aDirection parameter. If no direction is specified, we show the panel to ask for direction. So then it was just less code and keeping everything in one place. Maybe it works here?

> @@ +4798,5 @@
> > +                         checkItem
> > +                         // Downwards: Insert block items *after* checkItem,
> > +                         // i.e. *before* nextSibling.nextSibling of block,
> > +                         // which works according to spec even if that's null.
> > +                       : checkItem.nextSibling;
>
> Ugh, the split expression :)

I'm not sure if I understand you here, is it about comments splitting ternary operator? See, told ya, it's better to keep comments at the side... ;)
But I guess it's ok like this...

> @@ +4806,5 @@
> > +            }
> > +          }
> > +          // Else if checkItem doesn't exist, the block is already at the edge
> > +          // of the list, so we can't move it in the intended direction.
> > +          blockItems=[]; // Either way, we're done with the current block.
>
> blockItems.length = 0 is apparently a better way to truncate an already
> existing array.

Twice as fast, indeed (http://jsben.ch/7QyI1)! Freaky...

> @@ +4824,5 @@
> > +                       (bucket.itemCount - 1)
> > +                     : (upwards ? bucket.getIndexOfItem(selItems[0].previousSibling)
> > +                       : bucket.getIndexOfItem(selItems[selItems.length - 1].nextSibling)
> > +                       )
> > +                     );
>
> Ugh...

*lol*... Do you want comments with that? :p
It's not worth using full-blown if structures, which would bloat this a lot, and it just works :) But I understand your reaction :))

> @@ +4832,5 @@
> > +    case "bottom":
> > +    case "bundleUp":
> > +    case "bundleDown":
> > +      // Bundle selected attachments to top/bottom of the list or upwards/downwards.
> > +
>
> Intentional blank line?

Yes, because it describes the switch block, not the following line.

> @@ +4919,5 @@
> > +      } // next selItem
> > +
> > +      // Now let's sort our selItems according to sortDirection.
> > +      selItems.sort(
> > +          function(a, b) {
>
> Bad indent?

Yeah.

> @@ +4924,5 @@
> > +            let name1 = a.attachment.name.toLocaleLowerCase();
> > +            let name2 = b.attachment.name.toLocaleLowerCase();
> > +            if (sortDirection == "ascending") {
> > +              return (name1 < name2) ? -1
> > +                  : ((name1 > name2) ? 1
>
> We use to do
> a.attachment.name.toLowerCase().localeCompare(b.attachment.name.
> toLowerCase()) to do locale sorting. I'm not sure how toLocaleLowerCase()
> differs from toLowerCase() so you can keep it if you want, but do not use <
> and >, use .localeCompare().

Nice!!! Better still, with localeCompare(), toLocaleLowerCase isn't even needed any more, because localeCompare() takes care of everything, and in fact sorts lowercase and uppercase very correctly by default (after respective lowercase items in my case, so it's a1 < A1 < b1 < B1). I only used it before since it was trying to push *all* uppercase items first (not mixed with lowercase brothers), but that behaviour isn't seen with localeCompare().

> @@ +4926,5 @@
> > +            if (sortDirection == "ascending") {
> > +              return (name1 < name2) ? -1
> > +                  : ((name1 > name2) ? 1
> > +                    : 0)
> > +            } else { // sortDirection == "descending"
>
> You can drop this branch, just call selItems.reverse() after the sort in
> case sortDirection == "descending".

In terms of performance, sorting them correctly immediately (and only once) should be faster than sorting them wrongly and then reversing the entire array?

> @@ +4984,5 @@
> > +
> > +  // Check for ascending.
> > +  ascending = selItems1.every(function(item, index) {
> > +    return (item.attachment.name.toLocaleLowerCase() <
> > +            selItems[index + 1].attachment.name.toLocaleLowerCase());
>
> .localeCompare()

I was doubting this at first, because I needed a boolean return value so it seemed absurd to let localeCompare convert it into a number and then convert the number into boolean again, but then I realized that localeCompare does all the nice locale sorting more perfect than any simple string comparison operator ever could...

I'm happy that you made me revisit this function, because that's how I discovered that I had skipped "equivalent" sort order for multiple items with same name, so I reworked things accordingly (e.g. when you select multiple items with exactly the same name, sorting gets disabled).
In fact, this function was returning wrong results for equivalent pairs within ascending or descending order (fixed).
Then I tried to make the ternary variant of the function more performant by using a different logic with some() instead of every(), so we don't iterate the whole array multiple times when it's not needed. Now it's more or less down to just 1 full iteration, except for selections with many equivalently-named items, which is unlikely.

> @@ +4994,5 @@
> > +
> > +  // If not ascending, maybe it's descending.
> > +  descending = selItems1.every(function(item, index) {
> > +    return (item.attachment.name.toLocaleLowerCase() >
> > +            selItems[index + 1].attachment.name.toLocaleLowerCase());
>
> .localeCompare()
>
> @@ +5024,5 @@
> > +function attachmentsSelectionHasSortOrder(aDirection) {
> > +  let sortOrder = attachmentsSelectionGetSortOrder();
> > +
> > +  if (!aDirection)
> > +    return (sortOrder == "ascending" || sortOrder == "descending");
>
> What does this do? The description does not mention the option to not
> specify aDirection.

Well it did, sort of. Adjusted for even more clarity.
And I didn't think of equivalent case for both sort functions... now included, and handled correctly by command handler of cmd_sortAttachmentsToggle.
Btw, this function is not needed by my code, because it gets the selection sort order each time it is called, which isn't good for multiple calls.
Include this function anyway for future use?

> @@ +5077,5 @@
> > +}
> > +
> > +function attachmentBucketOnKeyUp(aEvent) {
> > +  // When ESC is pressed, close reorderAttachmentsPanel.
> > +  if (aEvent.key=="Escape") {
>
> Can you try aEvent.keyCode == aEvent.DOM_VK_ESCAPE ?

Why complicate things? keyCode is deprecated, platform-dependent, and harder to read. Please refer to bug 998312 comment 7 ff. where we've discussed this already.

https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/key
> If the pressed key is a control or special character, the returned value
> is one of the pre-defined key values.

https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/key/Key_Values
key == "Escape" is the official, platform-neutral equivalent of DOM_VK_ESCAPE.
This is NOT a locale-dependent string representation of Escape key.
And it's just so much more readable in code.

> ::: mail/components/compose/content/messengercompose.xul
> @@ +344,5 @@
> >
> >  <keyset id="baseMenuKeyset"/>
> >
> >  <keyset id="editorKeys"/>
> > +<panel id="reorderAttachmentsPanel"
>
> The doorhanger panel is interesting UI which we don't use elsewhere. But it
> looks nice and I don't know where you would put the commands otherwise.
> Maybe in the normal attachment context menu. Did you intentionally separate
> the reorder commands out into a different menu/panel?

Yes. We can't put all of the movement commands into top-level context menu, because it would violate ux-minimalism.
A submenu of context-menu would end up in strange places, we're already at the edge of the screen.
Plus context menus are the wrong type of control, because expected and technical behaviour is that they close after each click, which is not what we want here.

Panel is exactly the right type of UI here:
- Panels may remain open after certain user actions (cf. FF bookmark panel)
- Panel is a light-weight control in terms of looks
- Panel is around when needed, and disappears automatically when you're done.
- No permanent clutter in any other attachment-related UI.

> So the UI is uncommon, but if it passed UI review... :)

I totally love it because it's refreshingly different from a traditional, heavy
contextual menu.

> @@ +353,5 @@
> > +       onpopupshowing="reorderAttachmentsPanelOnPopupShowing();"
> > +       onpopupshown="reorderAttachmentsPanelOnPopupShown();"
> > +       consumeoutsideclicks="false"
> > +       noautohide="true">
> > +  <description class="panelTitle">&reorderAttachmentsPanel.label;</description>
>
> Maybe a separator line would be nice here.

I tried it and I don't like it, because it makes the entire panel look more heavy.
I think Richard will agree because it was him who removed all of my separators in this panel, and rightly so. Similar panels like FF bookmark panel do not use separators to separate the caption. Even windows title bars are no longer separated from their application content area... Less is more...

> @@ +360,5 @@
> > +                 label="&moveAttachmentTopPanelBtn.label;"
> > +                 accesskey="&moveAttachmentTopPanelBtn.accesskey;"
> > +                 key="key_moveAttachmentTop"
> > +                 command="cmd_moveAttachmentTop"/>
> > +  <toolbarbutton id="btn_moveAttachmentUp"
>
> Interesting that these are toolbarbuttons. The doorhanger panel is a strange
> beast.

It is indeed. I tried in vain to prevent the default event in onpopuphiding, which looks like a bug. I also tried using other controls, but somehow it didn't work out. If you put menuitems, they are dead (no hover effects), even though we actually specify upon opening that this panel is a context menu.
So panel is claimed to be a special type of popup, but it doesn't behave like a popup should.

> @@ +1136,3 @@
> >                            onselect="updateAttachmentItems();"
> > +                          ondragstart="nsDragAndDrop.startDrag(event, attachmentBucketDNDObserver);"
> > +                          onblur="attachmentBucketOnBlur();"/>
>
> This works when clicking in recipients field or the msg body, but not when
> you click menus or context menu on the toolbars. Is that intentional or can
> we do something about it?

It's not intentional, but I can't do anything about it, because it's an existing bug, as explained in my Comment 59. Due to that bug, visually and effectively we have double-focus in this situation, because if you press ESC, both the menu AND the panel will close. The panel will close as soon as attachmentBucket receives a blur event, which should happen whenever it loses focus.

> ::: mail/locales/en-US/chrome/messenger/messengercompose/messengercompose.dtd
> @@ +86,5 @@
> >  <!ENTITY renameAttachmentCmd.label "Rename Attachment…">
> >  <!ENTITY renameAttachmentCmd.accesskey "e">
> > +<!ENTITY reorderAttachmentsCmd.label "Reorder Attachments…">
> > +<!ENTITY reorderAttachmentsCmd.accesskey "s">
> > +<!ENTITY reorderAttachmentsCmd.key "#">
>
> Does this key work? I couldn't invoke it.

Wfm on Windows. Check key_reorderAttachments, it's ALT+# on Windows/Linux, and CMD+# on MAC.

> @@ +94,5 @@
> > +<!ENTITY moveAttachmentUpPanelBtn.accesskey "U">
> > +<!ENTITY moveAttachmentBundleUpPanelBtn.label "Move together">
> > +<!ENTITY moveAttachmentBundleUpPanelBtn.accesskey "v">
> > +<!ENTITY moveAttachmentDownPanelBtn.label "Move Down">
> > +<!ENTITY moveAttachmentDownPanelBtn.accesskey "D">
>
> How are the accesskeys invoked? I could't make them work when the reorder
> menu was open.

You need to use ALT+Accesskey (on Windows at least), because there's no focus on the panel (and maybe also because these aren't menuitems), and I don't think it makes much sense to operate the panel using access keys whilst there are proper shortcut keys which are much more generic and intuitive. That's also why I didn't bother to allow keyboard focus on the buttons.

Phew!!!
Attachment #8920325 - Attachment is obsolete: true
Attachment #8920633 - Flags: ui-review?(richard.marti)
Attachment #8920633 - Flags: review?(acelists)
I'm not sure about this:

MsgComposeCommands.js
5050 updateReorderAttachmentsItems();

I wanted to avoid updating buttons when they are already shown in the panel, so I moved that from onpopupshown into onpopupshowing, and it seems to work for now.
But I'm not sure where the focus really is/should be whilst the popup is showing, and command handlers are bound to attachmentBucket, not in their enabled conditions, but because they are part of AttachmentBucket controller.
So maybe if we fix the focus issues of bucket, it might fail.
So maybe it's safer in the old position (onpopupshown) where we focus bucket first, and then do command updates, which can never go wrong but might cause a bit of flickering on the panel.
Comment on attachment 8920633 [details] [diff] [review]
Patch V.7: (NITFIXES from review, improved cmd_sortAttachmentsToggle) Implement 'Reorder Attachments' feature

(In reply to Thomas D. (currently busy elsewhere) from comment #82)
> Created attachment 8920633 [details] [diff] [review]
> Patch V.7: (NITFIXES from review, improved cmd_sortAttachmentsToggle)
> Implement 'Reorder Attachments' feature
> 
> @Richard, can you please test all MAC keyboard shortcuts found on the panel,
> and Cmd+# to open the Reorder Attachments Panel.

This works on Mac. With Swiss keyboard it's a bit special as the '#' is not directly accessible. It's the third level on key '3'. On Windows I need the 'Alt Gr' to type '#' and this doesn't work together with 'Ctrl'. On Mac it works with 'CMD + Option + 3'. On Windows it's not such a problem as I can press the 'context menu' key and then select the 'Reorder Attachments' menu item.

> They should work with or without panel, when attachmentBucket has focus.

There is no clear indication that the attachment bucked is focused. I'm thinking about filing a bug to improve this.

> I added 2 which were missing on MAC:
> Cmd+Alt+Arrow Left --> Bundle Up
> Cmd+Alt+Arrow Right --> Bundle Down

Work on Mac.
Attachment #8920633 - Flags: ui-review?(richard.marti) → ui-review+
(In reply to Richard Marti (:Paenglab) from comment #84)
> On Windows I need the
> 'Alt Gr' to type '#' and this doesn't work together with 'Ctrl'.
With 'Ctrl'?

Alt (+Shift) + # works fine on Windows with an en-US keyboard (where the # is above the 3, hence the Shift).
(In reply to Jorg K (GMT+2) from comment #85)
> (In reply to Richard Marti (:Paenglab) from comment #84)
> > On Windows I need the
> > 'Alt Gr' to type '#' and this doesn't work together with 'Ctrl'.
> With 'Ctrl'?

Meant 'Alt'. Mixed after using such different key names between Mac and Windows.

> Alt (+Shift) + # works fine on Windows with an en-US keyboard (where the #
> is above the 3, hence the Shift).

'Alt' + 'Alt Gr' is a weird combination, not?
"Alt Gr" on Windows can be emulated by pressing "Alt" and "Ctrl" (try it!) - https://en.wikipedia.org/wiki/AltGr_key#Control_.2B_Alt_as_a_substitute, hence it can't be combined with "Alt" or "Ctrl".

But if you use a keyboard with only up to four variations per key (en-US) instead of six (de and may others), then it's all working. The en-US keyboard doesn't even have a specially labelled "Alt Gr" key, but the right "Alt" key has that function.

(I know these things since I use the Microsoft Keyboard Layout Creator to create custom keyboards.)
Yes, "Alt" and "Ctrl" does the same. But it doesn't help in this case here.

As I wrote above, I see this not as a problem as it's still doable by pressing the 'context menu' key and then pressing 's'.
(In reply to Richard Marti (:Paenglab) from comment #84)
> Comment on attachment 8920633 [details] [diff] [review]
> 
> This works on Mac. With Swiss keyboard it's a bit special as the '#' is not
> directly accessible. It's the third level on key '3'. On Windows I need the
> 'Alt Gr' to type '#' and this doesn't work together with 'Ctrl'.

Which looks like a general design error of keyboards that just because it happens to be a third-level key, it can't be used for certain keyboard shortcuts. It should have been very easy to define a certain sequence which prevents that problem, e.g.:
for Str+Alt+3rd-level key:
Str+Alt+ (Alt Gr [optional kurze Eingabepause] 3rd-level key)
Left Alt and right Alt are not technically the same keys, so it would be possible to treat their combination differently.
Anyway, I guess there's nothing we can do here because there's a lot of keys which we can't take.

> On Mac it
> works with 'CMD + Option + 3'. On Windows it's not such a problem as I can
> press the 'context menu' key and then select the 'Reorder Attachments' menu
> item.

There's also Edit > Reorder Attachments from main menu.

> > They should work with or without panel, when attachmentBucket has focus.
> 
> There is no clear indication that the attachment bucked is focused. I'm
> thinking about filing a bug to improve this.

There's a clear focus indication; but because it's matching the OS focus/selection patterns on Windows, it's not very strong. The focus indication is a blue wireframe around the focused item.
Focused and selected item get's blue background, which is also unique in the entire window, so a clear indication of focus.
It becomes a bit less clear with multiple selections, when multiple items get blue background.
But the background of selected item is slightly darker and has 4-sides-border, which normally isn't present on other selected items (see contacts side bar).

The real bug in attachment pane is the selection pattern of selected, but unfocused items when focus is still on another item: Selected but unfocused items should NOT have full border, and there shouldn't be small horizontal space between selected items. Please compare contacts side bar selection with attachments selection to see the difference. Contacts side bar does the right thing, and since it's OS-consistent, I guess that must suffice. Maybe we could make the border of a selected item 2px so that the border is slightly more emphasized?
 
> > I added 2 which were missing on MAC:
> > Cmd+Alt+Arrow Left --> Bundle Up
> > Cmd+Alt+Arrow Right --> Bundle Down
> 
> Work on Mac.

Great!
(In reply to Thomas D. (currently busy elsewhere) from comment #89)

> But the background of selected item is slightly darker and has
> 4-sides-border, which normally isn't present on other selected items (see
> contacts side bar).

typo: the background of *focused* item
Comment on attachment 8920633 [details] [diff] [review]
Patch V.7: (NITFIXES from review, improved cmd_sortAttachmentsToggle) Implement 'Reorder Attachments' feature

Review of attachment 8920633 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, this is getting better :)

::: mail/components/compose/content/MsgComposeCommands.js
@@ +1414,5 @@
>    goUpdateCommand("cmd_selectAll");
>    goUpdateCommand("cmd_openAttachment");
>  }
>  
> +function updateReorderAttachmentsItems() {

Is there an easy way to check if the panel is open and if not, just exit this function quickly?

@@ +4471,5 @@
> +    return [];
> +
> +  let bucketList = document.getElementById("attachmentBucket");
> +  // bucketList.selectedItems is a "live" and unsorted node list, but we want a
> +  // "dead" and sorted array of nodes, so massage things a bit.

So please add that we just sort by index in the list. It is not clear in what way .selectedItems is unsorted. You said it returns in the order of selection, which may not be from top to bottom.

@@ +4489,5 @@
> + * @param aListPosition (optional)  "top"   : Return true only if the block is
> + *                                            at the top of the list.
> + *                                  "bottom": Return true only if the block is
> + *                                            at the bottom of the list.
> + * @return boolean  true : The selected attachment items are a coherent block

{boolean}

@@ +4723,5 @@
> +
> +  let reorderAttachmentsPanel = document.getElementById("reorderAttachmentsPanel");
> +  let attachmentBucket = document.getElementById("attachmentBucket");
> +  if (reorderAttachmentsPanel.state == "open") {
> +    // Hack to ensure that reorderAttachmentsPanel does not get closed as we exit

Needs trailing dot.

@@ +4753,5 @@
> +
> +  if (aDirection == "showPanel") {
> +    // Open 'Reorder Attachments' panel (with focus on bucket). Done.
> +    document.getElementById("reorderAttachmentsPanel")
> +      .openPopup(bucket, "after_start", 15, 0, true);

I think this could be indented below the dot on the previous line.

@@ +4850,5 @@
> +                       // Upwards: Insert before first list item.
> +                       listEdgeItem
> +                       // Downwards: Insert after last list item, i.e.
> +                       // *before* non-existing listEdgeItem.nextSibling,
> +                       // which is null. It works because it's a feature.

Is listEdgeItem.nextSibling always null? Then why not use null directly? If it isn't the comment isn't very clear.

@@ +4951,5 @@
> + * Returns a string representing the current sort order of selected attachment
> + * items by their names. We don't check if selected items form a coherent block
> + * or not; use attachmentsSelectionIsBlock() to check on that.
> + *
> + * @return string "ascending" : Sort order is ascending.

{string}

@@ +4993,5 @@
> +    return "descending";
> +
> +  // No ascending pairs, no descending pairs, so all equivalent in sort order.
> +  // if (!someAscending && !someDescending)
> +  return "equivalent"

Missing semicolon.

@@ +5070,5 @@
> +}
> +
> +function attachmentBucketOnKeyUp(aEvent) {
> +  // When ESC is pressed, close reorderAttachmentsPanel.
> +  if (aEvent.key=="Escape") {

spaces around == .
Attachment #8920633 - Flags: review?(acelists) → review+
(In reply to Thomas D. (currently busy elsewhere) from comment #82)
> > I think the type if argument is denoted by braces, like
> > @return {array}
> 
> Ok. Is there any documentation about this?

I don't know, but you find examples of it in the tree.
 
> > @@ +4467,5 @@
> > > + *                   false         : sort return array descending
> > > + * @return array     a sorted, "non-live" array of selected listitem elements in
> > > + *                   attachmentBucket listbox; [] if no attachments selected
> > > + */
> > > +function attachmentsSelectedItemsGetSortedArray(ascending = true)
> >
> > aAscending
> 
> Of course. (Is there any documentation about this?)

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style

> > @@ +4479,5 @@
> > > +  let selItems = [...bucketList.selectedItems];
> > > +  selItems.sort(function(a, b) {
> > > +    return ascending ?
> > > +      bucketList.getIndexOfItem(a) - bucketList.getIndexOfItem(b)
> > > +    : bucketList.getIndexOfItem(b) - bucketList.getIndexOfItem(a);
> >
> > How does this work? Why does it sort by the position in the list, not by the
> > name of the file? Or is the description not accurate?
> 
> Ask Jim ;-) ... I clarified function description. We need to sort selected
> items by their index because listBox.selectedItems doesn't do that for us;
> instead, e.g. whichever item you selected first will be the first in
> .selectedItems nodeList, which isn't helpful.
> I'm not so sure about the "non-live" part, but I guess it could cause
> confusion if we're working on selectedItems and then their content changes
> whilst we're still on it.

Then you can't even call bucketList.getIndexOfItem as part of the sort. Store the items and their indexes in an array like [ {index, item}, ... ] and then .sort(function(a,b) { return a.index - b.index; })

> > @@ +4751,5 @@
> > > +
> > > +  // Ensure focus on bucket when we're coming from 'Reorder Attachments' panel.
> > > +  bucket.focus();
> > > +
> > > +  if (direction == "showPanel") {
> >
> > Does this "command" need to be inside this function? It is not a movement
> > operation.
> 
> I was asking myself the same question and decided for yes. Reason: I wanted
> to allow moveSelectedAttachments() without aDirection parameter. If no
> direction is specified, we show the panel to ask for direction. So then it
> was just less code and keeping everything in one place. Maybe it works here?

Why do you want to call it without arguments? You never do it. If some JS code calls this with an argument, the function does its thing. If you call it without arguments it does nothing just opens the panel. That seems like a side-effect that differs from the flow of all the other arguments.
 
> > @@ +4924,5 @@
> Nice!!! Better still, with localeCompare(), toLocaleLowerCase isn't even
> needed any more, because localeCompare() takes care of everything, and in
> fact sorts lowercase and uppercase very correctly by default (after
> respective lowercase items in my case, so it's a1 < A1 < b1 < B1). I only
> used it before since it was trying to push *all* uppercase items first (not
> mixed with lowercase brothers), but that behaviour isn't seen with
> localeCompare().

It seems to me with localeCompare you still get "a" < "A", to all lowercase "a"s get sorted first. This may not be wanted therefore we still use .toLowerCase() on the compared values.

> > @@ +4926,5 @@
> > > +            if (sortDirection == "ascending") {
> > > +              return (name1 < name2) ? -1
> > > +                  : ((name1 > name2) ? 1
> > > +                    : 0)
> > > +            } else { // sortDirection == "descending"
> >
> > You can drop this branch, just call selItems.reverse() after the sort in
> > case sortDirection == "descending".
> 
> In terms of performance, sorting them correctly immediately (and only once)
> should be faster than sorting them wrongly and then reversing the entire
> array?

Yes, but in my case you would save the 'sortDirection == "ascending"' check for each comparison.
But how many attachments do we realistically talk here? Maybe the speed does not matter much and we should aim for readability.
Can you benchmark it with e.g. 100 names? But the function is much more readable now thanks to the localeCompare, even with the ternary operator.
 
> > @@ +5024,5 @@
> > > +function attachmentsSelectionHasSortOrder(aDirection) {
> Btw, this function is not needed by my code, because it gets the selection
> sort order each time it is called, which isn't good for multiple calls.
> Include this function anyway for future use?

We don't usually keep unneeded code. What could be the future use?
 
> > @@ +5077,5 @@
> https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/key
> > If the pressed key is a control or special character, the returned value
> > is one of the pre-defined key values.

OK, I didn't know that. 

> > ::: mail/locales/en-US/chrome/messenger/messengercompose/messengercompose.dtd
> > @@ +86,5 @@
> > >  <!ENTITY renameAttachmentCmd.label "Rename Attachment…">
> > >  <!ENTITY renameAttachmentCmd.accesskey "e">
> > > +<!ENTITY reorderAttachmentsCmd.label "Reorder Attachments…">
> > > +<!ENTITY reorderAttachmentsCmd.accesskey "s">
> > > +<!ENTITY reorderAttachmentsCmd.key "#">
> >
> > Does this key work? I couldn't invoke it.
> 
> Wfm on Windows. Check key_reorderAttachments, it's ALT+# on Windows/Linux,
> and CMD+# on MAC.

Still doesn't work for me on Linux. I assume I need to be focused in the attachment list. But nothing happens. Also when the attachment context menu is open, pressing Alt closes it.
(In reply to :aceman from comment #91)
> Comment on attachment 8920633 [details] [diff] [review]
> Patch V.7: (NITFIXES from review, improved cmd_sortAttachmentsToggle)
> Implement 'Reorder Attachments' feature
> 
> Review of attachment 8920633 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks, this is getting better :)

Thanks a lot for motivating/appreciative communication style, and for fast review :)

> ::: mail/components/compose/content/MsgComposeCommands.js
> @@ +1414,5 @@
> >    goUpdateCommand("cmd_selectAll");
> >    goUpdateCommand("cmd_openAttachment");
> >  }
> >  
> > +function updateReorderAttachmentsItems() {
> 
> Is there an easy way to check if the panel is open and if not, just exit
> this function quickly?

No, there's a misunderstanding here. We must update these commands even when the panel is not open, because the keyboard shortcuts like Alt+Cursor down must work with selected and focused list items even when the panel is NOT open.

> @@ +4471,5 @@
> > +    return [];
> > +
> > +  let bucketList = document.getElementById("attachmentBucket");
> > +  // bucketList.selectedItems is a "live" and unsorted node list, but we want a
> > +  // "dead" and sorted array of nodes, so massage things a bit.
>
> So please add that we just sort by index in the list. It is not clear in
> what way .selectedItems is unsorted. You said it returns in the order of
> selection, which may not be from top to bottom.

OK. The main point is really that bucket.selectedItems are not sorted by their index in the list, but by the "random" order in which they were added to selection. As we create our own, flat array from selectedItems, that means it's no longer live and won't be updated when selection changes. Which is ok because we just play our tricks on the current selection and then release it, and I think it's all but impossible to change the selection in that split second of our operation, and also technically I don't see how that could ever happen at all.

> @@ +4723,5 @@
> > +
> > +  let reorderAttachmentsPanel = document.getElementById("reorderAttachmentsPanel");
> > +  let attachmentBucket = document.getElementById("attachmentBucket");
> > +  if (reorderAttachmentsPanel.state == "open") {
> > +    // Hack to ensure that reorderAttachmentsPanel does not get closed as we exit
> 
> Needs trailing dot.

It's not a full sentence, just a single noun (hack) with some attributes (who happen to include sub clauses), why should it have a dot/full stop?
In fact as it's not a sentence, it shouldn't start with a capital letter either...
In most places where we put a full stop after comments it's grammatically wrong, but we're just in the erroneous habit to expect initial capitals and full stops everywhere...

> @@ +4850,5 @@
> > +                       // Upwards: Insert before first list item.
> > +                       listEdgeItem
> > +                       // Downwards: Insert after last list item, i.e.
> > +                       // *before* non-existing listEdgeItem.nextSibling,
> > +                       // which is null. It works because it's a feature.
> 
> Is listEdgeItem.nextSibling always null? Then why not use null directly? If
> it isn't the comment isn't very clear.

Yes, it'll be a bit faster to just use null directly, thanks.

(In reply to :aceman from comment #92)
> > > @@ +4479,5 @@
> > > > +  let selItems = [...bucketList.selectedItems];
> > > > +  selItems.sort(function(a, b) {
> > I'm not so sure about the "non-live" part, but I guess it could cause
> > confusion if we're working on selectedItems and then their content changes
> > whilst we're still on it.
> 
> Then you can't even call bucketList.getIndexOfItem as part of the sort.
> Store the items and their indexes in an array like [ {index, item}, ... ]
> and then .sort(function(a,b) { return a.index - b.index; })

See above, "non-live" is just an inconsequential consequence of converting into a javascript array, but there's nothing which could change the content in that split second we're on it, and in that case, we would still fail even if we totally flatten the whole thing because we still depend on live items as targets for re-arranging, re-sorting etc.

> > > @@ +4751,5 @@
> > > > +
> > > > +  // Ensure focus on bucket when we're coming from 'Reorder Attachments' panel.
> > > > +  bucket.focus();
> > > > +
> > > > +  if (direction == "showPanel") {
> > >
> > > Does this "command" need to be inside this function? It is not a movement
> > > operation.
> > 
> > I was asking myself the same question and decided for yes. Reason: I wanted
> > to allow moveSelectedAttachments() without aDirection parameter. If no
> > direction is specified, we show the panel to ask for direction. So then it
> > was just less code and keeping everything in one place. Maybe it works here?
> 
> Why do you want to call it without arguments? You never do it. If some JS
> code calls this with an argument, the function does its thing. If you call
> it without arguments it does nothing just opens the panel. That seems like a
> side-effect that differs from the flow of all the other arguments.

Point taken, good point. Neater now!

> > > @@ +4924,5 @@
> > Nice!!! Better still, with localeCompare(), toLocaleLowerCase isn't even
> > needed any more, because localeCompare() takes care of everything, and in
> > fact sorts lowercase and uppercase very correctly by default (after
> > respective lowercase items in my case, so it's a1 < A1 < b1 < B1). I only
> > used it before since it was trying to push *all* uppercase items first (not
> > mixed with lowercase brothers), but that behaviour isn't seen with
> > localeCompare().
> 
> It seems to me with localeCompare you still get "a" < "A", to all lowercase
> "a"s get sorted first. This may not be wanted therefore we still use
> .toLowerCase() on the compared values.

Imagine following selItems:

a.txt
A.txt
a.txt* 
A.txt

Then you click on "Sort selection: A-Z". I think the natural expectation is this:
a.txt
a.txt
A.txt
A.txt

That's definitely wanted, and I cannot imagine any reason why this should NOT be wanted.
Sorting is about putting same things together, and not-same things in hierarchical order.
Plus there are parameters for localecompare() function where locales or addons can finetune the behaviour wrt. lowercase.
I think we're just using toLowerCase() erroneously because we *had* to use it *before* with simple comparison operators when localeCompare did not exist, which would then sort *any* lowercase items before *any* uppercase items without consideration of alphabetical order, which was obviously unwanted (because a.txt and A.txt could end up at totally different places in the list).

*: Yes, it's possible to attach several files with same file name, same/different capitalization, from different directories, or by way of renaming, or at different stages of composition - the latter being especially weird and unpredictable/inconsistent, bug on record wrt immediate snapshot vs. pointer to original file and mixtures thereof.

> > > @@ +4926,5 @@
> > > > +            if (sortDirection == "ascending") {
> > > > +              return (name1 < name2) ? -1
> > > > +                  : ((name1 > name2) ? 1
> > > > +                    : 0)
> > > > +            } else { // sortDirection == "descending"
> > >
> > > You can drop this branch, just call selItems.reverse() after the sort in
> > > case sortDirection == "descending".
> > 
> > In terms of performance, sorting them correctly immediately (and only once)
> > should be faster than sorting them wrongly and then reversing the entire
> > array?
> 
> Yes, but in my case you would save the 'sortDirection == "ascending"' check
> for each comparison.

Indeed! :/ I revisited this and I believe I've now found the most performant
AND most readable solution, using outer if (one time) and concise arrow functions inside.
So we do only and exactly what's needed, in a straightforward, minimalistic, and transparent way.
And better still, I was able to streamline the other spot of sorting using the same technique. Awesome!

      if (sortDirection == "ascending") {
        selItems.sort(
          (a, b) => a.attachment.name.localeCompare(b.attachment.name));
      } else { // "descending"
        selItems.sort(
          (a, b) => b.attachment.name.localeCompare(a.attachment.name));
      }

If we make this any shorter, there'll be nothing left ;)

> But how many attachments do we realistically talk here? Maybe the speed does
> not matter much and we should aim for readability.

We don't know, so we should perform as well as possible regardless of how many attachments are there.

> > > @@ +5024,5 @@
> > > > +function attachmentsSelectionHasSortOrder(aDirection) {
> > Btw, this function is not needed by my code, because it gets the selection
> > sort order each time it is called, which isn't good for multiple calls.
> > Include this function anyway for future use?
> 
> We don't usually keep unneeded code. What could be the future use?

Removed.

> > > ::: mail/locales/en-US/chrome/messenger/messengercompose/messengercompose.dtd
> > > @@ +86,5 @@
> > > >  <!ENTITY renameAttachmentCmd.label "Rename Attachment…">
> > > >  <!ENTITY renameAttachmentCmd.accesskey "e">
> > > > +<!ENTITY reorderAttachmentsCmd.label "Reorder Attachments…">
> > > > +<!ENTITY reorderAttachmentsCmd.accesskey "s">
> > > > +<!ENTITY reorderAttachmentsCmd.key "#">
> > >
> > > Does this key work? I couldn't invoke it.
> > 
> > Wfm on Windows. Check key_reorderAttachments, it's ALT+# on Windows/Linux,
> > and CMD+# on MAC.
> 
> Still doesn't work for me on Linux. I assume I need to be focused in the
> attachment list. But nothing happens. Also when the attachment context menu
> is open, pressing Alt closes it.

This shortcut cannot work when attachment context menu is already open.
This is how it should work:

STR:
1) focus attachmentBucket (at least 1 item must be selected or have focus frame)
2) Press Alt+#
Expected result:
- Reorder Attachments Panel shows up, focus re-set to attachmentBucket.
That doesn't work on Linux?

If it doesn't, assuming that my <key id="cmd_reorderAttachments" ...> is correctly defined for Linux, that looks like an OS-specific problem/bug beyond my control. Strange though...
Any reason why Linux would not support Alt+# as a shortcut?
Where is # on your keyboard? 3rd layer key perhaps? Does it need any other modifiers to just get "#"?

What about the other Alt+movement key shortcuts for reordering, do they work on Linux regardless if panel is shown or not?
Attachment #8920633 - Attachment is obsolete: true
Attachment #8921551 - Flags: review?(acelists)
Comment on attachment 8921551 [details] [diff] [review]
Patch V.8: (functions cleanup, improved sorting algorithms) Implement 'Reorder Attachments' feature

Review of attachment 8921551 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/components/compose/content/messengercompose.xul
@@ +363,5 @@
> +       orient="vertical"
> +       type="arrow"
> +       flip="slide"
> +       onpopupshowing="reorderAttachmentsPanelOnPopupShowing();"
> +       onpopupshown="reorderAttachmentsPanelOnPopupShown();"

Last bug: reorderAttachmentsPanelOnPopupShown is not defined anywhere.
(In reply to Thomas D. (currently busy elsewhere) from comment #93)
> STR:
> 1) focus attachmentBucket (at least 1 item must be selected or have focus
> frame)
> 2) Press Alt+#
> Expected result:
> - Reorder Attachments Panel shows up, focus re-set to attachmentBucket.
> That doesn't work on Linux?
> 
> Any reason why Linux would not support Alt+# as a shortcut?
> Where is # on your keyboard? 3rd layer key perhaps? Does it need any other
> modifiers to just get "#"?

# is second level on the '3' key, I'm using english keyboard. So I must press Alt-Shift-3 to get this.
But it doesn't work for me. On Linux with KDE4.

> What about the other Alt+movement key shortcuts for reordering, do they work
> on Linux regardless if panel is shown or not?

Yes the others work.
Comment on attachment 8921551 [details] [diff] [review]
Patch V.8: (functions cleanup, improved sorting algorithms) Implement 'Reorder Attachments' feature

Review of attachment 8921551 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/components/compose/content/MsgComposeCommands.js
@@ +4677,5 @@
>      }
>  
> +    // Bug workaround: Force update of selectedCount and selectedItem, both wrong
> +    // after item removal, to avoid confusion for listening command controllers.
> +    bucket.clearSelection();

This hunk doesn't apply anymore to c-c tip. Maybe due to the now changed .appendElement() call above.
Fixed 1 nit in xul.
JS hunk applies for me, maybe auto-updated.

Richard, can you please set ui-r+ again. Basically this hasn't changed.
Attachment #8921551 - Attachment is obsolete: true
Attachment #8921551 - Flags: review?(acelists)
Attachment #8923018 - Flags: ui-review?(richard.marti)
Attachment #8923018 - Flags: review?(acelists)
(In reply to :aceman from comment #95)
> (In reply to Thomas D. (currently busy elsewhere) from comment #93)
> > STR:
> > 1) focus attachmentBucket (at least 1 item must be selected or have focus
> > frame)
> > 2) Press Alt+#
> > Expected result:
> > - Reorder Attachments Panel shows up, focus re-set to attachmentBucket.
> > That doesn't work on Linux?
> > 
> > Any reason why Linux would not support Alt+# as a shortcut?
> > Where is # on your keyboard? 3rd layer key perhaps? Does it need any other
> > modifiers to just get "#"?
> 
> # is second level on the '3' key, I'm using english keyboard. So I must
> press Alt-Shift-3 to get this.
> But it doesn't work for me. On Linux with KDE4.

That looks like a bug to me, maybe an XUL bug where XUL considers Shift as another modifier, failing to realize that Shift+3 returns plain "#" character, so Shift is arguably NOT a modifier key in that case. Or an OS bug where the OS returns "Shift+3" whereas it's supposed to just return "#".
Could you try if it works when you add SHIFT to the modifiers:

  <key id="key_reorderAttachments"
       key="&reorderAttachmentsCmd.key;" modifiers="alt shift"
       command="cmd_reorderAttachments"/>


> > What about the other Alt+movement key shortcuts for reordering, do they work
> > on Linux regardless if panel is shown or not?
> 
> Yes the others work.

Which seems to somewhat confirm the theory that there's a problem due to the 2nd-level key "#".

Any ideas how we could work around this? Could we somehow include a runtime test if certain key combos return something (let's say to test if Shift+3 returns #), and if they do, dynamically adjust our <keys>, depending on OS if needed? But that would be very complex, error-prone and hacky, and how many keyboard variants are we going to cover? But maybe those variants are limited.
(In reply to Thomas D. (currently busy elsewhere) from comment #98)
> > # is second level on the '3' key, I'm using english keyboard. So I must
> > press Alt-Shift-3 to get this.
> > But it doesn't work for me. On Linux with KDE4.
> 
> That looks like a bug to me, maybe an XUL bug where XUL considers Shift as
> another modifier, failing to realize that Shift+3 returns plain "#"
> character, so Shift is arguably NOT a modifier key in that case. Or an OS
> bug where the OS returns "Shift+3" whereas it's supposed to just return "#".
> Could you try if it works when you add SHIFT to the modifiers:
> 
>   <key id="key_reorderAttachments"
>        key="&reorderAttachmentsCmd.key;" modifiers="alt shift"
>        command="cmd_reorderAttachments"/>

On second thoughts, also to try this, using "3" as key:

>   <key id="key_reorderAttachments"
>        key="3" modifiers="alt shift"
>        command="cmd_reorderAttachments"/>
Comment on attachment 8923018 [details] [diff] [review]
Patch V.8.1: (1 xul nitfix) Implement 'Reorder Attachments' feature

ui-r- because '#' is a too special character and makes troubles on some keyboards (it only works on German keyboards flawlessly). I propose to use 'x' instead for 'eXchange' (I know it's not really a exchange but 'x' looks a bit like '#' ;) ). It's not used now and is normally accessible on every keyboard.

Also change on Mac the modifier from 'command' to 'control'. Then it's simpler to use 'control-m' to focus the attachmentBucket and then 'control-x' to open the panel. Now I have to change from 'control' to 'command'. Sorry to come so late but this damn '#' took too much focus to overlook (correct Jörg?) the other things.

I made now also some tests on Linux. The last version which showed me the panel partly was 6.3. Partly because it works only the first time, then it shows never until restart of TB. Then also 'alt-x' works. All newer version never show or the first time only for a split second the panel.

On Mac right click in the attachmentBucked without a selected attachment un-focuses the bucked and the reorder menuitem is disabled. Over a selected attachment no problem.

PS: The patch applies again not cleanly (only in jar.inc.mn) because of landing of bug 1408154.
Attachment #8923018 - Flags: ui-review?(richard.marti) → ui-review-
(In reply to Richard Marti (:Paenglab) from comment #100)
> Comment on attachment 8923018 [details] [diff] [review]
> Patch V.8.1: (1 xul nitfix) Implement 'Reorder Attachments' feature
> 
> ui-r- because '#' is a too special character and makes troubles on some
> keyboards (it only works on German keyboards flawlessly).

Yes, but basically those are implementation bugs elsewhere.

> I propose to use
> 'x' instead for 'eXchange' (I know it's not really a exchange but 'x' looks
> a bit like '#' ;) ). It's not used now and is normally accessible on every
> keyboard.

Nice. Let's hope nobody else wants that key for their addons etc.
Otherwise X is quite good:
- normally accessible as you say
- one-handed, that's really "handy" (adjacent keys)
- good mnemonics: X is a good key to symbolize "movement". And if you look closely, you'll see that there's a down arrow "v" and up arrow "ʌ" in the X. That's an excellent iconic representation of the function of re-ordering/moving.

> Also change on Mac the modifier from 'command' to 'control'. Then it's
> simpler to use 'control-m' to focus the attachmentBucket and then
> 'control-x' to open the panel. Now I have to change from 'control' to
> 'command'. Sorry to come so late but this damn '#' took too much focus to
> overlook (correct Jörg?) the other things.

No problem.

> I made now also some tests on Linux. The last version which showed me the
> panel partly was 6.3. Partly because it works only the first time, then it
> shows never until restart of TB. Then also 'alt-x' works. All newer version
> never show or the first time only for a split second the panel.

I don't understand this passage, and except for making it alt+x there's nothing I can do for Linux because I don't have a Linux testing environment.
So if there are any problems after change to alt+x, please assist me to fix them.

> On Mac right click in the attachmentBucked without a selected attachment
> un-focuses the bucked and the reorder menuitem is disabled. Over a selected
> attachment no problem.

Dito, I can't fix MAC problems. It's not too bad so we could even do that in a followup bug if there are no immediate ideas how to fix that.
I have no focus conditions in the command enabling routines, only selected attachment counts; but the entire attachment controller (including all commands) somehow is enabled only when focus is in the bucket (or something like that). So I think what happens on MAC is that when bucket context menu occurs, it's regarded as "no focus on bucket", then bucket controller gets disabled, and hence the reorder command. Strangely, the same doesn't happen on windows. I'm surprised that Mozilla's implementations don't behave the same way regardless of OS.

> PS: The patch applies again not cleanly (only in jar.inc.mn) because of
> landing of bug 1408154.

Ah well, that's a cat and mouse game which I can't win.
Attachment #8923018 - Attachment is obsolete: true
Attachment #8923018 - Flags: review?(acelists)
Attachment #8923082 - Flags: review?(acelists)
Attachment #8923082 - Flags: feedback?(richard.marti)
Attachment #8923082 - Attachment filename: 663695_reorder_attachments.8.2patch → 663695_reorder_attachments.8.2.patch
Attachment #8923082 - Attachment is patch: true
maybe it's a similar problem you had with the context menus in the address sidebar. It worked flawlessly on Windows and Linux and macOS showed two menus.
(In reply to Thomas D. (currently busy elsewhere) from comment #101)

OK, Alt-x works fine for me.

> > I made now also some tests on Linux. The last version which showed me the
> > panel partly was 6.3. Partly because it works only the first time, then it
> > shows never until restart of TB. Then also 'alt-x' works. All newer version
> > never show or the first time only for a split second the panel.
> 
> I don't understand this passage, and except for making it alt+x there's
> nothing I can do for Linux because I don't have a Linux testing environment.
> So if there are any problems after change to alt+x, please assist me to fix
> them.

I also don't see (on Linux) what the problem is here. I get the menu open and stable whenever I want, with the hotkey or the menuitem.
Paenglab can you specify the steps to reproduce?
 
> > PS: The patch applies again not cleanly (only in jar.inc.mn) because of
> > landing of bug 1408154.
> 
> Ah well, that's a cat and mouse game which I can't win.

No worries, we will update the patch if needed when landing it.
Flags: needinfo?(richard.marti)
Comment on attachment 8923082 [details] [diff] [review]
Patch V.8.2: (nitfix shortcut key) Implement 'Reorder Attachments' feature

On Windows everything works, also alt-x.

On Mac everything works except the context menu on the empty attachmentBucket space -> okay for a follow-up bug. But I give the f- because it's still 'command-x' and not 'control-x' like we have as modifiers to select the fields (From, To, Subject).

Linux seems to be special on my Mint VM where it doesn't work (more below). So I tried my build in my Ubuntu VM and here everything works.

(In reply to :aceman from comment #103)
> (In reply to Thomas D. (currently busy elsewhere) from comment #101)
> 
> OK, Alt-x works fine for me.
> 
> > > I made now also some tests on Linux. The last version which showed me the
> > > panel partly was 6.3. Partly because it works only the first time, then it
> > > shows never until restart of TB. Then also 'alt-x' works. All newer version
> > > never show or the first time only for a split second the panel.
> > 
> > I don't understand this passage, and except for making it alt+x there's
> > nothing I can do for Linux because I don't have a Linux testing environment.
> > So if there are any problems after change to alt+x, please assist me to fix
> > them.
> 
> I also don't see (on Linux) what the problem is here. I get the menu open
> and stable whenever I want, with the hotkey or the menuitem.
> Paenglab can you specify the steps to reproduce?

Simply open a new composer window, attach some files, right click to open the context menu and select 'Reorder Attachments'. Now either it shows the panel for a split second or never. Also multiple tries to show it ends in either show for a split second or never.

It could be a problem with my VM or a Cinnamon window manager problem because it works on my Ubuntu 17.10. My Cinnamon version is 3.4.6 on kernel 4.4.0-97.
Flags: needinfo?(richard.marti)
Attachment #8923082 - Flags: feedback?(richard.marti) → feedback-
(In reply to Richard Marti (:Paenglab) from comment #104)
> Comment on attachment 8923082 [details] [diff] [review]
> Patch V.8.2: (nitfix shortcut key) Implement 'Reorder Attachments' feature
> 
> On Windows everything works, also alt-x.
> 
> On Mac everything works except the context menu on the empty
> attachmentBucket space -> okay for a follow-up bug. But I give the f-
> because it's still 'command-x' and not 'control-x' like we have as modifiers
> to select the fields (From, To, Subject).

Now I'm confused. I fixed this for MAC, didn't I?

#ifdef XP_MACOSX
  <key id="key_delete" keycode="VK_BACK" command="cmd_delete"/>
  <key id="key_delete2" keycode="VK_DELETE" command="cmd_delete"/>
  <key id="key_reorderAttachments"
       key="&reorderAttachmentsCmd.key;" modifiers="control"
       command="cmd_reorderAttachments"/>
#else
  <key id="key_delete" keycode="VK_DELETE" command="cmd_delete"/>
  <key id="key_renameAttachment" keycode="VK_F2"
       command="cmd_renameAttachment"/>
  <key id="key_reorderAttachments"
       key="&reorderAttachmentsCmd.key;" modifiers="alt"
       command="cmd_reorderAttachments"/>
#endif
Comment on attachment 8923082 [details] [diff] [review]
Patch V.8.2: (nitfix shortcut key) Implement 'Reorder Attachments' feature

Mac is sometimes a bit special. Like Linux it uses links to the source files when no preprocessing is needed like in your files. But sometimes it caches them and uses older versions than the updated (Mac builders do also sometimes bust because they don't use the correct files). So I built TB again and yes, now 'control' is used.
Attachment #8923082 - Flags: feedback- → feedback+
Comment on attachment 8923082 [details] [diff] [review]
Patch V.8.2: (nitfix shortcut key) Implement 'Reorder Attachments' feature

So I think we're done here, Aceman said "last bug", and Richard said "disabled reorder command on MAC whitespace context -> new bug", and whatever else was there is a system artefact because it doesn't reproduce on other versions of Linux.

So if both of you could kindly set your review/ui-review flags and checkin-needed.
For the avoidance of doubt, and a better feeling, I prefer having the flags set by the reviewers themselves rather than me setting them on their behalf, even if reviewers have already hinted that it's done.
Flags: needinfo?(acelists)
Attachment #8923082 - Flags: ui-review?(richard.marti)
(In reply to Richard Marti (:Paenglab) from comment #106)
> Comment on attachment 8923082 [details] [diff] [review]
> Patch V.8.2: (nitfix shortcut key) Implement 'Reorder Attachments' feature
> 
> Mac is sometimes a bit special. Like Linux it uses links to the source files
> when no preprocessing is needed like in your files. But sometimes it caches
> them and uses older versions than the updated (Mac builders do also
> sometimes bust because they don't use the correct files). So I built TB
> again and yes, now 'control' is used.

Would using -purgecaches parameter when starting Thunderbird help?

"C:\Program Files\Daily.xyz\thunderbird.exe" -p profile.xyz -no-remote -purgecaches -console
Whiteboard: [has ui-review+]
Comment on attachment 580025 [details]
Initial/partial UX specification.txt: Implement keyboard shortcuts for reordering attachments in composition (incl. MAC)

There's more now, so this specification is just the first cut.
Attachment #580025 - Attachment description: Full UX specification.txt: Implement keyboard shortcuts for reordering attachments in composition (incl. MAC) → Initial/partial UX specification.txt: Implement keyboard shortcuts for reordering attachments in composition (incl. MAC)
Comment on attachment 8923082 [details] [diff] [review]
Patch V.8.2: (nitfix shortcut key) Implement 'Reorder Attachments' feature

I hope, the panel issue is only on my system and not a Mint or Cinnamon problem.

Unfortunately -purgecaches didn't help. It isn't a TB caching issue but a Mac problem.
Attachment #8923082 - Flags: ui-review?(richard.marti) → ui-review+
(In reply to Richard Marti (:Paenglab) from comment #104)
> Simply open a new composer window, attach some files, right click to open
> the context menu and select 'Reorder Attachments'. Now either it shows the
> panel for a split second or never. Also multiple tries to show it ends in
> either show for a split second or never.

I see the menu (panel) reliably, it stays open fine and I can click the reordering items always.
Flags: needinfo?(acelists)
Comment on attachment 8923082 [details] [diff] [review]
Patch V.8.2: (nitfix shortcut key) Implement 'Reorder Attachments' feature

Review of attachment 8923082 [details] [diff] [review]:
-----------------------------------------------------------------

Yes, this works for me.
As I said I need to whip up a test now :)

I hope the premature panel closing is just some local fluke. Paneglab may you have "focus follows mouse" (but I have it too) or similar feature enabled in the DE UI on your system? The panel is supposed to closed when you focus something else in the compose window, or focus another window.
Attachment #8923082 - Flags: review?(acelists) → review+
No, no "focus follows mouse" or such things.
Pushed by acelists@atlas.sk:
https://hg.mozilla.org/comm-central/rev/5b6e551a23fb
Implement 'Reorder Attachments' functionality, UI, and keyboard shortcuts; fix focus/selection issues. r=aceman, ui-r=bwinton,paenglab
Status: ASSIGNED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
I decided to land the patch already due to following reasons:
1. it is done and has needed reviews.
2. since the last patch was done 14 days ago, it already got bitrotted at 3 places due to other checkins. I have fixed those places locally before pushing it now.
3. I have the test halfway done, but I am always distracted from it by unrelated bustage on c-c.
4. now that Jorg has a pause I'm not sure when I find someone for reviewing the test

Due to stalling for 3. and 4. the 2. may only get worse and when I make the test it will most probably be TB59 so let's close this bug for ease of tracking and to also get some testing on nightly (better than crash-land it short before 59 closes).

I changed the component to Thunderbird as the feature only got implemented there.
Blocks: 1412758
Component: Composition → Message Compose Window
Product: MailNews Core → Thunderbird
Target Milestone: --- → Thunderbird 58.0
Depends on: 1416474
(In reply to :aceman from comment #115)
> I decided to land the patch already due to following reasons:
Smart move. Only Paenglab is with a "P".
Blocks: 1416717
There is actually an addon called OrderAtt (https://addons.mozilla.org/en/thunderbird/addon/orderatt/) that implements a simple attachment reordering. The addon is marked to only be compatible with TB up to 31, so doesn't work anymore. You may want to contact the author to put up a notice on the addon page, or write a comment yourself that the addon is no longer needed and the feature is built in TB. Like we have done with "Extra folder columns" addon.
Depends on: 1417856
Depends on: 1418310
(In reply to Thomas D. from comment #82)
> (In reply to :aceman from comment #81)
> > While playing in the attachment list, I noticed some pre-existing ugliness:
> > 1. when you just click an attachment, its icon jumps around (it is probably
> > hidden for a moment)
> 
> It doesn't for me on Windows, so I think this is specific to your OS.
> 
> > 2. when the attachment has no icon (I tried a .class file), the name takes
> > its space, so then the other attachment names (with icons) are not aligned
> 
> Again, not on Windows.
> 
> > Maybe you want to look at these in the next bug, now that you have
> > experience in the area:)
> 
> Given that I don't see them on Windows, I can't.

I have filed bug 1419172 and bug 1419177 for these problems so they are not lost.
Depends on: 1425891
Depends on: 1426344
Depends on: 1427037
See Also: → 244347
Blocks: 1440951
You need to log in before you can comment on or make changes to this bug.