Closed
Bug 693210
Opened 13 years ago
Closed 13 years ago
Add support for some keys to Advanced Search
Categories
(SeaMonkey :: MailNews: General, enhancement)
SeaMonkey
MailNews: General
Tracking
(Not tracked)
RESOLVED
FIXED
seamonkey2.7
People
(Reporter: InvisibleSmiley, Assigned: InvisibleSmiley)
Details
Attachments
(1 file, 4 obsolete files)
25.47 KB,
patch
|
InvisibleSmiley
:
review+
neil
:
superreview+
|
Details | Diff | Splinter Review |
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.
Attachment #565835 -
Flags: superreview?(neil)
Attachment #565835 -
Flags: review?(mnyromyr)
Assignee | ||
Comment 1•13 years ago
|
||
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•13 years ago
|
||
Comment on attachment 565835 [details] [diff] [review] patch Add "Flag", perhaps?
Attachment #565835 -
Flags: superreview?(neil) → superreview+
Assignee | ||
Comment 3•13 years ago
|
||
(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•13 years ago
|
||
(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 …
Assignee | ||
Comment 5•13 years ago
|
||
Do you like this one better? We can surely talk about moving even more keys to the overlay if you want.
Attachment #565835 -
Attachment is obsolete: true
Attachment #565835 -
Flags: review?(mnyromyr)
Attachment #566617 -
Flags: superreview?(neil)
Attachment #566617 -
Flags: review?(mnyromyr)
Comment 6•13 years ago
|
||
(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•13 years ago
|
||
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. :)
Attachment #566617 -
Flags: review?(mnyromyr) → review-
Assignee | ||
Comment 8•13 years ago
|
||
Attachment #566617 -
Attachment is obsolete: true
Attachment #566617 -
Flags: superreview?(neil)
Attachment #567494 -
Flags: superreview?(neil)
Attachment #567494 -
Flags: review?(mnyromyr)
Comment 9•13 years ago
|
||
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.
Attachment #567494 -
Flags: review?(mnyromyr) → review-
Assignee | ||
Comment 10•13 years ago
|
||
The moved entities are only used in mailWindowOverlay.xul, which now includes mailKeysOverlay.dtd.
Attachment #567494 -
Attachment is obsolete: true
Attachment #567494 -
Flags: superreview?(neil)
Attachment #568170 -
Flags: superreview?(neil)
Attachment #568170 -
Flags: review?(mnyromyr)
Comment 11•13 years ago
|
||
Comment on attachment 568170 [details] [diff] [review] patch v2b Yay!
Attachment #568170 -
Flags: review?(mnyromyr) → review+
Comment 12•13 years ago
|
||
Comment on attachment 568170 [details] [diff] [review] patch v2b Oh, and please don't forget suitable ;-) license headers in the new files.
Assignee | ||
Comment 13•13 years ago
|
||
(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•13 years ago
|
||
(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•13 years ago
|
||
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?
Assignee | ||
Comment 16•13 years ago
|
||
(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.
Assignee | ||
Comment 17•13 years ago
|
||
Attachment #568170 -
Attachment is obsolete: true
Attachment #568170 -
Flags: superreview?(neil)
Attachment #568513 -
Flags: superreview?(neil)
Attachment #568513 -
Flags: review+
Comment 18•13 years ago
|
||
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).
Attachment #568513 -
Flags: superreview?(neil) → superreview+
Assignee | ||
Comment 19•13 years ago
|
||
Comment on attachment 568513 [details] [diff] [review] patch v2c [Checkin: comment 19] http://hg.mozilla.org/comm-central/rev/a523d5bffbd4
Attachment #568513 -
Attachment description: patch v2c → patch v2c [Checkin: comment 19]
Assignee | ||
Updated•13 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.7
You need to log in
before you can comment on or make changes to this bug.
Description
•