Text object: operators to move cursor to the top or bottom #106196

Merged

Harley Acheson merged 1 commits from pioverfour/blender:dp_font_move_home_end into main

2023-04-19 02:18:31 +02:00

Damien Picard commented

2023-03-28 03:00:22 +02:00

Member

The FONT_OT_move and FONT_OT_move_select operators had most of the
usual operations (char, line, block, page), but the option to go to
the top or bottom of the "file" were lacking, whereas they had been
implemented in the text editor.

I found these shortcuts lacking while editing many text objects in a
scene, as these operations are ubiquitous in text editors.

This commit adds the options using the rather standard Ctrl + Home and
Ctrl + End shortcuts to move. The same shortcuts select by using Shift
as well.

Both the Blender default and Industry compatible keymaps have been updated.

The FONT_OT_move and FONT_OT_move_select operators had most of the usual operations (char, line, block, page), but the option to go to the top or bottom of the "file" were lacking, whereas they had been implemented in the text editor. I found these shortcuts lacking while editing many text objects in a scene, as these operations are ubiquitous in text editors. This commit adds the options using the rather standard Ctrl + Home and Ctrl + End shortcuts to move. The same shortcuts select by using Shift as well. Both the Blender default and Industry compatible keymaps have been updated.

👍 2 ❤️ 1

Damien Picard added the

labels 2023-03-28 03:00:22 +02:00

Damien Picard added this to the User Interface project 2023-03-28 03:04:45 +02:00

Brecht Van Lommel requested review from Harley Acheson 2023-04-04 15:57:01 +02:00

Brecht Van Lommel changed title from ~~Font: add options to move the cursor to the top or bottom~~ to Text object: operators to move cursor to the top or bottom

2023-04-04 15:57:36 +02:00

Brecht Van Lommel commented

2023-04-04 15:57:50 +02:00

Owner

Seems like a good feature.

Seems like a good feature.

Harley Acheson commented

2023-04-04 17:48:50 +02:00

Member

Hey, that's pretty cool!

I think you'd want to handle this as new cursmove types - added to the defines at about line 37 of DNA_vfont_types.h. That way the define FO_CURS_IS_MOTION can use these correctly. Although not obvious, I think FO_CURS is meant for changes that keep the cursor on the same line and the code is treating moves that change the cursor's vertical position specially. The comment at the use of FO_CURS_IS_MOTION is "apply vertical cursor motion to position immediately otherwise the selection will lag behind".

The other thing is I'd think of a replacement for "file". "paragraph" seems to fit with other uses there.

Hey, that's pretty cool! I *think* you'd want to handle this as new cursmove types - added to the defines at about line 37 of DNA_vfont_types.h. That way the define FO_CURS_IS_MOTION can use these correctly. Although not obvious, I think FO_CURS is meant for changes that keep the cursor on the same line and the code is treating moves that change the cursor's vertical position specially. The comment at the use of `FO_CURS_IS_MOTION` is "apply vertical cursor motion to position immediately otherwise the selection will lag behind". The other thing is I'd think of a replacement for "file". "paragraph" seems to fit with other uses there.

Damien Picard commented

2023-04-05 00:59:22 +02:00

Author

Member

I think FO_CURS is meant for changes that keep the cursor on the same line and the code is treating moves that change the cursor's vertical position specially. The comment at the use of FO_CURS_IS_MOTION is "apply vertical cursor motion to position immediately otherwise the selection will lag behind".

It looks like it, but what I don’t understand is that the new modes I introduced don’t suffer from this lag, which do happen to the other modes when commenting the block you mentioned. So I can certainly add FO_TOP and FO_BOTTOM types in DNA_vfont_types.h, but then I don’t know how to modify vfont_to_curve() accordingly, since selection looks to be working already.

The other thing is I'd think of a replacement for "file". "paragraph" seems to fit with other uses there.

I used FILE because that is what was used in the text editor, but I agree it doesn’t really work here. However, to me “paragraph” would mean the next or previous new line character. How about simply TEXT_BEGIN and TEXT_END?

> I think FO_CURS is meant for changes that keep the cursor on the same line and the code is treating moves that change the cursor's vertical position specially. The comment at the use of `FO_CURS_IS_MOTION` is "apply vertical cursor motion to position immediately otherwise the selection will lag behind". It looks like it, but what I don’t understand is that the new modes I introduced don’t suffer from this lag, which do happen to the other modes when commenting the [block you mentioned](https://projects.blender.org/blender/blender/src/branch/main/source/blender/editors/curve/editfont.c#L1251-L1256). So I can certainly add `FO_TOP` and `FO_BOTTOM` types in DNA_vfont_types.h, but then I don’t know how to modify `vfont_to_curve()` accordingly, since selection looks to be working already. > The other thing is I'd think of a replacement for "file". "paragraph" seems to fit with other uses there. I used `FILE` because that is what was used [in the text editor](https://projects.blender.org/blender/blender/src/branch/main/source/blender/editors/space_text/text_ops.c#L1662), but I agree it doesn’t really work here. However, to me “paragraph” would mean the next or previous new line character. How about simply `TEXT_BEGIN` and `TEXT_END`?

Harley Acheson commented

2023-04-05 17:52:11 +02:00

Member

It looks like it, but what I don’t understand is that the new modes I introduced don’t suffer from this lag

I noticed the same thing. I tried to see the lag with your patch but could not. So I just assumed I was testing inadequitely. Which is a pretty good assumption; I wouldn't remove or ignore that comment until I was very certain that I knew circumstances had changed since it was written.

How about simply TEXT_BEGIN and TEXT_END?

Sounds great!

but then I don’t know how to modify vfont_to_curve() accordingly, since selection looks to be working already.

Fairly certain this is all you need:
FontBeginEnd2.diff

> It looks like it, but what I don’t understand is that the new modes I introduced don’t suffer from this lag I noticed the same thing. I tried to see the lag with your patch but could not. So I just assumed I was testing inadequitely. Which is a pretty good assumption; I wouldn't remove or ignore that comment until I was very certain that I knew circumstances had changed since it was written. > How about simply `TEXT_BEGIN` and `TEXT_END`? Sounds great! > but then I don’t know how to modify `vfont_to_curve()` accordingly, since selection looks to be working already. Fairly certain this is all you need: [FontBeginEnd2.diff](/attachments/4652291c-2ad2-4ca5-b560-0257180b8fea)

FontBeginEnd2.diff

53 KiB

Damien Picard commented

2023-04-05 18:10:32 +02:00

Author

Member

Fairly certain this is all you need:

Ok, thanks. That’s about what I had started to do, but since it didn’t add much in vfont_to_curve() I thought I was doing it wrong.

> Fairly certain this is all you need: Ok, thanks. That’s about what I had started to do, but since it didn’t add much in `vfont_to_curve()` I thought I was doing it wrong.

Damien Picard changed title from ~~Text object: operators to move cursor to the top or bottom~~ to WIP: Text object: operators to move cursor to the top or bottom

2023-04-05 18:14:15 +02:00

Damien Picard force-pushed dp_font_move_home_end from 72fb31f736 to 79104a7e0a

2023-04-05 18:20:28 +02:00

Compare

Damien Picard changed title from ~~WIP: Text object: operators to move cursor to the top or bottom~~ to Text object: operators to move cursor to the top or bottom

2023-04-05 18:22:39 +02:00

Harley Acheson commented

2023-04-05 23:03:30 +02:00

Member

Just in case you are not aware, your PR as it is right now is missing the addition of TEXT_BEGIN & TEXT_END to the enun in curve_intern.h at about line 45, so it won't compile. It was in my version of the diff I attached earlier so you might have just missed that one bit.

While looking at it I also thought I'd look at the only part (of my code LOL) that bothered me. In vfont.c in that huge and ugly vfont_to_curve function we are using a local variable called "lnr". Our changed code is assigning a new current line number to that variable. But we enter that section of code with that var set to the last line number, which results in that awkward "pass" for FO_TEXTEND.

The following is the same as my prior but with an earlier assignment of the value of "lnr" to a "last_line" variable that is used instead. That way FO_TEXTEND can have a more logical "lnr = last_line".

Just in case you are not aware, your PR as it is right now is missing the addition of `TEXT_BEGIN` & `TEXT_END` to the enun in curve_intern.h at about line 45, so it won't compile. It was in my version of the diff I attached earlier so you might have just missed that one bit. While looking at it I also thought I'd look at the only part (of my code LOL) that bothered me. In vfont.c in that huge and ugly vfont_to_curve function we are using a local variable called "lnr". Our changed code is assigning a new current line number to that variable. But we enter that section of code with that var set to the last line number, which results in that awkward "pass" for FO_TEXTEND. The following is the same as my prior but with an earlier assignment of the value of "lnr" to a "last_line" variable that is used instead. That way FO_TEXTEND can have a more logical "lnr = last_line". [106196c.diff](/attachments/1e68193e-53b8-4c2c-b21d-1391c1b49699)

106196c.diff

19 KiB

Damien Picard commented

2023-04-06 09:21:23 +02:00

Author

Member

Indeed, I didn’t stage everything when I applied your patch. Sorry about that…

The newer change seems all right, but what I don’t quite get is that last_line is already declared as an int earlier in the function. Won’t redeclaring it with a new type of const int risk being confusing or even breaking?

Indeed, I didn’t stage everything when I applied your patch. Sorry about that… The newer change seems all right, but what I don’t quite get is that `last_line` is already declared as an `int` earlier in the function. Won’t redeclaring it with a new type of `const int` risk being confusing or even breaking?

Damien Picard force-pushed dp_font_move_home_end from 79104a7e0a to ab772cc711

2023-04-06 09:22:01 +02:00

Compare

Harley Acheson commented

2023-04-06 23:54:46 +02:00

Member

Indeed, I didn’t stage everything when I applied your patch. Sorry about that…

No worries!

The newer change seems all right, but what I don’t quite get is that last_line is already declared as an int earlier in the function. Won’t redeclaring it with a new type of const int risk being confusing or even breaking?

Yikes, didn't notice that other last_line and that does indeed make it confusing. It works because it is in a different scope - anything declared inside of that "if (ELEM(mode, FO_CURSUP, FO_CURSDOWN..." will be local to that section within the curly brackets.

Unfortunately that global "last_line" doesn't hold what we need; it is just "-1" at that point, so we can't use it for that purpose. But feel free to rename it or do anything that makes sense. I just found my earlier revision with the FO_TEXTEND "pass" to look weird. But you can always put that back if it looked okay to you.

> Indeed, I didn’t stage everything when I applied your patch. Sorry about that… No worries! > The newer change seems all right, but what I don’t quite get is that `last_line` is already declared as an `int` earlier in the function. Won’t redeclaring it with a new type of `const int` risk being confusing or even breaking? Yikes, didn't notice that other `last_line` and that does indeed make it confusing. It works because it is in a different scope - anything declared inside of that "if (ELEM(mode, FO_CURSUP, FO_CURSDOWN..." will be local to that section within the curly brackets. Unfortunately that global "last_line" doesn't hold what we need; it is just "-1" at that point, so we can't use it for that purpose. But feel free to rename it or do anything that makes sense. I just found my earlier revision with the `FO_TEXTEND` "pass" to look weird. But you can always put that back if it looked okay to you.

Damien Picard commented

2023-04-07 01:37:49 +02:00

Author

Member

Thanks for the explanation, that’s what I had gathered but since that function is a bit of a mastodon I wasn’t sure I was following.

I spent quite a while examining the code, and I’m not sure any more that new cursmove modes are needed at all, based on these observations:

The selection seemed to work in the first version, even using text boxes and even commenting the call to BKE_vfont_to_curve();
the PREV_WORD, NEXT_WORD, PREV_CHAR, and NEXT_CHAR modes may change the line number too if they are at the first or last char in the line, yet they don’t use specific cursmove modes. All they do is calculate a new value for ef->pos and the selection still works fine.

I’m not entirely sure of this, but my intuition is that the point of letting vfont_to_curve() calculate the new ef->pos is that it is much more complex when going to another line, say for NEXT_LINE, because the position of each char first needs to be computed from the beginning of the text, depending on their width, kerning, box sizes, etc., to know which char the cursor will fall on.

In the case of the text beginning or end though, it’s trivial: 0 or ef->len.

So I’m honestly tempted to revert to the first commit, simply using TEXT_BEGIN instead of my first choice FILE_TOP.

Let me know what you think!

Thanks for the explanation, that’s what I had gathered but since that function is a bit of a mastodon I wasn’t sure I was following. ----- I spent quite a while examining the code, and I’m not sure any more that new `cursmove` modes are needed at all, based on these observations: 1. The selection seemed to work in the first version, even using text boxes and even commenting the call to `BKE_vfont_to_curve()`; 2. the `PREV_WORD`, `NEXT_WORD`, `PREV_CHAR`, and `NEXT_CHAR` modes may change the line number too if they are at the first or last char in the line, yet they don’t use specific `cursmove` modes. All they do is calculate a new value for `ef->pos` and the selection still works fine. I’m not entirely sure of this, but my intuition is that the point of letting `vfont_to_curve()` calculate the new `ef->pos` is that it is much more complex when going to another line, say for `NEXT_LINE`, because the position of each char first needs to be computed from the beginning of the text, depending on their width, kerning, box sizes, etc., to know which char the cursor will fall on. In the case of the text beginning or end though, it’s trivial: 0 or ef->len. So I’m honestly tempted to revert to the first commit, simply using `TEXT_BEGIN` instead of my first choice `FILE_TOP`. Let me know what you think!

Harley Acheson commented

2023-04-07 03:16:35 +02:00

Member

Let me know what you think!

Hey, I'm happy as long as it reads well, makes sense, and gets tested well. Your original was fine, I think I just make it more complex for you because of that warning comment. But if you can go back to the simpler version and it works and selects well then that is great.

> Let me know what you think! Hey, I'm happy as long as it reads well, makes sense, and gets tested well. Your original was fine, I think I just make it more complex for you because of that warning comment. But if you can go back to the simpler version and it works and selects well then that is great.

Damien Picard force-pushed dp_font_move_home_end from ab772cc711 to a8848a1c7e

2023-04-07 13:26:16 +02:00

Compare

Damien Picard commented

2023-04-07 13:33:05 +02:00

Author

Member

Cool, I reverted now, and couldn’t find anything wrong with the cursor behaviour—except for an issue which had been there since at least 2.79, but that’s a different story.

Cool, I reverted now, and couldn’t find anything wrong with the cursor behaviour—except for an issue which had been there since at least 2.79, but that’s a different story.

Harley Acheson commented

2023-04-19 02:13:10 +02:00

Member

Yes, is a nice addition to the feature set.

Yes, is a nice addition to the feature set.

Harley Acheson approved these changes 2023-04-19 02:13:26 +02:00

Harley Acheson referenced this issue from a commit

2023-04-19 02:18:30 +02:00

Text object: operators to move cursor to the top or bottom

Harley Acheson merged commit 6d2351d26b into main

2023-04-19 02:18:31 +02:00

Damien Picard deleted branch dp_font_move_home_end

2023-04-19 16:21:42 +02:00

Sign in to join this conversation.

No reviewers

No Label

Animation & Rigging

Asset Browser Project Overview

Automated Testing

Blender Asset Bundle

Dependency Graph

Development Management

EEVEE & Viewport

Images & Movies

Motion Tracking

Nodes & Physics

Pipeline, Assets & IO

Platforms, Builds & Tests

Render & Cycles

Render Pipeline

Sculpt, Paint & Texture

Video Sequencer

Virtual Reality

Blender 2.8 Project

Milestone 1: Basic, Local Asset Browser

Good First Issue

Animation & Rigging

Development Management

EEVEE & Viewport

Nodes & Physics

Pipeline, Assets & IO

Platforms, Builds & Tests

Render & Cycles

Sculpt, Paint & Texture

Needs Info from Developers

Needs Information from User

No Milestone

No project

No Assignees

3 Participants

Notifications

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#106196

No description provided.