Skip to content
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

Fixes #708 - Prompt for file path when missing from revision #2825

Merged
merged 5 commits into from Dec 6, 2023

Conversation

mogelbrod
Copy link
Contributor

@mogelbrod mogelbrod commented Jul 20, 2023

Description

Fixes #708 - Prompt for file path when missing from revision

Improves the openFileAtRevision() function by:

  • Asking user to enter another path when the requested file doesn't exist in the
    selected revision.
  • Showing an error message on subsequent failure (or if an error is thrown).

The above function is used by a number of commands:

  • gitlens.openFileRevision
  • gitlens.openFileRevisionFrom
  • gitlens.openRevisionFile

Checklist

  • I have followed the guidelines in the Contributing document
  • My changes follow the coding style of this project
  • My changes build without any errors or warnings
  • My changes have been formatted and linted
  • My changes include any required corresponding changes to the documentation (including CHANGELOG.md and README.md)
  • My changes have been rebased and squashed to the minimal number (typically 1) of relevant commits
  • My changes have a descriptive commit message with a short title, including a Fixes $XXX - or Closes #XXX - prefix to auto-close the issue that your PR addresses

@mogelbrod mogelbrod force-pushed the main branch 2 times, most recently from f27c94d to 9ea07d3 Compare July 20, 2023 18:12
@mogelbrod mogelbrod marked this pull request as ready for review July 20, 2023 18:34
@mogelbrod mogelbrod changed the title Fixes #708 - Prompt for file path when missing from revision Adds openFileByRevision command & prompts for file path when missing from revision Jul 23, 2023
@mogelbrod
Copy link
Contributor Author

Would love to get this reviewed (and hopefully merged) @eamodio 😃

@mogelbrod
Copy link
Contributor Author

Rebased the PR onto the latest main branch again, eagerly awaiting a review 😄

@mogelbrod mogelbrod changed the title Adds openFileByRevision command & prompts for file path when missing from revision Add ability to open any file on any branch using quickpick Nov 16, 2023
@eamodio
Copy link
Member

eamodio commented Nov 27, 2023

@mogelbrod Thanks for this -- I've started taking a pass through this and having issues with the OpenFileByRevisionCommand -- it needs an active editor (though shouldn't really) and the keyboarding doesn't feel right. In GitLens (and VS Code in general), the using the right arrow key in a quick pick, when appropriate, leaves the quick pick open and acts upon the selected item (opening it, etc). This functionality should be moved into the pickFileAtRevision quick pick rather than in the refs picker.

But before I dig more into this, I'd love to have the 2 parts split apart. So split out the OpenFileByRevisionCommand into a separate PR, as I think there are more UX implications (naming, etc).

Comment on lines 471 to 511
export async function pickFileAtRevision(
uri: GitUri,
options: {
title: string;
initialPath?: string;
},
): Promise<Uri | undefined> {
const disposables: Disposable[] = [];
try {
const picker = window.createQuickPick();
Object.assign(picker, {
title: options.title,
value: options.initialPath ?? uri.relativePath,
placeholder: 'Enter path to file...',
busy: true,
ignoreFocusOut: getQuickPickIgnoreFocusOut(),
});
picker.show();

const tree = await Container.instance.git.getTreeForRevision(uri.repoPath, uri.sha!);
picker.items = tree.filter(file => file.type === 'blob').map((file): QuickPickItem => ({ label: file.path }));
picker.busy = false;

const pick = await new Promise<string | undefined>(resolve => {
disposables.push(
picker,
picker.onDidHide(() => resolve(undefined)),
picker.onDidAccept(() => {
if (picker.activeItems.length === 0) return;
resolve(picker.activeItems[0].label);
}),
);
});

return pick
? Container.instance.git.getRevisionUri(uri.sha!, `${uri.repoPath}/${pick}`, uri.repoPath!)
: undefined;
} finally {
disposables.forEach(d => {
d.dispose();
});
Copy link
Member

Choose a reason for hiding this comment

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

Let's move this into src/quickpicks/revisionPicker.ts similar to the other pickers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure! Should it be called fileRevisionPicker (or a variation thereof) or revisionPicker? I'm leaning towards the former since it's technically picking a file at a specific revision, rather than picking a revision (unless the "file" part is implicit for revisions?).

src/git/actions/commit.ts Outdated Show resolved Hide resolved
src/git/actions/commit.ts Outdated Show resolved Hide resolved
Comment on lines 490 to 491
const tree = await Container.instance.git.getTreeForRevision(uri.repoPath, uri.sha!);
picker.items = tree.filter(file => file.type === 'blob').map((file): QuickPickItem => ({ label: file.path }));
Copy link
Member

Choose a reason for hiding this comment

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

I'd like to try only putting the base filename as the label and putting the rest of the folder path in the description -- closer to the vscode picker. Will need to enable picker.matchOnDescription as well.

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 tried this now, but vscode appears to not match any of the following items when
picker.value = 'directory/file.tsx':

[
  { label: 'file.tsx', description: 'directory' },
  { label: 'file.tsx', description: 'directory/' },
}

Comment on lines 494 to 526
const pick = await new Promise<string | undefined>(resolve => {
disposables.push(
picker,
picker.onDidHide(() => resolve(undefined)),
picker.onDidAccept(() => {
if (picker.activeItems.length === 0) return;
resolve(picker.activeItems[0].label);
}),
);
});
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice to add the extra keyboard (right arrow) support here to open the selected file in a preview editor.

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 didn't know about this functionality 😄 Not sure how though, do you have any pointers to existing implementations?

Would it potentially break text selections using left/right arrow keys in the text field?

@mogelbrod mogelbrod changed the title Add ability to open any file on any branch using quickpick Fixes #708 - Prompt for file path when missing from revision Nov 29, 2023
Comment on lines 33 to 35
// FIXME: Remove this unless we opt to show the directory in the description
// const parsed = path.parse(file.path)
// return { label: parsed.base, description: parsed.dir }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Left this code in if you wanted to try it out yourself @eamodio. It does depend on path, which appears to be a forbidden import.

@mogelbrod
Copy link
Contributor Author

Thanks for taking the time to dig into this @eamodio!

I've extracted the openFileByRevision command addition into a separate PR. It's currently marked as draft - I'll update it to reflect all changes once this PR lands in main.

 - Avoids breaking ctrl+[left|right] in input
 - Introduces structured `keyboard` object to handle keypress event bindings and actions
Improves the `openFileAtRevision()` function by:
- Asking user to enter another path when the requested file doesn't exist in the
  selected revision.
- Showing an error message on subsequent failure (or if an error is thrown).

The above function is used by a number of commands:
- `gitlens.openFileRevision`
- `gitlens.openFileRevisionFrom`
- `gitlens.openRevisionFile`
Adds keyboard nav to revision picker
Copy link
Member

@eamodio eamodio left a comment

Choose a reason for hiding this comment

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

Thanks for splitting that up -- looks great. I've cleaned up the UX and added some keyboard support.

Thank you so much for your contribution!

Thank you!

@eamodio eamodio merged commit faaf78f into gitkraken:main Dec 6, 2023
2 checks passed
@eamodio eamodio added this to the 14.6 milestone Dec 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

open any file from another branch
2 participants