Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add basic action command #197

Closed
wants to merge 1 commit into from
Closed

Add basic action command #197

wants to merge 1 commit into from

Conversation

arlolra
Copy link

@arlolra arlolra commented Jun 10, 2015

Rudimentary implementation of #13

Would you prefer to add an isActionRole to the model so the message can be styled a little differently?

case Qt::DisplayRole: return message.text;
case Qt::DisplayRole:
if (message.text.startsWith(QStringLiteral("/me "))) {
return QStringLiteral("* %1 %2").arg(m_contact->nickname(), message.text.mid(4, -1));
Copy link
Member

Choose a reason for hiding this comment

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

Some thoughts:

If the message is just "/msg " then the resulting message is "* nick " which is a little odd. I believe it would be better (and more friendly) if it was "nick did nothing"

Also these look like regular messages, it would be nice to do something like src/ui/LinkedText.cpp to alter the look of these messages so they stand out more (so then you could return the "/me..." string here and morph all strings that being like that on the UI end instead of here - this allows some more flexibility later on if we want to add more actions.)

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that customising text here is OK (better, even, in a lot of ways. The C++ string operations are a lot more straightforward from an operational & performance standpoint than having to do them in JS). Changing later, if it's required, is pretty straightforward.

For customizing the appearance, you could add a new role (IsActionMessageRole, maybe) using the startsWith check for the value, and on the UI side, change the text color or whatever fanciness you find appropriate.

@rburchell
Copy link
Contributor

Oh, I see you already arrived at the role conclusion here.

I personally think a role would be handy, but it's only actually useful if you also find a neat way to customize the styling. Maybe @special has an idea on how it could look.

@special
Copy link
Member

special commented Jun 16, 2015

Here's what it looks like in iMessage:

messages image 733433931

That approach could work nicely, although it adds another complex case to the message delegate.

I'm okay with this as-is. It would be neat if somebody finds a fancier way to present it eventually.

# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants