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

Fixed #5543: implemented "show all opened terminals" quick-open command #5577

Merged
merged 1 commit into from
Jul 3, 2019

Conversation

fangnx
Copy link

@fangnx fangnx commented Jun 25, 2019

Implemented the "Show All Opened Terminals" command in the quick open menu. The command displays all the currently opened terminal widgets, and the user can navigate to each of them by selection and have the option to create a new terminal.

The command has the global prefix: term , to be aligned to VS Code.

The Terminal package may require some refactoring. The terminal-frontend-contribution class seems to include functions that should be otherwise put into a separate class.

@vince-fugnitto vince-fugnitto added the terminal issues related to the terminal label Jun 26, 2019
@vince-fugnitto
Copy link
Member

@lmcbout could you test the PR as well?

@lmcbout
Copy link
Contributor

lmcbout commented Jun 26, 2019

Currently testing, Putting terminal-# seems repetitive. I can only see one terminal using (_terminal 6) , the others, I see the user path, so hard to match the list of terminal (Quick selection) and the actual terminal . I also notice the same number being used for the terminal (See picture) . Also, How is the number generated, if I open a new terminal, (Small number) and if I run a task, (Number 15, 26. , not even consecutive, why?
What do you think about having a separator between the list of terminals and the "Open new terminal" when there is one or more terminals?

ShowTerminal

@fangnx
Copy link
Author

fangnx commented Jun 26, 2019

Currently testing, Putting terminal-# seems repetitive. I can only see one terminal using (_terminal 6) , the others, I see the user path, so hard to match the list of terminal (Quick selection) and the actual terminal . I also notice the same number being used for the terminal (See picture) . Also, How is the number generated, if I open a new terminal, (Small number) and if I run a task, (Number 15, 26. , not even consecutive, why?
What do you think about having a separator between the list of terminals and the "Open new terminal" when there is one or more terminals?

ShowTerminal

Thanks Jacque! Just found out the sorting logic does not work from your screenshot.

Yes, adding a separator makes sense. The terminal-# is the terminal id. I am not sure why the numbers are not consecutive, but I think since it is difficult to match the terminal-# to the name displayed on the terminal widget, I would use title id (~/theia/.../...) as a label, just as Vince suggested.

@fangnx
Copy link
Author

fangnx commented Jun 26, 2019

I can't reproduce the repetitive terminal-# issue. How did you get the same terminal-6 when you tested it ;)? @lmcbout

@fangnx fangnx force-pushed the fnx-fix-5543 branch 3 times, most recently from 8685856 to 3b7992f Compare June 26, 2019 19:18
@vince-fugnitto
Copy link
Member

@lmcbout do you mind reviewing it again when you get the chance?

Copy link
Contributor

@lmcbout lmcbout left a comment

Choose a reason for hiding this comment

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

Minor comments, LGTM

@vince-fugnitto
Copy link
Member

@kittaakos could you give it a review as well whenever you get the chance?

…ck-open command

- Implemented the "Show All Opened Terminals" command in the quick command menu, which can display all the currently opened terminal widgets.
- User can navigate to each terminal widget by selection, and has the option to create a new terminal.
- The command has a global prefix: `term `, to be aligned to VS Code.

Signed-off-by: fangnx <naxin.fang@ericsson.com>
Copy link
Member

@akosyakov akosyakov left a comment

Choose a reason for hiding this comment

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

code wise looks good

if @vince-fugnitto @lmcbout are tested it and happy when please merge it

@lmcbout
Copy link
Contributor

lmcbout commented Jul 3, 2019

While testing with the latest code ( commit 8e42335 ), I noticed when selecting a terminal (there is a duplication of ID - terminal 4), it selects the first one with the "terminal-4", not the selected one which is the second terminal entry (last entry from the list).

Note the last two entries from the terminal list , both have terminal-4

WrongTerminalSelected

@vince-fugnitto
Copy link
Member

vince-fugnitto commented Jul 3, 2019

While testing with the latest code ( commit 8e42335 ), I noticed when selecting a terminal (there is a duplication of ID - terminal 4), it selects the first one with the "terminal-4", not the selected one which is the second terminal entry (last entry from the list).

@lmcbout

I don't believe this is an issue with @fangnx code.
I think it's more our current logic is incorrect when assigning terminal ids. Also, when we lookup
terminals by id it makes sense that it returns the first match anyways, so is this really an issue?

@vince-fugnitto
Copy link
Member

@lmcbout please open an issue regarding the terminal ids not being unique including steps to reproduce.

@vince-fugnitto vince-fugnitto merged commit 150c158 into eclipse-theia:master Jul 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
terminal issues related to the terminal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants