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

messages: allow to set the terminal window title #160

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

sergio-costas
Copy link
Contributor

@sergio-costas sergio-costas commented Jun 21, 2023

This patch allows to set the text in the title of a terminal window. The idea is to allow snapcraft to set in the title which step is currently doing.

This is a first proposal for #159

  • Have you followed the guidelines for contributing?
  • Have you signed the CLA?
  • Have you successfully run tox?

@codecov
Copy link

codecov bot commented Jun 21, 2023

Codecov Report

Merging #160 (04c2c41) into main (91c4b88) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #160      +/-   ##
==========================================
+ Coverage   94.62%   94.66%   +0.04%     
==========================================
  Files           7        7              
  Lines        1042     1050       +8     
  Branches      190      193       +3     
==========================================
+ Hits          986      994       +8     
  Misses         53       53              
  Partials        3        3              
Files Changed Coverage Δ
craft_cli/messages.py 100.00% <100.00%> (ø)
craft_cli/printer.py 100.00% <100.00%> (ø)

craft_cli/messages.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@tigarmo tigarmo 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! Can you do a companion PR in Snapcraft using this branch so we can manually test out this change a bit?

craft_cli/messages.py Outdated Show resolved Hide resolved
tests/unit/test_messages_integration.py Outdated Show resolved Hide resolved
@sergio-costas
Copy link
Contributor Author

Ok, you can use canonical/snapcraft#4217 to test this.

@sergio-costas sergio-costas marked this pull request as ready for review June 22, 2023 11:37
@sergio-costas
Copy link
Contributor Author

@tigarmo I forgot to change this from draft to ready. Done.

@tigarmo
Copy link
Collaborator

tigarmo commented Jun 22, 2023

Thanks. I'll try this out and give you feedback today

@tigarmo
Copy link
Collaborator

tigarmo commented Jun 22, 2023

I did some experimenting and found a couple of problems:

  1. The escape code to set the window title doesn't work on default Konsole. It does work on xterm. Which terminal are you using? I can get it to work on Konsole if I change the "Tab title format" option to %w:
    image
    I wonder if there's a "more portable" way to do this.

  2. The last set title persists after Snapcraft exits. See the end of this gif:
    craft-cli-window-title
    we should clear that up, ideally restoring the original title but I don't know how feasible that is.

@sergio-costas
Copy link
Contributor Author

I'm using Terminus, which uses VTE.Terminal class inside. The codes that I use in the patch are the XTerm ones, yes... https://tldp.org/HOWTO/Xterm-Title-3.html

Can you test this?

imagen

it seems that it's the ANSI code for Konsole...

About preserving the title... yes, you are right. I didn't implement that because, in my case, it already does that due to a script that I have for GIT, but it should be preserved and restored. I'll try it.

@sergio-costas
Copy link
Contributor Author

I'll try changing the ANSI command from 2 (set the title) to 0 (set the title and the icon) just in case...

@sergio-costas
Copy link
Contributor Author

Sent a bug report to KDE about the Konsole problem: https://bugs.kde.org/show_bug.cgi?id=471353

@sergio-costas
Copy link
Contributor Author

@tigarmo Mmm... odd... in ubuntu there is this line in ~/.bashrc:

imagen

It forces the window title to the current folder every time the prompt appears... which makes sense, because not all programs restore the window title.

Fedora has a similar line:

imagen

Are you using a ubuntu-derived distro? Which value returns in your system echo $TERM? (in my case, it returns xterm-256color).

Because all this seems to point to the konsole bug that delays the title change.

@sergio-costas sergio-costas force-pushed the UDENG-832-allow-to-set-the-window-title branch from 705ccae to 06fb781 Compare September 25, 2023 10:24
@sergio-costas
Copy link
Contributor Author

sergio-costas commented Sep 25, 2023

@tigarmo This should be ready for merging. As commented, the konsole bug seems to be a problem in konsole itseld, that doesn't refresh the UI unless there is a change in the terminal itself. Also, it seems to be not possible to read the current title in the terminal (at least in a standard way), since it is set using ANSI commands. And since as soon as a key is pressed, the title is updated in konsole, I think that it's not a true problem.

@sergio-costas
Copy link
Contributor Author

Also, the linting bug seems to be a thing of setuptools_scm.

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.

2 participants