Split CharAtlas types and utility functions off #1307
Conversation
I'm starting to pull some changes off of my large WIP branch/commit that adds an alternative dynamic character atlas. We're going to need to support multiple CharAtlas implementations, and this should make that easier.
|
Looks good, but I'd rather merge this as part of your final PR to be honest. I'm a little bit concerned this grows the code base without much gain otherwise. @Tyriar Any thoughts? |
If you'd like, I can close this and wait until the rest of my changes for #1294 are ready, and then have one PR with a bunch of commits. I just figured it might be easier to review if I got a few small changes out of the way first. (this and #1308). |
|
It doesn't look like this would affect the overall size of the codebase, seems like it's mostly moving code around rather than adding. Splitting the big change into several smaller changes will make them much easier to merge in. |
|
Also I like the idea of shipping 2 options to begin with, I caused a lot of problems when I did this change initially because I completely ripped out the DOM-based renderer. It would have been worth the extra overhead to have an option for users to switch back. |
There was a problem hiding this comment.
The overall idea behind the change looks good, just a note about the folders. The shared/ folder was created as a place to put code that could run in both a browser context and a web worker context, to allow future priming of the char atlas in a different thread and falling back when it's not supported by the browser.
Not sure we will ship web worker support any time soon but let's try stick to this rule in the meantime; basically anything inside shared/ can only reference other files inside shared/. So CHAR_ATLAS_CELL_SPACING for example should live inside shared/ and have the browser context parts pointing to it.
src/renderer/CharAtlas.ts
Outdated
| import { isFirefox } from '../shared/utils/Browser'; | ||
| import { generateCharAtlas, ICharAtlasRequest } from '../shared/CharAtlasGenerator'; | ||
| import { generateConfig, configEquals } from './atlas/CharAtlasUtils'; | ||
|
|
||
| export const CHAR_ATLAS_CELL_SPACING = 1; |
src/renderer/atlas/Types.ts
Outdated
| import { IColorSet } from '../Types'; | ||
|
|
||
| export const CHAR_ATLAS_CELL_SPACING = 1; | ||
| export const INVERTED_DEFAULT_COLOR = -1; |
There was a problem hiding this comment.
It doesn't look like are being used anywhere.
- Ensures that everything is only defined in one place, and everything imports from that place. - Moves CHAR_ATLAS_CELL_SPACING into a subdirectory of shared/ so that CharAtlasGenerator can pull from it. This addresses the comments on https://github.com/xtermjs/xterm.js/pull/1307/files/454fd4bfb1ab5c9ece
Moves CharAtlas into src/renderer/atlas/ and CharAtlasGenerator into src/shared/atlas/.
|
I've added a couple more commits that should address the comments. On a somewhat related note: @Tyriar, how do you feel about using absolute imports everywhere? |
|
Thanks again @bgw, looking forward to more in this area 😄 |
I'm starting to pull some changes off of my large WIP branch/commit that adds an alternative dynamic character atlas (#1294). We're going to need to support multiple CharAtlas implementations, and this should make that easier.