Tab identation for multiline text edit #246

DrOptix · 2021-03-22T16:32:33Z

With this PR we resolve #73, allowing opt-in tab indentation in TextEdit

When we load a string in TextEdit we keep the tabs as they are and by default tab_as_space is initialized to false and tab_moves_focus is initialized to true.

To config tab_as_spaces and tab_moves_focus to the values we want we can use the following helper functions:

TextEdit::tab_as_spaces()
TextEdit::tab_moves_focus()

Example:

ui.add(egui::TextEdit::multiline(&mut multiline_text_input)
    .tab_as_spaces(self.tab_as_spaces)
    .tab_moves_focus(self.tab_size));

Now depending on tab_as_spaces and tab_moves_focus properties of the TextEdit we have the following situations:

When tab_as_spaces is false, then when we press TAB the result is a TAB char is added to the string with the width of 4 spaces.
When tab_as_spaces is true, then when we press TAB the result is a chunk of 4 spaces are added to the string.

To fully understand how to use TextEdit with those two opt-in behaviors check the "Misc Demos" demo.

Other two helper functions were added to Ui and TextEdit to simplify the creation of a TextEdit focused on code editing:

code_editor
code_editor_with_config

The following key combinations are supported. Depending on the "tab-as-spaces" config the indentation block will be firstly converted to the appropriate type and the increased or decreased.

tab
Insert \t or needed number of spaces to align to the tabulation size. \t char will be rendered so that it will align to the tabulation size
shift+tab
Will remove indentation at the end of the indentation block to align the text to the tabulation size. This works on the current line or with multiple lines selected.
shift+[ and shift+]

Remove and add indentation at the end of the indentation block to align the text to the tabulation size.

Another improvement was done to build_demo_web.sh script to adapt the "open" command to the platform we work on. This is not covering all possible platforms, but it's a good template to have for when we need to adapt to a new platform:

xdg-open for Linux, Fedora for example
start for Windows, MSYS
open for anything else

When we load a string in `TextEdit` we keep the tabs as they are. Now depending on `tab_as_spaces` and `tab_size` properties of the `TextEdit` we have the following situations: - When `tab_as_spaces` is false, then when we press `TAB` a `TAB` char in added to the string. - When `tab_as_spaces` is true, then when we press `TAB` a chunck of `tab_size` spaces is added to the string. To config `tab_as_spaces` and `tab_size` to the values we want we can use the folliwing helper functions: - `TextEdit::tab_as_spaces()` - `TextEdit::tab_size()` Example usage: ```rust ui.add(egui::TextEdit::multiline(&mut self.multiline_text_input) .tab_as_spaces(self.tab_as_spaces) .tab_size(self.tab_size)); ``` Demo: A better demo of this new functionality was implemented in the misc widgets demo What's left to do: - Render actual `TAB` as a rect the size of `tab_size` spaces. - Without default parameters values it seems a pain to implement. I need to think if it is possible to avoid API breaking changes for the font system. From my checks so far I need to pass at least `tab_size` to the fonts system to know how big the glyph for `TAB` will be.

Added `tab_moves_focus()` to `TextEdit` so we can opt-in to the "tab-as-character" behaviour. Also because we decided to lock the tab size to 4 spaces, we gave up on the `tab_size` property, but we kept the opt-in for the "tab-as-spaces" behaviour. An updated example of how to use `TextEdit` with those two opt-in behaviours can be found in the "Misc Demos" demo.

With this new version we use $OSTYPE bash env var to query what OS are we running on. - On Linux, ex: Fedora, we use `xdg-open` - On Windows, ex: msys, we use `start` - For other other variants we try to use `open` We should update this script when we notice that `open` is not available on a particular platform.

For both `Ui` and `TextEdit` add two helper functions to simplify creation on `TextEdit` focused on code editing. - `code_editor` - `code_editor_with_config`

emilk

Nice work!

build_demo_web.sh

egui/src/memory.rs

emilk · 2021-03-24T20:44:56Z

egui/src/ui.rs

+        text: &mut String,
+        tab_as_spaces: bool,
+        tab_moves_focus: bool,
+    ) -> Response {


It is bad form to have multiple boolean arguments, as it leads to code that is very hard to decipher:

TextEdit::code_editor_with_config(code, true, false) // what does it mean!?

I think it is better in this case to just have the opinionated code_editor

I crated a CodingConfig struct for this purpose.
https://github.com/DrOptix/egui/blob/e14c5491b61574647ed837130e0a61f4c1ce98b4/egui/src/widgets/text_edit.rs#L107-L112

egui/src/ui.rs

emilk · 2021-03-24T20:50:05Z

egui/src/widgets/text_edit.rs

+    ///     .tab_as_spaces(tab_as_spaces)
+    ///     .tab_moves_focus(tab_moves_focus);
+    /// ```
+    pub fn code_editor_with_config(self, tab_as_spaces: bool, tab_moves_focus: bool) -> Self {


Same comment here about multiple boolean arguments. If you want to set multiple values with a single call, it is better to make a config struct and pass that in (struct CodeConfig { tabs_as_spaces: bool, tab_width: usize, … } ). This has the benefit of being more explicit and more extensible.

I crated a CodingConfig struct for this purpose.
https://github.com/DrOptix/egui/blob/e14c5491b61574647ed837130e0a61f4c1ce98b4/egui/src/widgets/text_edit.rs#L107-L112

egui/src/widgets/text_edit.rs

egui_demo_lib/src/apps/demo/widgets.rs

@emilk

Thank you @emilk for the suggestions Co-authored-by: Emil Ernerfeldt <[email protected]>

Depending on the type of identation we first convert the current identation in to spaces or tabs. For converting to spaces we have two cases: 1. The identation converts to a number of spaces, multiple of the tab size. In this case we remove a number of spaces equal to the tab size. 2. The identation converts to a number of space, not a multiple of tab size. In this case we remove spaces until we have a number of spaces multiple of tab size. For converting to tabs we have two cases: 1. The identation converts only to tabs. In this case we remove a tab char. 2. The identation converts to tabs, and 1, 2 or 3 spaces. In this case we remove the spaces

When inserting the identation first we check on which virtual column we are and then add enough spaces to accound for an identation size of `text::MAX_TAB_SIZE`. Above we say virtual column because a `tab` can occupy 1, 2, 3 or 4 columns and we need to keep in mind this.

If we have a single cursor on a paragraph then we will decrease identation only on that paragraph. If we have a selection we will decrease identation on all selected paragraphs.

- `Tab`: When multiple lines are selected add identation - `Ctrl`+`[`: Remove identation on single or multiple lines - `Ctrl`+`]`: Add identation on single or multiple lines

…eature/73_identation_text_edit_widget With this merge the identation management was reworked. Before adding or removing identation we convert the identation of the targeted lines to tabs or spaces depending on how we treat the tabs. The selection will be maintained. Now we can do this: - When we are in single cursor mode and we press `tab` and we will insert a `\t` or the needed number of spaces to complete a block according or `text::MAX_TAB_SIZE`. - Add identation to multiple selected lines using `tab`. - Use `shit`+`tab` to remove identation from the current line or the selected lines. - Use `ctrl`+`[` and `ctrl`+`]` to remove or add identation to the current line or a group of selected lines.

emilk · 2021-04-03T21:22:14Z

Oh wow, this kind of got out of hand didn't it :) It's amazing with a code editor in egui, but I'm a worried about adding 1550 lines of code. The whole egui crate is only 14kloc!

I won't have time to review this for another few days, but anything you can think of the reduce the complexity would be appreciated. In particular: do we really need to supports spaces for indentation? Making an editor that only supports tabs would make everything A LOT simpler (just add or remove the first tab on a line). If a user wants spaces in their code when they save to disk, then can just convert to/from spaces on load/save instead. The code to convert to/from tabs/spaces would also be a lot simpler as we wouldn't need to track the cursor position.

Or, going the other direction, if we want a full-fledged code editor for egui, maybe that should be a separate crate (egui-code or something) that builds on egui rather than being a part of egui.

DrOptix · 2021-04-03T21:32:51Z

Well yes, I went from ecstasy to frustration and back to ecstasy. I will try to think of a way to reduce the complexity. For me at least the biggest complexity was represented by keeping track of remaining tabulation size. I used VS Code as a blueprint for how to add, remove or convert indentation and also how to keep the selection while adding or removing indentation.
What I can tell you is that I added quite a few tests. A birds eye view gives me around 500 lines of tests in text_edit.rs.

@DrOptix

Co-authored with @DrOptix with code from #246

emilk · 2021-04-15T19:22:35Z

To explain my last post a bit more: I consider all code to be a liability and a burden. I always strive to express as much as possible with as little as possible. Each line of code is something that needs to be maintained in perpetuity, therefor each line of code must pull its weight.

In this particular case I know that the cursor handling in egui will need to be rewritten in the future, probably several times, to support things like right-to-left languages and moving over graphemes rather than characters. This PR will make such work a lot harder.

If an advanced code editor was a separate crate, then egui can be refactored and the code editor crate can follow, instead of doing both in one swoop (and by the same person).

But in this case I think we can get 90% of the result with 10% of the code by only supporting tabs for indentation, and force users to convert their code to that convention if they want to use egui as a code editor. As stated before, it is rather simple to convert to/from tabs when loading said code, so there is really no need for egui to support both.

PS:
Supporting variable-width tabs would also be great (https://twitter.com/sarah_federman/status/1146544481556033537?lang=en) but can also wait for a separate PR. It is always better with many small PR:s than one huge PR.

lunabunn · 2021-04-16T16:55:39Z

TL;DR: IMHO this should all be in a separate crate except for the part where you can disable tab moving focus and swap over to tab inserting '\t' instead.

Nice work! However, AFAICT this has some major flaws:

4-space tab is an arbitrary number
- 2-space tabs and even 3-space tabs are used
- if we were to have a code editor widget, I'd expect it would at least support those
tabbing/shift-tabbing keybinds should be customizable
tab indents does NOT mean code
- selecting and tabbing/shift-tabbing is a coding convention and should not be on by default... or be there at all
- the whole idea of spaces as tabs is also a coding convention and should not be on by default... or be there at all

Overall, although I like the idea, all the stuff aside from not moving focus on tab should at the very least be separated into a separate PR. Further, IMHO, the code here is too big to be wieldy while also being too "basic" for many use cases anyway. Both of these factors make it unfit for the core library and as such it should really be just a separate crate. egui is a GUI library. It should expose just enough features so the user can build whatever they want to using it. It shouldn't try to build the GUI for the user; that's still the user's job (and for good reason, as it's unreasonable for one library to support every opinionated use case).

DrOptix · 2021-04-16T17:07:56Z

Hello sorry for the inactivity, I was focusing on something else.

I will try to clean the PR this weekend, but my questions is, what should I leave in it?
Only the part where you can capture the focus and insert \t and rendering it as 4 spaces as @emilk initially proposed in one of it's commits, before I went and coded all of this?

Now about configurable key bindings, at the moment all key bindings are hard coded and from what I know there is no KeyMapManager available that fires events when some configurable key binding was detected.

I'm OK with both giving up on this PR or cleaning it and leave only what's needed and/or acceptable.
I would appreciate a lot to know what to leave and what to purge from the PR.

Thank you a lot for your feedback!

lunabunn · 2021-04-16T17:12:42Z

Now about configurable key bindings, at the moment all key bindings are hard coded and from what I know there is no KeyMapManager available that fires events when some configurable key binding was detected.

Right, which is why the code editor should be implemented in user code. Along with the user's own key map manager.

I'm not emilk, but I think this PR should implement, as you say, only the part where you can capture the focus and insert \t and rendering it as 4 spaces. However, definitely don't get rid of your current code! You've done a lot of great work and it could be used as a starting point when implementing a code editor in the future on top of egui, probably as an external crate.

lunabunn · 2021-04-16T17:14:52Z

And in any case, even if we decide to add a "code editor" to core egui sometime, that doesn't need to happen in this very PR. Those two basic changes mentioned above would be enough to fix #73. If we want to discuss this more and maybe merge more of your changes, you can always open a new PR.

emilk · 2021-04-18T08:56:17Z

I will try to clean the PR this weekend, but my questions is, what should I leave in it?
Only the part where you can capture the focus and insert \t and rendering it as 4 spaces as @emilk initially proposed in one of it's commits, before I went and coded all of this?

I think that would be good, yes. I think the main bulk of this PR could be turned into an egui-code crate or similar. Such a crate could have a lot more freedom to do things like adding integration with syntect.

DrOptix · 2021-04-26T06:22:14Z

Cleanup according to PR reviews

Removed tab_as_spaces idea.
Removed identation management

The only thing left is the tab rendering which adaps to the remaining
space inside the current tab block.

We consider the following sequence of characters, separated with
spaces for clarity:

\t 1 \t 12 \t 123 \t 1234 \t

It will be rendered like this, using ---> to denote the space occupied
by a tab:

1234 | 1234 | 1234 | 1234 | 1234
-----+------+------+------+-----
---> | 1--> | 12-> | 123> | --->

- Removed tab_as_spaces idea. - Removed identation management The only thing left is the tab rendering which adaps to the remaining space inside the current tab block. We consider the following sequence of characters, separated with spaces for clarity: ``` \t 1 \t 12 \t 123 \t 1234 \t ``` It will be rendered like this, using ---> to denote the space occupied by a tab: ``` 1234 | 1234 | 1234 | 1234 | 1234 -----+------+------+------+----- ---> | 1--> | 12-> | 123> | ---> ```

lunabunn · 2021-04-26T17:41:55Z

I don't really understand the adaptation thing. I don't seem to be able to replicate your example in Google Docs nor OSX Pages, could you clarify what this is actually supposed to be doing?

DrOptix · 2021-04-26T20:45:38Z

Even if I used a mono spaced font in Google Docs I just reproduced it without issues, the idea, but the rendering is not what I would expect from a mono spaced font.

From my checks a tab in Google Docs is around 5 spaces and a bit.

emilk

Let's do the stupid thing of always keeping tabs at width=4. Tabs should only be used for indentation anyway ("indent with tabs, align with spaces")

emilk · 2021-04-29T17:38:41Z

epaint/src/text/font.rs

        for c in text.chars() {
            if !self.fonts.is_empty() {
-                let (font_index, glyph_info) = self.glyph_info(c);
+                // For tabs it is nice to let them occupy only what's left of the current


this is very common behavior, but is it "nice"? Does anyone actually like this behavior? It adds a lot of complication for a dubious feature.

If we should do something smart with tabs, we should implement elastic tabstops instead (but not in this PR)

I'm still wrapping my head around it, but it looks really interesting

emilk · 2021-04-29T17:40:46Z

epaint/src/text/font.rs

+                } else {
+                    tab_size -= 1;
+                    self.glyph_info(c)
+                };


A lot simpler implementation of this would be to just modify glyph_info.advance_width directly here. No need for fn tab_glyph_info and a separate glyph cache for different tab widths

DrOptix · 2021-04-29T18:56:34Z

All the simplifications should be done now.

Add multiline TextEdit to the demo

86c4b76

DrOptix mentioned this pull request Mar 22, 2021

Allow tabs for indentation in text edit widget #73

Closed

DrOptix added 6 commits March 22, 2021 19:10

Lock focus on multiline text edit

2f806a8

When TAB is detected add 4 Space chars

94c1801

Sync with work done upstream

13ed86d

DrOptix marked this pull request as ready for review March 24, 2021 11:49

Add code editor shortuct functions

5557b17

For both `Ui` and `TextEdit` add two helper functions to simplify creation on `TextEdit` focused on code editing. - `code_editor` - `code_editor_with_config`

emilk requested changes Mar 24, 2021

View reviewed changes

DrOptix marked this pull request as draft March 24, 2021 21:17

DrOptix and others added 18 commits March 24, 2021 23:22

Apply partial suggestions from code review

cb66aaa

Thank you @emilk for the suggestions Co-authored-by: Emil Ernerfeldt <[email protected]>

Single line identation removal fix + tests

b15a5c7

Fix tab rendering

f2ddd8f

WIP: Rework identation management

4633084

Refactor: start from clean slate

5e6fc2d

Refactor: handle identation insert at cursor

aafcc03

Refactor: replace selection in paragraph with identation

d7f3805

Refactor: Decrease identation

939733b

If we have a single cursor on a paragraph then we will decrease identation only on that paragraph. If we have a selection we will decrease identation on all selected paragraphs.

Refactor: keep selection offsets

4186356

Additional identation management

b386599

- `Tab`: When multiple lines are selected add identation - `Ctrl`+`[`: Remove identation on single or multiple lines - `Ctrl`+`]`: Add identation on single or multiple lines

Fix selection update in tabs-as-tabs mode

5bc7857

cargo fmt

798748d

Sync with upstream

c81132f

Sync with upstream

da2392c

Fix build: add missing tab_glyph_info_cache

9e6db0f

DrOptix added 2 commits April 2, 2021 18:23

Sync with upstream

061b56b

Sync with upstream

e14c549

DrOptix marked this pull request as ready for review April 2, 2021 17:25

DrOptix requested a review from emilk April 2, 2021 17:26

DrOptix added 2 commits April 2, 2021 20:39

Remove ccursor_paragraph_end as it is no longer used

04b8cfb

Sync with upstream

78ac783

emilk added a commit that referenced this pull request Apr 5, 2021

build_demo_web.sh: fix opening link on windows and linux

49ef94d

Co-authored with @DrOptix with code from #246

Merge branch 'master' into feature/73_identation_text_edit_widget

1b3b5a4

DrOptix added 2 commits April 17, 2021 14:14

Sync with upstream

4396c87

Sync with upstream

ae4ea45

DrOptix added 2 commits April 26, 2021 08:13

Sync with upstream

f384003

Sync with upstream

0fe4cce

emilk requested changes Apr 29, 2021

View reviewed changes

Tabs are always rendered as 4 spaces no matter where they are

de7a474

emilk approved these changes May 2, 2021

View reviewed changes

emilk merged commit 35c7b09 into emilk:master May 2, 2021

DrOptix deleted the feature/73_identation_text_edit_widget branch May 2, 2021 18:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tab identation for multiline text edit #246

Tab identation for multiline text edit #246

DrOptix commented Mar 22, 2021 •

edited

Loading

emilk left a comment

emilk Mar 24, 2021

DrOptix Apr 2, 2021

emilk Mar 24, 2021

DrOptix Apr 2, 2021

emilk commented Apr 3, 2021

DrOptix commented Apr 3, 2021

emilk commented Apr 15, 2021

lunabunn commented Apr 16, 2021 •

edited

Loading

DrOptix commented Apr 16, 2021

lunabunn commented Apr 16, 2021 •

edited

Loading

lunabunn commented Apr 16, 2021

emilk commented Apr 18, 2021

DrOptix commented Apr 26, 2021

lunabunn commented Apr 26, 2021

DrOptix commented Apr 26, 2021 •

edited

Loading

emilk left a comment

emilk Apr 29, 2021

DrOptix Apr 29, 2021

emilk Apr 29, 2021

DrOptix commented Apr 29, 2021

Tab identation for multiline text edit #246

Tab identation for multiline text edit #246

Conversation

DrOptix commented Mar 22, 2021 • edited Loading

emilk left a comment

Choose a reason for hiding this comment

emilk Mar 24, 2021

Choose a reason for hiding this comment

DrOptix Apr 2, 2021

Choose a reason for hiding this comment

emilk Mar 24, 2021

Choose a reason for hiding this comment

DrOptix Apr 2, 2021

Choose a reason for hiding this comment

emilk commented Apr 3, 2021

DrOptix commented Apr 3, 2021

emilk commented Apr 15, 2021

lunabunn commented Apr 16, 2021 • edited Loading

DrOptix commented Apr 16, 2021

lunabunn commented Apr 16, 2021 • edited Loading

lunabunn commented Apr 16, 2021

emilk commented Apr 18, 2021

DrOptix commented Apr 26, 2021

lunabunn commented Apr 26, 2021

DrOptix commented Apr 26, 2021 • edited Loading

emilk left a comment

Choose a reason for hiding this comment

emilk Apr 29, 2021

Choose a reason for hiding this comment

DrOptix Apr 29, 2021

Choose a reason for hiding this comment

emilk Apr 29, 2021

Choose a reason for hiding this comment

DrOptix commented Apr 29, 2021

DrOptix commented Mar 22, 2021 •

edited

Loading

lunabunn commented Apr 16, 2021 •

edited

Loading

lunabunn commented Apr 16, 2021 •

edited

Loading

DrOptix commented Apr 26, 2021 •

edited

Loading