Tab identation for multiline text edit#246
Tab identation for multiline text edit#246emilk merged 42 commits intoemilk:masterfrom DrOptix:feature/73_identation_text_edit_widget
Conversation
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`
egui/src/ui.rs
Outdated
| text: &mut String, | ||
| tab_as_spaces: bool, | ||
| tab_moves_focus: bool, | ||
| ) -> Response { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
Outdated
| /// .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 { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I crated a CodingConfig struct for this purpose.
https://github.com/DrOptix/egui/blob/e14c5491b61574647ed837130e0a61f4c1ce98b4/egui/src/widgets/text_edit.rs#L107-L112
Thank you @emilk for the suggestions Co-authored-by: Emil Ernerfeldt <emil.ernerfeldt@gmail.com>
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.
|
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 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 ( |
|
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. |
|
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: |
|
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 Nice work! However, AFAICT this has some major flaws:
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). |
|
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? 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. Thank you a lot for your feedback! |
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. |
|
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. |
I think that would be good, yes. I think the main bulk of this PR could be turned into an |
|
Cleanup according to PR reviews
The only thing left is the tab rendering which adaps to the remaining We consider the following sequence of characters, separated with It will be rendered like this, using ---> to denote the space occupied |
- 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> | ---> ```
|
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? |
|
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
left a comment
emilk
left a comment
There was a problem hiding this comment.
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")
epaint/src/text/font.rs
Outdated
| 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 |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
I'm still wrapping my head around it, but it looks really interesting
epaint/src/text/font.rs
Outdated
| } else { | ||
| tab_size -= 1; | ||
| self.glyph_info(c) | ||
| }; |
There was a problem hiding this comment.
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
|
All the simplifications should be done now. |
3 participants
With this PR we resolve #73, allowing opt-in tab indentation in
TextEditWhen we load a string in
TextEditwe keep the tabs as they are and by defaulttab_as_spaceis initialized tofalseandtab_moves_focusis initialized totrue.To config
tab_as_spacesandtab_moves_focusto the values we want we can use the following helper functions:TextEdit::tab_as_spaces()TextEdit::tab_moves_focus()Example:
Now depending on
tab_as_spacesandtab_moves_focusproperties of theTextEditwe have the following situations:When
tab_as_spacesis false, then when we pressTABthe result is aTABchar is added to the string with the width of 4 spaces.When
tab_as_spacesis true, then when we pressTABthe result is a chunk of 4 spaces are added to the string.To fully understand how to use
TextEditwith those two opt-in behaviors check the "Misc Demos" demo.Other two helper functions were added to
UiandTextEditto simplify the creation of aTextEditfocused on code editing:code_editorcode_editor_with_configThe 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.
tabInsert
\tor needed number of spaces to align to the tabulation size.\tchar will be rendered so that it will align to the tabulation sizeshift+tabWill 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+[andshift+]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.shscript 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-openfor Linux, Fedora for examplestartfor Windows, MSYSopenfor anything else