Skip to content
This repository has been archived by the owner on Apr 4, 2023. It is now read-only.

Apply terminal title name configuration #606

Merged
merged 8 commits into from
Jan 30, 2020
Merged

Apply terminal title name configuration #606

merged 8 commits into from
Jan 30, 2020

Conversation

vinokurig
Copy link
Contributor

@vinokurig vinokurig commented Jan 24, 2020

What does this PR do?

Add a configuration that controls default terminal title override:
ezgif-2-d5ad920320ee

How to test?

Start a workspace from dev-file that contains a custom container that changes terminal title e.g:

- alias: my-container
    type: dockerimage
    image: ivinokur/rhel8
    mountSources: true
    memoryLimit: 256M

What issues does this PR fix or reference?

fixes eclipse-che/che#15242

Release Notes

Docs PR

Signed-off-by: Igor Vinokur <ivinokur@redhat.com>
@che-bot
Copy link
Contributor

che-bot commented Jan 24, 2020

✅ E2E Happy path tests succeed 🎉

Try Che-Theia editor only Try Che-Theia with Java/maven example Try Che-Theia with NodeJs example

See Details

name link
che-theia docker.io/maxura/che-theia:606
che-theia-endpoint-runtime-binary docker.io/maxura/che-theia-endpoint-runtime-binary:606

Tested with Eclipse Che Single User on K8S (minikube v1.1.1)

Copy link
Member

@azatsarynnyy azatsarynnyy left a comment

Choose a reason for hiding this comment

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

Looks well.
Just one question: where the tab labels come from? container-1, container-2, ...

@vinokurig
Copy link
Contributor Author

@azatsarynnyy The default terminal title name comes from the dev-file

Copy link
Member

@RomanNikitenko RomanNikitenko left a comment

Choose a reason for hiding this comment

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

Tested using component based on eivantsov/rhel8 image.
Works well for me!

Copy link
Contributor

@AndrienkoAleksandr AndrienkoAleksandr left a comment

Choose a reason for hiding this comment

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

General OK. But I think you can extend original theia terminal preference schema. Any way up to you.

@benoitf
Copy link
Contributor

benoitf commented Jan 27, 2020

Hello,

to me the property name is confusing. I expect that override is when container name is added (like the opposite)

BTW, as the naming of title/terminals is quite confusing in the Che context, shouldn't it be the default ?
By default, users opening terminals should see the container name as prefix without having to update some preferences.

@benoitf
Copy link
Contributor

benoitf commented Jan 27, 2020

@dmytro-ndp I tried with che-bot links to review but it seems it's not using the right images so I wasn't able to test by clicking.

  containers:
   - name: theia-ide
     image: "quay.io/eclipse/che-theia:next"

@vinokurig
Copy link
Contributor Author

@benoitf

to me the property name is confusing. I expect that override is when container name is added (like the opposite)

How about allowToChangeTitle ?

@benoitf
Copy link
Contributor

benoitf commented Jan 27, 2020

I'm still wondering if we need to have a property or just prefix with container all the time ?

@vinokurig
Copy link
Contributor Author

I prefer to keep the property. Title with both container name and overridden title looks too long:
screenshot-che-che 192 168 99 118 nip io-2020 01 27-10_53_11
@slemeur @l0rd WDYT?

@benoitf
Copy link
Contributor

benoitf commented Jan 27, 2020

I think we could shorter the title
We already know we're inside a terminal so no need to have terminal 0, terminal 1
container-short-name#1'is enough + hover with full name (I'm unsure as well that first terminal should be 0 for humans)

@che-bot
Copy link
Contributor

che-bot commented Jan 27, 2020

✅ E2E Happy path tests succeed 🎉

Try Che-Theia editor only Try Che-Theia with Java/maven example Try Che-Theia with NodeJs example

See Details

name link
che-theia docker.io/maxura/che-theia:606
che-theia-endpoint-runtime-binary docker.io/maxura/che-theia-endpoint-runtime-binary:606

Tested with Eclipse Che Single User on K8S (minikube v1.1.1)

@vinokurig
Copy link
Contributor Author

@benoitf I like your solution but we need to have agreement with @slemeur and @l0rd because the corresponding issue is about adding a property and viewing only default title or title from container: eclipse-che/che#15242

@l0rd
Copy link
Contributor

l0rd commented Jan 28, 2020

@vinokurig I agree with @benoitf
Moreover I am not sure if there is any value in having the property: it should always be overrideTitle=false. As a user why should I be interested in setting "terminal.overrideTitle": true? cc @AndrienkoAleksandr

@che-bot
Copy link
Contributor

che-bot commented Jan 28, 2020

❌ E2E Happy path tests failed ❗

Try Che-Theia editor only Try Che-Theia with Java/maven example Try Che-Theia with NodeJs example

See Details

name link
che-theia docker.io/maxura/che-theia:606
che-theia-endpoint-runtime-binary docker.io/maxura/che-theia-endpoint-runtime-binary:606

Tested with Eclipse Che Single User on K8S (minikube v1.1.1)

⚠️ https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

ℹ️ Use comment "crw-ci-test" to rerun happy path E2E test.

Signed-off-by: Igor Vinokur <ivinokur@redhat.com>
@che-bot
Copy link
Contributor

che-bot commented Jan 29, 2020

❌ E2E Happy path tests failed ❗

Try Che-Theia editor only Try Che-Theia with Java/maven example Try Che-Theia with NodeJs example

See Details

name link
che-theia docker.io/maxura/che-theia:606
che-theia-endpoint-runtime-binary docker.io/maxura/che-theia-endpoint-runtime-binary:606

Tested with Eclipse Che Single User on K8S (minikube v1.1.1)

⚠️ https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

ℹ️ Use comment "crw-ci-test" to rerun happy path E2E test.

@AndrienkoAleksandr
Copy link
Contributor

Moreover I am not sure if there is any value in having the property: it should always be overrideTitle=false. As a user why should I be interested in setting "terminal.overrideTitle": true? cc @AndrienkoAleksandr

Show terminal title - it is a feature. I think most users don't need it. But I can not predict desire all users, that's why for me make sense to have an option.

@che-bot
Copy link
Contributor

che-bot commented Jan 29, 2020

❌ E2E Happy path tests failed ❗

Try Che-Theia editor only Try Che-Theia with Java/maven example Try Che-Theia with NodeJs example

See Details

name link
che-theia docker.io/maxura/che-theia:606
che-theia-endpoint-runtime-binary docker.io/maxura/che-theia-endpoint-runtime-binary:606

Tested with Eclipse Che Single User on K8S (minikube v1.1.1)

⚠️ https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

ℹ️ Use comment "crw-ci-test" to rerun happy path E2E test.

@vinokurig
Copy link
Contributor Author

@benoitf @l0rd I have updated the PR to have both container name and container title in the terminal title:
screenshot-servere4jzwkrf-che-dev-server-3010 192 168 99 127 nip io-2020 01 30-10_48_07
Can we go with it?

@che-bot
Copy link
Contributor

che-bot commented Jan 30, 2020

✅ E2E Happy path tests succeed 🎉

Try Che-Theia editor only Try Che-Theia with Java/maven example Try Che-Theia with NodeJs example

See Details

name link
che-theia docker.io/maxura/che-theia:606
che-theia-endpoint-runtime-binary docker.io/maxura/che-theia-endpoint-runtime-binary:606

Tested with Eclipse Che Single User on K8S (minikube v1.1.1)

@vinokurig vinokurig merged commit d2cc000 into master Jan 30, 2020
@vinokurig vinokurig deleted the che-15242 branch January 30, 2020 14:09
vinokurig pushed a commit that referenced this pull request Apr 6, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Terminal change title feature should be configurable
7 participants