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

Made "Show in OS" OS-specific #6982

Merged
merged 4 commits into from
Feb 26, 2014
Merged

Conversation

marcelgerber
Copy link
Contributor

This is for #6970, it makes CMD_SHOW_IN_OS OS-specific (Finder on Mac, Explorer on Win).
@couzteau for the german strings

CommandManager.register(Strings.CMD_NEXT_DOC, Commands.NAVIGATE_NEXT_DOC, handleGoNextDoc);
CommandManager.register(Strings.CMD_PREV_DOC, Commands.NAVIGATE_PREV_DOC, handleGoPrevDoc);
CommandManager.register(Strings.CMD_SHOW_IN_TREE, Commands.NAVIGATE_SHOW_IN_FILE_TREE, handleShowInTree);
if (brackets.platform === "win") {
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of having an if for the register command, I think it would look better to do something like

var showInOS = Strings.CMD_SHOW_IN_OS;
if (brackets.platform === "win") {
    showInOS = Strings.CMD_SHOW_IN_EXPLORER;
} else if (brackets.platform === "mac") {
    showInOS = Strings.CMD_SHOW_IN_FINDER;
}

Before the we register all the commands and then just call CommandManager.register(showInOS, Commands.NAVIGATE_SHOW_IN_OS, handleShowInOS);.

The same thing could be applied to the exit command.

An alternative could be to set a string in Strings.js, but not sure about that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about that, too, but then I saw how it is done above (exit command).
But yes, this sounds reasonable. Will change it tomorrow.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure. We could use the same if to set the quit strings too.

@TomMalbran
Copy link
Contributor

BTW, it might be easier to not include the translations in the PR. If you do we need someone else to review them.

@ingorichter
Copy link
Contributor

@SAplayer @TomMalbran IMO, the strings look good.

@marcelgerber
Copy link
Contributor Author

Just pushed a commit.
This has some whitespace changes in it, so I recommend you to review using https://github.com/adobe/brackets/pull/6982/files?w=1
I can remove these changes if you want me to.

@ingorichter ingorichter self-assigned this Feb 24, 2014
showInOS = Strings.CMD_SHOW_IN_EXPLORER;
} else if (brackets.platform === "mac") {
showInOS = Strings.CMD_SHOW_IN_FINDER;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you place both if at lines 1552, so that we can then have all the commands together? You can also use the same if (brackets.platform === "win") for both showInOS and quitString.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the register commands are curently sorted by command type (file, navigate, edit, ...), so this could cause ambiguity.
I'll wait for another opinion.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand... I am saying about moving just the Strings declarations, not the CommandManager.register.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes, must have understood you wrong.
Just pushed another commit.

quitString = Strings.CMD_EXIT;
showInOS = Strings.CMD_SHOW_IN_EXPLORER;
} else if (brackets.platform === "mac") {
showInOS = Strings.CMD_SHOW_IN_FINDER;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

My point was to have it like this:

var quitString  = Strings.CMD_QUIT,
    showInOS    = Strings.CMD_SHOW_IN_OS;
if (brackets.platform === "win") {
...(the rest of the if, else if code)
}

// Register global commands
CommandManager.register(Strings.CMD_FILE_OPEN,          Commands.FILE_OPEN, handleFileOpen);
...(all the other commands)
CommandManager.register(showInOS,                       Commands.NAVIGATE_SHOW_IN_OS, handleShowInOS);

So that there is nothing in between the CommandManager.register and would make it easier to read.

@ingorichter
Copy link
Contributor

This looks good. Thank you both.

ingorichter added a commit that referenced this pull request Feb 26, 2014
Made "Show in OS" OS-specific
@ingorichter ingorichter merged commit 9caa3d5 into adobe:master Feb 26, 2014
@marcelgerber marcelgerber deleted the fix-6970 branch February 26, 2014 20:26
@JeffryBooher
Copy link
Contributor

Thank you @SAplayer -- this has annoyed me since the dawn of time!

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.

4 participants