WIP: UI: Text cursor behavior for Apple Devices #106724

Closed

Austin Berenyi wants to merge 3 commits from (deleted):ui-macos-text-field-behavior into main

When changing the target branch, be careful to rebase the branch in your fork to match. See documentation.

Austin Berenyi commented

2023-04-08 23:55:19 +02:00

First-time contributor

UI: Adds expected text cursor behavior for Apple Devices

macOS uses different key modifier sets from Windows for moving a text cursor in text fields. This adds that support, and keeps the previous behavior for backward compatibility.

CMD modifier jumps all
ALT/OPT modifier jumps by delimiter
modifer + SHIFT jumps and selects

This also works with DELETE and BACKSPACE events.

UI: Adds expected text cursor behavior for Apple Devices macOS uses different key modifier sets from Windows for moving a text cursor in text fields. This adds that support, and keeps the previous behavior for backward compatibility. - `CMD` modifier jumps all - `ALT/OPT` modifier jumps by delimiter - modifer + `SHIFT` jumps and selects This also works with `DELETE` and `BACKSPACE` events.

Austin Berenyi added 1 commit 2023-04-08 23:55:30 +02:00

abdc9cb3c9 UI: Adds expected behavior for text fields for Apple Devices

Austin Berenyi requested review from Pablo Vazquez 2023-04-11 17:36:51 +02:00

Pablo Vazquez refused to review 2023-04-11 18:08:53 +02:00

Pablo Vazquez added this to the User Interface project 2023-04-11 18:09:03 +02:00

Pablo Vazquez added the

label 2023-04-11 18:09:14 +02:00

Austin Berenyi commented

2023-04-18 17:14:14 +02:00

Author

First-time contributor

I'm new to the Blender dev process, so these questions are purely for more understanding. Is there anything keeping this PR from being confirmed? Or has it just not been reviewed based on priority? What is a likely timeframe for a decision to be made on something lower priority like this?

I'm new to the Blender dev process, so these questions are purely for more understanding. Is there anything keeping this PR from being confirmed? Or has it just not been reviewed based on priority? What is a likely timeframe for a decision to be made on something lower priority like this?

Harley Acheson commented

2023-04-20 23:25:57 +02:00

Member

Hey Austin. Thanks for working on this!

With a quick look this seems to be a bit more complex than necessary and probably has some unintended consequences.

Using the first change as an example if you examine the current code that handles EVT_RIGHTARROWKEY. Right now if someone presses their right arrow then ui_textedit_move will be called and it will do something. It will might select or not, or jump over delimiters or not depending on other modifier keys, but something will happen.

With your proposed changes if we enter that for non-mac we test for CTRL key held and if not we enter a section that has behavior that changes with CTRL. Similarly if we enter that function for Mac we test for modifier keys, and if pressed we test again that that event->type is EVT_RIGHTARROWKEY. We already know this is the event type because that is what is tested on the switch statement.

So if you follow this on Non-Mac with Ctrl held we first test event->type against EVT_RIGHTARROWKEY and then if event->modifier == KM_CTRL we again check that event->type is EVT_RIGHTARROWKEY and then have a different jump type if ctrl is held, which we already know to be true.

It is my understanding (possibly wrong) that for all platforms the modifier to select while using arrow keys is shift, so the only thing you really need to change for Mac is for the eStrCursorJumpType argument, so you should be able to get by with a single ui_textedit_move here. Rather than put the "if" in your platform defines you could put the assignment of a eStrCursorJumpType variable. Then use that as an argument to ui_textedit_move.

Jumping to the last set of changes, for EVT_BACKSPACEKEY we currently have differing behavior if you also have Ctrl pressed. But your addition of Alt to this will be for all platforms. Although adding this along the existing helps backward compatibility for existing Mac users, this will also add Alt-Backspace for other platforms. We want to make as minimal changes as possible as those platforms could conceivably have alt-backspace behavior.

Here you might consider assigning that KM_CTRL to a variable and then in a Mac-specific block you can OR KM_ALT to it. Then the function can just test against your variable.

Hey Austin. Thanks for working on this! With a quick look this seems to be a bit more complex than necessary and probably has some unintended consequences. Using the first change as an example if you examine the current code that handles EVT_RIGHTARROWKEY. Right now if someone presses their right arrow then `ui_textedit_move` will be called and it will do something. It will might select or not, or jump over delimiters or not depending on other modifier keys, but something will happen. With your proposed changes if we enter that for non-mac we test for CTRL key held and if not we enter a section that has behavior that changes with CTRL. Similarly if we enter that function for Mac we test for modifier keys, and if pressed we test again that that event->type is EVT_RIGHTARROWKEY. We already know this is the event type because that is what is tested on the switch statement. So if you follow this on Non-Mac with Ctrl held we first test event->type against EVT_RIGHTARROWKEY and then if event->modifier == KM_CTRL we again check that event->type is EVT_RIGHTARROWKEY and then have a different jump type if ctrl is held, which we already know to be true. It is my understanding (possibly wrong) that for all platforms the modifier to select while using arrow keys is shift, so the only thing you really need to change for Mac is for the eStrCursorJumpType argument, so you should be able to get by with a single `ui_textedit_move` here. Rather than put the "if" in your platform defines you could put the assignment of a eStrCursorJumpType variable. Then use that as an argument to ui_textedit_move. Jumping to the last set of changes, for EVT_BACKSPACEKEY we currently have differing behavior if you also have Ctrl pressed. But your addition of Alt to this will be for all platforms. Although adding this along the existing helps backward compatibility for existing Mac users, this will also add Alt-Backspace for other platforms. We want to make as minimal changes as possible as those platforms could conceivably have alt-backspace behavior. Here you might consider assigning that KM_CTRL to a variable and then in a Mac-specific block you can OR KM_ALT to it. Then the function can just test against your variable.

Brecht Van Lommel commented

2023-04-22 01:28:30 +02:00

Owner

To clarify, the code could be structured like this to simplify it:

case EVT_RIGHTARROWKEY:
case EVT_LEFTARROWKEY:
  const int direction = (event->type == EVT_RIGHTARROWKEY) ? STRCUR_DIR_NEXT : STRCUR_DIR_PREV;
  const bool select = (event->modifier & KM_SHIFT) != 0;
#ifdef __APPLE__
  const int jump = ...
#else
  const int jump = ...
#endif
  ui_textedit_move(but, data, direction, select, jump);
  retval = WM_UI_HANDLER_BREAK;
  break;

To clarify, the code could be structured like this to simplify it: ```Cpp case EVT_RIGHTARROWKEY: case EVT_LEFTARROWKEY: const int direction = (event->type == EVT_RIGHTARROWKEY) ? STRCUR_DIR_NEXT : STRCUR_DIR_PREV; const bool select = (event->modifier & KM_SHIFT) != 0; #ifdef __APPLE__ const int jump = ... #else const int jump = ... #endif ui_textedit_move(but, data, direction, select, jump); retval = WM_UI_HANDLER_BREAK; break; ```

Harley Acheson commented

2023-04-22 01:48:03 +02:00

Member

@Austin Berenyi - Do not hesitate to ask questions or for clarification. We'll help you out as much or as little as you need. We'd like you to succeed.

@Austin Berenyi - Do not hesitate to ask questions or for clarification. We'll help you out as much or as little as you need. We'd like you to succeed.

Austin Berenyi commented

2023-04-22 02:58:33 +02:00

Author

First-time contributor

Thank you @Harley and @brecht for the help! I reviewed your suggestions, and the logic make sense! I'm getting a couple errors probably due to my unfamiliarity with C.

I checked the parameters of ui_textedit_move and all the variables seem to match the data types. I'm not sure why I'm getting this first error.

The second error may be caused by the first. Otherwise it's probably because I'm not satisfying the switch somehow.

Thank you @Harley and @brecht for the help! I reviewed your suggestions, and the logic make sense! I'm getting a couple errors probably due to my unfamiliarity with C. I checked the parameters of `ui_textedit_move` and all the variables seem to match the data types. I'm not sure why I'm getting this first error. The second error may be caused by the first. Otherwise it's probably because I'm not satisfying the switch somehow.

Screenshot 2023-04-21 at 6.54.03 PM.png

447 KiB

Screenshot 2023-04-21 at 6.54.03 PM.png

Harley Acheson commented

2023-04-22 21:46:21 +02:00

Member

Hey @Austin-Berenyi. I think the only things incorrect in your sample are just the types used for jump and direction. But just in the case the following also creates a scope specific to that case as well (added curly brackets), since that can also cause issues if defining variables in them (otherwise in C the scope is the entire switch).

Not tested for your purpose, but the following should compile at least:

diff --git a/source/blender/editors/interface/interface_handlers.cc b/source/blender/editors/interface/interface_handlers.cc
index 7bd46ccd9b2..0b64e3374bc 100644
--- a/source/blender/editors/interface/interface_handlers.cc
+++ b/source/blender/editors/interface/interface_handlers.cc
@@ -3743,35 +3743,35 @@ static void ui_do_but_textedit(
             changed = ui_textedit_copypaste(but, data, UI_TEXTEDIT_COPY);
           }
           else if (event->type == EVT_XKEY) {
             changed = ui_textedit_copypaste(but, data, UI_TEXTEDIT_CUT);
           }
 
           retval = WM_UI_HANDLER_BREAK;
         }
         break;
       case EVT_RIGHTARROWKEY:
-        ui_textedit_move(but,
-                         data,
-                         STRCUR_DIR_NEXT,
-                         event->modifier & KM_SHIFT,
-                         (event->modifier & KM_CTRL) ? STRCUR_JUMP_DELIM : STRCUR_JUMP_NONE);
-        retval = WM_UI_HANDLER_BREAK;
-        break;
-      case EVT_LEFTARROWKEY:
-        ui_textedit_move(but,
-                         data,
-                         STRCUR_DIR_PREV,
-                         event->modifier & KM_SHIFT,
-                         (event->modifier & KM_CTRL) ? STRCUR_JUMP_DELIM : STRCUR_JUMP_NONE);
+      case EVT_LEFTARROWKEY: {
+        const eStrCursorJumpDirection direction = (event->type == EVT_RIGHTARROWKEY) ?
+                                                      STRCUR_DIR_NEXT :
+                                                      STRCUR_DIR_PREV;
+#if defined(__APPLE__)
+        eStrCursorJumpType jump = (event->modifier & KM_OSKEY) ? STRCUR_JUMP_DELIM :
+                                                                 STRCUR_JUMP_NONE;
+#else
+        eStrCursorJumpType jump = (event->modifier & KM_CTRL) ? STRCUR_JUMP_DELIM :
+                                                                STRCUR_JUMP_NONE;
+#endif
+        ui_textedit_move(but, data, direction, event->modifier & KM_SHIFT, jump);
         retval = WM_UI_HANDLER_BREAK;
         break;
+      }
       case WHEELDOWNMOUSE:
       case EVT_DOWNARROWKEY:
         if (data->searchbox) {
 #ifdef USE_KEYNAV_LIMIT
           ui_mouse_motion_keynav_init(&data->searchbox_keynav_state, event);
 #endif
           ui_searchbox_event(C, data->searchbox, but, data->region, event);
           break;
         }
         if (event->type == WHEELDOWNMOUSE) {

Hey @Austin-Berenyi. I _think_ the only things incorrect in your sample are just the types used for jump and direction. But just in the case the following also creates a scope specific to that case as well (added curly brackets), since that can also cause issues if defining variables in them (otherwise in C the scope is the entire switch). Not tested for your purpose, but the following _should_ compile at least: ```Diff diff --git a/source/blender/editors/interface/interface_handlers.cc b/source/blender/editors/interface/interface_handlers.cc index 7bd46ccd9b2..0b64e3374bc 100644 --- a/source/blender/editors/interface/interface_handlers.cc +++ b/source/blender/editors/interface/interface_handlers.cc @@ -3743,35 +3743,35 @@ static void ui_do_but_textedit( changed = ui_textedit_copypaste(but, data, UI_TEXTEDIT_COPY); } else if (event->type == EVT_XKEY) { changed = ui_textedit_copypaste(but, data, UI_TEXTEDIT_CUT); } retval = WM_UI_HANDLER_BREAK; } break; case EVT_RIGHTARROWKEY: - ui_textedit_move(but, - data, - STRCUR_DIR_NEXT, - event->modifier & KM_SHIFT, - (event->modifier & KM_CTRL) ? STRCUR_JUMP_DELIM : STRCUR_JUMP_NONE); - retval = WM_UI_HANDLER_BREAK; - break; - case EVT_LEFTARROWKEY: - ui_textedit_move(but, - data, - STRCUR_DIR_PREV, - event->modifier & KM_SHIFT, - (event->modifier & KM_CTRL) ? STRCUR_JUMP_DELIM : STRCUR_JUMP_NONE); + case EVT_LEFTARROWKEY: { + const eStrCursorJumpDirection direction = (event->type == EVT_RIGHTARROWKEY) ? + STRCUR_DIR_NEXT : + STRCUR_DIR_PREV; +#if defined(__APPLE__) + eStrCursorJumpType jump = (event->modifier & KM_OSKEY) ? STRCUR_JUMP_DELIM : + STRCUR_JUMP_NONE; +#else + eStrCursorJumpType jump = (event->modifier & KM_CTRL) ? STRCUR_JUMP_DELIM : + STRCUR_JUMP_NONE; +#endif + ui_textedit_move(but, data, direction, event->modifier & KM_SHIFT, jump); retval = WM_UI_HANDLER_BREAK; break; + } case WHEELDOWNMOUSE: case EVT_DOWNARROWKEY: if (data->searchbox) { #ifdef USE_KEYNAV_LIMIT ui_mouse_motion_keynav_init(&data->searchbox_keynav_state, event); #endif ui_searchbox_event(C, data->searchbox, but, data->region, event); break; } if (event->type == WHEELDOWNMOUSE) { ```

Austin Berenyi commented

2023-04-24 17:44:09 +02:00

Author

First-time contributor

@Harley Thank you! My only experience in code has been in Swift for iOS, so it was helpful for me to see how data types should be declared in your example.

The code compiles, and works great. However, this still doesn't allow the user to use the CMD (i.e KM_OSKEY) for STRCUR_JUMP_ALL, which is also standard behavior for Apple devices.

eStrCursorJumpType jump = (event->modifier & KM_ALT) ? STRCUR_JUMP_DELIM : STRCUR_JUMP_NONE;

This line only allows for the conditions of STRCUR_JUMP_DELIM or STRCUR_JUMP_NONE. Could a switch be used here to allow for a third condition of STRCUR_JUMP_ALL?

@Harley Thank you! My only experience in code has been in Swift for iOS, so it was helpful for me to see how data types should be declared in your example. The code compiles, and works great. However, this still doesn't allow the user to use the `CMD` (i.e `KM_OSKEY`) for `STRCUR_JUMP_ALL`, which is also standard behavior for Apple devices. `eStrCursorJumpType jump = (event->modifier & KM_ALT) ? STRCUR_JUMP_DELIM : STRCUR_JUMP_NONE;` This line only allows for the conditions of `STRCUR_JUMP_DELIM` or `STRCUR_JUMP_NONE`. Could a switch be used here to allow for a third condition of `STRCUR_JUMP_ALL`?

Harley Acheson commented

2023-04-24 20:17:42 +02:00

Member

@Austin-Berenyi - doesn't allow the user to use the CMD (i.e KM_OSKEY) for STRCUR_JUMP_ALL

I don't have an Apple so I'm not really familiar with the keyboard functions there. But yes the code above, for Mac, is only giving you two alternates for the "jump" type based on the pressing of the KM_OSKEY. If you want three different behaviors it would get slightly more complicated. But note that you can nest these ternaries. So something like the following might be closer to what you want?

        eStrCursorJumpType jump = (event->modifier & KM_OSKEY) ?
                                      STRCUR_JUMP_ALL :
                                      ((event->modifier & KM_ALT) ? STRCUR_JUMP_DELIM :
                                                                    STRCUR_JUMP_NONE);

Note that we will assume that you are enjoying yourself and the challenge of this. However if you stop having fun, get frustrated, and want to give up, I could also just do this and then you can take a role of testing instead. But for that I'd need a fairly complete table of what you are expecting for each key and modifier.

> @Austin-Berenyi - doesn't allow the user to use the `CMD` (i.e `KM_OSKEY`) for `STRCUR_JUMP_ALL` I don't have an Apple so I'm not really familiar with the keyboard functions there. But yes the code above, for Mac, is only giving you two alternates for the "jump" type based on the pressing of the KM_OSKEY. If you want three different behaviors it would get slightly more complicated. But note that you can nest these [ternaries](https://en.wikipedia.org/wiki/Ternary_conditional_operator). So something _like_ the following might be closer to what you want? ```C eStrCursorJumpType jump = (event->modifier & KM_OSKEY) ? STRCUR_JUMP_ALL : ((event->modifier & KM_ALT) ? STRCUR_JUMP_DELIM : STRCUR_JUMP_NONE); ``` Note that we will assume that you are enjoying yourself and the challenge of this. However if you stop having fun, get frustrated, and want to give up, I could also just do this and then you can take a role of testing instead. But for that I'd need a fairly complete table of what you are expecting for each key and modifier.

Harley Acheson changed title from ~~UI: Adds expected text cursor behavior for Apple Devices~~ to WIP: UI: Text cursor behavior for Apple Devices

2023-04-25 17:59:06 +02:00

Austin Berenyi added 1 commit 2023-04-29 16:26:46 +02:00

0d676ede92 Made changes based on suggestions from reviewers. Conditionals defining event modifiers are in correct place.

Austin Berenyi commented

2023-04-29 16:29:03 +02:00

Author

First-time contributor

@Harley I made suggested revisions, and applied the same for 'DELETE' and 'BACKSPACE' events. I tested the behavior, and everything is working exactly as I would expect.

@Harley I made suggested revisions, and applied the same for 'DELETE' and 'BACKSPACE' events. I tested the behavior, and everything is working exactly as I would expect.

Austin Berenyi commented

2023-04-29 16:31:29 +02:00

Author

First-time contributor

@Harley In my last commit it looks like there are some unrelated changes from someone else because I didn't merge with main before committing. If this is a problem, how would I go about fixing it?

@Harley In my last commit it looks like there are some unrelated changes from someone else because I didn't merge with main before committing. If this is a problem, how would I go about fixing it?

Austin Berenyi added 1 commit 2023-04-29 16:54:56 +02:00

5083f15742 Merge branch 'main' into ui-macos-text-field-behavior

Harley Acheson commented

2023-05-01 04:40:49 +02:00

Member

I made suggested revisions

That's looking nicer!

One trick though, when making these platform-specific changes is to sometimes change the #ifdef you are working on temporarily to a #ifndef to see what it looks like with the reverse. In this case you'd see that in your first changes direction isn't defined. Since that doesn't change between platforms you could just move it up a couple lines before your #ifdef so it is always visible.

applied the same for 'DELETE' and 'BACKSPACE' events

This will sound cruel, but you should probably go for the "extra credits" and combine those two in the same way that your PR combines EVT_RIGHTARROWKEY & EVT_LEFTARROWKEY. I think they are similar in that almost everything is the same between them except for the direction. Doing so would remove some complexity to balance out the complexity that your PR adds.

If this is a problem, how would I go about fixing it?

Its only a problem if there is something keeping you from working on it further, or from updating the PR here. If so let me know and I'll give you some help

> I made suggested revisions That's looking nicer! One trick though, when making these platform-specific changes is to sometimes change the `#ifdef` you are working on temporarily to a `#ifndef` to see what it looks like with the reverse. In this case you'd see that in your first changes `direction` isn't defined. Since that doesn't change between platforms you could just move it up a couple lines before your `#ifdef` so it is always visible. > applied the same for 'DELETE' and 'BACKSPACE' events This will sound cruel, but you should probably go for the "extra credits" and combine those two in the same way that your PR combines EVT_RIGHTARROWKEY & EVT_LEFTARROWKEY. I think they are similar in that almost everything is the same between them except for the direction. Doing so would remove some complexity to balance out the complexity that your PR adds. > If this is a problem, how would I go about fixing it? Its only a problem if there is something keeping you from working on it further, or from updating the PR here. If so let me know and I'll give you some help

Austin Berenyi closed this pull request

2023-05-02 01:26:10 +02:00

Harley Acheson referenced this pull request

2023-05-02 02:21:22 +02:00

UI: Text cursor behavior for Apple Devices #107522

Pull request closed

This pull request cannot be reopened because the branch was deleted.

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#106724

No description provided.