-
-
Notifications
You must be signed in to change notification settings - Fork 61
Widgets: Add TerminalView #867
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
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: Danielle Foré <[email protected]>
|
If it's possible with this widget, maybe it would be worth to show colour palette? Small snippets on how to archive those colours. e.g. for |
Supporting ANSI Graphics/Color escape codes would be useful. However, I don't know whether it would be better to delay this PR to add that support, or to include that support in a follow-up. —Edit: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still don't think this should be a public widget. The previous style class was really only used in MessageDialogs, it wasn't supposed to be a style that app developers were using regularly. So personally I'm -1 on this altogether. This isn't something I think we should be doing or supporting.
Tbh even in MessageDialogs it's been a bit of an antipattern. People have been really bad about not reading these messages, even when reporting an issue they won't expand the terminal view. So I'm kind of in favor of removing this feature altogether in a future version of MessageDialog and forcing developers to write some better error handling
| */ | ||
|
|
||
| [Version (since = "7.7.0")] | ||
| public class Granite.TerminalView : Granite.Bin { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs documentation. See some of the other widgets for an example. Just needs a simple description of what this widget is and what it's used for
| /** | ||
| * Internal style class for {@link Granite.TerminalView} to emulate the appearance of Terminal. This includes | ||
| * text color, background color, selection highlighting, and selecting the system monospace font. | ||
| */ | ||
| internal const string TERMINAL = "terminal"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this should be added as a constant here. We usually don't make constants for things that aren't used outside of their own class
| }; | ||
|
|
||
| child = scrolled_window; | ||
| add_css_class (Granite.CssClass.TERMINAL); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of adding a style class, this should be the css name. Right now you have css nodes like widget.terminal but it should just be terminal So you need to remove this css class and add something like:
class construct {
set_css_name ("terminal-view");
}
Introduces a
TerminalViewwidget thatTextViewAlso:
Terminal Viewto it's own page in the demo appFuture extensions