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

Remove empty group. Or want some other behaviour when empty group? #33

Closed
wants to merge 1 commit into from

Conversation

last-arg
Copy link
Contributor

@last-arg last-arg commented Dec 3, 2024

Not sure if this is the behaviour you want when there no more children in
a group. So this is more of a question what you want to do when there is an
empty group after removing last child?

Bug that brought this up:

  1. Create project (group)
  2. Remove project's editor
  3. Remove project's shell
  4. Try to open projects list (ctrl+a k)

Get and error:

{"level":"error","time":"2024-12-03T14:01:01+02:00","message":"an error occurred while running [ctrl+a k]: expected integer key for array in range [0, 0), got 0"}

This happens because project's children are removed but the project (group) itself
has been not.

When fixing this problem I realized that I was not sure what kind of behaviour
would be wanted when group becomes empty.
Things to consider:

  • Want to keep the empty group?
  • Treat projects and groups differently when they are empty?
  • Have confirmation popup if group is about to removed? Or ask if they want to
    keep empty group?
  • Consider something else?

And while writing this I realized that after removing group there might a
parent group that is now empty. So would have to walk up the tree and check.
So would have to code that if this is the wanted solution.

@cfoust
Copy link
Owner

cfoust commented Dec 4, 2024

Thank you for writing this up! I'm thrilled you're using cy! It seems there's two related things going on here:

  • The bug you encountered in (action/jump-project). Clearly this action should be able to handle empty groups, but it doesn't right now.
  • The semantics of (action/kill-and-reattach), the action run when you hit ctrl+a x that removes the current pane.

The former is just a bug in the Janet code and should be easy to fix. The latter is a bit more subtle.

(action/kill-and-reattach) ultimately calls (tree/rm) to remove a node from the tree. The (tree/rm) function is designed to remove the given node and all of its descendants. Groups are akin to directories, so from the perspective of the API, there's nothing wrong with having an empty group sitting around! If you wanted a version of (action/kill-and-reattach) that recursively removed empty groups upwards after killing a pane, that's quite easy to accomplish completely in Janet. Just let me know if that would be useful.

The more direct issue is probably that there's no quick way to remove an open "project" right now, once they're open, they're open forever. cy's default configuration is intended to be both comprehensive and simple, so this is an oversight for sure. It's also easy to remedy entirely using Janet. If you'd like to take a stab at that, I can offer some suggestions, but otherwise I'll make that happen as soon as I can.

Thanks for using cy! I'd love any other feedback you have.

@last-arg
Copy link
Contributor Author

last-arg commented Dec 5, 2024

Thank you for writing this up! I'm thrilled you're using cy! It seems there's two related things going on here:

* The bug you encountered in [`(action/jump-project)`](https://github.com/cfoust/cy/blob/main/pkg/cy/boot/actions.janet#L160). Clearly this action should be able to handle empty groups, but it doesn't right now.

* The semantics of [`(action/kill-and-reattach)`](https://github.com/cfoust/cy/blob/main/pkg/cy/boot/layout.janet#L543), the action run when you hit `ctrl+a` `x` that removes the current pane.

The former is just a bug in the Janet code and should be easy to fix. The latter is a bit more subtle.

At one point I was considering if this should be fixed in Janet or Go. I think why I leaned more Go is because I didn't see any keybinding or action to remove groups.

(action/kill-and-reattach) ultimately calls (tree/rm) to remove a node from the tree. The (tree/rm) function is designed to remove the given node and all of its descendants. Groups are akin to directories, so from the perspective of the API, there's nothing wrong with having an empty group sitting around! If you wanted a version of (action/kill-and-reattach) that recursively removed empty groups upwards after killing a pane, that's quite easy to accomplish completely in Janet. Just let me know if that would be useful.

I remembered why I wanted to remove a project in the first place. I accidentally created second project in the same directory. Didn't find any keybind or action to remove project. So try to remove project by removing its children. So I don't currently see need for recursive removal.

I did notice that the second project created had the same name as the first project. Maybe add index to project name if there is more than one with same name. Example: "/projects/name-1".

The more direct issue is probably that there's no quick way to remove an open "project" right now, once they're open, they're open forever. cy's default configuration is intended to be both comprehensive and simple, so this is an oversight for sure. It's also easy to remedy entirely using Janet. If you'd like to take a stab at that, I can offer some suggestions, but otherwise I'll make that happen as soon as I can.

Although the only reason I wanted to remove a project was because I created a "duplicate". It would be great to have a way to remove projects (groups).

Ideas/Things to consider:

  • Maybe can remove nodes, groups with keybinding in fuzzy finder?
  • Have separate fuzzy finder to remove nodes, groups?
  • Can remove group with a keybind from inside child node?
  • A way to create shells, tabs attached to group?
  • Want to remove group's children also?
  • Separate actions for removing just a group and removing group + children?

I would be probably faster for somebody who knows Janet to remedy this. Have been using cy out of the box and seeing when I hit a wall when I need to use scripting. Probably hit that wall and will start to look into Janet.

Closing this issue as empty groups are expected.

@last-arg last-arg closed this Dec 5, 2024
@cfoust
Copy link
Owner

cfoust commented Dec 5, 2024

This is all really interesting! Thanks again for all the time you've clearly put into thinking about this stuff. It means a lot to me that you seem to understand cy deeply! Really rewarding to see after all the work I've done. Most of what you describe is both easy to implement just using Janet and also should be in the default configuration anyway. Next week I am about to be very busy and will be unable to work on cy for a couple of months, but if I have a spare couple of hours I think I could write most of this.

Want to remove group's children also?
Separate actions for removing just a group and removing group + children?

Could you elaborate on these two points? I might be misunderstanding this--if you tree/rm a group, all the descendants will be killed, though they could be moved elsewhere beforehand.

A way to create shells, tabs attached to group?

This is a great idea! I think that being able to create nodes that consist of a layout (which is what tabs use) makes sense in cy's model. While this is straightforward to add to the API, I'm going to have to put some thought into how to present this functionality to the user without making it too confusing. Right now, layouts are specific to each client. Moving away from this is logical, but will be tricky to get right. It might be that the API functionality for this comes well before its use in the default configuration.

So in total we have the following, pending any further adjustments:

  • Action for removing projects.
  • Handle duplicate projects (e.g. /projects/name-1).
  • Action for removing any node in the node tree via input/find.
  • Action to remove the group of the current node (which will kill/remove all descendants.)

@last-arg
Copy link
Contributor Author

last-arg commented Dec 5, 2024

Want to remove group's children also?
Separate actions for removing just a group and removing group + children?

Could you elaborate on these two points? I might be misunderstanding this--if you tree/rm a group, all the descendants will be killed, though they could be moved elsewhere beforehand.

Ah yes, I mean tree/rm. Focused too much on the word group and try to find actions to do with group.

A way to create shells, tabs attached to group?

This is a great idea! I think that being able to create nodes that consist of a layout (which is what tabs use) makes sense in cy's model. While this is straightforward to add to the API, I'm going to have to put some thought into how to present this functionality to the user without making it too confusing. Right now, layouts are specific to each client. Moving away from this is logical, but will be tricky to get right. It might be that the API functionality for this comes well before its use in the default configuration.

Idea 1:
Have keybind that creates shell or tab in a group based on the current active pane. In what group the pane is. For example to create new tab in a group use keybind ctrl + a T.

Idea 2:
Or have a keybind that opens fuzzy finder that list groups where you want to create new node. For example have ctrl + a T open a fuzzy finder and ask where to create new tab.

Or even have both options.

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.

2 participants