Closed Bug 188934 Opened 22 years ago Closed 21 years ago

Junk Mail Controls: J key for mark as junk, Shift-J for mark as not junk

Categories

(SeaMonkey :: MailNews: Message Display, defect)

x86
Windows 2000
defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: jglick, Assigned: timeless)

References

Details

(Keywords: access)

Attachments

(1 file)

Implement J key for "toggle junk status"

----------------
From email:

Gervase Markham: It would be good to define a key for "toggle junk status". J is
the obvious choice, although I don't know if it's taken.  By analogy with M for
 Mark Read, we have J for Mark Junk.

jglick: If its available, and Aaron Leventhal approves, it would be fine with
me. http://www.mozilla.org/projects/ui/accessibility/mozkeyplan.html

Aaron: Yes, J is fine for Junk Toggle. Thanks for asking. Let me know when you
finish implementing it so I can update various docs pages.
Do we also need a keystroke to clear the current folder of messages marked as Junk?
*** Bug 185582 has been marked as a duplicate of this bug. ***
Well, once they are selected, you can press D. And you can select them by having
a "Junk" filter (which I'm surprised doesn't exist by default; "Not Junk" does.)
And once move is enabled, any given folder will probably be either all Junk or
all Not Junk. So I don't think there's a desparate need for such a function or key.

Gerv
How can you mark new junk with a junk filter?  Some junk is new junk and must 
be marked by hand.  That is why the J is needed.
*** Bug 209085 has been marked as a duplicate of this bug. ***
Even Control-J would be good.  Add the hotkey to the menu as well.
A volunteer writes:

This has been hanging around far too long.

So here's a deal:

someone tell what file I need to be adding the accesskey to, and I'll create the
patch.

stef
Attached patch J=>Junk, shift-J=>NotJunk — — Splinter Review
I was going to write a nice explanation of how to figure out what to change and
then an explanation of the problems involved in implementing a twiddle for a
tristate field, but it just wasn't worth it.

steps:
You know that the relatively rare string "Thread As Read" is in a localization
file hopefully near "As Junk"/"As Not Junk". And you know that it has a key
bound because you can see it ("R"). [Why not use 'junk'? well it might not be
rare and you want information about what you need to copy to bind a key.]

so you do this:
[lss thread as read]
http://lxr.mozilla.org/seamonkey/search?string=thread%20as%20read

What you see: a bunch of items, but especially a dtd (which is good because we
expected it to be localized):
/mailnews/base/resources/locale/en-US/messenger.dtd, line 382 -- <!ENTITY
markThreadAsReadCmd.label "Thread As Read">
Now you search for the entity:
[lss markThreadAsReadCmd.label]
It's only used in one xul file (which is good, more xul files => more work)
/mailnews/base/resources/content/mailWindowOverlay.xul

Unfortunately it's used a bunch of times in that xul file, so there will be
more work than we'd like.

Following the first lxr hit you see:
582	    <menuitem label="&markThreadAsReadCmd.label;"
583		      key="key_markThreadAsRead"
584		      accesskey="&markThreadAsReadCmd.accesskey;"
585		      observes="cmd_markThreadAsRead"/>
And scrolling down a bit you see:
601	    <menuitem label="&markAsJunkCmd.label;"
602		      accesskey="&markAsJunkCmd.accesskey;"
603		      observes="cmd_markAsJunk"/>
Hrm, the difference is this 'key' line. Ok, we'll need a key line.
{Note: it turns out we'll want to modify all the corresponding items, but
that's just some find/paste}

so we should find out about key, so
[lss key_markThreadAsRead]
http://lxr.mozilla.org/seamonkey/search?string=key_markThreadAsRead
it's only in one file, good, and there's even a definition visible as the first
hit, great:
/mailnews/base/resources/content/mailWindowOverlay.xul, line 306 -- <key
id="key_markThreadAsRead" key="&markThreadAsReadCmd.key;"
oncommand="goDoCommand('cmd_markThreadAsRead')"/>
we'll need to copy the key line and make some changes. obviously the id needs
to be unique, and the key entity should be distinct. In the dtd file we
probably saw:
382 <!ENTITY markThreadAsReadCmd.label "Thread As Read">
383 <!ENTITY markThreadAsReadCmd.accesskey "T">
384 <!ENTITY markThreadAsReadCmd.key "r">
vs only:
393 <!ENTITY markAsJunkCmd.label "As Junk">
394 <!ENTITY markAsJunkCmd.accesskey "J">
so we were sort of expecting that extra line. we'll make one for asjunk, and
there's even a naming convention, we'll follow it.

Last step, the goDoCommand thing. We lucked out there was a simple pattern to
follow. And I decided to bind a key to each command instead of trying to be
cute and make J toggle. If I had wanted to toggle here's what I would have
done:
[lss cmd_markThreadAsRead] and [lss cmd_markAsJunk] 
This turns up two files with 'case' statements, that kind of sucks, but it's
just because we needed to duplicate code and no one refactored it to combine
threepane and standalone views. Anyway you try each of the three hits in one of
the files, and two turn out to be duds (you could take some time and learn
about command dispatching if you like). the third does things like:
JunkSelectedMessages(true)

So time to find out about that function
[lss JunkSelectedMessages]
http://lxr.mozilla.org/seamonkey/search?string=JunkSelectedMessages

if you know js you'll recognize a function definition, which you might decide
to look at, and some calls mostly uninteresting. There's also an interesting
call:
/mailnews/base/resources/content/mailWindowOverlay.js, line 1287 --
JunkSelectedMessages(!SelectedMessagesAreJunk());

A similar pattern appears for mark as read (it's a simple toggle), and if you
follow that link you'll see it's in a function 'MsgJunk()' now you could just
wire the <key> to that function w/ oncommand='MsgJunk()'. But then you're stuck
trying to explain to the user that it's a toggle and that's too complicated.
Hopefully in this process you learned how to hack xul.

My bookmarks are available somewhere, but lss is just a keyword for
http://lxr.mozilla.org/seamonkey/search?string=%s i also have keywords for lbi
(/seamonkey1.0/ident?i=%s) and lms (/mozilla/find?string=%s) and combinations
for various lxrs.
Comment on attachment 131194 [details] [diff] [review]
J=>Junk, shift-J=>NotJunk

Ok, so I made the changes.
to test them i expanded the files i had modified from their nightly builds
(this creates a directory structure).
to figure out where the files lived i would have to look for a jar.mn file.
so
[lss messenger.dtd]
http://lxr.mozilla.org/seamonkey/search?string=messenger.dtd
looking for a jar.mn file i find:
http://lxr.mozilla.org/seamonkey/source/mailnews/jar.mn#197
[lss mailWindowOverlay.xul]
http://lxr.mozilla.org/seamonkey/search?string=mailWindowOverlay.xul
looking for a jar.mn file i find:
http://lxr.mozilla.org/seamonkey/source/mailnews/jar.mn#91

It turns out they're in the same manifest but they might not be. anyway jar.mn
files have a simple format there's a line
filename.jar:
and then a list of files that get stuffed into it.
anotherfile.jar:
another list of files.

I just scroll up until i find the names of the jar files (en-US.jar and
messenger.jar).

So I load those in winzip, find the files, extract them [use foldernames], then
i want to replace them with the versions i made.  I could either
cvs diff messenger.dtd | patch -d t:chrome\en-US\locale\en-US\messenger
which makes sure I test the patch, or simply copy the file over.

Then I drag the folder back into winzip. to do this you want to drag the top
level folder that winzip created (locale in this case) into winzip.

repeat the process for the other files.

run mozilla, verify that things work.

now create your patch 
cvs diff -u > patchfile
attach it
http://bugzilla.mozilla.org/attachment.cgi?bugid=188934&action=enter
(there's a link on the bug to "Create a New Attachment")

And then the fun part: chasing reviewers.
the official place to go is
http://www.mozilla.org/owners.html
which has a little field where you can enter a path to a file and pray that
it's officially owned in despot
[fo mozilla/mailnews/base]
http://despot.mozilla.org/despot.cgi?command=FindPartition&repid=3&view=1&file=
mozilla/mailnews/base
redirects to
http://www.mozilla.org/owners.html#Mail/News
Any owner or peer is acceptable for an r=, although they may decline or ignore
you.

Hrm, but how to figure out who would be a good reviewer.

Some tricks.
1. find out if the possible reviewers have away names.
To do this go to the cc field and enter each of their email addresses (space or
comma between items) with one letter removed from the end, plus the additional
address: '.'. When you press commit you'll get an error 
Match Failed
...
CC:	. was too short for substring match (minimum 3 characters)

This prevents you from actually affecting the bug.
But it lets you see who the person might be and what their name (message) is.

Here's one of my matches:

dmose@mozilla.or matched Dan Mosedale: Away from moz until 9/15
<dmose@mozilla.org> 

Today's 9/10, so as a hacker I might be in a hurry (I shouldn't be, we're
frozen, but suppose i am), I cross dmose off my list for today and make a note
to consider him later.

some people failed to match e.g.
sspitzer@netscape.co did not match anything 
you can try just their names (each comma/space separated) in this case I tried
sspitzer because that's what his email username was before:
Seth Spitzer <sspitzer@mozilla.org>
Seth Spitzer <sspitzer@sspitzer.org>

Armed with this list of people and an idea of who might be unavailable I turn
to the request queue

2. find out how long their request queues are.
When you aren't logged in you'll see 'Requests' at the bottom it's
http://bugzilla.mozilla.org/request.cgi

Looking through the list I see that sspitzer's queue is a few screens long.
I'll rule him out for now.

I don't see a request queue for cavin song, which might mean he isn't reviewing
anymore or that he's always on top of his queue.

In this case I'm going to settle on neil@parkwaycc.co.uk
<neil.parkwaycc.co.uk@myrealbox.com> because he has a relatively short queue
(5) items, and has some current requests of his own (which indicates he's still
working on the project).

If I didn't have editbugs at this point, i'd be in trouble. I'd probably go to
irc.mozilla.org #mozillazine and ask someone to make the request for me.

To make the request I click edit by my patch
http://bugzilla.mozilla.org/attachment.cgi?id=131194&action=edit
and select the request dropdown and click on "?". Then I enter neil's email
address into the field. bugzilla would complete things [neil is 'parkwaycc'],
but as a newbie and as someone who just copied the email addresses from a
completion list there's no reason for me to rely on it.

click submit and wait a few days. we'll see what happens
Attachment #131194 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #131194 - Flags: review?(neil.parkwaycc.co.uk) → review+
Comment on attachment 131194 [details] [diff] [review]
J=>Junk, shift-J=>NotJunk

Review in hand from a module peer I can select any sr from the list at
http://www.mozilla.org/hacking/reviewers.html

It's usually a good idea to pick someone who isn't overloaded and won't
entirely balk at the code in question. Since I've actually explained what I did
and that I tested it, I could probably pick anyone.

Both tor and bryner are active and have relatively short queues. The patch is
simple and bryner does xul work, whereas tor doesn't really do xul work, so i'm
going to ask bryner to do the sr.

Again, i edit the attachment, this time i select ? from the superreview
dropdown and enter 'bryner@b' for the requestee. - I know I need @b because I
tried bryner@ before and it resulted in a choice, (which is ok, I could select
the one i want from the confirm match screen) but I don't want a choice.
Attachment #131194 - Flags: superreview?(bryner)
Comment on attachment 131194 [details] [diff] [review]
J=>Junk, shift-J=>NotJunk

So, this patch doesn't actually do what the bug calls for, which is for J to
toggle junk status off and on.	I'd like a mail owner to decide what UI is
wanted here.
*** Bug 219229 has been marked as a duplicate of this bug. ***

I'm guess that timeless, with his fantastic explanation of how this all works,
left that bit as 'an exercise for the volunteer' - I'll get a revised patch in,
in a while.

stef
Comment on attachment 131194 [details] [diff] [review]
J=>Junk, shift-J=>NotJunk

neil is a mailnews peer. his approval is supposed to be sufficient. trying
again.
Attachment #131194 - Flags: superreview?(bryner) → superreview?(dveditz+bmo)
"Mark as Read" is a toggle; it is consistent that "Mark as Junk" should be one too.

Gerv
Well, this wouldn't be a mozilla ui bug without some controversy.  To readers
following along with the howto, I'm sorry, but controversy is a typical
component of any mozilla ui bug. This is also one of the reasons that people
avoid working on ui bugs.

So here's my volley into that portion of the bug:

* I'm not sure why Mark as Junk isn't a toggle.
* Perhaps it's because the thing is a tristate: IsJunk, IsNotJunk, not IsJunk
and not IsNotJunk.
* If someone fixes the menus to actually have a single item with a checkmark
like the read item then we can fix the keybindings.
* There is no place to list a toggle key in the current ui because the current
ui doesn't toggle - it marks.

Fixing the menuitems is a different issue, If you want a single menuitem then
you should file another bug for that. The issue here is getting a patch to add
keybindings to junkmail controls landed.

The only interface I could design which would express the behavior requested for
J currently w/o consolidating the menuitems would be to have the J key binding
literally jump to the item which it would currently trigger. A user isn't going
to be able to figure that out.
Assignee: sspitzer → timeless
Summary: Junk Mail Controls: J key for "toggle junk status" → Junk Mail Controls: J key for mark as junk
> To readers following along with the howto, I'm sorry, but controversy is a 
> typical component of any mozilla ui bug.

It's often less controversial if you implement the function as specced in the
bug. ;-)

However, in this case, you have a point. Is the dual-menu-item thing just a UI
glitch, or is the underlying model some sort of tristate rather than a boolean?
Seth: can you explain?

Gerv

Comment on attachment 131194 [details] [diff] [review]
J=>Junk, shift-J=>NotJunk

I'm happy enough with the j/J keys. Matches the existing menu/command
structure, and as a bonus you can get what you want when you've got a mixed
group selected.
Attachment #131194 - Flags: superreview?(dveditz+bmo) → superreview+
And the last step in the process when you find that bugmail which says you have
the necessary reviews is to find someone to check in your patch.

In my case I needed to find myself sometime around now and arrange to check in
the patch. Part of the checkin process is sitting around watching
http://tinderbox.mozilla.org/showbuilds.cgi?tree=SeaMonkey to make sure that
your checkin clears. For xul/js only patches this is mostly a formality,
especially for mailnews patches. Checkins of course should be done while the
tree is open and green according to the tree rules (we're in the pre 1.6a cycle
which means you don't need any additional stamps, r+sr is sufficient).

It's traditional to sit on irc.mozilla.org in #mozilla while you're waiting for
your checkin to clear (you should expect to spend 3 hours watching the tree).
If necessary you can ask someone else to do the watching.

After you've made a bunch of patches (this is subjective) you can consider
applying for cvs access. http://www.mozilla.org/hacking/getting-cvs-write-access.html
the instructions there should be clear, if they aren't feel free to ask about
them (of me or someone else, on irc or by email).
Personally I'd suggest having made at least 5 patches and collecting sr+ from 3
or more srs. The reason is that it makes it much easier to hunt for srs to
support your cvs account request.

OK, that's about it for this tutorial. The only other thing to say is a general
bookkeeping thing: please don't reopen this bug for unrelated concerns.
If you like gerv feel there should be only one key and therefore one menu then
you or gerv or someone should file a new bug, you're welcome to mention it here
in this bug, but you shouldn't reopen this bug to address that issue.
The only reasons to reopen this bug are if it breaks something, doesn't work,
or is backed out.

Oh, to readers, I checked in the change about when I made this comment, that
means that a build from before I made this comment will not have the fix, so
don't complain that a build from saturday or friday or last week doesn't have
it. WRT branches, e.g. 1.5 that branched a while ago, so while a build from the 
1.5 branch might be made tomorrow it you need to consider it as if it was built
when it branched, it won't have this fix unless someone does additional work.

WRT 1.5, this is a small low risk feature which has some visible value, it
might be worth asking for permission to land it on the 1.5 branch.
Traditionally one does not try to land something on a public branch until after
it has 'baked' on the trunk. In this case if I or someone else decides say on
Tuesday (but really not before then) that the change worked without causing any
regressions, then I or someone else can follow the current procedure to ask for
permission to land the change on the branch. If I remember I'll outline that on
Tuesday. -- Note that FIXED generally relates to the trunk. I will not REOPEN
the bug just because it isn't fixed on some branch, we currently use flags for
approval and keywords for branch status (fixed/verified).

Normally the comment for when a bug is checked in would have something like:
Fix checked in
So that people will understand why you're resolving the bug as fixed.
some people also indicate the checkin comment or the cvs commit output
(which includes the new cvs revisions of the affected files) or if someone else
did the checkin for you you might indicate that: Fix checked in by timeless.
I tend to avoid resolving other people's bugs when I check in patches for them
because I don't necessarily know what else they might plan to do for a given
bug. In this case I know that the bug is done, because it's my bug :).
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Summary: Junk Mail Controls: J key for mark as junk → Junk Mail Controls: J key for mark as junk, Shift-J for mark as not junk
I am very happy that "Mark as Junk" is not a toggle. I let Junk mails move to
Trash automatically so toggle would not work. Thanks guys. 
Comment on attachment 131194 [details] [diff] [review]
J=>Junk, shift-J=>NotJunk

Risk Assesment: Minimal. The change has baked on the trunk since Sunday, 
is localized to two mailnews files (xul, dtd), and there have been no 
reports of problems. The patch has r=neil(moa) and sr=dveditz.

Benefit: Improved keyboard functionality, junk mail can safely be marked 
as junk or not junk with the keystrokes J and shift+J respectively. Junk 
Mail support is one of the key selling points of this release, this change 
improves ease of use.

--
OK, so the patch has been on the trunk for two days without any reports of 
problems. Now would be the time for me to ask for permission to land on 
the 1.5 branch.

For seamonkey there are four types of branches:

Unmanaged: Any unofficial branch. People are allowed to create their own 
branches and set their own rules and procedures. These don't concern us.

* the next two have traditionally been freezes on the trunk followed by 
static tags.

Alpha*: This branch period should focus on stabalizing the trunk and 
catching major problems introduced since the previous milestone branched. 
If there's a big change that someone needs for final and that change needs 
large scale testing they can ask for approval for alpha, they should 
describe the need and explain what testing they want from users and what 
they expect to do after they get their feedback.

Beta*: This branch period precedes a real branch for final and blocks 
trunk development, so everyone wants it to be relatively short. This is a 
shakeout mini release, requests should include a risk assessment and 
justification.

Final (we are here): Drivers do not really want features (especially 
untested ones). The focus is on stability because this is the product that 
end users will be using. As with Beta you need a risk assessment and 
justification for the change.

Given that I'd like to land a feature on a Final branch, it wouldn't be 
unreasonable or unexpected for this request to be rejected, after all it
isn't a crash/hang fix. That said, here's what I do:

To make the request for the Mozilla1.5 branch, I click edit by my patch
http://bugzilla.mozilla.org/attachment.cgi?id=131194&action=edit
and select the request dropdown "approval1.5" and select "?". [Note that 
I'm not setting blocking1.5? on the bug - that's for something which is an 
absolute show stopper in someone's mind, this hardly qualifies, it's just 
a nice, low risk, feature.]

Then I enter a nice relatively short (oops) approval request message in 
the small edit field.  If I need a larger edit field and I'm using 
Mozilla, I can click edit attachment as comment (but don't try to use 
lynx's view source to find the name of the button, it'll eat your 
comment...), clear the text and write my longer comment.

Note that my comment doesn't really need to indicate who gave r= and sr= 
since the attachment lists those flags, if one person marked a flag on 
behalf of someone else then I would definitely want to indicate it. I also 
don't really need to indicate that neil is a module peer since drivers 
should know who the peers are, but it won't hurt.
Attachment #131194 - Flags: approval1.5?
Comment on attachment 131194 [details] [diff] [review]
J=>Junk, shift-J=>NotJunk

Too late for features and l10n impact changes (that aren't extremely
high-reward)
Attachment #131194 - Flags: approval1.5? → approval1.5-
Oh well. The last stage in a bug's life is verification. It's
actually described in a lifecycle of a bug. It does help to have a
qa who still works on or uses the product. Traditionally the QA will
indicate on which builds and platforms the item was tested and that
it worked (or that it didn't) and any other useful observations
(possibly spinning off new bugs).
Keywords: verifyme
QA Contact: laurel → technutz
*** Bug 217544 has been marked as a duplicate of this bug. ***
Product: Browser → Seamonkey
verified fixed on WinXP and Linux with a 20050106 trunk build.
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: