Workspace move editor actions #21760
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cla-bot
bot
added
the
cla-signed
1 task
|
Marked as ready for review based on this comment: #20205 (comment) |
|
One counterpoint I can think of, is if you start by creating all the splits the way you like and then proceed to move items around, you may close a pane when you didn't meant to. But otherwise there is no merit to keep it, it looks wrong and there is no way to close it through UI at the moment.
I tried moving an editor to the terminal pane with the mouse and it didn't work (I mean it pasted the file path as, I assume, intended). Now after your comment I realize that moving terminals within the terminal pane should be possible in the same fashion, right... I'll look into it and add it tomorrow. Have you thought about a more unified model to pane splitting and navigation for editors and termianls? |
That reason is good, but seems rather small compared to the inactionable panels?
I have thought about it, and decided that's hard to do given the fact that terminal panel restricts other kinds of items, and differently reacts to files being dropped inside it. zed/crates/terminal_view/src/terminal_panel.rs Lines 1122 to 1135 in 26ba48e This PR does pretty much the same, but not in a unified way, so I can return the question back. |
|
Fair enough. Foolish consistency, yada yada... I'll add similar calls to the terminal crate, and adjust the original if needed. Found a bug in 2024-12-10_14-56-11.mp4Error log from the linux machineAlso, question from the issue thread about keybinding: "ctrl-shift-1": ["workspace::MoveItemToPane", 0],
"ctrl-shift-2": ["workspace::MoveItemToPane", 1],
"ctrl-shift-3": ["workspace::MoveItemToPane", 2],
"ctrl-shift-4": ["workspace::MoveItemToPane", 3],
"ctrl-shift-5": ["workspace::MoveItemToPane", 4],
"ctrl-shift-6": ["workspace::MoveItemToPane", 5],
"ctrl-shift-7": ["workspace::MoveItemToPane", 6],
"ctrl-shift-8": ["workspace::MoveItemToPane", 7],
"ctrl-shift-9": ["workspace::MoveItemToPane", 8]
|
For that, it's better to do incremental, potentially smaller updates instead of changing the entire mechanism at once. Later, after allowing to split/move things however we like, one can go and land an approach on top, dealing with all actions that may leave the pane empty — deciding what to do in this case seems like a good task on its own. So the only foolish thing I see here is the appearance of this word in the discussion and, overall, hastiness to slap labels on things.
|
Igonato
force-pushed
the
workspace-move-editor-actions
branch
from
a58bf75 to
0c2340f
Compare
1 task
Igonato
force-pushed
the
workspace-move-editor-actions
branch
2 times, most recently
from
f8d3f2c to
a9252ae
Compare
Igonato
commented
Dec 10, 2024
|
Implemented the actions for the terminal panel and cleaned things up a bit. I think, in it's current state, it can pass as an "incremental, potentially smaller update". Intentionally missing:
And, question about hotkeys, @ConradIrwin, please, give us your judgement about #20205 (comment) and, maybe look at the #14817 as well? I'd like to see that implemented, maybe after this PR, I'll try making that happen next. Thanks! |
Igonato
force-pushed
the
workspace-move-editor-actions
branch
from
a9252ae to
6ed4888
Compare
|
@Igonato for vim we should use In the current state of things, closing the empty pane is consistent with how Zed does things (and with vim :D), so for getting this shipped, we should close them. If we come back to #14187, I think the two questions to answer are:
|
That's how it's implemeneted right now.
This had me confused for a minute :D
If one to get an inspiration from zellij layouts, then something similar to |
|
Can any mac user install Sublime and post contents of the |
Sublime keymap |
Igonato
force-pushed
the
workspace-move-editor-actions
branch
from
6ed4888 to
63a60f9
Compare
|
Noticed something, when switching ( 2024-12-11_19-42-07.mp4 |
|
Question: I'd like to add an option to keep the focus in the source pane (as suggested in #14817). What is the best way to add it? Is there an action that has default params I can look at? Would it be better to add as set of separate cations On a related note: what do you think about a set of unparameterized actions that will appear in the command palette for discoverability ( |
|
Not sure about the lag, one idea might be that a moved element is not redrawing timely (
I think that adding another parameter makes more sense than a setting or separate actions, as people can bind it however they like this way. zed/crates/editor/src/actions.rs Lines 18 to 23 in 937186d zed/assets/keymaps/default-macos.json Lines 116 to 117 in 937186d is one example of such definitions. |
crates/workspace/src/pane_group.rs
Outdated
| @@ -121,6 +121,35 @@ impl PaneGroup { | |||
| }; | |||
| } | |||
|
|
|||
| pub fn move_active_item( | |||
There was a problem hiding this comment.
Looking at the code better, it looks like we have
zed/crates/workspace/src/workspace.rs
Lines 6150 to 6180 in 7b7d5dc
which does almost the same thing but also focuses the right pane?
Let's unity them (by removing this method and moving the existing one here; optionally we can also replace remove_item_and_focus_on_pane noop wrapper with remove_item).
I think, that focusing the last pane is important, and might solve the observed lag issues.
There was a problem hiding this comment.
Yep. I missed it when I was reading the code. I'll see where and how it is used and merge them into one if it makes sense.
The lag isn't related to the move. It happens on ctrl-tab too. Something to do with chagning the active tab when the tab has a certain LSP enabled, maybe? I'll try to narrow it down
There was a problem hiding this comment.
I would have used the workspace one instead of the new one, just moved it into the PaneGroup (and removed that useless remove_item_and_focus_on_pane wrapper).
For the lag, I wonder if it already exists on main before the changes?
If focus-related changes do not improve things and it is on main already, we can move on and fix that lag separately.
|
Thank you for the review @SomeoneToIgnore. I'll go over everything later tonight. |
Igonato
force-pushed
the
workspace-move-editor-actions
branch
2 times, most recently
from
20acc2c to
0cc4d13
Compare
|
Added support for optional focus. Example bindings: "ctrl-shift-1": ["workspace::MoveItemToPane", { "destination": 0 }],
"ctrl-shift-2": [
"workspace::MoveItemToPane",
{ "destination": 1, "focus_destination": false }
],
"alt-shift-h": [
"workspace::MoveItemToPaneInDirection",
{ "direction": "Left" }
],
"alt-shift-l": [
"workspace::MoveItemToPaneInDirection",
{ "direction": "Right" }
],
"alt-shift-k": [
"workspace::MoveItemToPaneInDirection",
{ "direction": "Up" }
],
"alt-shift-j": [
"workspace::MoveItemToPaneInDirection",
{ "direction": "Down" }
],
"ctrl-alt-shift-h": [
"workspace::MoveItemToPaneInDirection",
{ "direction": "Left", "focus_destination": false }
],
"ctrl-alt-shift-l": [
"workspace::MoveItemToPaneInDirection",
{ "direction": "Right", "focus_destination": false }
],
"ctrl-alt-shift-k": [
"workspace::MoveItemToPaneInDirection",
{ "direction": "Up", "focus_destination": false }
],
"ctrl-alt-shift-j": [
"workspace::MoveItemToPaneInDirection",
{ "direction": "Down", "focus_destination": false }
],Reviewed The lag is in the |
SomeoneToIgnore
approved these changes
Dec 17, 2024
Analogous to the Sublime's ctrl+shif+*number* shortcut
Closes zed-industries#20205 An action to move the active editor to a pane in a specified direction
Also add an example of a new settings field usage into the default keymaps.
# Conflicts: # crates/terminal_view/src/terminal_panel.rs
SomeoneToIgnore
force-pushed
the
workspace-move-editor-actions
branch
from
7fad903 to
2bcd893
Compare
helgemahrt
pushed a commit
to helgemahrt/zed
that referenced
this pull request
Dec 18, 2024
Closes zed-industries#20205 Release Notes: - Added `MoveItemToPane` and `MoveItemToPaneInDirection` actions --------- Co-authored-by: Kirill Bulatov <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #20205
Release Notes:
MoveItemToPaneandMoveItemToPaneInDirectionactions