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 and improvements to pane context menu #21000

Closed

Conversation

Poldraunic
Copy link
Contributor

@Poldraunic Poldraunic commented Nov 21, 2024

Small bunch of QoL changes that I've noticed today.

For the out-of-project files action "Copy Relative Path" was available, but copied empty string. Check for this action was in place, but the relative path was empty string and not None. For the same out-of-project file action "Reveal in Project Panel" was available and did nothing.

And new action "Reveal in File Manager" that allows to open this file in the file manager. This is especially useful for the same out-of-project file.

Release Notes:

  • N/A or Added/Fixed/Improved ...

@@ -2190,7 +2195,9 @@ impl Pane {
.read(cx)
.item_for_entry(entry, cx)
.and_then(|item| item.project_path(cx))
.map(|project_path| project_path.path);
.map(|project_path| project_path.path)
.map_or(None, |path| if path.exists() { Some(path) } else { None });
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 followed it down to this

pub fn find_worktree(
&self,
abs_path: &Path,
cx: &AppContext,
) -> Option<(Model<Worktree>, PathBuf)> {
for tree in self.worktrees() {
if let Ok(relative_path) = abs_path.strip_prefix(tree.read(cx).abs_path()) {
return Some((tree.clone(), relative_path.into()));
}
}
None
}

I am not familiar how Worktrees work yet, but it seems that even for a single out-of-project file Worktree is created with the same Path. Meaning this Path::strip_prefix() will nuke whole path and we'll be left with empty string.

I considered checking paths here for equality and returning None if they're equal. This code is pretty low level and I don't know if this would be correct thing to do.

.map(|project_path| project_path.path);
.map(|project_path| project_path.path)
.map_or(None, |path| if path.exists() { Some(path) } else { None });
let has_relative_path = relative_path.is_some();
Copy link
Contributor Author

@Poldraunic Poldraunic Nov 21, 2024

Choose a reason for hiding this comment

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

This is also sus. when_some() call requires moving value, but the same value is used to check in two different places. I don't know what would be an idiomatic way. Calling clone() would be fine, I guess? But for this check I don't need a value at all.

@notpeter notpeter added the cla-signed The user has signed the Contributor License Agreement label Nov 22, 2024
This is applicable when the file does not belong to any real worktree. We get relative path by trying to remove every available worktree path as prefix of the current file. And when we get to worktree this file "belongs" to we get an empty string.

With this change "Copy Relative Path" action will no longer be available for these kinds of files.
… a worktree

Building on the previous commit before we now also hide this action.
@Poldraunic Poldraunic force-pushed the pane-context-menu-tweaks branch from cde6680 to d2321fc Compare December 2, 2024 09:39
@Poldraunic
Copy link
Contributor Author

Any chance someone takes a look at this? :^)

@mikayla-maki
Copy link
Contributor

mikayla-maki commented Dec 13, 2024

Hi @Poldraunic

Could you add some succinct release notes to your PR? Not quite sure what this PR is intended to address :)

@maxdeviant
Copy link
Member

@Poldraunic Could we split these changes up into separate PRs that are independently reviewable/mergeable?

@maxdeviant maxdeviant closed this Jan 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed The user has signed the Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants