Desktop: feat: Added search list for font input fields #10248

ab-elhaddad · 2024-04-02T09:57:25Z

Added a list showing the installed fonts, that acts as a search suggestion based on the user input, so the user can choose from them.

Notes:

I have moved the input field element with the list to a new component so the states changes don't re-render the whole configScreen component, but just re-render the input field and the list

font-list.mp4

Styling with various themes

@PackElend label me please

laurent22 · 2024-04-02T10:24:49Z

packages/app-desktop/gui/ConfigScreen/ConfigScreen.tsx

+						<FontSearch
+							_key={key}
+							updateSettingValue={updateSettingValue}
+							inputType={inputType}
+							inputStyle={inputStyle}
+							settings={this.state.settings}
+							fonts={this.state.fonts}


Doesn't it mean that all text input fields are replaced with a font search field?

Oops, you are right. my bad
I have fixed it now.
bef11bb

laurent22 · 2024-04-02T10:24:57Z

packages/app-desktop/gui/ConfigScreen/FontSearch.tsx

+	cursor: 'pointer',
+};
+
+const FontSearch = (props: any) => {


fixed with 9832a9e

laurent22 · 2024-04-02T10:27:31Z

packages/app-desktop/gui/ConfigScreen/ConfigScreen.tsx

+		const child = fork(join(__dirname, 'loadFonts'));
+		child.on('message', (fonts: string[]) => {
+			this.setState({ fonts }, () => child.kill());
+		});


That will fail if the component is unmounted before the loadFonts script returns

Killing the child in componentWillUnmount will prevent any failures, right?

laurent22 · 2024-04-02T10:27:50Z

packages/app-desktop/gui/ConfigScreen/FontSearch.tsx

+	const showList = (display: boolean) => {
+		document.getElementById(`searchList-${key}`).style.display = display ? 'block' : 'none';
+	};
+	const onTextChange = (event: any) => {
+		setInputText(event.target.value);
+		updateSettingValue(key, event.target.value);
+		showList(true);
+	};
+	const onFocusHandle = (event: any) => {
+		setInputText(event.target.value); // To handle the case when the value is already set
+		showList(true);
+	};
+	const onFontClick = (font: string) => {
+		(document.getElementById(key) as HTMLDivElement).innerText = font;
+		updateSettingValue(key, font);
+		showList(false);
+	};


All callback handler should use useCallback

laurent22 · 2024-04-02T10:28:06Z

packages/app-desktop/gui/ConfigScreen/FontSearch.tsx

+					onTextChange(event);
+				}}
+				onFocus={onFocusHandle}
+				onBlur={() => setTimeout(() => showList(false), 150)} // Delay the hiding of the list to allow the click event to fire


This should be a useCallback too

laurent22 · 2024-04-02T10:28:12Z

packages/app-desktop/gui/ConfigScreen/FontSearch.tsx

+				onChange={(event: any) => {
+					onTextChange(event);
+				}}


useCallback

laurent22 · 2024-04-02T10:28:35Z

packages/app-desktop/gui/ConfigScreen/FontSearch.tsx

+};
+
+const FontSearch = (props: any) => {
+	const { _key: key, updateSettingValue, inputType, inputStyle, settings, fonts } = props;


Only pass what you need, and you don't need all settings here

fixed with 9832a9e

laurent22 · 2024-04-02T10:28:57Z

packages/app-desktop/gui/ConfigScreen/FontSearch.tsx

+								onClick={() => onFontClick(font)}
+								onMouseEnter={() => setHoveredFont(font)}
+								onMouseLeave={() => setHoveredFont('')}


Why the indirection? Just set onClick={onFontClick}, etc.

laurent22 · 2024-04-02T10:30:25Z

packages/app-desktop/gui/ConfigScreen/FontSearch.tsx

+			<div id={`searchList-${key}`} style={searchListStyle} >
+				{
+					areFontsLoading ? <div style={{ padding: '5px' }}>Loading...</div> :
+						filteredFonts.map((font: string) =>
+							<div
+								key={font}
+								style={{ ...optionStyle, fontFamily: `"${font}"`,
+									color: hoveredFont === font ? 'var(--joplin-background-color)' : 'var(--joplin-color)',
+									backgroundColor: hoveredFont === font ? 'var(--joplin-color)' : 'var(--joplin-background-color)',
+								}}


No dynamic styles, search for "rscss" in the doc

laurent22 · 2024-04-02T10:31:26Z

packages/app-desktop/gui/ConfigScreen/loadFonts.ts

+import fontList = require('font-list');
+
+fontList.getFonts({ disableQuoting: true })
+	.then((fonts: string[]) => process.send(fonts))
+	.catch((error: any) => console.error(error));


Use async/await. How does it actually handle errors? It's printed to the console?

ok no problem
Yes, it just prints the error to the console and the list just keeps showing Loading... like this

personalizedrefrigerator · 2024-04-02T16:52:20Z

packages/app-desktop/package.json

@@ -161,6 +161,7 @@
    "countable": "3.0.1",
    "debounce": "1.2.1",
    "electron-window-state": "5.0.3",
+    "font-list": "1.5.1",


Rather than adding a dependency, consider using window.queryLocalFonts (which should be available in Electron >= 25).

@ab-elhaddad, indeed we would rather not add this font-list package, so please confirm if you can update the PR using the above API

Yes, we can get the same result with the above API. Although typescript doesn't seem to recognize it, it exists and works efficiently.
updated with 1362f78

personalizedrefrigerator · 2024-04-03T22:46:04Z

packages/app-desktop/gui/ConfigScreen/ConfigScreen.tsx

-							spellCheck={false}
-						/>
+						{
+							md.label() === 'Editor font family' || md.label() === 'Editor monospace font family' ?


Rather than comparing md.label() with a hardcoded string, consider adding a new setting subType to the font settings. This approach would be similar to how the file/directory chooser is currently handled.

Comparing the label with a string could be problematic if the label for one of the font settings is updated at some point in the future or if the user changes Joplin to a non-English language.

fixed with 67800cd

laurent22 · 2024-04-04T10:45:53Z

packages/app-desktop/gui/ConfigScreen/ConfigScreen.tsx

+						{
+							md.subType === SettingItemSubType.FontFamily ?
+								<FontSearch
+									_key={key}


Why is _key needed?

The main purpose of passing the key is to be able to update the settings value (updateSettingValue), as it takes key as the first argument. But if you are asking about the naming and why didn't I use just key because key is a reserved word in react components and when using it, I get this warning ``key`` is not a prop. Trying to access it will result in ``undefined`` being returned. If you need to access the same value within the child component, you should pass it as a different prop.

The normal pattern for a React controlled component is to have a "value" and "onChange" (or similar) event. If you do that you won't have to pass around unnecessary properties and can create more reusable components.

So in this case, add an onChange event to your component which would pass to the parent the selected font. Then here just do onChange={fontFamily => updateSettingValue(key, fontFamily)}

I know there's a lot of code in this project and you can't know everything, but it often helps to see how things are done in the code near where you want to add something. For example you can see that all components here follow this value/onChange pattern. Maybe also read about controlled components and why they're generally preferred.

laurent22 · 2024-04-04T10:46:59Z

packages/app-desktop/gui/ConfigScreen/FontSearch.tsx

@@ -0,0 +1,82 @@
+import React = require('react');
+import { useMemo, useState, useCallback } from 'react';
+import { CSSProperties } from 'styled-components';


Import this from React - we don't use styled-components for new code

laurent22 · 2024-04-04T10:47:15Z

packages/app-desktop/gui/ConfigScreen/ConfigScreen.tsx

+									updateSettingValue={updateSettingValue}
+									inputType={inputType}
+									inputStyle={inputStyle}
+									fieldValue={this.state.settings[key]}


fieldValue => value

laurent22 · 2024-04-04T10:47:27Z

packages/app-desktop/gui/ConfigScreen/ConfigScreen.tsx

+								<FontSearch
+									_key={key}
+									updateSettingValue={updateSettingValue}
+									inputType={inputType}


inputType => type

laurent22 · 2024-04-04T10:47:55Z

packages/app-desktop/gui/ConfigScreen/ConfigScreen.tsx

+									_key={key}
+									updateSettingValue={updateSettingValue}
+									inputType={inputType}
+									inputStyle={inputStyle}


inputStyle => style

laurent22 · 2024-04-04T10:51:02Z

packages/app-desktop/gui/ConfigScreen/FontSearch.tsx

+	}, [showList]);
+
+	const onBlurHandle = useCallback(() =>
+		setTimeout(() => showList(false), 150) // Delay the hiding of the list to allow the click event to fire


Why would we want the click event to fire on blur? And it looks like you're already handling the click event below. Please remove the requirement for this timeout in any case because it will randomly fail

laurent22 · 2024-04-04T10:53:12Z

packages/app-desktop/gui/ConfigScreen/FontSearch.tsx

+
+	const onFontClick = useCallback((event: any) => {
+		const font = event.target.innerText;
+		(document.getElementById(key) as HTMLDivElement).innerText = font;


Don't use document.getElementById. Set the font as data-font on the element, then use event.currentTarget.getAttribute('data-font') to get it back

I was using getElementById to set the value in the input field when a font is clicked from the list, But I replaced it using the inputText state. But I am not sure if this is better or worse from the performance perspective. e86079f

packages/app-desktop/gui/ConfigScreen/FontSearch.tsx

packages/app-desktop/gui/ConfigScreen/ConfigScreen.tsx

laurent22 · 2024-04-05T09:45:37Z

packages/app-desktop/gui/ConfigScreen/ConfigScreen.tsx


 const settingKeyToControl: any = {
 	'plugins.states': control_PluginsStates,
 };

+interface FontData {


Why "FontData"? Everything is data. It's just a Font

I thought it would be better to define the type as it mentioned in the docs, but no problem I will change it

laurent22 · 2024-04-05T09:47:06Z

packages/app-desktop/gui/ConfigScreen/ConfigScreen.tsx

+	family: string;
+	fullName: string;
+	postscriptName: string;
+	style: string;
+	blob: ()=> Promise<Blob>;


In my comment I mentioned defining the properties that you need and you don't need most of these. I think you only use .family?

When a type is not built-in we just define what we need, otherwise it's too much noise especially for the DOM which can have hundreds of properties per object.

laurent22 · 2024-04-05T09:51:16Z

packages/app-desktop/gui/ConfigScreen/ConfigScreen.tsx

+						{
+							md.subType === SettingItemSubType.FontFamily ?
+								<FontSearch
+									_key={key}


The normal pattern for a React controlled component is to have a "value" and "onChange" (or similar) event. If you do that you won't have to pass around unnecessary properties and can create more reusable components.

So in this case, add an onChange event to your component which would pass to the parent the selected font. Then here just do onChange={fontFamily => updateSettingValue(key, fontFamily)}

I know there's a lot of code in this project and you can't know everything, but it often helps to see how things are done in the code near where you want to add something. For example you can see that all components here follow this value/onChange pattern. Maybe also read about controlled components and why they're generally preferred.

laurent22 · 2024-04-05T09:55:55Z

packages/app-desktop/gui/ConfigScreen/FontSearch.tsx

+			/>
+			<div className={'font-search-list'} style={{ display: showList ? 'block' : 'none' }}>
+				{
+					areFontsLoading ? <div>Loading...</div> :


_('Loading...') - please always make sure that user-facing text can be localised.

laurent22 · 2024-04-05T09:59:19Z

packages/app-desktop/gui/ConfigScreen/FontSearch.tsx

+		setTimeout(() => setShowList(false), 150) // Delay the hiding of the list to allow the click event to fire
+	, []);
+
+	const onFontClick = useCallback((event: any) => {


No "any", please always use the correct types. It should be "onFontClick: React.MouseEventHandler", then you TypeScript should infer the type for "event"

laurent22 · 2024-04-05T09:59:31Z

packages/app-desktop/gui/ConfigScreen/FontSearch.tsx

+		);
+	}, [fonts, inputText]);
+
+	const onTextChange = useCallback((event: any) => {


laurent22 · 2024-04-06T13:40:06Z

There are UX issues:

When moving the cursor to the font field, it freezes for a second or two (could be 5 or 10 on slower computers, or with more fonts). And it doesn't display "Loading...". I think you need to use requestAnimationFrame so that the UI has time to update before you load the fonts
The monospace font gives as option non-monospace fonts, so that should be filtered
If I choose a font, then click on the field again, the only proposed font is the currently chosen one. i.e. it looks like this:

I can see why it's doing this (the fonts are filtered by the currently selected one), but that's not terribly useful and probably unexpected for most users who would instead expect to be able to see the fonts and scroll through them

ab-elhaddad · 2024-04-08T07:54:23Z

About filtering the monospaced fonts, there is no straightforward way to do this because the querLocalFonts() doesn't expose whether the font is monospaced or not, like, for example, the font-manager package, which does provide an attribute declaring whether the font is monospaced or not.

To solve this issue, we have two options:

Filtering OS fonts using static array containing the most popular monospaced fonts. which is a light solution and won't exhaust the renderer process but may introduce some bugs.

const monospacedFonts = [
    'Courier',
    'Courier New',
    'Lucida Console',
    'Monaco',
    'Consolas',
    'Andale Mono',
    'Monospace',
    'Roboto Mono',
    'Source Code Pro',
    // More monospaced fonts
];

Implementing a function to calculate the width of two characters using the same font and see if they have the same width. This solution is very resource-exhausting, and it causes a freeze for a second or two, but it is guaranteed that it won't introduce any bugs.

const isMonospacedFont = (fontName: string) => {
  const testElement = document.createElement('span');
  testElement.style.fontFamily = fontName;
  testElement.style.fontSize = '16px';
  testElement.innerText = 'i';

  document.body.appendChild(testElement);
  const iWidth = testElement.getBoundingClientRect().width;

  testElement.innerText = 'm';
  const mWidth = testElement.getBoundingClientRect().width;

  return Math.abs(iWidth - mWidth) < 0.01; // Tolerance for minor variations
}

laurent22 · 2024-04-08T11:41:55Z

Filtering OS fonts using static array containing the most popular monospaced fonts. which is a light solution and won't exhaust the renderer process but may introduce some bugs.

Let's do this but please search for an existing list of monospaced fonts, as exhaustive as possible. Ideally something that's regularly updated and add the link as a comment on top of your list (so that we can update it later on).

You may also want to add some heuristics such as any font that certain keywords such as "mono", "courier", "source code", "console", etc. should be considered monospace.

tomasz1986 · 2024-04-08T13:03:06Z

Let's do this but please search for an existing list of monospaced fonts, as exhaustive as possible. Ideally something that's regularly updated and add the link as a comment on top of your list (so that we can update it later on).

I don't think it will be possible to find one really complete list. The reason is that if you search for "monospace font list" in the Web, you will find mainly fonts that are for English (and possibly other Western languages). However, those lists completely miss all monospace fonts that exist e.g. for East Asian languages. Also, the fonts may even be named in non-Latin scripts, so the generic keywords will likely not catch them.

In other words, what I want to say is that it would be so much more universal if the script could just list all installed monospace fonts as they are reported by the OS without looking for specific names 🙁.

laurent22 · 2024-04-08T13:59:23Z

In other words, what I want to say is that it would be so much more universal if the script could just list all installed monospace fonts as they are reported by the OS

That's true, but unfortunately we can't do that. But I'm thinking there should be an option to show either only the monospace ones or all fonts as a fallback.

So @ab-elhaddad, would you mind adding a "Show monospace fonts only" checkbox to your list? It would be checked by default when picking the monospace font, and unchecked when picking the editor font.

tomasz1986 · 2024-04-08T18:55:08Z

That's true, but unfortunately we can't do that. But I'm thinking there should be an option to show either only the monospace ones or all fonts as a fallback.

Maybe it would still be better to use one of those 3rd party packages that can filter monospace fonts with no guesswork involved? It seems that querLocalFonts() is very limited in terms of the actual functionality…

roman-r-m · 2024-04-08T19:17:59Z

And if the list of all fonts is slow to load, can it be loaded once on startup and kept in memory? I don't suppose the memory overhead will be huge.

ab-elhaddad · 2024-04-08T20:31:48Z

I think the main problem with using a third-party package is its maintenance. The only package I have found (until now) exposing whether the fonts are monospaced or not is the font-manager, which was last updated 4 years ago. So, I am not sure if using such a package is a good decision.

laurent22 · 2024-04-09T10:39:57Z

I think the main problem with using a third-party package is its maintenance. The only package I have found (until now) exposing whether the fonts are monospaced or not is the font-manager, which was last updated 4 years ago. So, I am not sure if using such a package is a good decision.

We can't use this unfortunately. It's a binary package so will cause too many problems for little benefits.

…fonts

laurent22 · 2024-04-15T17:31:57Z

Thanks for the update @ab-elhaddad but the "Show monospace fonts only" checkbox needs to be within the font list component and only shows up when the list is open, like this:

Otherwise it's not clear at all what this "monospace" setting is for.

ab-elhaddad · 2024-04-16T03:13:57Z

No problem, it's done

laurent22 · 2024-04-20T13:22:45Z

Looks good now, thanks @ab-elhaddad!

ab-elhaddad · 2024-04-21T02:31:10Z

@laurent22
Thank you so much for your guidance and patience, I can't deny I have learned a lot through this PR.

ab-elhaddad added 2 commits April 2, 2024 11:37

Desktop: feat: Added search list to font input fields

56520c9

Desktop: fix: Made the font-list verion absolute

6618102

laurent22 reviewed Apr 2, 2024

View reviewed changes

ab-elhaddad added 2 commits April 2, 2024 13:10

Desktop: fix: Specify font input fields

bef11bb

Desktop: refactor: Refactored props and added types to it

9832a9e

personalizedrefrigerator reviewed Apr 2, 2024

View reviewed changes

ab-elhaddad added 4 commits April 3, 2024 21:41

Desktop: refactor: Replaced font-list with window.queryLocalFonts

1362f78

Desktop: refactor: Refactored component functions and used useCallback

2baed73

Desktop: refactor: Switched dynamic styles to rscss

f791467

Desktop: refactor: Deleted memo

95e96e0

personalizedrefrigerator reviewed Apr 3, 2024

View reviewed changes

Desktop: fix: used setting subType to determine font input fields

67800cd

laurent22 reviewed Apr 4, 2024

View reviewed changes

packages/app-desktop/gui/ConfigScreen/ConfigScreen.tsx Outdated Show resolved Hide resolved

ab-elhaddad added 3 commits April 4, 2024 21:30

Desktop: refactor: updated vars names and types

5113b57

Desktop: refactor: removed DOM with its dependencies

e86079f

Desktop: refactor: removed useless code

d9cbbe4

laurent22 reviewed Apr 5, 2024

View reviewed changes

ab-elhaddad and others added 8 commits April 6, 2024 03:12

Desktop: refactor: redefined Font type

32fee41

Merge branch 'dev' into font-drop-list

72ac22b

Desktop: refactor: removed all any

da9047a

Desktop: refactor: switched to value/onChange pattern

d18ec92

Desktop: refactor: removed useless code

e719c97

Desktop: fix: make the user-facing text localisable

fde9798

Desktop: fix: blur and click event conflict

c75cf70

Desktop: refactor: added some types

81ba47f

PackElend added the gsoc-2024 label Apr 6, 2024

ab-elhaddad added 2 commits April 7, 2024 21:18

Desktop: feat: Shows the full list even if a font is selected

b38cd5d

Desktop: fix: added Lazy Loading to prevent the onClick freeze

d345bf2

Desktop: fix: fonts show loading when no search results

81479d1

ab-elhaddad added 3 commits April 11, 2024 14:57

Desktop: feat: Add the option (checkbox) to show only the monospaced …

c5bc4d7

…fonts

Merge branch 'font-drop-list' of remote/font-drop-list

2eb04e4

Desktop: refactor: renamed vars

0c8304a

Desktop: fix: moved checkbox to be within the font list

7e82b61

laurent22 merged commit 7ec02fc into laurent22:dev Apr 20, 2024
10 checks passed

Desktop: feat: Added search list for font input fields #10248

Desktop: feat: Added search list for font input fields #10248

Conversation

ab-elhaddad commented Apr 2, 2024 • edited Loading

Choose a reason for hiding this comment

ab-elhaddad Apr 2, 2024 • edited Loading

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

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

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

personalizedrefrigerator Apr 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ab-elhaddad Apr 3, 2024 • edited Loading

Choose a reason for hiding this comment

personalizedrefrigerator Apr 3, 2024 • edited Loading

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

laurent22 Apr 5, 2024 • edited Loading

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

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

Choose a reason for hiding this comment

laurent22 Apr 5, 2024 • edited Loading

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

laurent22 commented Apr 6, 2024

ab-elhaddad commented Apr 8, 2024

laurent22 commented Apr 8, 2024 • edited Loading

tomasz1986 commented Apr 8, 2024 • edited Loading

laurent22 commented Apr 8, 2024

tomasz1986 commented Apr 8, 2024 • edited Loading

roman-r-m commented Apr 8, 2024

ab-elhaddad commented Apr 8, 2024

laurent22 commented Apr 9, 2024

laurent22 commented Apr 15, 2024

ab-elhaddad commented Apr 16, 2024

laurent22 commented Apr 20, 2024

ab-elhaddad commented Apr 21, 2024

ab-elhaddad commented Apr 2, 2024 •

edited

Loading

ab-elhaddad Apr 2, 2024 •

edited

Loading

personalizedrefrigerator Apr 2, 2024 •

edited

Loading

ab-elhaddad Apr 3, 2024 •

edited

Loading

personalizedrefrigerator Apr 3, 2024 •

edited

Loading

laurent22 Apr 5, 2024 •

edited

Loading

laurent22 Apr 5, 2024 •

edited

Loading

laurent22 commented Apr 8, 2024 •

edited

Loading

tomasz1986 commented Apr 8, 2024 •

edited

Loading

tomasz1986 commented Apr 8, 2024 •

edited

Loading