303805 - RFE: tab switching via command+option arrows (like Camino and Deer Park2 on Mac)

Status: RESOLVED FIXED

(Other Applications :: ChatZilla, enhancement)

Description

[Mick Weiss]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b3) Gecko/20050712 Firefox/1.0+
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b3) Gecko/20050712 Firefox/1.0+

This is a patch to allow tab switching via command+option arrows (like Camino
and Deer Park2 on Mac).

Reproducible: Always

Comment 1

[Mick Weiss]

Attachment: this adds option+command+ arrow tab switching

I'd like to see this in the official builds :-)

Comment 2

[:Gijs (he/him)]

Comment on attachment 191906 [details] [diff] [review]
this adds option+command+ arrow tab switching

I'd set additional-review-, but since you haven't asked for review yet, that's
impossible.

The break statements inside the if-blocks are redundant, they should be
removed.

Furthermore, patches are a lot more readable if you use cvs diff with the -uN
option, where N is the number of lines of context diff will generate. Generally
I use 6 or 8, but that's a matter of personal preference. I've seen people use
12, I've seen them use 2... just make sure there's enough context to understand
where this code ends up and what it does.

Comment 3

[Mick Weiss]

Comment on attachment 191906 [details] [diff] [review]
this adds option+command+ arrow tab switching

sorry, made the wrong change before.

Comment 4

[:Gijs (he/him)]

Comment on attachment 191906 [details] [diff] [review]
this adds option+command+ arrow tab switching

You misunderstood. I'm not a module owner / peer, and therefor not the
appropriate person to ask for review - but I can say what I think about it, and
then set an *additional* review. Which is what I've done now, and I've asked
for a 'real' review from one of rginda's peers (rginda is module owner for
ChatZilla, as far as I'm aware.

Please consider submitting a new patch which copes with the problems I pointed
out (even though my opinion doesn't matter as much as that of the 'real'
reviewer). If you disagree with me, reply here or ask on IRC :-).

Comment 5

[Mick Weiss]

It is pretty straight forward what it does (so didn't think it would matter too
much, how it was diff'ed - it is still readable)... for reference, it is taken
from the latest stable. 

Quite frankly, you could just cut-n-paste :-P

I did misunderstand you, I thought that you had suggestions and wanted to review
it and make your changes (that you mentioned) - before sending it for review to
the module owner. Feel free to upload a newer version of this with the things
that you pointed out (I agree with the points -btw). IMHO I think that a decent
patch should be made before sending it for review to a module owner.

I'm not running a version from cvs, so that diff may break something. When I get
back from the airport, I'll grab a version from cvs and do what you said, if you
haven't already updated it.

for reference: I'm mick_(home|work|laptop|lweiss) on irc.

- Mick

Comment 6

[:Gijs (he/him)]

Attachment: Patch with my own nits fixed.

Seeing as the original maker of this patch seems not to care anymore, here is a
new patch. I *would* want to request *someone* with a mac (*cough* reporter
*cough*) to test this patch. I can't get it to work on this Linux machine (at
uni), but I'm guessing that's catching the commands to do 'something'. Then
again, I presumed the logic of the original patch was sound. The question is,
was that assumption correct?

Comment 7

[Samuel Sieb]

I suppose it might as well be confirmed. :-)
Is someone going to test it?

Comment 8

[:Gijs (he/him)]

Mick tested the patch, and told me it worked on IRC. He's too busy to comment on the bug (that's what he says, anyway :-) ).

Comment 9

[Mick Weiss]

WFM, so can someone just commit it to CVS already ;-)

Comment 10

[Samuel Sieb]

Comment on attachment 197694 [details] [diff] [review]
Patch with my own nits fixed.

works on Linux too ;-)

Comment 11

[Samuel Sieb]

checked in