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 text selection support to TextFieldWidget. #14980

Merged
merged 1 commit into from Apr 8, 2018

Conversation

Projects
None yet
7 participants
@joealam
Copy link
Contributor

joealam commented Mar 22, 2018

Implements feature requested in issue #2794.
Use Shift and navigation key (cursor, home, end) to select a portion of
text, and replace/delete/cut as appropriate.
Also provides support for selection with mouse (click and drag)

Proposed changelog entry:
Added text selection and editing support in text boxes.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Mar 22, 2018

This looks really great, thanks for the PR!

At a first look I have a couple of quick comments, and will hopefully follow up in more detail within the next few days:

  • OpenRA code style omits braces around single line blocks
  • Cut (cmd+x) is implemented, but copy (cmd+c) isn't?
  • We may want to tweak the selection color to fit the color palette in each mod
@CatGirls420-NerevarII

This comment has been minimized.

Copy link

CatGirls420-NerevarII commented Mar 23, 2018

Not all heroes wear capes!

@pchote
Copy link
Member

pchote left a comment

Code changes and functionality LGTM aside from my points above.

@joealam joealam force-pushed the joealam:bleed branch from a3c6849 to 2b021c9 Mar 26, 2018

@joealam

This comment has been minimized.

Copy link
Contributor Author

joealam commented Mar 26, 2018

Addressed your above points and updated the fork.

"OpenRA code style omits braces around single line blocks"
Done! Removed all of the "additional" braces in my changes.

"Cut (cmd+x) is implemented, but copy (cmd+c) isn't?"
Good point! This was because cut was already implemented without selection, so I just spotted than and added to it. I've updated the fork with copy support too.

"We may want to tweak the selection color to fit the color palette in each mod"
I believe this is supported by my existing changes, simply requiring the entry added to the metrics.yaml file for whichever mod a different colour is required (unless I'm mistaken).

&& !string.IsNullOrEmpty(Text))
{
// If a portion of text is selected, copy it to the clipboard
if (selectionStartIndex != -1)

This comment has been minimized.

@MunWolf

MunWolf Mar 27, 2018

Contributor

Why is this not a part of the if statement above?

@MunWolf

This comment has been minimized.

Copy link
Contributor

MunWolf commented Mar 27, 2018

Other then that one question the code looks good from my end.

@joealam

This comment has been minimized.

Copy link
Contributor Author

joealam commented Mar 27, 2018

It's just layed out the same way as the Cut case for easy readability, but I'm happy to change it if preferred?

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Mar 30, 2018

LGTM, just a couple more small requests:

  • Can you please merge the two fixup commits into the first one?
  • Keeping the old behaviour where the copy/cut commands will act on the whole text box if nothing is selected feels wrong now that we have proper selection support. Can we remove that?
  • I suggest the following mod-specific highlight colors to better fit with the UI (merge into the first commit):
    • cnc: 800000
    • ra: 562020
    • ts: 1a1a1a
    • d2k: 7f4d29
Added text selection and copy support to TextFieldWidget.
Use Shift and navigation key (cursor, home, end) to select a portion of
text, and replace/delete/cut as appropriate.
Also provides support for selection with mouse (click and drag)

@pchote pchote force-pushed the joealam:bleed branch from 2b021c9 to 9a1035f Apr 7, 2018

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Apr 7, 2018

I hope you don't mind @joealam, but I have made the changes above and pushed over your branch so we can move ahead with this.

@pchote

pchote approved these changes Apr 7, 2018

Copy link
Member

pchote left a comment

LGTM and works as advertised.

@pchote pchote added the PR: Needs +2 label Apr 7, 2018

@Smittytron
Copy link
Contributor

Smittytron left a comment

Works for me

@joealam

This comment has been minimized.

Copy link
Contributor Author

joealam commented Apr 7, 2018

@pchote I don't mind at all! It's ideal in fact, is I'm going to be away from my PC for a while so wouldn't have had chance to do them myself.

@reaperrr reaperrr merged commit 7221c29 into OpenRA:bleed Apr 8, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@reaperrr

This comment has been minimized.

Copy link
Contributor

reaperrr commented Apr 8, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.