Skip to content

Implementing Stephens comments from #966#972

Merged
vadi2 merged 4 commits intoMudlet:developmentfrom
vadi2:address-966-comments
Apr 28, 2017
Merged

Implementing Stephens comments from #966#972
vadi2 merged 4 commits intoMudlet:developmentfrom
vadi2:address-966-comments

Conversation

@vadi2
Copy link
Copy Markdown
Member

@vadi2 vadi2 commented Apr 27, 2017

Implementing Stephens review from #966.

@SlySven keep reviews on topic please! Will not be doing this again.

Tagging @Mudlet/core-cpp for review.

@vadi2 vadi2 self-assigned this Apr 27, 2017
@vadi2 vadi2 requested review from SlySven and ahmedcharles April 27, 2017 14:26
if( ke->modifiers() & Qt::ControlModifier )
{
moveCursor(QTextCursor::Up, QTextCursor::MoveAnchor );
ke->accept();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This ought to go outside the if(...) {...} as well. 😁

, mpHost( pHost )
, mpConsole( pConsole )
, mSelectedText( "" )
, mSelectedText( QString() )
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This already is a QString so it can just be mSelectedText().

ke->accept();
return true;
}
break;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You could move the return true; and ke->accept(); here.

return true;
}
break;
return true;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can also move ke->accept().

@vadi2 vadi2 merged commit 2731fec into Mudlet:development Apr 28, 2017
@vadi2 vadi2 deleted the address-966-comments branch April 28, 2017 06:13
vadi2 added a commit to vadi2/Mudlet that referenced this pull request Apr 28, 2017
They got lost in the merge conflicts that occured in Mudlet#972.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants