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

consider proportional font char width #656

Closed
wants to merge 3 commits into from
Closed

consider proportional font char width #656

wants to merge 3 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Mar 10, 2018

This makes the package consider the actual width of characters from proportional fonts when displaying them on the minimap.

Notes:

  • The method I created still needs documentation.
  • The width of the digit zero is used as a reference for the width of the characters (a setting can probably be added to allow users to configure this).
  • The behavior for monospaced fonts remains unchanged.
  • Someone still needs to update other packages such as minimap-selection. (If no‐one else wants to, I can probably try to do it.)
    • Perhaps provide a way for people to get coordinates on the minimap from a line and column (if that’s not already provided.

This makes the package consider the actual width of characters from proportional fonts when displaying them on the minimap.
Zambonifofex added 2 commits March 10, 2018 12:44
@abe33
Copy link
Contributor

abe33 commented Sep 20, 2018

Hi @Zambonifofex, sorry for the very late reply.

While I would have enjoyed see support for proportional font, it looks like this implementation have a catastrophic effect on performances. I pulled the PR locally to test it and opening the minimap.js file (a reasonable ~1000 lines file) I was no longer able to do anything in atom. AFAICT you're creating a dom node, getting a ref to the editor and build an extra array for every token on every render of that token, that's way too much costly things occuring in what should be a really fast loop.
To be honest I thought about a way to implement that for a long time while actively working on the minimap and never found a satisfying solution, if you do find one that does not impact performances I'll be happy to merge it.

@abe33 abe33 self-requested a review September 20, 2018 12:04
Copy link
Contributor

@abe33 abe33 left a comment

Choose a reason for hiding this comment

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

As is the performances are lacking so I can't merge it, but if any improvements were to be made making the whole thing almost as fast as the current system then it'll be good to merge.

@ghost
Copy link
Author

ghost commented Sep 22, 2018

Hello, @abe33. You don’t need to worry about taking time to reply at all!

I have realized this problem soon after I made this pull request. I was hoping to mention it, but eventually I forgot and moved away because of the lack of interest.

But now, since you actually showed interest, I figured I should spend some time thinking about it again.

I spent yesterday trying a couple different approaches such as reading the size directly from the DOM in the Atom editor itself. Eventually I came accross the measureText method of the 2D canvas rendering context.

I created this method called estimateTextOffsets on the lib/mixin/canvas-drawer.js file that uses the measureText method to estimate the size of the text in the real editor. It seems to be really snappy now!

I called it “estimate” because it’s exactly that, unfortunately. It calculates the advance‐width of each token, but it fails to realize that kerning between tokens and ligatures might make the estimate wrong.

For example, consider that I have the following text in an editor:

ILTLTLTLTLTLTLTLTLTLTLTLTLTLT
lLTLTLTLTLTLTLTLTLTLTLTLTLTLT

And suppose that I have a grammar that considers L to be a different token than T in a word that starts with I, and considers LT chains in a word that starts with l to be a single token. Now, I have the exact opposite problem than the one I described in #655 — that is, the lines have the same size on the editor, but a different size on the minimap.

A better estimative would be to measure the advance‐width of the entire line, perform the current estimation on each token of that line, then finally scale the estimate of each token so that they all fit within that line.

Of course, that’d still be just a estimative. Consider that I have the following text in an editor, and the same grammar I already mentioned:

ILTLTLTLTLTLTLTLTLTLTLTLTLTLT lLTLTLTLTLTLTLTLTLTLTLTLTLTLT
lLTLTLTLTLTLTLTLTLTLTLTLTLTLT ILTLTLTLTLTLTLTLTLTLTLTLTLTLT

Now, the lines have same width, but the spaces in the middle that are aligned in the editor are offset from each other in the minimap.

Unfortunately, I don’t think there is a clear solution to this problem.


Something to note is that I got rid of the use of the character width setting from the methods I edited as well as of the placeholder character 0 in favor of multiplying by the character height.

I estimate the character height by assuming it is equal to the font size. I commented out a more sofisticated approach using attributes of the result of the measureText method that are not implemented in Chrome/Electron yet because — well — they are not implemented yet.

My hope is that the character width setting as well as the interline setting can eventually go away, as they can be derived from the minimap plugin’s character height setting and Atom’s builtin line height setting.

Something else kinda cute that could be done once the fancier measureText result arrives in Chrome is to size tokens on the minimap based on their actual height, so, for example, . would be shown as smaller in height than | even in the minimap.


Not particularly related to this pull request and issue, but noteworthy nonetheless:

Something I’ve worked on when I first started playing around with the minimap codebase is the ability to resize the minimap with a handle.

Something else related to that I also worked on is to have the minimap be naturally sized based on the actual width of the editor, making so that characters that don’t appear on the editor when it is scrolled all the way left also won’t appear on the minimap. (As well as characters that do appear on the editor will always appear on the minimap.)

Whenever I dragged the handle, it changed the size of the minimap. Whenever I double‐clicked the handle, it resetted the minimap to the be based on the width of the editor.

Unfortunately, I don’t think I have that code anymore. But if there is interest, I can try to work on it again.


Talking about proportinal font support once again, I’m not sure whether I can use this pull request to share the code I wrote, and even if I can, I’m not sure it’s a good idea, since the code I wrote now has nothing to do with the code I wrote then.

Worst case scenario I end up creating a different pull request to share my code.

Also, I’m not sure if I should start working on the second estimation approach I mentioned before sharing my code, or if I should just share it as it is and think about it later. 🤔


Finally, sorry for the lengthy comment. I feel like generally I’m kinda bad at containing my own thoughts when I’m writing. 😅

@ghost
Copy link
Author

ghost commented Sep 24, 2018

So, I created the following grammar to test out what I said:

{
	"name": "Test Language",
	"scopeName": "my-language",
	"fileTypes": [],
	"patterns":
	[
		{"match" : "l(LT)*", "name": "keyword"},
		{"match": "L", "name": "keyword"}
	]
}

And, as it turns out, the behavior I said would happen in the minimap also happened in the actual editor. I’ve reported the bug to the Chromium issue tracker, but, until it gets fixed, I think it’ll be a good idea for me to just share the code I’ve written already.

@abe33
Copy link
Contributor

abe33 commented Sep 25, 2018

Hi @Zambonifofex and thank you for taking the time of writing such a detailed post 🙇. There's definitely interesting information here. As I said, I had started looking into this a long time ago and this is definitely a healthy refresher about current possible solutions. I'll try to take some time today or tomorrow to write down some of the things I had in mind at the time (I need to find the notes I wrote at the time).
As for the process, I think it'll be interesting to keep that PR alive, even if that means resetting its content, so that we can keep the discussion in one place. Also, that might be a good thing to make that feature hidden behind a setting from the get go (ultimately, if we want to release it it might be just a matter of changing the default).

Stay tuned

@UziTech
Copy link
Collaborator

UziTech commented May 22, 2021

Closing since it is marked wontfix

@UziTech UziTech closed this May 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants