Last Comment Bug 693210 - Add support for some keys to Advanced Search
: Add support for some keys to Advanced Search
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: MailNews: General (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: seamonkey2.7
Assigned To: Jens Hatlak (:InvisibleSmiley)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-10-09 14:52 PDT by Jens Hatlak (:InvisibleSmiley)
Modified: 2011-11-04 15:58 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (5.06 KB, patch)
2011-10-09 14:52 PDT, Jens Hatlak (:InvisibleSmiley)
neil: superreview+
Details | Diff | Review
patch v2 (14.25 KB, patch)
2011-10-12 13:25 PDT, Jens Hatlak (:InvisibleSmiley)
mnyromyr: review-
Details | Diff | Review
patch v2a (19.58 KB, patch)
2011-10-17 10:57 PDT, Jens Hatlak (:InvisibleSmiley)
mnyromyr: review-
Details | Diff | Review
patch v2b (20.18 KB, patch)
2011-10-19 13:31 PDT, Jens Hatlak (:InvisibleSmiley)
mnyromyr: review+
Details | Diff | Review
patch v2c [Checkin: comment 19] (25.47 KB, patch)
2011-10-20 14:12 PDT, Jens Hatlak (:InvisibleSmiley)
jh: review+
neil: superreview+
Details | Diff | Review

Description Jens Hatlak (:InvisibleSmiley) 2011-10-09 14:52:51 PDT
Created attachment 565835 [details] [diff] [review]
patch

Currently, hardly any keys (shortcuts) work in the result list of the Advanced Search dialog. I think we should support some that make sense under the circumstances and which effects can be observed directly. I identified these in addition to what we already have (like Ctrl+A):

 <m>  Mark message(s) as read: Toggles read state and boldness
<0-9> Tag message(s): Sets or removes tags and changes color; optional Tags column
<Ctrl+O> Open message(s)

Other commands are either better suited for cases where you have the message in front of you (e.g. Reply, Forward) or potentially unexpected (e.g. Archive) if people accidentally switch focus from the search field.
Comment 1 Jens Hatlak (:InvisibleSmiley) 2011-10-10 14:27:55 PDT
On second thought, including messenger.dtd just for twelve key entities is a bit drastic (at best, could also be just wrong). I guess it's better to copy them to SearchDialog.dtd instead, possibly together with a short l10n note that they should be kept in sync with the originals. Or create a shared DTD file?
Comment 2 neil@parkwaycc.co.uk 2011-10-10 16:23:06 PDT
Comment on attachment 565835 [details] [diff] [review]
patch

Add "Flag", perhaps?
Comment 3 Jens Hatlak (:InvisibleSmiley) 2011-10-10 23:40:05 PDT
(In reply to neil@parkwaycc.co.uk from comment #2)
> Add "Flag", perhaps?

I thought about that, too, and am not totally opposed, but the issue is that its effect cannot be observed in this view (unless I missed something). OTOH, AFAIK you cannot see the effect in the main window(s) either, at least by default. So why not. Karsten, what do you think?
Comment 4 Karsten Düsterloh 2011-10-11 15:23:28 PDT
(In reply to Jens Hatlak (:InvisibleSmiley) from comment #1)
> On second thought, including messenger.dtd just for twelve key entities is a
> bit drastic (at best, could also be just wrong). I guess it's better to copy
> them to SearchDialog.dtd instead, possibly together with a short l10n note
> that they should be kept in sync with the originals. Or create a shared DTD
> file?

The DTD isn't that much of a problem, imo.
I'm less convinced of duplicating all those key definitions.

(In reply to Jens Hatlak (:InvisibleSmiley) from comment #0)
> Other commands are either better suited for cases where you have the message
> in front of you (e.g. Reply, Forward) or potentially unexpected (e.g.
> Archive) if people accidentally switch focus from the search field.

Agreed. 

(In reply to Jens Hatlak (:InvisibleSmiley) from comment #3)
> (In reply to neil@parkwaycc.co.uk from comment #2)
> > Add "Flag", perhaps?
> 
> I thought about that, too, and am not totally opposed, but the issue is that
> its effect cannot be observed in this view (unless I missed something).
> OTOH, AFAIK you cannot see the effect in the main window(s) either, at least
> by default. So why not. Karsten, what do you think?

Being able to toggle the flag without having any chance to see it is bad (the flag column isn't part of the column picker).
But actually I don't see why it shouldn't be there — and then the key would make sense …
Comment 5 Jens Hatlak (:InvisibleSmiley) 2011-10-12 13:25:23 PDT
Created attachment 566617 [details] [diff] [review]
patch v2

Do you like this one better?

We can surely talk about moving even more keys to the overlay if you want.
Comment 6 neil@parkwaycc.co.uk 2011-10-12 14:30:19 PDT
(In reply to Jens Hatlak from comment #3)
> (In reply to comment #2)
> > Add "Flag", perhaps?
> 
> I thought about that, too, and am not totally opposed, but the issue is that
> its effect cannot be observed in this view (unless I missed something).

Sorry, I hadn't realised that.
Comment 7 Karsten Düsterloh 2011-10-16 11:35:40 PDT
Comment on attachment 566617 [details] [diff] [review]
patch v2

>+++ b/suite/mailnews/mailKeysOverlay.xul

Yeah, that's a better idea.
Although, in this case, you should move the relevant l10n stuff into its own mailKeysOverlay.dtd as well.

>+<?xul-overlay href="chrome://navigator/content/platformMailOverlay.xul"?>

As Ian pointed out on IRC, this should be platformMailnewsOverlay.xul. (Especially important on Mac because of the delete keys.)

No other objections. :)
Comment 8 Jens Hatlak (:InvisibleSmiley) 2011-10-17 10:57:29 PDT
Created attachment 567494 [details] [diff] [review]
patch v2a
Comment 9 Karsten Düsterloh 2011-10-18 15:51:54 PDT
Comment on attachment 567494 [details] [diff] [review]
patch v2a

>+++ b/suite/locales/en-US/chrome/mailnews/mailKeysOverlay.dtd
>+<!ENTITY markAsReadCmd.key        "m">
>+<!ENTITY markFlaggedCmd.key       "i">
>+<!ENTITY openMessageWindowCmd.key "o">

Please don't split the .key from the other command entities like .label, .accesskey etc., they should move en bloc. (This might require including the nre .dtd in other places as well, I didn't check in depth.)

r- just for this issue.
Comment 10 Jens Hatlak (:InvisibleSmiley) 2011-10-19 13:31:39 PDT
Created attachment 568170 [details] [diff] [review]
patch v2b

The moved entities are only used in mailWindowOverlay.xul, which now includes mailKeysOverlay.dtd.
Comment 11 Karsten Düsterloh 2011-10-19 16:12:26 PDT
Comment on attachment 568170 [details] [diff] [review]
patch v2b

Yay!
Comment 12 Karsten Düsterloh 2011-10-19 22:49:11 PDT
Comment on attachment 568170 [details] [diff] [review]
patch v2b

Oh, and please don't forget suitable ;-) license headers in the new files.
Comment 13 Jens Hatlak (:InvisibleSmiley) 2011-10-20 00:18:41 PDT
(In reply to Karsten Düsterloh from comment #12)
> Oh, and please don't forget suitable ;-) license headers in the new files.

Bah, I had hoped you wouldn't say that. A license header in such a short DTD file means:
* large overhead for little to no avail (it's not code, just text!)
* inconsistency (other DTD files don't have it and no-one complained or changed it!)

Which is why I removed it (I had added it once, then changed my mind for the above reasons). I think we either need to enforce this for all files or relax the rule. IMO there is no point in just enforcing it for new files.

My consistency point also applies to (overlay) XUL files, but there I'd actually understand the need for a license header.
Comment 14 Karsten Düsterloh 2011-10-20 01:15:48 PDT
(In reply to Jens Hatlak (:InvisibleSmiley) from comment #13)
> * large overhead for little to no avail (it's not code, just text!)

That's not a valid reason. And DTD/XUL *is* code.

> * inconsistency (other DTD files don't have it and no-one complained or
> changed it!)

Huh? There shouldn't be any such files. If there are, it's a bug.

> Which is why I removed it (I had added it once, then changed my mind for the
> above reasons). I think we either need to enforce this for all files

Yes.
(There have been numerous bugs in the past just for adding license boiler plates.)
Comment 15 neil@parkwaycc.co.uk 2011-10-20 04:01:09 PDT
Comment on attachment 568170 [details] [diff] [review]
patch v2b

> <?xul-overlay href="chrome://messenger/content/platformMailnewsOverlay.xul"?>
>+<?xul-overlay href="chrome://messenger/content/mailKeysOverlay.xul"?>
Aren't we pulling in platformMailnewsOverlay via mailKeysOverlay?
[Same applies to the search dialog!]

> <!DOCTYPE overlay [
>   <!ENTITY % messengerDTD SYSTEM "chrome://messenger/locale/messenger.dtd">
>   %messengerDTD;
>+  <!ENTITY % mailKeysDTD SYSTEM "chrome://messenger/locale/mailKeysOverlay.dtd">
>+  %mailKeysDTD;
Did you forget to remove this?

>         case "cmd_open":
>+        case "cmd_openMessage":
Is it worth changing the open button to cmd_openMessage?
Comment 16 Jens Hatlak (:InvisibleSmiley) 2011-10-20 04:37:55 PDT
(In reply to Karsten Düsterloh from comment #14)
> Huh? There shouldn't be any such files. If there are, it's a bug.

Filed bug 696031.

> > <?xul-overlay href="chrome://messenger/content/platformMailnewsOverlay.xul"?>
> >+<?xul-overlay href="chrome://messenger/content/mailKeysOverlay.xul"?>
> Aren't we pulling in platformMailnewsOverlay via mailKeysOverlay?
> [Same applies to the search dialog!]

Probably, will check later.

> > <!DOCTYPE overlay [
> >   <!ENTITY % messengerDTD SYSTEM "chrome://messenger/locale/messenger.dtd">
> >   %messengerDTD;
> >+  <!ENTITY % mailKeysDTD SYSTEM "chrome://messenger/locale/mailKeysOverlay.dtd">
> >+  %mailKeysDTD;
> Did you forget to remove this?

No. I had guessed that including the XUL file (which includes the DTD) would make the DTD available here, too, but when I checked, it didn't, so I added it.

> >         case "cmd_open":
> >+        case "cmd_openMessage":
> Is it worth changing the open button to cmd_openMessage?

Will check.
Comment 17 Jens Hatlak (:InvisibleSmiley) 2011-10-20 14:12:04 PDT
Created attachment 568513 [details] [diff] [review]
patch v2c [Checkin: comment 19]
Comment 18 neil@parkwaycc.co.uk 2011-10-20 15:06:52 PDT
Comment on attachment 568513 [details] [diff] [review]
patch v2c [Checkin: comment 19]

>+  <!ENTITY % mailKeysDTD SYSTEM "chrome://messenger/locale/mailKeysOverlay.dtd">
>+  %mailKeysDTD;
Sorry, I failed to notice that the keys moved but the menuitems didn't, so you still need the DTD for those.

>         switch(command) {
Nit: might be worth moving cmd_openMessage here for consistency (it's first in doCommand).
Comment 19 Jens Hatlak (:InvisibleSmiley) 2011-11-04 15:57:51 PDT
Comment on attachment 568513 [details] [diff] [review]
patch v2c [Checkin: comment 19]

http://hg.mozilla.org/comm-central/rev/a523d5bffbd4

Note You need to log in before you can comment on or make changes to this bug.