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

Warn users if the terminal font is not monospace #45226

Closed
Tyriar opened this issue Mar 7, 2018 · 12 comments · Fixed by #48028
Closed

Warn users if the terminal font is not monospace #45226

Tyriar opened this issue Mar 7, 2018 · 12 comments · Fixed by #48028
Assignees
Labels
feature-request Request for new features or functionality *out-of-scope Posted issue is not in scope of VS Code terminal General terminal issues that don't fall under another label upstream Issue identified as 'upstream' component related (exists outside of VS Code)
Milestone

Comments

@Tyriar
Copy link
Member

Tyriar commented Mar 7, 2018

As a workaround for #35681, we could measure characters, maybe i and w and tell the user to use a monospace font if they differ.

@Tyriar Tyriar added help wanted Issues identified as good community contribution opportunities feature-request Request for new features or functionality good first issue Issues identified as good for first-time contributors terminal General terminal issues that don't fall under another label labels Mar 7, 2018
@Tyriar Tyriar self-assigned this Mar 7, 2018
@RubenMateus
Copy link

RubenMateus commented Mar 7, 2018

@Tyriar I would really like to help on this one has my first issue on vscode repo.

@Tyriar
Copy link
Member Author

Tyriar commented Mar 9, 2018

@RubenMateus that would be great!

To give you some pointers, there is an element used for measuring characters here:

https://github.com/Microsoft/vscode/blob/05b995dd9c99b709f41577ab04fe53740f67ccea/src/vs/workbench/parts/terminal/electron-browser/terminalConfigHelper.ts#L31

I see this as a new function on TerminalConfigHelper similar to _measureFont which checks the 2 characters and verifies they're the same width. This can be called by TerminalPanel when the font changes and show a notification (by adding INotificationService to constructor) if it's different:

https://github.com/Microsoft/vscode/blob/05b995dd9c99b709f41577ab04fe53740f67ccea/src/vs/workbench/parts/terminal/electron-browser/terminalPanel.ts#L78-L82

@jay-khatri
Copy link

@Tyriar have there been any developments on this issue? I'd love to take a look!

@Tyriar
Copy link
Member Author

Tyriar commented Mar 16, 2018

@jay-khatri not yet, go for it 😃

@jay-khatri
Copy link

jay-khatri commented Mar 19, 2018

@Tyriar I took a look and wanted to clarify. I understand that the point of this is to confirm whether or not a font is monospace. With that being said, am I expected to render an actual html element and measure the difference among two separate letters (eg. in pixels)? I'm now pretty familiar with the code organization, just a little confused as to how to approach measuring the font 🙂

@jay-khatri
Copy link

Ping @Tyriar 😬 🙂

@Tyriar
Copy link
Member Author

Tyriar commented Mar 22, 2018

@jay-khatri yes, by utilizing _charMeasureElement which I believe has font assigned to it

@Tyriar
Copy link
Member Author

Tyriar commented May 25, 2018

#48028 adds the following notification when the terminal font is changed:

image

@Tyriar Tyriar added this to the May 2018 milestone May 25, 2018
@Tyriar Tyriar added the verification-needed Verification of issue is requested label May 25, 2018
Tyriar added a commit that referenced this issue May 25, 2018
Attempts to make described workaround from issue #45226
@roblourens
Copy link
Member

Works, but I don't see the notification if I have the terminal closed, then change the font setting, then open the terminal. I think it should be shown in that case too.

@roblourens roblourens reopened this May 30, 2018
@roblourens roblourens added the verification-found Issue verification failed label May 30, 2018
ahmadawais pushed a commit to ahmadawais/vscode that referenced this issue May 31, 2018
When warning the user about non-monospace font, the notification lacks context. This PR addresses that by giving user a bit more context about what went wrong. 

For Issue microsoft#45226
@Tyriar Tyriar modified the milestones: May 2018, June 2018 May 31, 2018
@Tyriar Tyriar removed the verification-found Issue verification failed label May 31, 2018
@Tyriar
Copy link
Member Author

Tyriar commented Jun 18, 2018

@rebornix any ideas on whether the editor has something to leverage here? Basically I need some element to measure the width of w and i to check if it's really a monospace font but it only works when the panel is visible. I don't really want to break out of the panel element and touch things that the terminal doesn't own.

@Tyriar
Copy link
Member Author

Tyriar commented Jul 9, 2018

I'm toying with the idea of supporting this in xterm.js by specifying a measureContainer element to do the measurements in xtermjs/xterm.js#702. That would move the logic upstream, it would still be a little weird for me to put this element somewhere outside of the terminal panel though.

Ping @rebornix

@Tyriar Tyriar added upstream Issue identified as 'upstream' component related (exists outside of VS Code) and removed good first issue Issues identified as good for first-time contributors help wanted Issues identified as good community contribution opportunities labels Jul 9, 2018
@Tyriar Tyriar modified the milestones: July 2018, Backlog Jul 9, 2018
@bpasero bpasero removed the verification-needed Verification of issue is requested label Aug 30, 2018
@Tyriar Tyriar added the *out-of-scope Posted issue is not in scope of VS Code label Oct 8, 2019
@vscodebot
Copy link

vscodebot bot commented Oct 8, 2019

This issue is being closed to keep the number of issues in our inbox on a manageable level, we are closing issues that are not going to be addressed in the foreseeable future: We look at the number of votes the issue has received and the number of duplicate issues filed. More details here. If you disagree and feel that this issue is crucial: We are happy to listen and to reconsider.

If you wonder what we are up to, please see our roadmap and issue reporting guidelines.

Thanks for your understanding and happy coding!

@vscodebot vscodebot bot closed this as completed Oct 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request Request for new features or functionality *out-of-scope Posted issue is not in scope of VS Code terminal General terminal issues that don't fall under another label upstream Issue identified as 'upstream' component related (exists outside of VS Code)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants