Skip to content

Subset of clang-format changes.#966

Merged
ahmedcharles merged 1 commit intoMudlet:developmentfrom
ahmedcharles:f
Apr 27, 2017
Merged

Subset of clang-format changes.#966
ahmedcharles merged 1 commit intoMudlet:developmentfrom
ahmedcharles:f

Conversation

@ahmedcharles
Copy link
Copy Markdown
Contributor

No description provided.

Copy link
Copy Markdown
Member

@SlySven SlySven left a comment

Choose a reason for hiding this comment

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

Not all of my 24 comments actually need action!

: QPlainTextEdit(parent)
, mpHost(pHost)
, mpConsole(pConsole)
, mSelectedText("")
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.

Isn't the "" surplus to requirements here - and if it is a QString this is not ideal either {will trip up on future #define QT_NO_CAST_{TO|FROM}_ASCII}... ☹️

QString spell_aff = path + pHost->mSpellDic + ".aff";
QString spell_dic = path + pHost->mSpellDic + ".dic";
mpHunspell = Hunspell_create( spell_aff.toLatin1().data(), spell_dic.toLatin1().data() );//"en_US.aff", "en_US.dic");
mpHunspell = Hunspell_create(spell_aff.toLatin1().data(), spell_dic.toLatin1().data()); //"en_US.aff", "en_US.dic");
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.

We can lose the comments I think as well?

mRegularPalette.setColor(QPalette::Base,mpHost->mCommandLineBgColor);//QColor(255,255,225));
mRegularPalette.setColor(QPalette::Text, mpHost->mCommandLineFgColor); //QColor(0,0,192));
mRegularPalette.setColor(QPalette::Highlight, QColor(0, 0, 192));
mRegularPalette.setColor(QPalette::HighlightedText, QColor(255, 255, 255));
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.

I suspect this is Qt::white isn't it - which is easier on a (human) parser... 😀

handleTabCompletion( true );
ke->accept();
return true;
break;
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.

Unreachable...!

}
ke->accept();
return true;
break;
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.

Unreachable...!

}
if( toPlainText().size() > 0 )
{
if (toPlainText().size() > 0) {
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.

or:
if (! toPlainText().isEmpty()) {
I suppose?

void TCommandLine::slot_sendCommand(const char* pS)
{
mpHost->sendRaw( QString(pS) );
mpHost->sendRaw(QString(pS));
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.

🌍 Might this be Latin1 specific and need tweaking for I18n?


buffer.replace(QChar( 0x21af ), "\n");
buffer.replace(QChar('\n'), " " );
buffer.replace(QChar(0x21af), "\n");
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.

🌍 That appear to be the '↯' codepoint {U+21af DOWNWARDS ZIGZAG ARROW in mathematical symbols, arrows}. Is this going to be tolerable if that character CAN appear in the data stream in future versions of Mudlet.

|| pH->mUrl.contains(QStringLiteral("aetolia.com"), Qt::CaseInsensitive)
|| pH->mUrl.contains(QStringLiteral("imperian.com"), Qt::CaseInsensitive)
|| pH->mUrl.contains(QStringLiteral("lusternia.com"), Qt::CaseInsensitive)) {
downloadMapOptions->setVisible(true);
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.

Is this positioning now compulsory - I'd still like a line break before this to push it away from the conditions - helps my 👀 as they are getting older...!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

clang-format doesn't add blank lines to my knowledge. Is there a specific change you wanted to the .clang-format?

pushButton_background_color->setStyleSheet( QStringLiteral( "QPushButton{background-color: %1;}" ).arg( mpHost->mBgColor.name() ) );
pushButton_command_foreground_color->setStyleSheet( QStringLiteral( "QPushButton{background-color: %1;}" ).arg( mpHost->mCommandFgColor.name() ) );
pushButton_command_background_color->setStyleSheet( QStringLiteral( "QPushButton{background-color: %1;}" ).arg( mpHost->mCommandBgColor.name() ) );
pushButton_command_line_foreground_color->setStyleSheet(QStringLiteral("QPushButton{background-color: %1;}").arg(mpHost->mCommandLineFgColor.name()));
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.

Pity about the loss of the indentation to line up on the assignment but dem's da breaks I guess...

@ahmedcharles
Copy link
Copy Markdown
Contributor Author

You know, this whole clang-format thing isn't going to finish if the PR's keep getting blocked by non-formatting issues. And I'm also not comfortable changing non-formatting because that may break something that currently works. The fact that I add {}s is enough to change behavior and I've actually inserted them wrong and caught myself twice.

Copy link
Copy Markdown
Member

@vadi2 vadi2 left a comment

Choose a reason for hiding this comment

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

I'm fine with the formatting changes

@SlySven I'll apply your review comments in another PR focused around code quality improvements. This one is just about the formatting (I'm sure you're onto the idea now after #965 😉)

vadi2 added a commit to vadi2/Mudlet that referenced this pull request Apr 27, 2017
vadi2 added a commit to vadi2/Mudlet that referenced this pull request Apr 27, 2017
@ahmedcharles ahmedcharles merged commit 037368f into Mudlet:development Apr 27, 2017
@ahmedcharles ahmedcharles deleted the f branch April 27, 2017 21:00
vadi2 added a commit that referenced this pull request Apr 28, 2017
* Implementing Stephens comments from #966
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