Cleanup loading terminal fonts #6937

Nokel81 · 2023-01-13T17:01:39Z

Follow up #6913

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 sebastian@malton.name

- 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 <sebastian@malton.name>

Signed-off-by: Sebastian Malton <sebastian@malton.name>

ixrock · 2023-01-14T15:13:03Z

src/features/terminal/renderer/fonts/preload-fonts.injectable.ts

+      const terminalFonts = di.inject(terminalFontsInjectable);
+      const loadTerminalFont = di.inject(loadTerminalFontInjectable);


Shouldn't these be outside of run()?

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.

ixrock · 2023-01-14T15:18:06Z

...preferences/renderer/preference-items/terminal/terminal-font-family/terminal-font-family.tsx

-    userStore: di.inject(userStoreInjectable),
-    logger: di.inject(loggerInjectable),
-    terminalFonts: di.inject(terminalFontsInjectable),
+    model: di.inject(terminalFontPreferenceModelInjectable),


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.

For further reading: https://en.wikipedia.org/wiki/Occam's_razor

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.

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.

ixrock · 2023-01-14T18:03:59Z

src/features/terminal/renderer/fonts/red-hat-mono.injectable.ts

+const redHatMonoTerminalFontInjectable = getInjectable({
+  id: "red-hat-mono-terminal-font",
+  instantiate: () => ({
+    name: "Red Hat Mono",
+    url: RedHatMono,
+  }),
+  injectionToken: terminalFontInjectionToken,
+});
+
+export default redHatMonoTerminalFontInjectable;


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, });

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.

ixrock · 2023-01-14T18:08:50Z

...renderer/preference-items/terminal/terminal-font-family/terminal-font-options.injectable.tsx

+      current: computed(() => userStore.terminalConfig.fontFamily),
+      set: action(selection => {
+        userStore.terminalConfig.fontFamily = selection?.value ?? defaultTerminalFontFamily;
+      }),


nit: better readability + set() should not know anything about SelectOption, better to pass just a font value:

Suggested change

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;

}),

I can update the name of the functions sure but using computed(() => ...) better communicates the contract then a getter.

Better for who? Why end-user of this let's say api should care about computed that on that side or not?

In other words to distinguish observable this field or not?

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 <sebastian@malton.name>

Nokel81 added chore area/terminal labels Jan 13, 2023

Nokel81 added this to the 6.4.0 milestone Jan 13, 2023

Nokel81 requested a review from a team as a code owner January 13, 2023 17:01

Nokel81 requested review from jansav and Iku-turso and removed request for a team January 13, 2023 17:01

Fix tests

ac92299

Signed-off-by: Sebastian Malton <sebastian@malton.name>

ixrock previously approved these changes Jan 14, 2023

View reviewed changes

ixrock reviewed Jan 14, 2023

View reviewed changes

Cleanup formatting and change model to presenter

fc42120

Signed-off-by: Sebastian Malton <sebastian@malton.name>

Nokel81 dismissed ixrock’s stale review via fc42120 January 16, 2023 14:37

aleksfront approved these changes Jan 17, 2023

View reviewed changes

Nokel81 merged commit e635d82 into master Jan 17, 2023

Nokel81 deleted the cleanup-loading-terminal-fonts branch January 17, 2023 13:11

Nokel81 mentioned this pull request Jan 18, 2023

Release 6.4.0-alpha.4 #6975

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup loading terminal fonts #6937

Cleanup loading terminal fonts #6937

Nokel81 commented Jan 13, 2023 •

edited by ixrock

ixrock Jan 14, 2023

Nokel81 Jan 16, 2023

ixrock Jan 14, 2023

ixrock Jan 16, 2023 •

edited

Nokel81 Jan 16, 2023

ixrock Jan 16, 2023

ixrock Jan 14, 2023

Nokel81 Jan 16, 2023

ixrock Jan 14, 2023 •

edited

Nokel81 Jan 16, 2023

ixrock Jan 16, 2023

ixrock Jan 16, 2023

Nokel81 Jan 16, 2023

		const terminalFonts = di.inject(terminalFontsInjectable);
		const loadTerminalFont = di.inject(loadTerminalFontInjectable);

-      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;
+      }),

Cleanup loading terminal fonts #6937

Cleanup loading terminal fonts #6937

Conversation

Nokel81 commented Jan 13, 2023 • edited by ixrock

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ixrock Jan 16, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ixrock Jan 14, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nokel81 commented Jan 13, 2023 •

edited by ixrock

ixrock Jan 16, 2023 •

edited

ixrock Jan 14, 2023 •

edited