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

Resizable terminal #4633

Closed

Conversation

Projects
None yet
6 participants
@larskarlitski
Copy link
Contributor

commented Jun 26, 2016

Add a resize control message to pty channels and make system/terminal fullscreen.

@stefwalter

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2016

@andreasn Have you been able to take a look at the layout and design here?

@stefwalter

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2016

The integration test is failing.

# testBasic (check_terminal.TestTerminal)
#
Restarting browser...
Wrote TestTerminal-testBasic-FAIL.png
Journal extracted to TestTerminal-testBasic-10.111.125.156-FAIL.log
not ok 66 testBasic (check_terminal.TestTerminal)
Traceback (most recent call last):
  File "./verify/check-terminal", line 58, in testBasic
    self.assertIn(":~$", prompt)
AssertionError: ':~$' not found in u'\xa0\xa0\xa0\xa0\xa0\xa0\xa0\xa0\xa0\xa0\xa0\xa0\xa0\xa0\xa0\xa0\xa0\xa0\xa0\xa0\xa0\xa0\xa0\xa0\xa0\xa0\xa0\xa0\xa0\xa0\xa0\xa0\xa0\xa0\xa0\xa0\xa0\xa0\xa0\xa0\xa0\xa0\xa0\xa0\xa0\xa0\xa0\xa0\xa0\xa0\xa0\xa0\xa0\xa0\xa0\xa0'
line-height: 1;
}

.console-ct-fullscreen {
position: absolute;
top: 44px;

This comment has been minimized.

Copy link
@stefwalter

stefwalter Jun 26, 2016

Contributor

What is this relative to?

This comment has been minimized.

Copy link
@larskarlitski

larskarlitski Jun 28, 2016

Author Contributor

It's relative to the page. The 44px is the header, which is also hard-coded to be 44px high.

This is the best way I've found to determine the height of the page. I'm very open to changing that if there's a better way.

@@ -1,17 +1,3 @@
.terminal {
float: left;

This comment has been minimized.

Copy link
@stefwalter

stefwalter Jun 26, 2016

Contributor

What replaces these styles now?

This comment has been minimized.

Copy link
@larskarlitski

larskarlitski Jun 28, 2016

Author Contributor

Those have always been overridden by the ones in cockpit.js.

{
gint64 rows, cols;

if (cockpit_json_get_int (options, "window_rows", default_rows, &rows) &&

This comment has been minimized.

Copy link
@stefwalter

stefwalter Jun 26, 2016

Contributor

This should be documented in doc/protocol.md

In addition I would suggest:

{
   "window": {
       "rows": 80,
       "cols": 25,
    }
}

This comment has been minimized.

Copy link
@larskarlitski

larskarlitski Jun 28, 2016

Author Contributor

I agree. Will change that

if (cockpit_json_get_int (options, "window_rows", default_rows, &rows) &&
cockpit_json_get_int (options, "window_cols", default_cols, &cols) &&
rows >= 0 && rows < G_MAXUINT16 && cols >= 0 && cols < G_MAXUINT16)
{

This comment has been minimized.

Copy link
@stefwalter

stefwalter Jun 26, 2016

Contributor

Ooops Edited ... These limitations shuold be documented in doc/protocol.md when you document the window.rows and window.cols options.

gint fd;

g_object_get (self->pipe, "in-fd", &fd, NULL);
ioctl (fd, TIOCSWINSZ, &size);

This comment has been minimized.

Copy link
@stefwalter

stefwalter Jun 26, 2016

Contributor

We should print out with a g_warning() here (if we expect this never to fail on a valid system).

@@ -833,7 +833,7 @@ test_pty_shell (void)

const gchar *argv[] = { "/bin/bash", "-i", NULL };

pipe = cockpit_pipe_pty (argv, NULL, NULL);
pipe = cockpit_pipe_pty (argv, NULL, NULL, 24, 80);

This comment has been minimized.

Copy link
@stefwalter

stefwalter Jun 26, 2016

Contributor

Is there a way we can unit test:

  • Opening a channel with the right options.
  • Changing the options inside an already open channel.

I believe that spawning a shell from test-pipe and having the shell send out its ROWS and COLS environment variables after receiving some trigger input ... that would do the trick no?

This comment has been minimized.

Copy link
@martinpitt

martinpitt Jul 3, 2017

Member

The following commit now has exactly such a test case.

@andreasn

This comment has been minimized.

Copy link
Contributor

commented Jun 27, 2016

If I make the window smaller, and then bigger again I seem to lose some information.
screenshot from 2016-06-27 12-43-08

@andreasn

This comment has been minimized.

Copy link
Contributor

commented Jun 27, 2016

I was pondering if it needs a switch to switch between this fullscreen mode and the 80x56 (or whatever the standard is), but I can't really find any good rationale for keeping the 80x56 mode. Does anyone else?

@larskarlitski

This comment has been minimized.

Copy link
Contributor Author

commented Jun 28, 2016

If I make the window smaller, and then bigger again I seem to lose some information.

Ya. This is a limitation of term.js. I'm not sure if it's worth to fix that, as it would be quite some work on the internals of that. The alternative is to not make it fullscreen, but just bigger. That could lead to another level of scrolling for some window sizes, though.

I was pondering if it needs a switch to switch between this fullscreen mode and the 80x56 (or whatever the standard is), but I can't really find any good rationale for keeping the 80x56 mode. Does anyone else?

I agree that fullscreen is enough.

@larskarlitski larskarlitski force-pushed the larskarlitski:resizable-terminal branch from 9743e69 to e14b96d Jul 6, 2016

@larskarlitski

This comment has been minimized.

Copy link
Contributor Author

commented Jul 6, 2016

Update: change the options to window.cols and .rows and document them; throw a warning when setting the ioctl fails; add a test; rebase.

@stefwalter stefwalter added needswork and removed needswork labels Jul 6, 2016

@stefwalter

This comment has been minimized.

Copy link
Contributor

commented Jul 6, 2016

This test failure seems related:

# testBasic (check_terminal.TestTerminal)
#
Wrote TestTerminal-testBasic-FAIL.png
Journal extracted to TestTerminal-testBasic-10.111.118.244-FAIL.log
not ok 72 testBasic (check_terminal.TestTerminal)
Traceback (most recent call last):
  File "./verify/check-terminal", line 64, in testBasic
    wait_line(2, "admin")
  File "./verify/check-terminal", line 46, in wait_line
    b.wait_text(line_sel(i), line_text(t))
  File "/build/cockpit/test/common/testlib.py", line 246, in wait_text
    return self.wait_js_func('ph_text_is', selector, text)
  File "/build/cockpit/test/common/testlib.py", line 210, in wait_js_func
    return self.phantom.wait("%s(%s)" % (func, ','.join(map(jsquote, args))))
  File "/build/cockpit/test/common/testlib.py", line 664, in <lambda>
    return lambda *args: self._invoke(name, *args)
  File "/build/cockpit/test/common/testlib.py", line 687, in _invoke
    raise Error(res['error'])
Error: timeout

@larskarlitski larskarlitski force-pushed the larskarlitski:resizable-terminal branch 2 times, most recently from 3a053f1 to 9f0bba7 Jul 7, 2016

@larskarlitski

This comment has been minimized.

Copy link
Contributor Author

commented Jul 7, 2016

This test failure seems related:

Indeed, it assumed that the terminal was always 80 columns wide. Fixed.

@larskarlitski larskarlitski removed the needswork label Jul 7, 2016

background: #000000;
}

.terminal-cursor {

This comment has been minimized.

Copy link
@stefwalter

stefwalter Jul 8, 2016

Contributor

Why was this removed?

This comment has been minimized.

Copy link
@larskarlitski

larskarlitski Jul 8, 2016

Author Contributor

Because cockpit.css has the similar rules which already overrode these [1]. Would you prefer to remove the ones there and keep them here in term.css?

[1] https://github.com/cockpit-project/cockpit/blob/master/src/base1/cockpit.css#L315


if (cockpit_json_get_int (window, "rows", default_rows, &rows) &&
cockpit_json_get_int (window, "cols", default_cols, &cols) &&
rows >= 0 && rows <= G_MAXUINT16 && cols >= 0 && cols <= G_MAXUINT16)

This comment has been minimized.

Copy link
@stefwalter

stefwalter Jul 8, 2016

Contributor

I know it's a long shot, but the front end doesn't seem to limit itself to G_MAXUINT16? How about just clamping it to these maximums, rather than disconnecting the front end for passing invalid data?

This comment has been minimized.

Copy link
@larskarlitski

larskarlitski Jul 8, 2016

Author Contributor

I agree, thanks.


if (!cockpit_pipe_channel_read_window_size_options (message, 0, 0, &rows, &cols))
{
g_warning ("invalid \"window.rows\" or \"window.cols\" option for stream channel");

This comment has been minimized.

Copy link
@stefwalter

stefwalter Jul 8, 2016

Contributor

Could we include the channel name for a bit easier debugging? That is g_warning("%s: invalid ...", self->name); and similar in the message below.

This comment has been minimized.

Copy link
@larskarlitski

larskarlitski Jul 8, 2016

Author Contributor

Yep.

gint fd;

g_object_get (self->pipe, "in-fd", &fd, NULL);
if (ioctl (fd, TIOCSWINSZ, &size) < 0)

This comment has been minimized.

Copy link
@stefwalter

stefwalter Jul 8, 2016

Contributor

If fd is -1 then in-fd is already closed, lets not call ioctl in that case.

@@ -165,6 +165,27 @@ cockpit_transport_emit_recv (CockpitTransport *transport,
}

void
cockpit_transport_emit_control (CockpitTransport *transport,

This comment has been minimized.

Copy link
@stefwalter

stefwalter Jul 8, 2016

Contributor

Should cockpit_transport_default_recv() call this function? Otherwise we have code duplication.

@stefwalter

This comment has been minimized.

Copy link
Contributor

commented Jul 8, 2016

screenshot from 2016-07-08 08-59-43

I get scrollbars on the terminal window.

screenshot from 2016-07-08 09-02-42

The kubernetes log window is also displaying strangely.

@stefwalter stefwalter added the needswork label Jul 8, 2016

@stefwalter

This comment has been minimized.

Copy link
Contributor

commented Jul 8, 2016

screenshot from 2016-07-08 09-13-06

Ditto for containers displayed without logs.

@larskarlitski

This comment has been minimized.

Copy link
Contributor Author

commented Jun 7, 2017

Rebased and updated. Would appreciate another look :)

@martinpitt

This comment has been minimized.

Copy link
Member

commented Jun 7, 2017

The tests all fail with

Module not found: Error: Cannot resolve 'file' or 'directory' ../pkg/systemd/terminal.css in /build/cockpit/tmp-dist
resolve file
  /build/cockpit/pkg/systemd/terminal.css doesn't exist
  /build/cockpit/pkg/systemd/terminal.css.webpack.js doesn't exist
  /build/cockpit/pkg/systemd/terminal.css.web.js doesn't exist
  /build/cockpit/pkg/systemd/terminal.css.js doesn't exist
  /build/cockpit/pkg/systemd/terminal.css.json doesn't exist

@larskarlitski larskarlitski force-pushed the larskarlitski:resizable-terminal branch from a12c709 to f2f030d Jun 7, 2017

@larskarlitski

This comment has been minimized.

Copy link
Contributor Author

commented Jun 7, 2017

The tests all fail with

Of course they do. Forgot a file. Sorry and thanks!

@larskarlitski larskarlitski force-pushed the larskarlitski:resizable-terminal branch from f2f030d to e207a9d Jun 9, 2017

@larskarlitski

This comment has been minimized.

Copy link
Contributor Author

commented Jun 9, 2017

The check-terminal test failed on Ubuntu, because it has a different sudo help text which it also shows after clicking "Reset".

Updated to make the text more robust (we were hard coding 4 lines of sudo message before).

@larskarlitski larskarlitski force-pushed the larskarlitski:resizable-terminal branch from e207a9d to f60c296 Jun 9, 2017

@larskarlitski larskarlitski force-pushed the larskarlitski:resizable-terminal branch from f60c296 to 8e3e5d0 Jun 19, 2017

@larskarlitski larskarlitski removed the needswork label Jul 3, 2017

@@ -401,7 +461,7 @@ cockpit_pipe_channel_prepare (CockpitChannel *channel)
"invalid \"directory\" option for stream channel");
goto out;
}
if (!cockpit_json_get_bool (options, "pty", FALSE, &pty))
if (!cockpit_json_get_bool (options, "pty", FALSE, &self->pty))

This comment has been minimized.

Copy link
@martinpitt

martinpitt Jul 3, 2017

Member

Smells like the gboolean pty function-local variable should be dropped now?

This comment has been minimized.

Copy link
@larskarlitski

larskarlitski Jul 3, 2017

Author Contributor

I actually did drop it, but in a later commit. Moved it into the one that it belongs to. Thanks!

// ensure react never reuses this div by keying it with the terminal widget
return <div ref="terminal" className="console-ct" key={this.state.terminal} />;
return <div ref="terminal" className={cls} key={this.state.terminal} />;

This comment has been minimized.

Copy link
@martinpitt

martinpitt Jul 3, 2017

Member

What's the purpose of the new cls? As far as I can see it's being used just once, right here. Debugging leftover?

This comment has been minimized.

Copy link
@larskarlitski

larskarlitski Jul 3, 2017

Author Contributor

Yeah, this was a leftover from a previous version.

@martinpitt
Copy link
Member

left a comment

Just some small nitpicks, but looks great overall. I tested this, and it's a night and day difference - an usable terminal now! Thanks!


this.setState({
rows: Math.floor((node.parentElement.clientHeight - padding) / height),
cols: Math.floor((node.parentElement.clientWidth - padding)/ width)

This comment has been minimized.

Copy link
@martinpitt

martinpitt Jul 3, 2017

Member

nitpick: missing space before /

@@ -36,15 +36,10 @@
background-color: black;
border: 1px solid black;
padding: 10px;
display: inline-block; /* size DIV to contents */
/* display: inline-block; */ /* size DIV to contents */

This comment has been minimized.

Copy link
@martinpitt

martinpitt Jul 3, 2017

Member

debugging leftover?

@andreasn

This comment has been minimized.

Copy link
Contributor

commented Jul 3, 2017

Looks good to me!

@andreasn andreasn requested review from andreasn and removed request for andreasn Jul 3, 2017

@larskarlitski larskarlitski force-pushed the larskarlitski:resizable-terminal branch from 8e3e5d0 to 7234c1f Jul 3, 2017

@larskarlitski

This comment has been minimized.

Copy link
Contributor Author

commented Jul 3, 2017

@pitti, thanks! Fixed both.

@pitti

This comment has been minimized.

Copy link

commented Jul 3, 2017

@larskarlitski @martinpitt

No problem, always happy to help! :)

larskarlitski added some commits Jun 7, 2017

bridge: Add control message to set pty size
Read 'window.rows' and 'window.cols' on the create and control
commands for pty channels.

@larskarlitski larskarlitski force-pushed the larskarlitski:resizable-terminal branch from 7234c1f to 5c28caa Jul 3, 2017

@larskarlitski

This comment has been minimized.

Copy link
Contributor Author

commented Jul 3, 2017

Rebased again taking into account @martinpitt's other suggestions.

@martinpitt
Copy link
Member

left a comment

Thanks, looks great now!

@martinpitt

This comment has been minimized.

Copy link
Member

commented Jul 3, 2017

All green, let's land this at last ☺

@martinpitt martinpitt closed this in 6d31ad4 Jul 3, 2017

@PeterFalken

This comment has been minimized.

Copy link

commented Jul 5, 2017

Thanks Martin - looking forward to using it on our servers.

@martinpitt

This comment has been minimized.

Copy link
Member

commented Jul 7, 2017

Current screenshot, for release notes:

terminal-full-window

@larskarlitski larskarlitski deleted the larskarlitski:resizable-terminal branch Jul 10, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.