Better handling loading terminal fonts #6913
Conversation
Signed-off-by: Roman <[email protected]>
ixrock
requested review from
jansav,
Nokel81 and
aleksfront
and removed request for
a team
src/renderer/components/dock/terminal/terminal-fonts.injectable.ts
Outdated
Show resolved
Hide resolved
ixrock
force-pushed
the
fix/terminal_fonts_loading
branch
2 times, most recently
from
28059c3 to
cb140c9
Compare
Signed-off-by: Roman <[email protected]>
ixrock
force-pushed
the
fix/terminal_fonts_loading
branch
from
cb140c9 to
e3961ad
Compare
Signed-off-by: Roman <[email protected]>
ixrock
force-pushed
the
fix/terminal_fonts_loading
branch
from
6232894 to
4a271d5
Compare
| "Source Code Pro", "Space Mono", "Ubuntu Mono", | ||
| ].map(customFont => { | ||
| ({ userStore, logger, terminalFonts }: Dependencies) => { | ||
| const bundledFonts: SelectOption<string>[] = Array.from(terminalFonts.keys()).map(font => { | ||
| const { fontFamily, fontSize } = userStore.terminalConfig; |
There was a problem hiding this comment.
nit: this line could be moved to outside the .map(() => {...}) which could simplify the code
| @@ -9,7 +9,8 @@ import type { TabId } from "../dock/store"; | |||
| import type { TerminalApi } from "../../../api/terminal-api"; | |||
| import terminalSpawningPoolInjectable from "./terminal-spawning-pool.injectable"; | |||
| import terminalConfigInjectable from "../../../../common/user-store/terminal-config.injectable"; | |||
| import terminalCopyOnSelectInjectable from "../../../../common/user-store/terminal-copy-on-select.injectable"; | |||
| import terminalCopyOnSelectInjectable | |||
There was a problem hiding this comment.
this file probably doesn't need to be changed in this PR
|
|
||
| export default getGlobalOverride(preloadAllTerminalFontsInjectable, () => { | ||
| return { | ||
| id: "", |
There was a problem hiding this comment.
nit: might be a good idea to keep the same id
There was a problem hiding this comment.
I intentionally didn't put any id to avoid duplication and keeping in sync with the injectable in future (it feels like it should not be responsible for this, maybe we should not ask id at all and use it from passed injectable automatically) cc @jansav
There was a problem hiding this comment.
This ID is different from the Injectable ID because it is part of the data which is created, same as any other field (could have been call name).
There was a problem hiding this comment.
Why it's different? If I duplicate the string it would be the same. Moreover, we can use different ID from returned object (from runnable):
export const preloadAllTerminalFontsInjectable = getInjectable({
id: "preloadAllTerminalFontsInjectable",
instantiate(di) {
const terminalFonts = di.inject(terminalFontsInjectable);
const preloadFont = di.inject(preloadTerminalFontInjectable);
return {
id: "preload-all-terminal-fonts", // <--- THAT ONE
async run() {
await Promise.allSettled(
Array.from(terminalFonts.keys()).map(preloadFont),
);
},
};
},
injectionToken: beforeFrameStartsFirstInjectionToken,
causesSideEffects: true,
});There was a problem hiding this comment.
id: "preload-all-terminal-fonts", // <--- THAT ONE This is the one that I am talking about
There was a problem hiding this comment.
Yes, I mean this ID should be utilized automatically from passed injectable to make id optional.
There was a problem hiding this comment.
Fair enough that would be nicer indeed
| id: "terminalFontsInjectable", | ||
|
|
||
| instantiate() { | ||
| return new Map([ |
There was a problem hiding this comment.
nit: either this map should be created from a call to di.injectMany(terminalFontInjectionToken) (for some new injection token) and then be fully OCP or it doesn't need to be wrapped in an injectable.
|
|
||
| injectionToken: beforeFrameStartsFirstInjectionToken, | ||
|
|
||
| causesSideEffects: true, |
There was a problem hiding this comment.
actually on further note, given that preloadTerminalFont is causesSideEffects: true this one doesn't have to be since it doesn't introduce any new side effects.
|
Merging, the above comments can be resolved in a future PR |
Moving out from loading fonts only by CSS to JS with FontFace.load() interface due rendering terminal on canvas and since it's more reliable/controllable way of loading font files in general).
Screen.Recording.2023-01-11.at.20.04.17.mov
close #5019