space-indicator first draft

[lpjjj1222]

Working Process:
To design a SpaceSetting similar to DarkSetting:

• Create a SpaceSetting.ts (imitating DarkSetting.ts).
• Since SettingDatabase.ts imports DarkSetting.ts, import SpaceSetting.ts as well and handle related logic similarly.
• Go to Database.ts, imitate the approach used for darksetting, and add: export const spaceIndicator = Settings.settings.space.value;
• Since Settings.svelte is the frontend code for settings, imitate the approach used for dark/theme and add space indicator options.
• Since Space.svelte is used for rendering spaces in the editor, we can pass a spaceIndicator (boolean) parameter to decide how to render spaces. When spaceIndicator is false, render whitespace (like other computer editors), and when true, render a dot like the original Wordplay editor. Hence, we can imitate the +layout.svelte file which retrieves the dark setting from the database and then decides whether to render the page in dark or light color.

Already Implemented:

• The frontend is already set up, allowing users to choose space indicator on/off.
• When users select on/off, spaceSetting can already detect the value of the space indicator.
• It is now implemented so that when the space indicator is true, dots are rendered; when false, whitespace is rendered.

bug:
There seems to be a bug when typing spaces... I suspect it may be related to insertionIndex, beforeSpaces, afterSpaces, additionalSpaces, or related issues in space.svelte

[amyjko]

Thanks @lpjjj1222! Is this a draft PR, or a final proposal? Given that you're aware of least one bug, it should be marked a draft. For drafts, I want to know what feedback you're looking for. Are there questions you have, things you want me to review, things I should ignore?

[amyjko]

No reply, converting to draft. I'll review it as a draft.

[amyjko]

Thanks for getting started on this enhancement! It's a good start. Here are things to resolve before its ready for release:

• There shouldn't be any changes to package-lock.json; use npm ci to sync dependencies, not npm update. Revert those changes in the PR.
• Remove the console.logs in src/routes/layout.svelte, Setting.ts,Mode.svelte, and Space.svelte
• Add placeholders for the new string in the other locales; see npm run locales to verify you have resolved all of the missing string errors.
• Yes, there is a defect in Space.svelte. In the else case of the spaceIndicator check, there's no non-breaking space being inserted to represent the space, only a tab. The return statement should be refactored to reuse the else case of the explicit check, so that it only renders the explicit space if explicit and spaceIndicator is true, and render a non-breaking space if not.
• Use the locale's labels for the mode descriptions in Settings.svelte, not hard coded-English labels.
• The name of spaceIndicator in Space.svelte:render shadows the imported spaceIndicator. Rename to avoid confusion.
• Remove the unused imports in Space.svelte

[lpjjj1222]

Apologies for the delayed response, I've just seen your comment:(
Thank you so much for your guidance Professor Amy!
I will make the necessary improvements to my code based on your suggestions.

[lpjjj1222]

I followed your advice and ran npm run locales, and there was an error in en-US: /ui/dialog/settings/mode: must NOT have additional properties. When I opened the file, I found that the error was caused by a new 'space' property that I had added, following the structure of the 'dark' property. However, there was a warning on 'space' indicating that this property is not allowed. I asked ChatGPT4 for help, and it suggested that the error might be due to other files that use en-US.json having certain expectations about its structure.

In the 'Settings.svelte' file, I used <Mode descriptions={$locales.get( (l) => l.ui.dialog.settings.mode.space, )} choice={$spaceIndicator === false ? 1 : 0} select={(choice) => Settings.setSpace( choice === 0 ? true : false, )} modes={['On', 'Off']} />, I guess this has already used en-US.json indirectly. So I am more confused about why it would say 'this property is not allowed'...

BTW, I tried to find all the files related to en-US.json using a global search, but it was challenging because the files are nested...

Could you offer any assistance?

[amyjko]

Hi @lpjjj1222. ChatGPT doesn't know anything about this code base, it's not a good resource.

The definition of what properties are allowed are defined in UITexts.ts. Any properties defined in locale files need to be declared there. See the settings field there for where other localization strings for settings are declared. For example, see mode > dark for a good example.

I can elaborate further if you have more questions!

[lpjjj1222]

Thank you Professor Amy.

However, I discovered that even after declaring 'space' in the UITexts.ts, the 'this property is not allowed' is still there :(
mode: { /** The project tile layout mode */ layout: ModeText<[string, string, string, string]>; /** The animation on/off/slowdown mode */ animate: ModeText<[string, string, string, string, string]>; /** The dark on/off/automatic mode */ dark: ModeText<[string, string, string]>; /** The writing layout direction */ writing: ModeText<[string, string, string]>; /** The space_indicator on/off mode */ space: ModeText<[string, string]>; };

[amyjko]

@lpjjj1222 Ah, when you modify the locale definitions, you also need to run npm run schemas to update the schemas that check for violations. I don't think that's documented very well anywhere; I added this to the develop wiki best practices.

[lpjjj1222]

@amyjko Hi Professor Amy, I have follow your suggestions and modify my code but I still have some problems:

• I added translation of ‘space indicator’, ‘open’, ‘closed’ for the new string in en-US to en-MX, en-CN, example locales. And then I ran npm run locales, it said that

Locale has XX unwritten strings ("$?"). Keep writing!

So I opened the locale file and discovered that there indeed are some ‘&?’ to be replaced by translation. I am not sure is it related to the issue I am working on. If not, should I just ignore them?

• I've made changes to the Space.svelte file as you suggested, and the strange bug I was encountering seems to have been resolved. However, I've come across a new issue where the existing spaces in the user's coding input are not being correctly re-rendered when the space indicator is toggled. I'm wondering if this might be related to the logic within the Space.svelte file. I don't quite understand what the beforeSpace, afterSpace, additionalSpace, and insertion variables represent, as well as what the 'text' parameter in the render function specifically refers to — whether it's all existing strings, a single line of string, or just the existing spaces. I tried to figure it out using console.log and by entering some code myself, but I'm still not clear on what these variables actually mean. Could you please explain them if it's convenient for you?

Thank you so much Professor Amy😭

[amyjko]

Thanks for the update @lpjjj1222!

• The unwritten string warnings are for people doing localization. They're not related to the new strings you added, aside from them not yet being written. We delegate that work to the localization team.

• Space.svelte works as follows. It takes a token whose space is being rendered, the space to render for it, any additional space that is rendered to pretty print code, and an insertion point for drag and drop, in case someone is dropping something in the middle of space. It takes this information and computes where the insertion is happening, if there is one; the space before it, and the space after it, and any additional space being added for pretty printing, and then renders some span tags to reflect the spaces to be rendered. It also renders an InsertionPointView if there's drag and drop on the space. The statements that compute the space to be rendered are reactive (using Svelte's reactivity feature), so they should reevaluate when the $spaceIndicator store changes. You should verify that they actually are and that the logic you're using to render spaces is actually using the state you want it to.

• You need to remove the package-lock.json and package.json updates from your PR; we manage dependency updates centrally, so they should not be included in PRs. Make sure you're running npm ci, not npm install.

[lpjjj1222]

Hi Professor Amy @amyjko , thank you for telling me how does Space.svelte works. I 've been solving the problem that the user's coding input that has already existed is not being correctly re-rendered when the space indicator is toggled.
To be more specific, I open the space indicator and type following codes:

And then, I toggle the space indicator, make it closed. It shows like:

Then, if I type a space or a random letter in the bracket, '·' that are outside the bracket still exists, however, if I type a random letter or space outside the bracket, everything behaves well, all of '·' disappear.

I tried console.log to see eg. in the case of screenshot, what would beforeSpaces and afterSpaces stand for. It turns out that beforeSpaces is undefined (it is because there is no insertion), and afterSpaces is ['···'] instead of ['··','···'], which makes me confused.

Could you give me more help to proceed with this problem?

[amyjko]

Hi @lpjjj1222. I think I know what's happening. There's actually quite a bit of undocumented behavior around reactivity in Svelte with whitespace changes in spans. For example, see this issue:

sveltejs/svelte#189

I worked around that behavior by wrapping the rendering block in a {#key space} so that when the space being rendered changes, it re-renders the space component, so whitespace is correct. But you've introduced a new dependency -- the $spaceIndicator store -- but when it changes, Svelte doesn't know to re-render the space component. You can wrap the HTML in another {#key}, but refer to $spaceIndicator, like this {$key $spaceIndicator} so that when the store changes, the space is re-rendendered.

All of this is a bit of a hack, but necessary because of this gap in reactivity in Svelte.

A notes reminders about other things to fix:

• Variable names should only be upper case when they represent a type declaration or a constant. Move them all to lower case.
• You need to remove the changes to package.json and package-lock.json
• The icons you've chosen for the setting are vague; what do a circle and x have to do with showing and not showing space indicators? Is the idea that they represent on and off? Which one means on, which one means off?

[lpjjj1222]

Hi @amyjko. Thank you for your guidance! It seems that I've already solve the re-render issue.
For the package.json and package-lock.json, would it be appropriate to withdraw the modification by:
git checkout 256-space-indicator
git checkout origin/main -- package.json package-lock.json
git add package.json package-lock.json git commit -m "Revert changes to package.json and package-lock.json"
git push origin 256-space-indicator
I have already done as above, if there is a better way to withdraw the modification, please let me know :)

[amyjko]

Yes, that's adequate for removing them. There are still a few differences visible, but they are minor.

In general, undoing a commit is complicated:

https://stackoverflow.com/questions/2938301/remove-specific-commit

Let me know when you're ready for another review.

[lpjjj1222]

Hi@amyjko
I've addressed the re-render issue and believe the implementation of the new feature is now ready for your review, since it's adequate for removing the changes to package.json file as you said.

If there are any additional adjustments needed, I'm on standby to make further updates. Looking forward to your feedback!

[amyjko]

Thanks! When you think a PR is ready for review, convert it from a draft PR to ready for review. That way I know you want me to review it, and it makes it eligible to merge.

[lpjjj1222]

@amyjko I have converted it to be ready for review. Looking forward to your feedback~

[amyjko]

This is great progress! It's getting very close. A few things that I think still need improvement:

• There are unused imports that need to be removed in Space.svelte
• I think the check mark icon in the settings shouldn't be one with the black background, since the x does not have a black background, for consistency.
• Precede the Spanish and Chinese space indicator label with a $! to flag them for review by the locale teams, just to make sure the translations make sense
• The English tooltip labels for the settings are "open" and "closed" which are not descriptive of what the setting actually does. They should be something like "show space and tab indicators explicitly" and "do not show space and tab indicators". These need to teach creators what the effect of the setting change will be, especially since the change is not visible when the dialog is covering the editor.

I think that's it, then it will be ready for deployment! Congrats on getting this far.

[lpjjj1222]

@amyjko
Thank you so much for your guidance!
I have finished the last modification =)

[amyjko]

I think this is ready for production. Congratulations on a successful PR!