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

Improve tabs title #892

Merged
merged 10 commits into from
Oct 22, 2016
Merged

Improve tabs title #892

merged 10 commits into from
Oct 22, 2016

Conversation

parro-it
Copy link
Contributor

As discussed in #49
First commit add an hook on hterm instance to catch ANSI escape instructions to set terminal title.
Second commit remove process polling code to set window title.
The commented code in this commit has to be replaced with something that listen to active tab changes, and set the window title the same as the current tab title.

Do you know if there is an easy way to listen for active tab changes from session class?

Actually, the window title is always the one taken from html file.

if (this.subscribed) {
this.titlePoll = setTimeout(() => this.getTitle(), TITLE_POLL_INTERVAL);
}
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This code has to be replaced with something that improve the usefulness of the window title.
We could set the window title to the one of current tab, and keep track of its changes.

Copy link
Member

Choose a reason for hiding this comment

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

We can straight up remove

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I didn't mean to leave this part commented... but I think it could be useful to replace this code with some logic to set the window title in a way that it reflect the title of the current tab.e.g. iTerm works this way...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used this on macOS today (I wrote this PR working on linux). I noticed the window title is not used at all on macOS, so the comment above apply only for linux OS...


// emit onTitle event when hterm instance
// whants to set the title of the app
this.term.setWindowTitle = props.onTitle.bind(props);
Copy link
Member

Choose a reason for hiding this comment

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

I think null is fine as a bind

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, binding to props is nonsensical...

if (this.subscribed) {
this.titlePoll = setTimeout(() => this.getTitle(), TITLE_POLL_INTERVAL);
}
});
Copy link
Member

Choose a reason for hiding this comment

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

We can straight up remove

@parro-it
Copy link
Contributor Author

In latest commit, I go ahead and I removed all parts of code that seems now unused or redundant. Please double check this, I fear to had removed something useful 😨

Also I removed SESSION_SET_PROCESS_TITLE action, since it appear was only emitted by the process polling code.

But, apart from my hyperterm-title plugin, it seems to be used also by https://github.com/patrikholcak/hypertitle and https://github.com/sdomino/hyperterm-tab-cwd. So this could be a breaking change... let me know if you prefer I revert these code cleanup commits

@rauchg
Copy link
Member

rauchg commented Oct 19, 2016

Honestly I think it's ok. If the polling is not happening anymore, nor should the action.

@parro-it
Copy link
Contributor Author

Honestly I think it's ok. If the polling is not happening anymore, nor should the action.

Fine 🎉. Could you please restart the Travis build? Seems that the macOs build was stalled and never ran.

@rauchg
Copy link
Member

rauchg commented Oct 22, 2016

Just restarted this

@rauchg rauchg merged commit 97432df into vercel:master Oct 22, 2016
chabou added a commit to chabou/hyper that referenced this pull request Nov 2, 2016
* master:
  chore(package): update electron to version 1.4.5 (vercel#949)
  add onRendererWindow, because it actually available. check ./lib/utils/plugins.js:193 (vercel#924)
  increase timeout for update checks (vercel#928)
  Change URL regex - fixes vercel#867 (vercel#943)
  Make the close menu items less ambiguous
  Comply to XO's no-warning-comments rule
  Prefer default export to make XO happy (vercel#931)
  chore(package): update ms to version 0.7.2 (vercel#933)
  chore(package): update copy-webpack-plugin to version 4.0.0 (vercel#927)
  log plugins' errors in Electron console (vercel#923)
  Improve tabs title   (vercel#892)
  increase notification timeout up to 30m (vercel#913)
  Create github templates (vercel#919)
  chore(package): update electron to version 1.4.4 (vercel#907)
  Prevent Hterm to share same preferences between instances (vercel#906)
  0.8.3
  0.9.0
  Revert "Quick full screen (vercel#803)"
  Ignore file watch errors. Fixes vercelGH-225 (vercel#893)
chabou added a commit to chabou/hyper that referenced this pull request Nov 2, 2016
* master:
  chore(package): update electron to version 1.4.5 (vercel#949)
  add onRendererWindow, because it actually available. check ./lib/utils/plugins.js:193 (vercel#924)
  increase timeout for update checks (vercel#928)
  Change URL regex - fixes vercel#867 (vercel#943)
  Make the close menu items less ambiguous
  Comply to XO's no-warning-comments rule
  Prefer default export to make XO happy (vercel#931)
  chore(package): update ms to version 0.7.2 (vercel#933)
  chore(package): update copy-webpack-plugin to version 4.0.0 (vercel#927)
  log plugins' errors in Electron console (vercel#923)
  Improve tabs title   (vercel#892)
  increase notification timeout up to 30m (vercel#913)
  Create github templates (vercel#919)
  chore(package): update electron to version 1.4.4 (vercel#907)
tylerong pushed a commit to tylerong/hyper that referenced this pull request Jul 3, 2017
* Set tab title when asked by the contained shell process via ANSI code

* Commented out process polling code

* Removed unused module and constant

* Remove getTitle method

* Removed props binding

* Removed polling clearTimeout

* Removed session focus & blur

* Remove listening for session `title` event

* Removed SESSION_SET_PROCESS_TITLE action

* Removed remainig blur
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.

None yet

2 participants