Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve text editor status bar and zooming UX #88474

Merged
merged 1 commit into from
Feb 22, 2024

Conversation

MajorMcDoom
Copy link
Contributor

@MajorMcDoom MajorMcDoom commented Feb 18, 2024

ZoomLagFix.mp4

Old:
Screenshot 2024-02-17 214007
New:
Screenshot 2024-02-17 220131

Changes

  • Text editor zooming no longer triggers a big editor freeze after completion. Fixes lagging when zooming a script using Distraction Free Mode #82144.
  • Text editor zooming feature now works by adding a multiplier on top of the editor code font size setting, instead of directly modifying the editor setting itself. This is consistent with most modern IDEs.
  • The zoom level is now a transient state of each text editor instance. This means for example that it can be different between the script editor and the shader editor, and even between each script tab.
  • The zoom level is now displayed in the status bar alongside the line-and-column stats. The zoom level is a clickable MenuButton that allows the user to quickly select between a few common zoom levels.
  • The status bar now uses of a few VSeparators to separate the various stats, instead of relying on vague empty space and a printed | character like before.
  • The indentation indicator (Tabs / Spaces) has been separated from the line-and-column indicators and responds to the appropriate changes (e.g. indentation conversions and editor indentation setting changes), instead of responding to caret changes.
  • Some cleanup of code to remove redundancies.

Rationale

This represents a pretty big change to the role of the zoom feature in the user's workflow. Previously, the zoom feature was a shortcut for making a persistent change to the editor itself, even though it was not obvious to the user that this was happening. It was also easy to accidentally trigger.

In the new system, the zoom feature behaves as a quick, transient operation - similar to a scroll or a drag, while the editor code font size setting serves as the preferred neutral size (at 100% zoom). In this way, the purpose of the zoom feature is to give the user a quick way to navigate their view of a document. They are free to leave some documents zoomed out for at-a-glance interaction, while zooming in on others where they are focused on specific areas.

Notes

  • This is my first time contributing changes that involve the editor interface. Please excuse me if I did some weird things out of ignorance.
  • The zoom levels in the popup are chosen arbitrarily based on my own intuition, and are free to change if there is a good reason to.
  • The zoom levels of each open document are currently not persistent, and will be reset between each session. However, they can totally be saved to the editor cache if we decide that is worthwhile (perhaps in another PR).

@MewPurPur
Copy link
Contributor

I prefer if all my scripts are at a persistent zoom level and it seems like this will make it hard to do so. Beside this, I fully agree with the approach of not changing a whole editor setting to achieve zooming.

@MajorMcDoom
Copy link
Contributor Author

MajorMcDoom commented Feb 18, 2024

I prefer if all my scripts are at a persistent zoom level and it seems like this will make it hard to do so. Beside this, I fully agree with the approach of not changing a whole editor setting to achieve zooming.

@MewPurPur Just to clarify, do you mean persistent (saves between sessions), or consistent (synced between all tabs)? Though in truth, making it consistent would actually make it much easier to make it persistent as well, since there is only one value to save to cache. 🤔

@Mickeon
Copy link
Contributor

Mickeon commented Feb 18, 2024

I myself am concerned about exposing this indent_type_changed signal. It's way too niche, I feel. Consider another way of accomplishing this without requiring the signal. All of these great changes don't look like they should affect the CodeEdit node, but if they have to, they should probably be way more noteworthy than this signal.

@MajorMcDoom
Copy link
Contributor Author

MajorMcDoom commented Feb 18, 2024

I myself am concerned about exposing this indent_type_changed signal. It's way too niche, I feel. Consider another way of accomplishing this without requiring the signal.

@Mickeon I was wondering about that actually. How does one accomplish a subscription-like behaviour in the Godot codebase without the use of signals? Unfortunately there are many places from which the indent type of CodeEdit is changed, not all of which are directly within the purview of the script editor.

@Mickeon
Copy link
Contributor

Mickeon commented Feb 18, 2024

There are some things in the API that are "exposed" but not really "exposed". These are prefixed with an underscore and do not show up on autocompletion or documentation, and by all means are discouraged to be used. I believe there's never been a case of this for signals, however, so it's probably not supported at all.

So right now, I am not sure how. Again, I'm not against adding more things, but that would warrant people will actually need and use it.

If you want to ask for some help and feedback remember that Godot also has a public chat specifically tailored for development of the engine: https://chat.godotengine.org

@Mickeon
Copy link
Contributor

Mickeon commented Feb 18, 2024

Right off the bat, I do have a suggestion. Thinking outside of the box.

A new signal would be much more beneficial if it was emitted on more than one situation, not just when specifically the tab type changes. Resources have a changed signal. TileMap also has the changed signal as well (the doc's description is lying to you, what the hell, it is actually emitted in more situations).
There's many settings that influence TextEdit/CodeEdit's behaviour. If they all emitted the same signal on change it would probably be a whole lot more beneficial.
Performance is not a concern, either, since these settings are unlikely to change very frequently, and the settings' previous values can be cached and checked before acting, anyhow.

However, this suggestion is only somewhat related to this PR, and may require another PR and proposal to be potentially accepted.

@MewPurPur
Copy link
Contributor

It'd be better to first do the less controversial changes in this PR so it's approved more easily.

@Mickeon Mickeon removed the request for review from a team February 18, 2024 14:10
@MajorMcDoom
Copy link
Contributor Author

It'd be better to first do the less controversial changes in this PR so it's approved more easily.

@MewPurPur It's not quite so simple, since the "less controversial" fixes were only possible by decoupling from editor settings. And by doing so, all the script tabs now have to do additional work to stay in sync with each other or save their zoom. I thought it would be one interface, but no, every script in the tab actually holds onto its own text editor in memory.

That said, I am working on some changes.

@MajorMcDoom
Copy link
Contributor Author

@Mickeon After doing some more deep studying of the class hierarchy and their interplay, I implemented and pushed a solution which doesn't involve any changes to CodeEdit. Would this alleviate your concerns?

@Mickeon
Copy link
Contributor

Mickeon commented Feb 18, 2024

It does, marginally. Now this feature can be taken a look at without having any interference with the public API, and it can be changed later if necessary.

I for one approve a lot but @MewPurPur made a good point before and it would be nice to see addressed. Never seen a "Per document" zooming level in an IDE (even if it does sound interesting).

@MewPurPur
Copy link
Contributor

My approach would've been to save one value in the editor state, just a script zoom level that'd hold across all scripts. This would be consistent with the current behavior, just no lag spikes from changed editor settings.

@Calinou
Copy link
Member

Calinou commented Feb 18, 2024

Script zoom should definitely be consistent across saves. Persistence is a bonus but not strictly required.

Web browsers store zoom levels on a per-domain basis, but this behavior isn't really suited to code editors.

@AThousandShips AThousandShips changed the title Improved text editor status bar and zooming UX Improve text editor status bar and zooming UX Feb 18, 2024
@MajorMcDoom MajorMcDoom requested a review from a team as a code owner February 18, 2024 20:58
@MajorMcDoom
Copy link
Contributor Author

MajorMcDoom commented Feb 18, 2024

@Calinou @MewPurPur @Mickeon

I've added zoom consistency between different tabs of each editor. The Script Editor and the Shader Editor each have their own zoom levels, and I think that's a good thing.
consistent_zoom

I've also added persistence, implemented through the script and shader editor plugins themselves.
editor_layout cfg

It was a huge pain to get access to the CodeTextEditor (where the zoom functionality is implemented) from the higher-level classes, especially across the two very differently-implemented script and shader systems. But I went for shortest path and added a couple of methods, and did some renamings to avoid ending up with confusing things like
shader_editor->get_shader_editor().

@Mickeon
Copy link
Contributor

Mickeon commented Feb 18, 2024

Outstanding job. I hope this gets evaluated soon. The zooming was always a massive pain of the current text editor. It got to the point where @KoBeWi wanted to disable the shortcut outright for accidentally triggering it.

@Mickeon Mickeon requested a review from KoBeWi February 18, 2024 22:01
@KoBeWi
Copy link
Member

KoBeWi commented Feb 19, 2024

It's not as smooth for me:

godot.windows.editor.dev.x86_64_LDwOQWlhPu.mp4

Though it's maybe because of dev build. It's still better than before of course and accidental zoom happen in small increments. But I'm still in favor of removing the shortcut >_>

Overall works great, I just found a small bug. When you set 75% zoom and restart, it will be restored as 79%.

@MajorMcDoom MajorMcDoom force-pushed the text-editor-zoom branch 2 times, most recently from e7e2070 to d9b5b8e Compare February 19, 2024 18:39
@MajorMcDoom
Copy link
Contributor Author

@Mickeon @KoBeWi
I've changed CodeTextEditor::set_zoom_factor, and now the presets will now display properly when closing and restarting the editor. Thanks for catching that.

editor/code_editor.cpp Outdated Show resolved Hide resolved
editor/code_editor.cpp Outdated Show resolved Hide resolved
editor/code_editor.cpp Outdated Show resolved Hide resolved
Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested locally (at 100% and 175% editor scales), it works as expected.

Note that the first time you zoom in an editor session (causing a new font size to be displayed), there will be some slowdown due to the font cache having to be regenerated. This doesn't occur afterwards. Using MSDF rendering for the code font would alleviate this, at the cost of making it less crisp (so I don't recommend this approach, at least for now).

Copy link
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine now.

Needs rebase after #69032
Note that every function that updates editor settings (either from NOTIFICATION_EDITOR_SETTINGS_CHANGED or from settings_changed signal) should use check_changed_settings_in_group() now.

@akien-mga akien-mga modified the milestones: 4.x, 4.3 Feb 21, 2024
@MajorMcDoom MajorMcDoom force-pushed the text-editor-zoom branch 3 times, most recently from 2116ac9 to 6f6de77 Compare February 21, 2024 23:24
@MajorMcDoom
Copy link
Contributor Author

@KoBeWi Did the rebase! Should be ready to go. 🤞

@akien-mga akien-mga merged commit 0550abf into godotengine:master Feb 22, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

lagging when zooming a script using Distraction Free Mode
7 participants