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

Addressing features/bugs with the terminal #1117

Merged
merged 7 commits into from
Mar 7, 2023

Conversation

bombardier200
Copy link
Contributor

@bombardier200 bombardier200 commented Mar 7, 2023

Description

Address bugs and features of the built in terminal
Updated SwiftTerm to point to 1.2.0
Currently added changing the cursor style
Fixed bleeding edge issue

Related Issue

Checklist

  • I read and understood the contributing guide as well as the code of conduct
  • My changes generate no new warnings
  • My code builds and runs on my machine
  • I documented my code
  • Review requested

Screenshots

@iggy890
Copy link
Contributor

iggy890 commented Mar 7, 2023

Nice job in finding this, just want to comment that it's called caret not carrot, this could cause confusion with future contributors

@austincondiff
Copy link
Collaborator

raw

And this whole time I thought we were picking carrots! 😄🥕

@iggy890
Copy link
Contributor

iggy890 commented Mar 7, 2023

raw

And this whole time I thought we were picking carrots! 😄🥕

Well, I guess we were… 🐰

Copy link
Member

@lukepistrol lukepistrol left a comment

Choose a reason for hiding this comment

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

Just some considerations about a tiny spelling error (🥕) and one line_length violation.

Otherwise great job 😉

@CodeEditApp CodeEditApp deleted a comment from iggy890 Mar 7, 2023
@bombardier200
Copy link
Contributor Author

Thanks for looking, lets just say I was a little tired when I did this lol!

@lukepistrol
Copy link
Member

One additional thing I'd like to discuss:

Instead of having a list of all the curser styles like you can see below, just have the main 3 styles (block, underline, bar) and have a separate boolean toggle the blinking behavior instead. This would make selection a lot cleaner IMO.

Screenshot 2023-03-07 at 16 29 03

Any thoughts on this? @austincondiff @Wouter01 @0xWDG @KaiTheRedNinja

@bombardier200
Copy link
Contributor Author

bombardier200 commented Mar 7, 2023

@lukepistrol I agree with that, that make it look a lot better. We made a discussion post in SwiftTerm to discuss what all options we could actually do with the cursor styles and other terminal related ideas, but this will take some time to develop. See migueldeicaza/SwiftTerm#279 For merging the current changes since there are a few bug fixes, once we decide on the layout we want to go with for settings, I will mark it as ready and implement other changes in another PR.

@austincondiff
Copy link
Collaborator

austincondiff commented Mar 7, 2023

I don’t like the duplication. Blink will eventually be much more than a simple toggle. See the discussion here.

@bombardier200
Copy link
Contributor Author

@austincondiff so for now would you just recommend leaving it as the picker?

@bombardier200 bombardier200 marked this pull request as ready for review March 7, 2023 17:28
@austincondiff
Copy link
Collaborator

If you can, go ahead and add the blink toggle if you can. We can change it to a blink style picker later on.

@bombardier200
Copy link
Contributor Author

@austincondiff So the picker will be the style(block, underline, bar), and then a toggle for if you want it to blink or not?

@lukepistrol
Copy link
Member

@bombardier200 JFYI SwiftTerm now has a new release. Please change the dependency to use exact: 1.2.0.

https://github.com/migueldeicaza/SwiftTerm/releases/tag/1.2.0

@austincondiff
Copy link
Collaborator

@bombardier200 Yes, that works great.

@bombardier200
Copy link
Contributor Author

Screenshot 2023-03-07 at 3 13 47 PM

New preferences view, this PR should be ready to go now!

Copy link
Member

@lukepistrol lukepistrol left a comment

Choose a reason for hiding this comment

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

Looks good! 👍

Copy link
Collaborator

@austincondiff austincondiff left a comment

Choose a reason for hiding this comment

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

Looks great @bombardier200! 👏

@austincondiff
Copy link
Collaborator

austincondiff commented Mar 7, 2023

@Wouter01 Should we wait for the splitview branch to be merged before merging this PR into main? (No rush, we can wait if we need to.)

@Wouter01
Copy link
Member

Wouter01 commented Mar 7, 2023

@Wouter01 Should we wait for the splitview branch to be merged before merging this PR into main?

Nope, all fine!

Copy link
Member

@Wouter01 Wouter01 left a comment

Choose a reason for hiding this comment

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

nice work!

@lukepistrol lukepistrol merged commit 5659ba0 into CodeEditApp:main Mar 7, 2023
@bombardier200 bombardier200 deleted the terminalChanges branch March 7, 2023 20:28
@CodeEditApp CodeEditApp deleted a comment from iggy890 Mar 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants