Skip to content
This repository has been archived by the owner on Feb 26, 2021. It is now read-only.

Fix #1081, switch to next/previous window #1129

Merged
merged 4 commits into from
Mar 3, 2020

Conversation

yuanyu90221
Copy link
Contributor

No description provided.

Copy link
Contributor

@ianb ianb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few style notes. Also it looks you need to run npm run format to put everything in the normal style.

if ( direction === "next" ) {
targetIndex = Math.floor((currentWindowIndex + 1) % len);
} else {
targetIndex = Math.floor((currentWindowIndex - 1 + len) % len);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Math.floor won't be necessary here, everything is an integer so it should stay an integer.

const len = windowArray.length;
// find currentWindowId postion in array
const currentWindowIndex = windowArray.findIndex((window) => (window.id === currentWindowId));
let targetIndex = 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't need to be initialized to anything, since it will definitely be reset below.

// error handle for
function onError(error) {
// console.log(`Error: ${error}`);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There doesn't need to be any error handler, if there's an error in .run() the intentRunner will catch it.

let direction = "next";
if ( context.parameters ) {
direction = context.parameters.direction;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will definitely be set, so you can just do let direction = context.parameters.direction without the if.

await browser.windows.update(targetWindowId, {focused: true});
} catch (err) {
onError(err);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can remove the entire try/catch, and the comments all match the code so they aren't really necessary either.

description = "Switch the current window"
match = """
switch to next window [direction=next]
switch to previous window [direction=back]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd do the phrase something like:

match = """
(switch | activate | focus) (the | to |) next window [direction=next]
(switch | activate | focus) (the | to |) (previous | last) window [direction=back]
"""

@ianb
Copy link
Contributor

ianb commented Mar 3, 2020

Looking good, thank you for your contribution!

@ianb ianb merged commit 7550ac9 into mozilla-extensions:master Mar 3, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants