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

Right click to copy support #5935

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

IamTheLime
Copy link

@IamTheLime IamTheLime commented Feb 22, 2025

Adds right click to copy support to ghostty.

Only gtk context menu is currently being disabled.

Issue 4404

.linux => .true,
.macos => .true,
else => .false,
},

@"copy-on-right-click": CopyOnMouseAction = switch (builtin.os.tag) {
Copy link
Contributor

@pluiedev pluiedev Feb 22, 2025

Choose a reason for hiding this comment

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

Docs (also mention this is GTK only for now)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that this will partially work on macOS, except that it will both copy the selection and open up the context menu. I can't test, but that's my intuition.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, unfortunately I don't have a macbook at hand to test this on but the copy should work however the menu will still pop up. I will add the option to the docs

Comment on lines 1516 to 1518
.linux => .false,
.macos => .false,
else => .false,
Copy link
Contributor

Choose a reason for hiding this comment

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

If it's just disabled everywhere then... just make it false?

Copy link
Author

Choose a reason for hiding this comment

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

Good point

@jcollie
Copy link
Collaborator

jcollie commented Feb 23, 2025

Before we start merging pull requests, we should really spend some more time thinking about the best config for this.
Having a dedicated copy-on-right-click config seems way too specialized. Things will quickly get out of hand as we want to do other things with mouse button actions. In particular, pasting (instead of copying) with right click seems like a viable option as well. If we don't develop full-blown mouse button remapping as discussed in #3848 we should at least make the right click config a little more extensible.

I'd recommend something like this:

right-click-action = open-context-menu
right-click-action = copy
right-click-action = paste

open-context-menu would be the default.

src/Surface.zig Outdated
@@ -3103,14 +3109,15 @@ pub fn mouseButtonCallback(
// If we already have a selection and the selection contains
// where we clicked then we don't want to modify the selection.
if (self.io.terminal.screen.selection) |prev_sel| {
try self.copyOnMouseAction(prev_sel, self.config.copy_on_right_click);
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you already have a selection and then right click outside the selection you get two "Copied to Clipboard" toasts (with --copy-on-right-click=clipboard) so one of these calls to copyOnMouseAction seems redundant or misplaced.

Copy link
Author

Choose a reason for hiding this comment

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

You are absolutely correct, I moved this inside the subsequent conditional, where you are clicking inside an existing selection.

src/Surface.zig Outdated
// If copy on select is false then exit early.
if (self.config.copy_on_select == .false) return;

// clear the clipboard. If the selection is set, we only set the clipboard
Copy link
Contributor

Choose a reason for hiding this comment

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

Insanely minor but

Suggested change
// clear the clipboard. If the selection is set, we only set the clipboard
// Clear the clipboard. If the selection is set, we only set the clipboard

Comment on lines 1515 to 1528
/// Whether to copy selected text to the clipboard on right mouse click. `true`
/// will prefer to copy to the selection clipboard, otherwise it will copy to
/// the system clipboard.
///
/// The value `clipboard` will always copy text to the selection clipboard
/// as well as the system clipboard.
///
/// Middle-click paste will always use the selection clipboard. Middle-click
/// paste is always enabled even if this is `false`.
///
/// The default value is false for all systems.
///
/// At present this will only disable the context menu on Linux in the future
/// this shoul also disable the context menu on macOS
Copy link
Author

Choose a reason for hiding this comment

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

Kept the copy of this identical to copy-on-select Is the preference to reference the existing config for things like the behaviour of clipboard?

@IamTheLime
Copy link
Author

Before we start merging pull requests, we should really spend some more time thinking about the best config for this. Having a dedicated copy-on-right-click config seems way too specialized. Things will quickly get out of hand as we want to do other things with mouse button actions. In particular, pasting (instead of copying) with right click seems like a viable option as well. If we don't develop full-blown mouse button remapping as discussed in #3848 we should at least make the right click config a little more extensible.

I'd recommend something like this:

right-click-action = open-context-menu
right-click-action = copy
right-click-action = paste

open-context-menu would be the default.

This is a reasonable enough ask, and I think it makes sense.

I just read through @mitchellh's response on the original issue to this same suggestion

Should I close this MR and re-open one with this behaviour or even close it on a permanent basis until there is a proposal for a more permanent solution?

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.

4 participants