Skip to content

Add a new panel for all OpenShift commands#2879

Merged
datho7561 merged 1 commit intoredhat-developer:mainfrom
datho7561:2819-create-new-terminal
Jul 13, 2023
Merged

Add a new panel for all OpenShift commands#2879
datho7561 merged 1 commit intoredhat-developer:mainfrom
datho7561:2819-create-new-terminal

Conversation

@datho7561
Copy link
Copy Markdown
Contributor

@datho7561 datho7561 commented May 18, 2023

It's a webview, since we need to use either a treeview or webview for all panels. It wraps xtermjs, the same library VS Code uses for its terminal. It exposes an API for interacting with the terminal multiplexer, as well as with the individual running processes. For example, we can run odo dev and update the UI to show the state of the deployment created by odo (like we were doing before this PR).

What's missing and can probably saved for a future PR:

  • Copy/paste using Ctrl+Shift+C/Ctrl+Shift+V (using the context menu to copy and paste works)
  • Tests (pending on Add page object for WebviewView vscode-extension-tester#855)
  • Reordering the tabs (I think we should gauge interest on this feature, since it would be hard to implement)
  • Tab bar scrolling when there are too many tabs. Supposedly the Material UI component comes with this built it, but it wasn't working when I tested it

Known bugs:

  • If you create two terminal tabs (through commands or running odo dev), and switch back to the first tab, the font size and family changes on that tab. All subsequent tab creation/switching doesn't seem to cause this issue.

Closes #2819

Signed-off-by: David Thompson davthomp@redhat.com

@codecov
Copy link
Copy Markdown

codecov bot commented May 18, 2023

Codecov Report

Patch coverage: 17.87% and project coverage change: +1.14 🎉

Comparison is base (d5d3ff0) 35.41% compared to head (ce9a3c7) 36.56%.

❗ Current head ce9a3c7 differs from pull request most recent head 10c0bdc. Consider uploading reports for the commit 10c0bdc to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2879      +/-   ##
==========================================
+ Coverage   35.41%   36.56%   +1.14%     
==========================================
  Files          64       54      -10     
  Lines        4238     3758     -480     
  Branches      836      736     -100     
==========================================
- Hits         1501     1374     -127     
+ Misses       2737     2384     -353     
Impacted Files Coverage Δ
.../webview/create-service/createServiceViewLoader.ts 31.25% <0.00%> (ø)
src/openshift/component.ts 21.72% <8.00%> (+2.22%) ⬆️
...rc/webview/openshift-terminal/openShiftTerminal.ts 10.89% <10.89%> (ø)
src/cli.ts 83.78% <25.00%> (+4.29%) ⬆️
src/openshift/cluster.ts 10.27% <40.00%> (-0.26%) ⬇️
src/webview/cluster/clusterViewLoader.ts 9.66% <50.00%> (+0.25%) ⬆️
src/extension.ts 61.36% <100.00%> (-2.18%) ⬇️
src/k8s/build.ts 97.53% <100.00%> (ø)
src/k8s/deploymentConfig.ts 97.33% <100.00%> (ø)
src/odo.ts 45.63% <100.00%> (+3.04%) ⬆️
... and 1 more

... and 19 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@datho7561
Copy link
Copy Markdown
Contributor Author

datho7561 commented Jun 5, 2023

blocked on redhat-developer/vscode-extension-tester#804 through discussion with Mohit, we decided to add UI tests in a future PR

@datho7561
Copy link
Copy Markdown
Contributor Author

datho7561 commented Jun 15, 2023

I just realized that, after this change, it will be very difficult to write automated tests for anything that uses the terminal. We are testing using Selenium, which can scrape data off the HTML DOM. The library I am using to render the terminal output uses WebGL for performance reasons, so the terminal text is not in a DOM node, it's a rendered image.

The two ways I can think of to test the terminal output:

  • Take a screenshot using selenium and compare the screenshot to a cached screenshot, which is really flaky
  • Switch the rendering strategy of the terminal to use a DOM-based version during the test suite, and trust that the renderers are similar enough

I'll try the second version and see if I get anywhere

edit: it should be feasible to use the second strategy, I had some code that did that as a part of this PR, but I removed it.

@datho7561
Copy link
Copy Markdown
Contributor Author

datho7561 commented Jun 15, 2023

Requires redhat-developer/vscode-extension-tester#855 (I have been using npm link locally) removed tests

@datho7561 datho7561 force-pushed the 2819-create-new-terminal branch 2 times, most recently from c95f6f9 to 6437cc4 Compare June 22, 2023 16:30
@datho7561 datho7561 marked this pull request as ready for review June 22, 2023 16:47
@datho7561 datho7561 force-pushed the 2819-create-new-terminal branch from 6437cc4 to 3fe6b56 Compare June 22, 2023 16:55
Copy link
Copy Markdown
Collaborator

@msivasubramaniaan msivasubramaniaan left a comment

Choose a reason for hiding this comment

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

The below error on npm install

ERR! find VSfind VS msvs_version not set from command line or npm config?

After resolved the above error and opened the extension, I was trying to run the Start Dev command, then the extension was stuck and I couldn't able to perform anything further on the extension.

@datho7561 datho7561 force-pushed the 2819-create-new-terminal branch 2 times, most recently from ddeab40 to e54b2a6 Compare June 28, 2023 19:14
@datho7561
Copy link
Copy Markdown
Contributor Author

npm i works for me on Windows, and the webview loads, but when running a command, the output is not transferred into the webview properly. @JessicaJHee or @msivasubramaniaan , can you confirm that the terminal works on Linux?

@datho7561 datho7561 force-pushed the 2819-create-new-terminal branch from e54b2a6 to 9f2157e Compare June 29, 2023 16:04
Copy link
Copy Markdown
Member

@JessicaJHee JessicaJHee left a comment

Choose a reason for hiding this comment

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

Works great now! The only thing I noticed was that if I tried to close the middle tab, it will close the tabs to the right of it as well.

Peek 2023-06-29 14-28

@datho7561 datho7561 force-pushed the 2819-create-new-terminal branch from 9f2157e to 01c574e Compare June 29, 2023 19:54
@datho7561
Copy link
Copy Markdown
Contributor Author

datho7561 commented Jun 29, 2023

if I tried to close the middle tab, it will close the tabs to the right of it as well.

Good catch! Should be fixed

Comment thread src/webview/openshift-terminal/app/terminalMultiplexer.tsx Outdated
@datho7561 datho7561 force-pushed the 2819-create-new-terminal branch from 01c574e to e9973d1 Compare June 29, 2023 21:14
@datho7561
Copy link
Copy Markdown
Contributor Author

@JessicaJHee @rgrunber let me know if you think it's good. I'll have to address the merge conflict before we can merge the PR.

@datho7561 datho7561 force-pushed the 2819-create-new-terminal branch from e9973d1 to 77dad6f Compare July 12, 2023 17:39
Copy link
Copy Markdown
Member

@JessicaJHee JessicaJHee left a comment

Choose a reason for hiding this comment

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

LGTM!

@datho7561 datho7561 force-pushed the 2819-create-new-terminal branch from 77dad6f to fa1e8c4 Compare July 13, 2023 14:58
It's a webview, since we need to use either a treeview or webview for all
panels.
It wraps xtermjs, the same library VS Code uses for its terminal.
It exposes an API for interacting with the terminal multiplexer, as well
as with the individual running processes.
For example, we can run `odo dev` and update the UI
(like we were doing before this PR).

What's missing:
- [ ] copy/paste
- [ ] Tests (pending on redhat-developer/vscode-extension-tester#855)
- [ ] reordering the tabs (I think we should gauge interest and save this for a future PR)
- [ ] tab bar scrolling (supposedly the Material UI component comes with
      this built it, but it wasn't working when I tested it)

Signed-off-by: David Thompson <davthomp@redhat.com>
@datho7561 datho7561 force-pushed the 2819-create-new-terminal branch from fa1e8c4 to 10c0bdc Compare July 13, 2023 16:19
Copy link
Copy Markdown
Collaborator

@msivasubramaniaan msivasubramaniaan left a comment

Choose a reason for hiding this comment

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

LGTM

@datho7561 datho7561 merged commit 4d121d6 into redhat-developer:main Jul 13, 2023
@datho7561 datho7561 deleted the 2819-create-new-terminal branch July 13, 2023 18:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Create a new Terminal Column for OpenShift Output

3 participants