/desktop

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Reveal in Explorer option to changed file's context menu #1677

Merged
merged 1 commit into from Jun 7, 2017

Conversation

Reviewers

@shiftkey shiftkey

@rlabrecque rlabrecque

@niik niik

Assignees

@niik niik

Labels
ready-for-review
Projects
None yet
Milestone
No milestone
4 participants
@zhuowei @niik @rlabrecque @shiftkey
@zhuowei
Contributor

zhuowei commented May 21, 2017 •
edited by shiftkey

Fixes #1566.

I tested this on Windows 10; I don't have a Mac, but I think this would still work there since it just uses electron's shell api.

ghdesktop_openinfinder

Questions:

  • should I refactor the path normalization code into a helper function? I just copied and pasted it from the repo clone code from the same file.

@shiftkey shiftkey added the ready-for-review label May 22, 2017

@shiftkey

shiftkey requested changes May 22, 2017
View changes

A great start @zhuowei! Some feedback to get things moving...

app/src/ui/changes/changed-file.tsx Outdated
items.push(
{ type: 'separator' },
{
label: 'Open in external editor',

This comment has been minimized.

Sign in to view
Copy link
@shiftkey

shiftkey May 22, 2017

Member

This needs to be platform-arized to Open in External Editor for __DARWIN__.

I'm also not sold on external editor here for Windows, but that's because we didn't get around to implementing this in Classic Window - oops. @niik feels on what we should call this?

This comment has been minimized.

Sign in to view
Copy link
@rlabrecque

rlabrecque May 22, 2017

SourceTree just has 'Open' which actually feels pretty natural. Depending on what the file is it might not be an editor.

image
app/src/lib/dispatcher/dispatcher.ts Outdated
public openInExternalEditor(repository: Repository, filepath: string): boolean {
const fullPath = Path.join(repository.path, filepath)
// because Windows uses different path separators here
const normalized = Path.normalize(fullPath)

This comment has been minimized.

Sign in to view
Copy link
@shiftkey

shiftkey May 22, 2017

Member

Regarding this question:

should I refactor the path normalization code into a helper function? I just copied and pasted it from the repo clone code from the same file.

While it's rather trivial, I think extracting this to a local function would be 💎.

I went with this, but you can totally try a different result if you find something better:

function normalizePath(repository: Repository, filepath: string): string {
  const fullPath = Path.join(repository.path, filepath)
  // because Windows uses different path separators here
  const normalized = Path.normalize(fullPath)
  return normalized
}

This then makes the usage:

const path = normalizePath(repository, filepath)
shell.openItem(path)

@zhuowei zhuowei force-pushed the zhuowei:master branch from 3c5e0ed to 4d906a6 May 22, 2017

@zhuowei

This comment has been minimized.

Sign in to view
Contributor

zhuowei commented May 22, 2017

Pull request updated:

  • per @rlabrecque's suggestion, the open in editor command is now just named "Open"
  • moved the path join and normalization code to a helper function
@niik

niik requested changes May 22, 2017
View changes

app/src/ui/changes/changed-file.tsx Outdated
action: () => this.props.onOpenInExternalEditor(this.props.path),
},
{
label: __DARWIN__ ? 'Reveal in Finder' : 'Reveal in Explorer',

This comment has been minimized.

Sign in to view
Copy link
@niik

niik May 22, 2017

Contributor

On Windows the expected terminology would be Show in Explorer
@niik

This comment has been minimized.

Sign in to view
Contributor

niik commented May 22, 2017

In the classic app on Windows we would always open a file with the 'Edit' verb to prevent the user from accidentally executing arbitrary code (like .exe, .js, .com etc). IIRC the default 'Open' action on Windows for .js is the windows script host which will happily run whatever you throw at it.

If we couldn't find a registered Edit verb we still wouldn't launch with the 'Open' verb but rather fall back to opening the file in Explorer.

I think we need to implement similar safeguards here although I'm not quite sure how to specify a verb in this environment.

@zhuowei zhuowei force-pushed the zhuowei:master branch from 4d906a6 to 5e8726b May 22, 2017

@zhuowei

This comment has been minimized.

Sign in to view
Contributor

zhuowei commented May 22, 2017

@niik Updated text per comment.

For opening with a separate verb, I can't find any npm modules that can do this, so we may either need to write a module that wraps ShellExecute, or use node-ffi to wrap the function through ffi.

@joshaber joshaber assigned niik and shiftkey May 22, 2017

@rlabrecque

This comment has been minimized.

Sign in to view

rlabrecque commented May 23, 2017 •
edited

So, my problem with the Edit verb is, how do you even change it? I wouldn't want to open my .js files with notepad which is the default 'Edit' action. In this case I'd expect that I would be opening the file in VSCode which is my default after going through, open with -> set as default.

Opening explorer if there was no default association feels weird too, why not just let the OS deal with this?
image
@niik

This comment has been minimized.

Sign in to view
Contributor

niik commented May 23, 2017

For opening with a separate verb, I can't find any npm modules that can do this, so we may either need to write a module that wraps ShellExecute, or use node-ffi to wrap the function through ffi.

Yeah, I think we should land this PR with just the 'Reveal in Finder/Show in Explorer' menu item and push the open option further down the line. It's tricky to get right and on macOS it seems like although there's an equivalent of the edit verb it can't be trusted to not execute code. I think as we start to get into 'Open in Atom/Open in [insert favorite IDE here]' the need for a generic open item will diminish somewhat.
@shiftkey

shiftkey reviewed May 25, 2017
View changes

@shiftkey

This comment has been minimized.

Sign in to view
Member

shiftkey commented May 25, 2017

@zhuowei this looks like it's close, however there's a merge conflict that needs to be 🔥'd first.
@niik

This comment has been minimized.

Sign in to view
Contributor

niik commented May 25, 2017

@shiftkey also #1677 (comment)
@shiftkey

This comment has been minimized.

Sign in to view
Member

shiftkey commented May 25, 2017

@niik 👍 gonna drop myself off this review, but feel free to pull me in if you want an extra set of 👀

@shiftkey shiftkey removed their assignment May 25, 2017

@zhuowei zhuowei dismissed shiftkey’s stale review via 025845a May 26, 2017

@zhuowei zhuowei force-pushed the zhuowei:master branch from 5e8726b to 025845a May 26, 2017

@zhuowei zhuowei changed the title from Add Open in external editor, Reveal in Explorer options to context menu to Add Reveal in Explorer option to changed file's context menu May 26, 2017

@zhuowei

This comment has been minimized.

Sign in to view
Contributor

zhuowei commented May 26, 2017

@niik Open button removed; only the Reveal in Finder/Open in Explorer menu item is left. Also, conflict resolved.
@niik

niik requested changes May 26, 2017
View changes

Looking good @zhuowei, I had a few questions 👇

app/src/lib/dispatcher/dispatcher.ts Outdated
function joinAndNormalizePath(repository: Repository, filepath: string): string {
const fullPath = Path.join(repository.path, filepath)
// because Windows uses different path separators here
const normalized = Path.normalize(fullPath)

This comment has been minimized.

Sign in to view
Copy link
@niik

niik May 26, 2017

Contributor

I'm not sure I see why this is necessary? Some initial testing on my machine seems to suggest that Path.join will normalize paths for us.

> Path.join('c:\\', 'users/markus olsson')
'c:\\users\\markus olsson'
> Path.join('c:\\', 'users/markus olsson/Documents')
'c:\\users\\markus olsson\\Documents'
> Path.join('c:\\users', 'niik/markus olsson/Documents')
'c:\\users\\niik\\markus olsson\\Documents'
> Path.join('c:\\users', 'niik/markus olsson/Documents\\GitHub')
'c:\\users\\niik\\markus olsson\\Documents\\GitHub'
> Path.join('c:\\users', 'niik/markus olsson/Documents\\GitHub/desktop')
'c:\\users\\niik\\markus olsson\\Documents\\GitHub\\desktop'
> Path.join('c:\\users', 'niik/../markus olsson/Documents\\GitHub/desktop')
'c:\\users\\markus olsson\\Documents\\GitHub\\desktop'

@shiftkey It seems you wrote this back in the day can you recall why this was necessary? If we can come up with a case where it is necessary I think we should add a test which highlights that scenario. If not I think we should ditch this function in favor of just joining the paths inline

This comment has been minimized.

Sign in to view
Copy link
@shiftkey

shiftkey May 26, 2017

Member

@niik gosh, I'm trying to recall but I really can't. Maybe this was something that came from the Node 6.x days that is no longer needed?
app/src/ui/changes/changed-file.tsx Outdated
@@ -100,6 +101,15 @@ export class ChangedFile extends React.Component<IChangedFileProps, void> {
})
}
if (this.props.status !== FileStatus.Deleted) {

This comment has been minimized.

Sign in to view
Copy link
@niik

niik May 26, 2017

Contributor

Could we still display this item but set it as disabled when the file is not around on disk?
app/src/ui/changes/sidebar.tsx Outdated
@@ -113,6 +113,10 @@ export class ChangesSidebar extends React.Component<IChangesSidebarProps, void>
this.props.dispatcher.ignore(this.props.repository, pattern)
}
private onRevealInFileManager = (path: string) => {

This comment has been minimized.

Sign in to view
Copy link
@niik

niik May 26, 2017

Contributor

Can we document this please, specifically with information about how the path parameter is expected to be a path relative to the repository root.
app/src/ui/changes/changes-list.tsx Outdated
@@ -26,6 +26,7 @@ interface IChangesListProps {
readonly onCreateCommit: (message: ICommitMessage) => Promise<boolean>
readonly onDiscardChanges: (file: WorkingDirectoryFileChange) => void
readonly onDiscardAllChanges: (files: ReadonlyArray<WorkingDirectoryFileChange>) => void
readonly onRevealInFileManager: (path: string) => void

This comment has been minimized.

Sign in to view
Copy link
@niik

niik May 26, 2017

Contributor

Can we document this please, specifically with information about how the path parameter is expected to be a path relative to the repository root.
app/src/ui/changes/changed-file.tsx Outdated
@@ -16,6 +16,7 @@ interface IChangedFileProps {
readonly include: boolean | null
readonly onIncludeChanged: (path: string, include: boolean) => void
readonly onDiscardChanges: (path: string) => void
readonly onRevealInFileManager: (path: string) => void

This comment has been minimized.

Sign in to view
Copy link
@niik

niik May 26, 2017

Contributor

Can we document this please, specifically with information about how the path parameter is expected to be a path relative to the repository root.
app/src/lib/dispatcher/dispatcher.ts Outdated
@@ -841,6 +841,11 @@ export class Dispatcher {
return this.appStore._setConfirmRepoRemoval(value)
}
public revealInFileManager(repository: Repository, filepath: string): boolean {

This comment has been minimized.

Sign in to view
Copy link
@niik

niik May 26, 2017

Contributor

Can we document this please, specifically with information about how the path parameter is expected to be a path relative to the repository root. Could we also rename 'filepath' to either just 'path' or (preferably) 'relativeFilePath'

@zhuowei zhuowei force-pushed the zhuowei:master branch from 025845a to 386ce36 Jun 6, 2017

@zhuowei

This comment has been minimized.

Sign in to view
Contributor

zhuowei commented Jun 6, 2017

Addressed @niik's comments.
@zhuowei
Add Reveal in Explorer option to changed file's context menu 
This fixes parts of issue #1566.
Loading status checks…
9e23aa8

@zhuowei zhuowei force-pushed the zhuowei:master branch from 386ce36 to 9e23aa8 Jun 7, 2017

@zhuowei

This comment has been minimized.

Sign in to view
Contributor

zhuowei commented Jun 7, 2017

Rebased to fix build.
@niik

niik approved these changes Jun 7, 2017
View changes

@niik

This comment has been minimized.

Sign in to view
Contributor

niik commented Jun 7, 2017

Nicely done @zhuowei, thank you!

@niik niik merged commit ab3be5f into desktop:master Jun 7, 2017
2 checks passed

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ofek ofek referenced this pull request Jun 29, 2017

Closed

Feature request: rebase #1652

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment