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

Added OS X compatible behaviour for text editing #473

Closed
wants to merge 2 commits into from
Closed

Added OS X compatible behaviour for text editing #473

wants to merge 2 commits into from

Conversation

zhiayang
Copy link

@zhiayang zhiayang commented Jan 6, 2016

Addresses issue raised by me in issue #456, near the bottom.
Changes these things:

  1. KeySuper is now a field in ImGuiIO, and is now set by the various backends where it makes sense: GLFW (OpenGL and OpenGL3), SDL (OpenGL and OpenGL3), Allegro and Marmalade. I couldn't figure out if Windows passes Win key events to applications, so I left out the DirectX backends.
  2. Four new fields have been added to ImGuiIO; bool WordMovementUsesAltKey, bool ShortcutsUseSuperKey, bool DoubleClickSelectsWord, and bool MultiSelectUsesSuperKey. These fields control the new, modified behaviour that I set up.
  3. Made a small patch to stb_textedit such that moving between words follows Linux and OS X behaviour, where the position of the cursor (before or after the space) is dependent on the direction of movement, unlike Windows where it always puts it after the space. A new #define has been added.

Based on these fields, on OS X (or when __APPLE__ is defined), the following changes to text editing take place:

  1. Keyboard shortcuts that traditionally use Ctrl (eg. Copy/Paste, Cut) now use Cmd, when ShortcutsUseSuperKey == true.
  2. Double clicking in a text field now selects the word that the cursor is on, instead of the entire field, if DoubleClickSelectsWord == true.
  3. Making the cursor jump between words, previously done with Ctrl+Arrow, is now done with Alt, if WordMovementUsesAltKey == true.

@ocornut: I didn't put LSuper/RSuper in the enum field for ImGuiKey_, since there aren't entries for Ctrl/Alt etc. Given that there's ImGuiIO::KeySuper, I don't think they'll be necessary either.

EDIT: looks like I touched a couple more files than I was supposed to, deleted a bunch of trailing spaces which probably doesn't hurt anyone.

@emoon
Copy link
Contributor

emoon commented Jan 6, 2016

This is a great PR but imo it would have been better to put the trailing spaces cleanup in a separate PR as it makes it more clear what code that actually has been changes. I of course leave it to @ocornut to decide this :)

@ocornut
Copy link
Owner

ocornut commented Jan 6, 2016

I agree I'd prefer if the trailing spaces were in a separate commit. I'm probably going to remove trailing spaces locally before merging or something, and skip stb_textedit.h there. Don't worry too much (unless it is easy for you to reapply).

Will review in more details later (as InputTextEx() is notoriously full of traps) but it looks good

@zhiayang
Copy link
Author

zhiayang commented Jan 6, 2016

Oh okay, yea I totally forgot I had set my editor to trim trailing spaces. Oops.

@ocornut
Copy link
Owner

ocornut commented Mar 26, 2016

@zhiayang Would you mind suggesting the changes to stb_textedit.h via a PR in its repo?

@itamago
Copy link

itamago commented Apr 1, 2016

Any news about this feature ?
That would be very great to support OSX shortcuts :)

@ocornut
Copy link
Owner

ocornut commented Apr 1, 2016

Wanted to look at it last weekend but the PR is rather messy with all the removed spaces. Will look into it this week-end probably!

@itamago
Copy link

itamago commented Apr 2, 2016

Thanks !

@ocornut
Copy link
Owner

ocornut commented Apr 2, 2016

Also need to submit the stb_textedit.h part to the stb repro

ocornut added a commit that referenced this pull request Apr 2, 2016
… Windows key in theory although the later is hard to read) (#473)

NB: Value not used.
ocornut added a commit that referenced this pull request Apr 2, 2016
ocornut added a commit that referenced this pull request Apr 2, 2016
ocornut added a commit that referenced this pull request Apr 2, 2016
@ocornut
Copy link
Owner

ocornut commented Apr 2, 2016

Merged everything (with minor tweaks/comments) apart from the stb_textedit.h patch which a/ breaks windows behavior b) should be PR-ed upstream even if we merge ourselves earlier.

This became very tricky because of the white-space changes, having everything in the same commit, and altering stb_textedit.I had to hand-merge everything now (unfortunately you don't get the karma points from Github because of that). Learning for next time! Even how small the feature are, split them in different commits so we can cherry-pick and let the validated features "pass" asap, to leave only the the debatable ones and clear the noise.

PS: MultiSelectUsesSuperKey isn't used but I left it with a comment, I think it makes sense to leave it now.

ocornut added a commit that referenced this pull request Apr 2, 2016
@ocornut
Copy link
Owner

ocornut commented Apr 2, 2016

I have now rearranged the modifications to stb_textedit in a way that makes more sense for a pull request there and applied the similar change for OSX style behavior on OSX only (based on __APPLE__). Not sure anymore about leaving all those runtime options for the other features, perhaps it'd more simpler to just use __APPLE__ for now. If someone really need them, perhaps.

Note that this will break user application that update core imgui and but have KeySuper set.

@ocornut ocornut closed this Apr 2, 2016
@ocornut
Copy link
Owner

ocornut commented Apr 2, 2016

FYI stb_textedit.h patch posted to nothings/stb#273

michaelbartnett added a commit to michaelbartnett/imgui that referenced this pull request Jul 7, 2016
ocornut added a commit that referenced this pull request Jul 29, 2016
ocornut added a commit that referenced this pull request Nov 12, 2017
… Renamed io.OSXBehaviors to io.OptMacOSXBehaviors. Should affect users as the compile-time default is usually enough. (#473, #650)
ocornut added a commit that referenced this pull request Aug 1, 2018
… io.OptCursorBlink to io.ConfigCursorBlink, io.OptMacOSXBehaviors to ConfigMacOSXBehaviors for consistency. (#1427, #1495, #822, #473, #650)

Demo: Exposed flags in Demo.
@ocornut
Copy link
Owner

ocornut commented Aug 1, 2018

FYI in the latest version I have renamed io.OptMacOSXBehaviors to io.ConfigMacOSXBehaviors for consistency with other options. This shouldn't affect anyone normally, as the flag is set by default by a pre-processor check.

(Even though there is a io.ConfigFlags thing it is more forward thinking to keep them separate so different types than bool can be used for variety of configuration options)

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.

4 participants