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.

Sign up for GitHub

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

Jump to bottom

Make REPLPanel not blink when it is not selected #801

Merged
merged 14 commits into from
Oct 31, 2022
Merged

Make REPLPanel not blink when it is not selected #801

merged 14 commits into from
Oct 31, 2022

Conversation

xsebek
Copy link
Member

@xsebek xsebek commented Oct 29, 2022 •
edited
Loading

@xsebek xsebek requested a review from byorgey October 29, 2022 17:12
@xsebek
Copy link
Member Author

xsebek commented Oct 29, 2022 •
edited
Loading

@byorgey and @kostmo, feel free to have a look. 🙂 I would in particular be interested in better naming and further refactoring.

I might have introduced some bugs while porting the code from Form to Editor, but they should be easy to fix. 🐛
Likely I have just left searching on instead of switching to command mode or something.

EDIT1: looks like I broke both searching and completion ✅ fixed

EDIT2: the expression type is also not shown ✅ fixed

xsebek
xsebek commented Oct 29, 2022
View reviewed changes
src/Swarm/TUI/Model.hs
-- computed values. (Note that `it{n}` variables are set in the
-- base robot context; we only set `it` here because it's so transient)
topContext :: AppState -> RobotContext
topContext s = ctxPossiblyWithIt
Copy link
Member Author

Choose a reason for hiding this comment

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

I just moved this from controller to model, where it belongs. 😉

xsebek added 2 commits October 29, 2022 20:43
@xsebek
Use common replPromptText lens
605abce 
I have no idea if it is right, but at least I can fix it in one place.
@xsebek
Use concat to fix search and tab complete
b735194
@xsebek
Copy link
Member Author

xsebek commented Oct 29, 2022

This also addresses some of the refactoring suggestions from 23817a5 by @lsmor.

In particular, it moves the REPL parts to one data type.

@restyled-io restyled-io bot mentioned this pull request Oct 29, 2022
Merged
@restyled-io @restyled-commits
Restyled by fourmolu (#804)
e47470f 
Co-authored-by: Restyled.io <commits@restyled.io>
@byorgey
Copy link
Member

byorgey commented Oct 30, 2022 •
edited
Loading

Everything seems to work, except that #750 introduced the ability to click in the middle of the REPL input to move the cursor there (#470) and that no longer seems to do anything. I'm still going to go through the code carefully but just wanted to note that regression first.

EDIT: I think the reason clicking on the REPL input no longer works is that we had REPLInput as a resource name that got applied to the REPL form, but it does not seem to be used any more. The controller is still checking mouse down events to see if the thing labelled REPLInput was clicked, but there is no such label any more. So this should be an easy fix, just calling clickable REPLInput on the REPL editor widget.

@byorgey
Copy link
Member

byorgey commented Oct 30, 2022

Completion also seems to be broken. Once you complete something, the completion queue persists even when you erase what you were typing and type something else. For example, try typing "t at the REPL and then hitting Tab; it should expand to "tank treads as expected. But now erase everything at the prompt, type m, and hit Tab. Instead of expanding to something like move it expands to teeter-totter.

@xsebek
Copy link
Member Author

xsebek commented Oct 30, 2022 •
edited
Loading

Thanks, @byorgey! I was not even aware that clicking worked in REPL, I guess that is the only reason we need REPLInput as a special Name as otherwise it just acts as part of REPLPanel. 🤔

I wish we had some mock tests for this, that would make finding that bug faster. 😩

@xsebek
Copy link
Member Author

xsebek commented Oct 31, 2022

@byorgey should be fixed now 😉

byorgey
byorgey approved these changes Oct 31, 2022
View reviewed changes
Copy link
Member

@byorgey byorgey left a comment

Choose a reason for hiding this comment

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

I think this is looking great. I still think clicking in the REPL is not perfect, and I hope it is easy to fix, but I haven't tried yet. If you're sick of this and just want to merge I can play with it in a separate PR. 😄

src/Swarm/TUI/Controller.hs Outdated
@@ -282,7 +282,7 @@ handleMainEvent ev = do
WorldPanel -> do
mouseCoordsM <- Brick.zoom gameState (mouseLocToWorldCoords mouseLoc)
uiState . uiWorldCursor .= mouseCoordsM
REPLInput -> handleREPLEvent ev
REPLPanel -> handleREPLEvent ev
Copy link
Member

Choose a reason for hiding this comment

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

So this still isn't a perfect solution: now, clicking anywhere on the entire REPL panel can cause the cursor to jump around inside the input. To see this, type some long input, click on a different panel, and then click somewhere inside the REPL panel on the left side, but not on the line of input. The cursor will jump to somewhere in the middle of the input --- and the position where it jumps doesn't even correspond exactly with the x-coordinate of where you click.

I was thinking of keeping REPLInput and simply wrapping the REPL editor widget in clickable REPLInput --- does that not work?

src/Swarm/TUI/Controller.hs Outdated Show resolved Hide resolved
src/Swarm/TUI/Model.hs Outdated Show resolved Hide resolved
src/Swarm/TUI/View.hs Outdated
replEditor = repl ^. replPromptEditor
color = if repl ^. replValid then id else withAttr redAttr
ps1 = replPromptAsWidget (T.concat $ getEditContents replEditor) prompt
replE = withFocusRing focus (renderEditor (color . vBox . map txt)) replEditor
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
replE = withFocusRing focus (renderEditor (color . vBox . map txt)) replEditor
replE = clickable REPLInput (withFocusRing focus (renderEditor (color . vBox . map txt)) replEditor)

Like this, maybe?

Copy link
Member Author

Choose a reason for hiding this comment

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

It seems to work with my fix, so this may be unnecessary. 🙂

@xsebek
Copy link
Member Author

xsebek commented Oct 31, 2022

@byorgey I am indeed quite tired of this but it all seems to work now. 🤞

If you find the clicking does not work as expected, then feel free to improve it in another PR. 🙂

@xsebek xsebek added the merge me Trigger the merge process of the Pull request. label Oct 31, 2022
@mergify mergify bot merged commit 7185700 into main Oct 31, 2022
@mergify mergify bot deleted the replPanel branch October 31, 2022 19:42
@byorgey byorgey mentioned this pull request Oct 31, 2022
Merged
mergify bot pushed a commit that referenced this pull request Oct 31, 2022
@byorgey
set focus to REPL panel when clicking on REPL input (#808)
1d92881 
Ensure that focus changes to the REPL panel also when clicking on the REPL input itself.

Cleaning up a small leftover issue from #801.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@byorgey byorgey byorgey approved these changes

Assignees
No one assigned
Labels
merge me Trigger the merge process of the Pull request.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

REPL cursor should stop blinking when focus is elsewhere
2 participants
@xsebek @byorgey