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

Jump to bottom

some fixes for multiple-mode textinput #4273

Closed
wants to merge 1 commit into from
Closed

some fixes for multiple-mode textinput #4273

wants to merge 1 commit into from

Conversation

ajnirp
Copy link
Contributor

@ajnirp ajnirp commented Dec 7, 2014

@hoppipolla-critic-bot
Copy link

Critic review: https://critic.hoppipolla.co.uk/r/3411

This is an external review system which you may optionally use for the code review of your pull request.

In order to help critic track your changes, please do not make in-place history rewrites (e.g. via git rebase -i or git commit --amend) when updating this pull request.

@Manishearth Manishearth added the S-awaiting-review There is new code that needs to be reviewed. label Dec 8, 2014
@jdm
Copy link
Member

jdm commented Dec 8, 2014

Can this be rebased on top of #4267?

@jgraham
Copy link
Contributor

jgraham commented Dec 8, 2014

Trying to move and squash at the same time doesn't interact too well with critic. Please just move the review without doing any squashing (you can get the old head of the branch from git reflog if you need).

@Manishearth Manishearth added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. S-needs-squash Some (or all) of the commits in the PR should be combined. and removed S-awaiting-review There is new code that needs to be reviewed. S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Dec 8, 2014
@Manishearth
Copy link
Member

Note: This time feel free to rebase+squash indiscriminately, it's already passed review :)

@jdm jdm added S-needs-rebase There are merge conflict errors. and removed S-needs-squash Some (or all) of the commits in the PR should be combined. labels Dec 9, 2014
@ajnirp
Copy link
Contributor Author

ajnirp commented Dec 9, 2014

I think I messed up the merge, because there were a ton of bugs after I rebase --continue'd. Fixed them all, though.

@jdm jdm added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-rebase There are merge conflict errors. labels Dec 9, 2014
jdm
jdm reviewed Dec 9, 2014
View reviewed changes
components/script/textinput.rs
self.edit_point.index = 0;
self.adjust_vertical(1, select);
self.adjust_horizontal(adjust - remaining as int, select);
if self.multiline {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure that this re-introduction of the multiline check is needed. Can't we just add the -1 bit to the adjust_horizontal call instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Amended and pushed.

@jdm jdm added S-awaiting-answer Someone asked a question that requires an answer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Dec 9, 2014
jdm
jdm reviewed Dec 10, 2014
View reviewed changes
components/script/textinput.rs
self.edit_point.index = 0;
self.adjust_vertical(1, select);
self.adjust_horizontal(adjust - remaining as int, select);
let not_last_line: bool = self.lines.len() > self.edit_point.line + 1;
Copy link
Member

Choose a reason for hiding this comment

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

Let's move this inside the if branch, as that's the only place it's used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

bors-servo pushed a commit that referenced this pull request Dec 10, 2014
auto merge of #4273 : wenderen/servo/multiple-line-textintput, r=jdm
3e94c4d 
#4258
@jdm
Copy link
Member

jdm commented Dec 11, 2014

Tidy failure: components/script/textinput.rs:79: tab on line

@jdm jdm added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-answer Someone asked a question that requires an answer. labels Dec 11, 2014
@ajnirp
Copy link
Contributor Author

ajnirp commented Dec 11, 2014

Cleaned

@jdm jdm added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Dec 11, 2014
bors-servo pushed a commit that referenced this pull request Dec 11, 2014
auto merge of #4273 : wenderen/servo/multiple-line-textintput, r=Mani…
7ce3763 
…shearth

#4258
@bors-servo bors-servo closed this Dec 11, 2014
@ajnirp ajnirp deleted the multiple-line-textintput branch December 12, 2014 18:53
@mttr mttr mentioned this pull request Dec 15, 2014
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@ajnirp @hoppipolla-critic-bot @jdm @jgraham @Manishearth @bors-servo