Skip to content

Re-enable separate OpenShift Terminal#3065

Merged
datho7561 merged 6 commits intoredhat-developer:mainfrom
datho7561:fix-openshift-terminal
Sep 20, 2023
Merged

Re-enable separate OpenShift Terminal#3065
datho7561 merged 6 commits intoredhat-developer:mainfrom
datho7561:fix-openshift-terminal

Conversation

@datho7561
Copy link
Copy Markdown
Contributor

@datho7561 datho7561 commented Jul 26, 2023

Revert the revert commit, and address the bug preventing the terminal from working on Windows. The resolution was to wrap all program executions in a Windows command line shell. I chose cmd.exe, although I expect PowerShell would also have worked.

WIP:

@datho7561
Copy link
Copy Markdown
Contributor Author

@msivasubramaniaan I still need to fix a few places where commands are still being run in the default VS Code terminal, but features like 'Describe' and 'Dev' should work now. If you have time at some point, could you please confirm that this works for you?

@codecov
Copy link
Copy Markdown

codecov bot commented Jul 26, 2023

Codecov Report

Patch coverage: 25.50% and project coverage change: +0.18% 🎉

Comparison is base (b0af0a1) 36.94% compared to head (4753b33) 37.12%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3065      +/-   ##
==========================================
+ Coverage   36.94%   37.12%   +0.18%     
==========================================
  Files          77       77              
  Lines        5319     5336      +17     
  Branches     1008     1025      +17     
==========================================
+ Hits         1965     1981      +16     
- Misses       3354     3355       +1     
Files Changed Coverage Δ
src/cli.ts 97.43% <ø> (+17.84%) ⬆️
src/openshift/openshiftItem.ts 34.42% <ø> (ø)
.../webview/create-service/createServiceViewLoader.ts 31.25% <0.00%> (ø)
src/serverlessFunction/functions.ts 8.75% <4.34%> (+4.42%) ⬆️
src/openshift/component.ts 28.60% <9.61%> (+1.85%) ⬆️
...rc/webview/openshift-terminal/openShiftTerminal.ts 11.17% <11.17%> (ø)
src/openshift/cluster.ts 10.63% <50.00%> (+0.40%) ⬆️
src/serverlessFunction/view.ts 34.51% <60.00%> (ø)
src/webview/cluster/clusterViewLoader.ts 9.88% <75.00%> (+0.45%) ⬆️
src/k8s/build.ts 97.72% <95.55%> (-2.28%) ⬇️
... and 5 more

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@datho7561
Copy link
Copy Markdown
Contributor Author

I added support for the serverless commands and fixed a race condition that I ran into.

@datho7561 datho7561 force-pushed the fix-openshift-terminal branch from a58ad7a to 906c0a2 Compare August 14, 2023 20:58
@datho7561
Copy link
Copy Markdown
Contributor Author

@msivasubramaniaan I've updated this PR to fix it on Windows and take into account the Serverless commands (build, run, deploy). If you have time could you please let me know if it's working for you?

@datho7561 datho7561 force-pushed the fix-openshift-terminal branch from 906c0a2 to e51567a Compare August 24, 2023 15:25
@datho7561
Copy link
Copy Markdown
Contributor Author

Rebased so that the serverless configuration works

@datho7561 datho7561 removed the request for review from JessicaJHee August 24, 2023 15:58
@datho7561 datho7561 force-pushed the fix-openshift-terminal branch from e51567a to fe6c9a8 Compare August 25, 2023 14:51
@datho7561
Copy link
Copy Markdown
Contributor Author

rebased on Muthu's and Victor's PRs:

  • invoke should run in the separate terminal
  • odo run should run in the separate terminal if "openshiftToolkit.useWebviewInsteadOfTerminalView": false

Copy link
Copy Markdown
Contributor

@vrubezhny vrubezhny left a comment

Choose a reason for hiding this comment

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

Please look at the question in-line

Comment thread src/webview/openshift-terminal/openShiftTerminal.ts Outdated
@vrubezhny
Copy link
Copy Markdown
Contributor

vrubezhny commented Aug 25, 2023

The most annoying problems (running on linux) I found are:

  • OS Terminal doesn't have a ruler at right to make possible to scroll the terminal contents by mouse (a "plain" vscode terminal has such a ruler)
  • OS Terminal has an issue that does not allow to scroll down to the very bottom line - sometimes it works, but after switching tabs or doing some actions (like trying to create a serverless function) and switching back to OS Terminal tab, it doesn't allow to see the bottom line (or even lines) - the bottom line(-s) are "cut" or even not visible at all. This is certainly reported here: The new "OpenShift Terminal" doesn't decrease size correctly #3028
  • A new OS Terminal tab is opened for every run of the same Component Command (personally I think we should reuse tabs at least if we run the same command multiple times and probably to have a possibility to clear the contents of a terminal tab)
  • No "Clear Tab Contents"-like action available
  • There are Cut/Copy/Paste actions, but there is no Select All action available
  • Pressing Ctrl-X/Ctrl-C/Ctrl-V closes the OS Terminal Tab instead of performing a Cut/Copy/Paste action (we're in VSCode, not in a *nix console, right?)

@datho7561
Copy link
Copy Markdown
Contributor Author

Thanks for the feedback! I think I will need to go back to this feature and do more work on it, so we should avoid trying to get it in the next release.

@vrubezhny
Copy link
Copy Markdown
Contributor

Thanks for the feedback! I think I will need to go back to this feature and do more work on it, so we should avoid trying to get it in the next release.

I agree, probably we should postpone it for a while.
But my overall impression is good and, when resizing/scrolling issues are fixed, I'd like to see it included, so we can open the issues for the rest of the problems and solve them one by one.

@datho7561
Copy link
Copy Markdown
Contributor Author

Pressing Ctrl-X/Ctrl-C/Ctrl-V closes the OS Terminal Tab instead of performing a Cut/Copy/Paste action (we're in VSCode, not in a *nix console, right?)

This is the same behaviour as the regular VS Code Terminal. In order to cut/copy/paste using the keyboard, use Ctrl+Shift+X/C/V

@datho7561 datho7561 marked this pull request as ready for review August 29, 2023 18:39
@datho7561 datho7561 force-pushed the fix-openshift-terminal branch from d7857ae to 1461673 Compare August 29, 2023 18:43
@datho7561 datho7561 force-pushed the fix-openshift-terminal branch 2 times, most recently from c74a82b to f7bd1cb Compare September 5, 2023 18:38
@datho7561 datho7561 force-pushed the fix-openshift-terminal branch 2 times, most recently from 0150fdc to 13e92ee Compare September 18, 2023 15:28
@datho7561
Copy link
Copy Markdown
Contributor Author

Hello Victor. If you have time, could you please re-review this PR? I addressed the first two of your suggestions. For the other ones:

  • A new OS Terminal tab is opened for every run of the same Component Command

I think we should handle this in a separate PR.

  • No "Clear Tab Contents"-like action available
  • There are Cut/Copy/Paste actions, but there is no Select All action available

The default VS Code Terminal has buttons in the top right:

button-gutter-openshift-terminal

What do you think about me adding buttons for "select all" and "clear tab contents" here? I don't think the extension API gives access to add a "select all" context menu.

  • Pressing Ctrl-X/Ctrl-C/Ctrl-V closes the OS Terminal Tab instead of performing a Cut/Copy/Paste action (we're in VSCode, not in a *nix console, right?)

The terminal behaves like a *nix or Windows terminal, since in come cases you need to send Ctrl+C to the process (eg. to stop component dev mode). In order to copy, you can use Ctrl+Shift+C. This is the default behaviour of the library I'm using, which is the same library and behaviour as VS Code's built-in terminal.

vrubezhny
vrubezhny previously approved these changes Sep 20, 2023
Copy link
Copy Markdown
Contributor

@vrubezhny vrubezhny 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, works smooth. The main problem, when it was impossible to scroll to the very beginning/very end of terminal text, is definitely solved.

I'm 👍 for it to be merged once successfully rebased.

All the other issues and improvements may be discussed/solved in separate issues.

datho7561 and others added 5 commits September 20, 2023 14:19
The resolution was to wrap all program executions in a Windows command
line shell.
I chose `cmd.exe`, although I expect PowerShell would also have worked.

Signed-off-by: David Thompson <davidethompson@me.com>
- Also fix a race condition that caused the terminal spawn to fail

Signed-off-by: David Thompson <davthomp@redhat.com>
 - Turn conpty off; for some reason it doesn't work as expected
 - Use relative reference to cmd.exe

Signed-off-by: David Thompson <davidethompson@me.com>
- Show scroll bar
- Style scroll bar similar to VS Code's
- Prevent text from being hidden past the bottom of the screen after resizing
- Debounce resizing to prevent flashing

Signed-off-by: David Thompson <davthomp@redhat.com>
@datho7561
Copy link
Copy Markdown
Contributor Author

Annoying bug on Windows: Right click -> copy doesn't work, and Ctrl+Shift+C is apparently a keybinding to open the Windows Terminal app, so there is no way to copy text.

Signed-off-by: David Thompson <davthomp@redhat.com>
@datho7561 datho7561 force-pushed the fix-openshift-terminal branch from dd5650a to 4753b33 Compare September 20, 2023 19:02
@datho7561
Copy link
Copy Markdown
Contributor Author

The keybinding for copy on Windows is Ctrl+Ins

@datho7561 datho7561 assigned datho7561 and unassigned datho7561 Sep 20, 2023
@datho7561 datho7561 merged commit 2da328d into redhat-developer:main Sep 20, 2023
@datho7561 datho7561 deleted the fix-openshift-terminal branch September 20, 2023 19:43
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.

Killing terminal from older start dev action stops current dev Create a new Terminal Column for OpenShift Output

2 participants