Cleanup loading terminal fonts #6937
Conversation
- Make list of possible fonts fully OCP - Only mark the loading of a font as 'causesSideEffects' - Make a view model for the terminal font preferences component - Move the TTF files next to where they are registered to help with finding them Signed-off-by: Sebastian Malton <[email protected]>
Nokel81
requested review from
jansav and
Iku-turso
and removed request for
a team
Signed-off-by: Sebastian Malton <[email protected]>
| const terminalFonts = di.inject(terminalFontsInjectable); | ||
| const loadTerminalFont = di.inject(loadTerminalFontInjectable); |
There was a problem hiding this comment.
Shouldn't these be outside of run()?
There was a problem hiding this comment.
They could be, however I have found that for Runnable's having the di.inject calls within the run() causes less headaches. It is not that important here, but when there are runAfter's involved it is very important.
| userStore: di.inject(userStoreInjectable), | ||
| logger: di.inject(loggerInjectable), | ||
| terminalFonts: di.inject(terminalFontsInjectable), | ||
| model: di.inject(terminalFontPreferenceModelInjectable), |
There was a problem hiding this comment.
Where this model would be useful/utilized besides terminal settings?
Currently nowhere and probably would not need anywhere.. So why to make it complex from the beginning and split to maximum amount of files? No benefits currently, imho.
There was a problem hiding this comment.
For further reading: https://en.wikipedia.org/wiki/Occam's_razor
There was a problem hiding this comment.
This is an implementation of the https://en.wikipedia.org/wiki/Model-view-presenter pattern. Like similar patterns it helps make the view very passive and simple.
There was a problem hiding this comment.
Yes, it's kinda improvement I agree.
But maybe done too early before any real some needs/system optimizations requirements, meanwhile making a whole system a bit more complex for nothing.
| const redHatMonoTerminalFontInjectable = getInjectable({ | ||
| id: "red-hat-mono-terminal-font", | ||
| instantiate: () => ({ | ||
| name: "Red Hat Mono", | ||
| url: RedHatMono, | ||
| }), | ||
| injectionToken: terminalFontInjectionToken, | ||
| }); | ||
|
|
||
| export default redHatMonoTerminalFontInjectable; |
There was a problem hiding this comment.
btw: why do we need to name some injectable when it's name never used in many files? we could do direct default export like this:
export default getInjectable({
id: "red-hat-mono-terminal-font",
instantiate: () => ({
name: "Red Hat Mono",
url: RedHatMono,
}),
injectionToken: terminalFontInjectionToken,
});There was a problem hiding this comment.
In this case we could just do a export default since this will only ever be di.injectMany-ed. The reason to add the const ...; export default ...; is to help with intellisense and auto-import when using an injectable with di.inject(). This way it can easily be auto-imported if someone ever wants to import just this one injectable.
| current: computed(() => userStore.terminalConfig.fontFamily), | ||
| set: action(selection => { | ||
| userStore.terminalConfig.fontFamily = selection?.value ?? defaultTerminalFontFamily; | ||
| }), |
There was a problem hiding this comment.
nit: better readability + set() should not know anything about SelectOption, better to pass just a font value:
| current: computed(() => userStore.terminalConfig.fontFamily), | |
| set: action(selection => { | |
| userStore.terminalConfig.fontFamily = selection?.value ?? defaultTerminalFontFamily; | |
| }), | |
| get fontFamily(){ | |
| return userStore.terminalConfig.fontFamily; | |
| }, | |
| setFont: action(fontFamily => { | |
| userStore.terminalConfig.fontFamily = fontFamily ?? defaultTerminalFontFamily; | |
| }), |
There was a problem hiding this comment.
I can update the name of the functions sure but using computed(() => ...) better communicates the contract then a getter.
There was a problem hiding this comment.
Better for who? Why end-user of this let's say api should care about computed that on that side or not?
There was a problem hiding this comment.
In other words to distinguish observable this field or not?
There was a problem hiding this comment.
Why end-user of this let's say api should care about computed that on that side or not?
So that the API user knows that they should only read the field in an observable context or that reading it multiple times outside of a context may result in different values.
Signed-off-by: Sebastian Malton <[email protected]>
Follow up #6913
Signed-off-by: Sebastian Malton [email protected]