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

Split every tab in two panes #206

Draft
wants to merge 20 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 27 additions & 9 deletions src/Termonad/App.hs
Original file line number Diff line number Diff line change
Expand Up @@ -154,9 +154,10 @@ import Termonad.PreferencesFile (saveToPreferencesFile)
import Termonad.Term
( createTerms
, relabelTabs
, termExitFocused
, termNextPage
, termPrevPage
, termExitFocused
, termTogglePane
, setShowTabs
, showScrollbarToPolicy
)
Expand Down Expand Up @@ -393,17 +394,22 @@ setupTermonad tmConfig app win builder = do
else setShowTabs tmConfig note

void $ onNotebookSwitchPage note $ \_ pageNum -> do
modifyMVar_ mvarTMState $ \tmState -> do
followUp <- modifyMVar mvarTMState $ \tmState -> do
let notebook = tmStateNotebook tmState
tabs = tmNotebookTabs notebook
maybeNewTabs = updateFocusFL (fromIntegral pageNum) tabs
case maybeNewTabs of
Nothing -> pure tmState
Nothing -> do
pure (tmState, pure ())
Just (tab, newTabs) -> do
widgetGrabFocus $ tab ^. lensTMNotebookTabFocusedTerm . lensTerm
pure $
tmState &
lensTMStateNotebook . lensTMNotebookTabs .~ newTabs
let followUp = do
let newFocus = tab ^. lensTMNotebookTabFocusedTerm . lensTerm
widgetGrabFocus newFocus
Comment on lines +405 to +407
Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

tmState'
= tmState
& lensTMStateNotebook . lensTMNotebookTabs .~ newTabs
pure (tmState', followUp)
followUp

void $ onNotebookPageReordered note $ \childWidg pageNum -> do
maybePaned <- castTo Paned childWidg
Expand Down Expand Up @@ -433,17 +439,29 @@ setupTermonad tmConfig app win builder = do
actionMapAddAction app newTabAction
applicationSetAccelsForAction app "app.newtab" ["<Shift><Ctrl>T"]

nextPaneAction <- simpleActionNew "nextpane" Nothing
void $ onSimpleActionActivate nextPaneAction $ \_ ->
termTogglePane mvarTMState
actionMapAddAction app nextPaneAction
applicationSetAccelsForAction app "app.nextpane" ["<Ctrl>Page_Down"]

prevPaneAction <- simpleActionNew "prevpane" Nothing
void $ onSimpleActionActivate prevPaneAction $ \_ ->
termTogglePane mvarTMState
actionMapAddAction app prevPaneAction
applicationSetAccelsForAction app "app.prevpane" ["<Ctrl>Page_Up"]

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"]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Owner

Choose a reason for hiding this comment

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

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 !

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.


prevPageAction <- simpleActionNew "prevpage" Nothing
void $ onSimpleActionActivate prevPageAction $ \_ ->
termPrevPage mvarTMState
actionMapAddAction app prevPageAction
applicationSetAccelsForAction app "app.prevpage" ["<Ctrl>Page_Up"]
applicationSetAccelsForAction app "app.prevpage" ["<Ctrl><Shift>Page_Up"]

closeTabAction <- simpleActionNew "closetab" Nothing
void $ onSimpleActionActivate closeTabAction $ \_ ->
Expand Down
32 changes: 32 additions & 0 deletions src/Termonad/Lenses.hs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,11 @@ module Termonad.Lenses where
import Termonad.Prelude

import Control.Lens (Lens', Traversal', makeLensesFor, makePrisms)
import Data.FocusList (FocusList)
import Termonad.Types
import qualified Data.FocusList as FocusList
import qualified Data.Maybe as Unsafe (fromJust)
import qualified Data.Sequence as Seq

$(makeLensesFor
[ ("tmTermScrolledWindow", "lensTMTermScrolledWindow")
Expand All @@ -20,6 +24,7 @@ $(makeLensesFor
[ ("tmNotebookTabPaned", "lensTMNotebookTabPaned")
, ("tmNotebookTabLeftTerm", "lensTMNotebookTabLeftTerm")
, ("tmNotebookTabRightTerm", "lensTMNotebookTabRightTerm")
, ("tmNotebookTabFocusIsOnLeft", "lensTMNotebookTabFocusIsOnLeft")
, ("tmNotebookTabLabel", "lensTMNotebookTabLabel")
]
''TMNotebookTab
Expand All @@ -31,6 +36,12 @@ lensTMNotebookTabFocusedTerm f notebookTab
then lensTMNotebookTabLeftTerm f notebookTab
else lensTMNotebookTabRightTerm f notebookTab

lensTMNotebookTabNonFocusedTerm :: Lens' TMNotebookTab TMTerm
lensTMNotebookTabNonFocusedTerm f notebookTab
= if tmNotebookTabFocusIsOnLeft notebookTab
then lensTMNotebookTabRightTerm f notebookTab
else lensTMNotebookTabLeftTerm f notebookTab

traversalTMNotebookTabTerms :: Traversal' TMNotebookTab TMTerm
traversalTMNotebookTabTerms f notebookTab
= (\termL termR -> notebookTab
Expand All @@ -47,6 +58,27 @@ $(makeLensesFor
''TMNotebook
)

-- TODO: upstream this to the focuslist package
gelisam marked this conversation as resolved.
Show resolved Hide resolved
traversalFLItem :: forall a. Traversal' (FocusList a) a
traversalFLItem f flA
= let seqA = FocusList.toSeqFL flA
maybeFocus = FocusList.getFocus (FocusList.getFocusFL flA)
maybeFocusItem = FocusList.getFocusItemFL flA
in case (maybeFocus, maybeFocusItem) of
(Just i, Just a)
-> let makeUpdatedFL :: a -> FocusList a
makeUpdatedFL a'
= Unsafe.fromJust -- safe because i and the length are unchanged
$ FocusList.fromFoldableFL
(FocusList.Focus i)
(Seq.update i a' seqA)
in makeUpdatedFL <$> f a
_
-> pure flA

traversalTMNotebookFocusedTab :: Traversal' TMNotebook TMNotebookTab
traversalTMNotebookFocusedTab = lensTMNotebookTabs . traversalFLItem

$(makeLensesFor
[ ("tmStateApp", "lensTMStateApp")
, ("tmStateAppWin", "lensTMStateAppWin")
Expand Down
25 changes: 25 additions & 0 deletions src/Termonad/Term.hs
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ import GI.Gtk
, notebookSetTabReorderable
, onButtonClicked
, onWidgetButtonPressEvent
, onWidgetFocusInEvent
, onWidgetKeyPressEvent
, scrolledWindowNew
, scrolledWindowSetPolicy
Expand Down Expand Up @@ -106,13 +107,16 @@ import Termonad.Lenses
, lensShowScrollbar
, lensShowTabBar
, lensTMNotebookTabFocusedTerm
, lensTMNotebookTabFocusIsOnLeft
, lensTMNotebookTabLabel
, lensTMNotebookTabNonFocusedTerm
, lensTMNotebookTabPaned
, lensTMNotebookTabs
, lensTMStateApp
, lensTMStateConfig
, lensTMStateNotebook
, lensTerm
, traversalTMNotebookFocusedTab
)
import Termonad.Types
( ConfigHooks(createTermHook)
Expand Down Expand Up @@ -142,6 +146,14 @@ focusTerm i mvarTMState = do
altNumSwitchTerm :: Int -> TMState -> IO ()
altNumSwitchTerm = focusTerm

termTogglePane :: TMState -> IO ()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Owner

Choose a reason for hiding this comment

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

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!

termTogglePane mvarTMState = do
tabs <- tmNotebookTabs . tmStateNotebook <$> readMVar mvarTMState
let maybeFocusedTab = getFocusItemFL tabs
for_ maybeFocusedTab $ \focusedTab -> do
let newFocus = focusedTab ^. lensTMNotebookTabNonFocusedTerm . lensTerm
widgetGrabFocus newFocus

termNextPage :: TMState -> IO ()
termNextPage mvarTMState = do
note <- tmNotebook . tmStateNotebook <$> readMVar mvarTMState
Expand Down Expand Up @@ -493,6 +505,19 @@ createTerms handleKeyPress mvarTMState = do
for_ @[ScrolledWindow] [scrolledWinL, scrolledWinR] $ \scrolledWin -> do
void $ onWidgetKeyPressEvent scrolledWin $ handleKeyPress mvarTMState

void $ onWidgetFocusInEvent vteTermL $ \_ -> do
modifyMVar_ mvarTMState $ \tmState -> do
pure $ tmState & lensTMStateNotebook
. traversalTMNotebookFocusedTab
. lensTMNotebookTabFocusIsOnLeft .~ True
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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?

Copy link
Owner

Choose a reason for hiding this comment

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

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.

pure False
void $ onWidgetFocusInEvent vteTermR $ \_ -> do
modifyMVar_ mvarTMState $ \tmState -> do
pure $ tmState & lensTMStateNotebook
. traversalTMNotebookFocusedTab
. lensTMNotebookTabFocusIsOnLeft .~ False
pure False

-- Put the keyboard focus on the left term
setFocusOn tmStateAppWin vteTermL

Expand Down