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 EditAction::SelectNone #1092

Merged
merged 3 commits into from
Sep 7, 2020
Merged

added EditAction::SelectNone #1092

merged 3 commits into from
Sep 7, 2020

Conversation

sysint64
Copy link
Contributor

this PR adds a new edit action SelectNone as the opposite of SelectAll.

Copy link
Member

@cmyr cmyr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay!

I think this is equivalent to moving the selection to the start of the document, and that should be handled by a variant of MoveTo(); StartOfDocument or BeginningOfDocument should be one of the Movement variants.

If you'd like to update this with that change I would be happy to merge. :)

@cmyr cmyr added S-waiting-on-author waits for changes from the submitter and removed S-needs-review waits for review labels Jul 26, 2020
@sysint64
Copy link
Contributor Author

@cmyr okay, I got it.

@cmyr
Copy link
Member

cmyr commented Aug 17, 2020

@sysint64 ping on this?

@sysint64
Copy link
Contributor Author

@cmyr I'm still here, just a lot of work, I'll work on my PR's on the following weekends.

@sysint64
Copy link
Contributor Author

sysint64 commented Aug 18, 2020

@cmyr does Movement::LeftOfLine do the same thing as should BeginningOfDocument? at least the current implementation seems to me to be what I have tried to do.

@cmyr
Copy link
Member

cmyr commented Aug 18, 2020

It may be the same now, but it shouldn't be the same when we are handling multi-line text correctly; if it is that's likely a bug in the current implementation.

@sysint64 sysint64 force-pushed the select-none branch 2 times, most recently from 772546a to 46e07a3 Compare August 31, 2020 07:51
@sysint64
Copy link
Contributor Author

@cmyr Sorry for the delay, I have implemented two methods - line_beginning and line_ending for EditableText, and added Movement::BeginningOfDocument and Movement::EndingOfDocument.

Copy link
Member

@cmyr cmyr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! a few notes inline.

It's also worth mentioning that this won't currently work correctly when we have linebreaking implemented, but I think it's reasonable to merge anyway and we can worry about that later.

CHANGELOG.md Outdated Show resolved Hide resolved
druid/src/text/movement.rs Outdated Show resolved Hide resolved
druid/src/text/editable_text.rs Show resolved Hide resolved
druid/src/text/editable_text.rs Show resolved Hide resolved
druid/src/text/editable_text.rs Outdated Show resolved Hide resolved
@sysint64 sysint64 force-pushed the select-none branch 2 times, most recently from 42d5381 to 2716f2c Compare September 3, 2020 08:16
@sysint64
Copy link
Contributor Author

sysint64 commented Sep 3, 2020

@cmyr fixed.

@cmyr cmyr added S-needs-review waits for review and removed S-waiting-on-author waits for changes from the submitter labels Sep 7, 2020
Copy link
Member

@cmyr cmyr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay apologies, I was a little brain dead during my previous review. There's still a naming issue here, and a larger issue with the naming of the movement types themselves. I'd like to at least get the naming correct here, and we can address the larger issue separately. Sorry for the run-around!

druid/src/text/editable_text.rs Outdated Show resolved Hide resolved
druid/src/text/movement.rs Outdated Show resolved Hide resolved
@cmyr cmyr added S-waiting-on-author waits for changes from the submitter and removed S-needs-review waits for review labels Sep 7, 2020
@sysint64
Copy link
Contributor Author

sysint64 commented Sep 7, 2020

@cmyr it looks like there is an error on the master branch located in druid/src/box_constraints.rs

@sysint64
Copy link
Contributor Author

sysint64 commented Sep 7, 2020

this is what I get after rebase:

error[E0428]: the name `tests` is defined multiple times
   --> druid/src/box_constraints.rs:375:1
    |
276 | mod tests {
    | --------- previous definition of the module `tests` here
...
375 | mod tests {
    | ^^^^^^^^^ `tests` redefined here
    |
    = note: `tests` must be defined only once in the type namespace of this module

@sysint64
Copy link
Contributor Author

sysint64 commented Sep 7, 2020

fixed here: #1187

Copy link
Member

@cmyr cmyr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@cmyr cmyr merged commit 25ffc75 into linebender:master Sep 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author waits for changes from the submitter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants