Split every tab in two panes #206

gelisam · 2021-10-28T13:32:08Z

My goal with this PR is to eventually implement a split-panes feature:

the user can split any terminal into two smaller terminals, either by splitting the area horizontally or vertically.
the user can close any terminal by exiting the shell, in which case the unused area is given back to some other terminal.
each tab has its own tree of panes, and each tab remembers which leaf in that tree has the focus, so that it can be restored when switching between tabs.

Furthermore, I would like to make that implementation customizable. As a step towards that goal, in this draft PR every tab is always split vertically into two panes; one terminal on the left, and one terminal on the right.

on my keyboard, those are much more convenient to press than the regular plus and minus keys. the regular plus key is shift+equals, so increasing the font size requires a ctrl+shift combination while decreasing it only requires ctrl, which is awkward.

I eventually want to split terminals by replacing one terminal by two terminals inside a Paned. As a first step towards that, can I put every terminal inside a Paned (with a dummy button on the other side of the pane) without breaking the rest of the code?

I want to add a TabPaned, and it seems weird for the ScrolledWindow to be identified more vaguely than the other widgets surrounding the terminal.

compareScrolledWinAndTab was used to find the FLTab entry corresponding to a given Widget in a Notebook. Now that Notebook entries are Paned widgets containing a ScrolledWindow containing a terminal, and not simply a ScrolledWindow containing a terminal, we need to search for the Paned, not for the ScrolledWindow.

We can't show a Paned any more that we can show a ScrolledWindow, but at least we can include a dummy entry for it in the Show instance, to match the record's real shape.

We were previously trying to detach the ScrolledWindow, but since the Notebook now contains Paned widgets, it was not found and the tab was not detached.

so that it also describes the new Paned field.

Like the last few commits, the relabelling code was looking for a ScrolledWindow but should now be looking for a Paned.

I previously edited the invariant checks so they would drill down the Paned in order to find the ScrolledWindow and compare it with the TMNotebookTab's ScrolledWindow. But directly comparing the Paned with the TMNotebookTab's Paned is much simpler.

Since each of the two terminals have a ScrolledWindow associated with them, this required moving the ScrolledWindow field from the TMNotebookTab to the TMTerm. More importantly, this refactoring revealed many places where the two terminals were treated differently, and that code was changed to treat both terminals the same. This was made easier by the addition of a lens pointing to the focused terminal (which is currently always the left terminal), and a traversal pointing at both terminals.

Also, update the tmNotebookTabFocusIsOnLeft field to reflect the currently-focused pane. The (followUp <- do ...) pattern is needed in order to avoid a deadlock when the onWidgetFocusInEvent callback tries to grab the MVar which is already locked.

gelisam · 2021-10-28T13:32:50Z

src/Termonad/App.hs

@@ -478,13 +478,13 @@ setupTermonad tmConfig app win builder = do
  void $ onSimpleActionActivate enlargeFontAction $ \_ ->
    modifyFontSizeForAllTerms (modFontSize 1) mvarTMState
  actionMapAddAction app enlargeFontAction
-  applicationSetAccelsForAction app "app.enlargefont" ["<Ctrl>plus"]
+  applicationSetAccelsForAction app "app.enlargefont" ["<Ctrl>KP_Add"]


oops! this doesn't belong to this draft PR...

gelisam · 2021-10-28T13:46:21Z

src/Termonad/Types.hs

+      for_ maybeContainer $ \container -> do
+        children <- Gtk.containerGetChildren container
+        for_ children $ \child -> do
+          go ("  " <> indent) child


I implemented this function in order to help me debug my implementation, but the now-debugged code is not using it anywhere, so I should probably remove it.

This is really nice. I've wanted something like this before, so I opened an issue on haskell-gi to see if it could potentially be included in gi-gtk: haskell-gi/haskell-gi#374

I don't think it makes sense to only have this in the Termonad codebase, since it seems widely helpful.

Although, if this type of function is not accepted upstream, let's move it to the Termonad.Gtk module, since there are already a few other helper functions in there.

I have now moved printWidgetTree to Termonad.Gtk, let's remove when/if it is accepted upstream.

gelisam · 2021-10-28T13:48:02Z

src/Termonad/App.hs

  nextPageAction <- simpleActionNew "nextpage" Nothing
  void $ onSimpleActionActivate nextPageAction $ \_ ->
    termNextPage mvarTMState
  actionMapAddAction app nextPageAction
-  applicationSetAccelsForAction app "app.nextpage" ["<Ctrl>Page_Down"]
+  applicationSetAccelsForAction app "app.nextpage" ["<Ctrl><Shift>Page_Down"]


Probably a controversial change! Switching between tabs is a bigger change than switching between panes, and it makes more sense to me to use the the Shift version of the hotkey for the bigger change.

I agree with your reasoning here, although I don't personally use the <Ctrl>Page_Down key.

I imagine I might have to finally start working on #83 !

Hmm, now that I think about it, this key combination only makes sense because with the two-panes approach, there is an obvious next-pane and prev-pane. Once arbitrary splittings are allowed, we'll need key combinations to move up, down, left and right, not prev and next. So I think <Ctrl>Page_Down can continue to be reserved for tabs.

I left some example solutions here earlier.

tmux by way of example, employs a "previous / next" concept so that you can use:

ctl+b then ; to toggle previous pane (effectively switches between two panes)

ctl+b then { or ctl+b then } for moving panes

ctl+b then arrow key for simply changing to the next pane in that direction.

src/Termonad/Lenses.hs

gelisam · 2021-10-28T13:58:46Z

src/Termonad/Term.hs

@@ -141,6 +146,14 @@ focusTerm i mvarTMState = do
 altNumSwitchTerm :: Int -> TMState -> IO ()
 altNumSwitchTerm = focusTerm

+termTogglePane :: TMState -> IO ()


Since there are only two panes, "nextPane" and "prevPane" are currently both equivalent to "togglePane". Once I implement recursively splitting the panes, "nextPane" and "prevPane" will become a lot more difficult, because the panes are layed out in 2D, not in 1D. I think moving up, down, left and right makes more sense than moving to the previous and next panes.

I spent some time thinking about which semantics would make more sense for those movements, and simple algebraic operations for moving to a sibling node in the tree of panes didn't result in very natural movement; for example, if the panes are neatly organized as a 2x2 grid, moving right from the bottom left terminal might lead to the top-right terminal rather than the bottom-right. So the algorithm I currently have in mind is to plot the center points of all the terminals in 2D space, filter down to those points which are in the desired direction, e.g. if moving to the right, only looking at the center points whose X coordinates are larger than the current pane's X coordinate, then picking the remaining terminal whose center point is the closest to the current terminal in Euclidian or Manhattan distance.

This is another good question. I'm not sure what would be the best idea here.

I said it in https://github.com/cdepillabout/termonad/pull/206/files#r785424016 as well, but if I was going to implement this feature myself, I would probably start by taking a look at a few other programs that have panes and seeing exactly what operations they expose. And then try to work out how those operations work.

Other than that, I don't really have any good suggestions. It sounds like you've definitely put more thought into this than I have!

gelisam · 2021-10-28T19:28:41Z

src/Termonad/Term.hs


  -- Connect callbacks
  void $ onButtonClicked tabCloseButton $ termClose notebookTab mvarTMState
-  void $ onTerminalWindowTitleChanged vteTerm $ do
+  void $ onTerminalWindowTitleChanged vteTermL $ do
+    -- TODO: use the title of the focused pane


this TODO shouldn't be too hard, I just haven't got around to it yet

gelisam · 2021-10-28T19:29:27Z

src/Termonad/Term.hs

+    modifyMVar_ mvarTMState $ \tmState -> do
+      pure $ tmState & lensTMStateNotebook
+                     . traversalTMNotebookFocusedTab
+                     . lensTMNotebookTabFocusIsOnLeft .~ True


Here, and in many other places, I really wasn't sure how to wrap lines around to match your style. Do you use a code formatter?

I used to use hindent, so the style I use sort of comes from that, but I don't have a strong opinion on code style. At this point multiple people have touched the code-base, so I don't think there is a single style used absolutely everywhere.

I think your code here is fine as-is.

At some point I guess it would be nice to pick a code formatter and set it up to run automatically.

gelisam · 2021-10-28T19:30:58Z

src/Termonad/Term.hs

-  -- Put the keyboard focus on the term
-  setFocusOn tmStateAppWin vteTerm
+    relabelTab notebook tabLabel paned vteTermL
+  for_ @[Terminal] [vteTermL, vteTermR] $ \vteTerm -> do


I don't quite know how ClassyPrelude's type inference manages to be so poor, but even though [vteTermL, vteTermR]'s type is not ambiguous in the slightest, ClassyPrelude's for_ still needs a type application here.

This looks like it might be a bad interaction between ClassyPrelude and OverloadedLists:

termonad/termonad.cabal

Line 122 in 36be8b0

, OverloadedLists

I like your use of TypeApplications here. It is very readable.

gelisam · 2021-10-29T01:55:19Z

src/Termonad/Types.hs

+    -- ^ The left 'TMTerm'.
+  , tmNotebookTabRightTerm :: !TMTerm
+    -- ^ The right 'TMTerm'.
+  , tmNotebookTabFocusIsOnLeft :: !Bool


Thinking out loud about this field, tmNotebookTabFocusIsOnLeft.

Currently, there are only two panes, a left one and a right one. With proper split panes, there would be a binary tree of panes, where leaves are terminals and internal nodes are either vertical or horizontal splits. Another possibility would be to use a tree of FocusLists instead of a binary tree. That is, we'd never have two consecutive internal nodes which split in the same direction, instead internal nodes at odd depths would be always be vertical splits while internal nodes at even depths would always be horizontal splits.

With only a left and a right terminal, a Bool is sufficient to specify the focus. However, in order to clarify which pane is specified by True, I had to use a very long name, tmNotebookTabFocusIsOnLeft, which I'm not super happy with. So I'm thinking of creating a dedicated FocusLeft | FocusRight datatype in order to support renaming this field to tmNotebookTabFocus.

Whether I keep a Bool or create a new datatype, "Left" is a poor choice of name, because in the binary tree case, there are still two choices, but sometimes the two choices are Left and Right, while other times the two choices are Top and Bottom. So should there be a single type with constructors named LeftOrTop and RightOrBottom? Or First and Second? Or just stick to True and False? Or should there be four constructors, Left, Right, Top and Bottom, and an invariant stating it is not valid to use Top or Bottom when the split direction is Vertical, and similarly for Horizontal? Or should the split direction be a constructor instead of a field, to make illegal states unrepresentable at the cost of more code duplication?

Using the tree of FocusLists avoids these complications, because the focus is an Int, but that doesn't seem like the best justification for that design. One good justification for it though would be to enable a pane-resizing strategy whereby instead of splitting the space of the current terminal into two, the space of the entire row or column of terminals could be redistributed among all the terminals.

Hmm, I am now realizing that with both kinds of trees, every internal node has a field indicating which of their children has the focus. This doesn't seem right, since many of those internal nodes are not the ancestor of the focused terminal. Maybe I instead need a FocusTree equivalent to a FocusList, that is, a Tree whose focus is a path to a leaf?

You have a bunch of good questions here, and I don't really have any good answers.

When hearing "Panes", my first thought is vim, since that's probably the program I use the most that has a Pane-like feature. Vim has a concept of vertical panes vs horizontal panes. Each set of vertical panes (or horizontal panes) allows you to add arbitrarily many vertical panes (or horizontal panes). If you try to add a horizontal pane to a set of vertical panes, you start a new set of horizontal panes that exists in a single vertical pane. So in this case, I think a FocusTree data-type would be appropriate, where each layer of the tree has a different orientation.

Maybe I instead need a FocusTree equivalent to a FocusList, that is, a Tree whose focus is a path to a leaf?

This was my first thought. I'd be happy to give you commit rights to focus-list, or possibly instead create a separate package for some sort of FocusTree datatype.

Although, I guess it would be better to have some implementation of Panes in Termonad, even if it is not ideal. Creating a whole new FocusTree datatype might be a lot of work. We could always improve/generalize it at some point in the future.

If I was going to try to implement panes in Termonad, the first thing I would do is probably take a look at how a few programs that use panes work, including what sorts of operations they provide for panes. The programs I personally use that have panes are:

XMonad

Vim

Emacs

Of these, I think XMonad might be the only program that has a significantly different handling for panes.

A couple questions that would be nice to answer for each of the above programs:

Do panes seem to get held in a binary tree, with each pane individually able to be split? Or are panes held in more of a forest (like Tree)? How are panes automatically laid-out / sized?

What operations are available for re-arranging panes? What are the default key-shortcuts for these operations?

What operations are available for re-sizing panes? What are the default key-shortcuts for these operations?

Let's answer those questions in the discussion for #205, where @craigem already answered the key-shortcuts question for a few applications.

gelisam · 2021-10-29T19:29:48Z

src/Termonad/Types.hs

@@ -546,70 +593,63 @@ invariantTMState' tmState =
    runInvariants = fmap catMaybes . sequence

    invariantFocusSame :: IO (Maybe TMStateInvariantErr)
-    invariantFocusSame = do
+    invariantFocusSame = execExceptT $ do


I switched the invatiant-checking code to use ExceptT, because it simplified the logic when I had repeated checks which could fail. However, I no longer have repeated checks (I used to check that the notebook page was a Paned and that its first child was the expected ScrolledWindow, but now I simply check that the notebook page is the expected Pane), so it would be easy to switch back.

That seems reasonable.

Once we figure out how to represent the Tree(?) of Panes, I imagine we should add another invariant to make sure the Pane Tree in a Tab is represented correctly in tmState. But I imagine this would probably come near the end of the PR, or possibly in a follow-up PR.

cdepillabout · 2021-11-01T06:50:54Z

Thanks for creating this PR. Give me a couple days to find the time to do a good review.

cdepillabout · 2021-11-28T13:48:04Z

Hey, just wanted to let you know that I haven't forgotten about this. I still plan on giving this a good review at some point! I'm not sure when I'll find the time to get to this (since I think this is a really good addition and I want to give adequate time to answer your questions), but hopefully sometime in December. Sorry to keep you waiting on this.

gelisam · 2021-11-28T19:03:55Z

Hmm, I shared what I have so far (the version with exactly 2 panes per tab) so that you could redirect my efforts in the right direction if needed, but if you're too busy to review it, maybe it would make more sense for me to finish the work (allowing an arbitrary number of panes per tab), so that you only have to do one review and not two?

cdepillabout · 2021-11-29T00:11:00Z

maybe it would make more sense for me to finish the work (allowing an arbitrary number of panes per tab), so that you only have to do one review and not two?

Yes, going ahead and writing the code for allowing an arbitrary number of panes per tab does sound reasonable!

craigem · 2021-11-29T00:44:59Z

I was waiting on the arbitrary number of panes too, from a user-testing perspective.

cdepillabout · 2022-01-16T02:29:47Z

src/Termonad/App.hs

+          let followUp = do
+                let newFocus = tab ^. lensTMNotebookTabFocusedTerm . lensTerm
+                widgetGrabFocus newFocus


I'm glad you were able to figure out how to do this. I should really document the need to pull out these types of follow-up IO actions and run them after modifying the MVar. Hope this didn't take too long to figure out.

It wasn't too bad; my initial attempt deadlocked, but I was able to figure out by adding a few print statements here and there.

cdepillabout · 2022-01-16T12:09:51Z

@gelisam This PR looks like a really good start! I'm really happy with your code here.

I've sent you a request to become a maintainer for Termonad. Please feel free to use your judgement and continue on with this PR as you see best. Don't feel like you need to wait for my approval to make decisions about how to implement anything. After you get everything working, feel free to merge in without waiting for me.

I don't want you to feel blocked by my (slow) reviews. If I do end up wanting to change something after you've already merged this PR in, I'm completely fine with either opening an issue, or sending a follow-up PR myself changing something.

Feel free to make this change as well:

diff --git a/README.md b/README.md
index b907df8..3cc3544 100644
--- a/README.md
+++ b/README.md
@@ -519,3 +519,4 @@ would like to add, please submit an issue or PR.
 ## Maintainers
 
 - [cdepillabout](https://github.com/cdepillabout)
+- [gelisam](https://github.com/gelisam)

(If you absolutely don't want to be a maintainer for Termonad, I can try be faster with my reviews as well.)

gelisam · 2022-01-16T18:29:15Z

Thanks! I don't promise to be a very active co-maintainer, as I already maintain too many projects, but precisely for that reason, I appreciate how important it is to have co-maintainers in order to help share the load, so I'll do my best!

gelisam added 16 commits October 13, 2021 22:16

typo

7080542

debugging function, printWidgetTree

51816b8

rename TabTermContainer to TabScrolledWindow

240f1d8

I want to add a TabPaned, and it seems weird for the ScrolledWindow to be identified more vaguely than the other widgets surrounding the terminal.

track the Paned in the TMNotebookTab

e5226ed

include Paned field in Show instance

e582ec2

We can't show a Paned any more that we can show a ScrolledWindow, but at least we can include a dummy entry for it in the Show instance, to match the record's real shape.

detach the Paned on terminal exit

5a6afe6

We were previously trying to detach the ScrolledWindow, but since the Notebook now contains Paned widgets, it was not found and the tab was not detached.

correct TMNotebookTab's comment

b8aa4b0

so that it also describes the new Paned field.

relabel based on the Paned

484ed1d

Like the last few commits, the relabelling code was looking for a ScrolledWindow but should now be looking for a Paned.

more straightforward invariant checks

1cb1bc9

I previously edited the invariant checks so they would drill down the Paned in order to find the ScrolledWindow and compare it with the TMNotebookTab's ScrolledWindow. But directly comparing the Paned with the TMNotebookTab's Paned is much simpler.

two terminals per tab

c1d0834

split the space equally between terminals

4088f70

hotkeys to navigate between panes

9c06b97

Also, update the tmNotebookTabFocusIsOnLeft field to reflect the currently-focused pane. The (followUp <- do ...) pattern is needed in order to avoid a deadlock when the onWidgetFocusInEvent callback tries to grab the MVar which is already locked.

gelisam commented Oct 28, 2021

View reviewed changes

src/Termonad/Lenses.hs Outdated Show resolved Hide resolved

gelisam commented Oct 28, 2021

View reviewed changes

gelisam commented Oct 29, 2021

View reviewed changes

gelisam mentioned this pull request Oct 29, 2021

split panes #205

Open

update comment to reflect focuslist-0.1.1.0 release

bbcd3fa

cdepillabout reviewed Jan 16, 2022

View reviewed changes

cdepillabout mentioned this pull request Jan 16, 2022

debugging function for printing a tree of widgets haskell-gi/haskell-gi#374

Closed

move printWidgetTree to Termonad.Gtk

6dd0c15

adding myself as a co-maintainer

9568b68

gelisam mentioned this pull request Jan 17, 2022

CI failure due to backwards-incompatible aeson change #209

Closed

Merge branch 'master' into always-split

8913213

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split every tab in two panes #206

Split every tab in two panes #206

gelisam commented Oct 28, 2021

gelisam Oct 28, 2021

gelisam Oct 28, 2021

cdepillabout Jan 16, 2022 •

edited

gelisam Jan 16, 2022

gelisam Oct 28, 2021

cdepillabout Dec 7, 2021

gelisam Dec 7, 2021

craigem Dec 9, 2021

gelisam Oct 28, 2021

cdepillabout Jan 16, 2022

gelisam Oct 28, 2021

gelisam Oct 28, 2021

cdepillabout Jan 16, 2022

gelisam Oct 28, 2021 •

edited

cdepillabout Jan 16, 2022

gelisam Oct 29, 2021

cdepillabout Jan 16, 2022

gelisam Jan 16, 2022

gelisam Oct 29, 2021

cdepillabout Jan 16, 2022

cdepillabout commented Nov 1, 2021

cdepillabout commented Nov 28, 2021 •

edited

gelisam commented Nov 28, 2021

cdepillabout commented Nov 29, 2021

craigem commented Nov 29, 2021

cdepillabout Jan 16, 2022

gelisam Jan 16, 2022

cdepillabout commented Jan 16, 2022

gelisam commented Jan 16, 2022

Split every tab in two panes #206

Are you sure you want to change the base?

Split every tab in two panes #206

Conversation

gelisam commented Oct 28, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdepillabout Jan 16, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gelisam Oct 28, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdepillabout commented Nov 1, 2021

cdepillabout commented Nov 28, 2021 • edited

gelisam commented Nov 28, 2021

cdepillabout commented Nov 29, 2021

craigem commented Nov 29, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdepillabout commented Jan 16, 2022

gelisam commented Jan 16, 2022

cdepillabout Jan 16, 2022 •

edited

gelisam Oct 28, 2021 •

edited

cdepillabout commented Nov 28, 2021 •

edited