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

New privuser gummys #2305

Merged
merged 10 commits into from
Dec 9, 2016
Merged

Conversation

woogerboy21
Copy link
Contributor

@woogerboy21 woogerboy21 commented Dec 8, 2016

This PR add's new gummy images based on the users privilege level. For VIP's there is now a star in front of the gummy. And for registered user that are donor's, there is a purple gummy. Sample image:

image

This change add's new user gummy icons for vip/donator priv levels that
are now available.
Corrected missing gummy in chat view.
@woogerboy21 woogerboy21 added the App - Cockatrice Tickets relating to the cockatrice application label Dec 8, 2016
@ZeldaZach
Copy link
Member

YaY for Zach!
There is something seriously wrong with the bitmap cache for the
userlevel pixmap generation
if (privLevel.size() == 0)
privkeysize = 1;

int key = height * 10000 + (int)userLevel + (int)isBuddy + privkeysize;
Copy link
Member

Choose a reason for hiding this comment

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

This can easily collide as userLevel, isBuddy, and privkeysize are all single numbers... Try changing to something like int key = height * 10000 + (int)userLevel*88 + (int)isBuddy*54 + privkeysize*31;

@@ -276,7 +276,7 @@ void UserListTWI::setUserInfo(const ServerInfo_User &_userInfo)
userInfo = _userInfo;

setData(0, Qt::UserRole, userInfo.user_level());
setIcon(0, QIcon(UserLevelPixmapGenerator::generatePixmap(12, UserLevelFlags(userInfo.user_level()), false)));
setIcon(0, QIcon(UserLevelPixmapGenerator::generatePixmap(12, UserLevelFlags(userInfo.user_level()), false, QString::fromStdString(userInfo.privlevel()))));
Copy link
Member

Choose a reason for hiding this comment

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

This won't work for offline users due to server configurations in the query ordering. I've fixed it in https://gist.github.com/ZeldaZach/d901c34d770d75618244a6489125b201

Copy link
Contributor Author

@woogerboy21 woogerboy21 Dec 8, 2016

Choose a reason for hiding this comment

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

Oh I see. That's odd.... ill have to test this out.

Copy link
Contributor Author

@woogerboy21 woogerboy21 Dec 8, 2016

Choose a reason for hiding this comment

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

It looks like the evalUserQueryResult function has a method to pass in if you want all the user data or just some of it. With the current way its written privlevel is only returned when all the data is requested. Which doesnt happen when a user is offline(?). Not entirely sure as to why this is, but there probably was a reasoning behind it at some point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@woogerboy21 woogerboy21 Dec 8, 2016

Choose a reason for hiding this comment

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

@ZeldaZach Even with this change I see the following when the user is offline (maybe i missed something in your gist):
image

Copy link
Member

Choose a reason for hiding this comment

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

The gist only re-ordered the query to have the privlevel called each time. Maybe I did something wrong. Can you manual the queries at hand and see what returns?

Copy link
Contributor

Choose a reason for hiding this comment

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

Now i understand the problem, thank you

Copy link
Contributor Author

@woogerboy21 woogerboy21 Dec 8, 2016

Choose a reason for hiding this comment

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

This should be fixed now with the most recent commit. Missed the change in the Buddy/Ignore list queries in the servatrice_database_interface and is why i didnt see the icon's in the above post.

Copy link
Member

Choose a reason for hiding this comment

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

Seems like I'm getting better at this ^ 👍

if (privLevel.size() == 0)
privkeysize = 1;

int key = height * 10000 + (int)userLevel * 88 + (int)isBuddy * 54 + privkeysize * 31;
Copy link
Contributor

Choose a reason for hiding this comment

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

These magic numbers are quite hard to understand; also, using the length of privLevel as a key will cause collisions if in the future we'll add new levels.
Maybe a better idea is to change the pmCache map to use string keys:

QMap<QString, QPixmap> UserLevelPixmapGenerator::pmCache;

and then generate the key like this:

QString key = QString::number(height * 10000 + (int) userLevel + (int) isBuddy) + privLevel;

Copy link
Member

Choose a reason for hiding this comment

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

These were magic numbers added to avoid collisions, but a QString key should work too

@@ -146,7 +146,8 @@ void ChatView::appendMessage(QString message, RoomMessageTypeFlags messageType,
(sender == QString::fromStdString(tabSupervisor->getUserInfo()->name()))) {
senderFormat.setForeground(QBrush(getCustomMentionColor()));
senderFormat.setFontWeight(QFont::Bold);
} else {
}
else {
Copy link
Contributor

Choose a reason for hiding this comment

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

@@ -155,12 +156,14 @@ void ChatView::appendMessage(QString message, RoomMessageTypeFlags messageType,
senderFormat.setAnchorHref("user://" + QString::number(userLevel) + "_" + sender);
if (sameSender) {
cursor.insertText(" ");
} else {
}
else {
Copy link
Contributor

Choose a reason for hiding this comment

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

if (!sender.isEmpty() && tabSupervisor->getUserListsTab()) {
const int pixelSize = QFontInfo(cursor.charFormat().font()).pixelSize();
QMap<QString, UserListTWI *> buddyList = tabSupervisor->getUserListsTab()->getBuddyList()->getUsers();
cursor.insertImage(UserLevelPixmapGenerator::generatePixmap(pixelSize, userLevel,
buddyList.contains(sender)).toImage());
QMap<QString, UserListTWI *> userList = tabSupervisor->getUserListsTab()->getAllUsersList()->getUsers();
Copy link
Contributor

Choose a reason for hiding this comment

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

This works fine, but it will need some refactoring in the future; right now we are retrieving data from the QGroupBox in the userlist tab, instead we should have a generic "connection" object that provides this data.
This is related to point 2 discussed in PR #2267

Copy link
Contributor Author

@woogerboy21 woogerboy21 Dec 8, 2016

Choose a reason for hiding this comment

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

As in through something like tabSupervisor or did you have another means of getting the usersInfo ? Also currently (without the changes made) the way that the chatview works is broken given what you state isnt it?

Copy link
Member

Choose a reason for hiding this comment

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

I knew this would need changes. My first goal was to make this work and from there try and figure out a more optimal solution.

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 fine with this as is until we can come up with a better solution in the future. No need to withhold this PR for it.

Copy link
Member

Choose a reason for hiding this comment

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

I think that's more or less what he was saying

woogerboy21 and others added 3 commits December 8, 2016 08:14
Corrected issue's  per request.
Updated query order for userinfo to return privlevel information
properly
@woogerboy21
Copy link
Contributor Author

Please re-test this PR and make sure its working as expected before committing

Copy link
Member

@ZeldaZach ZeldaZach left a comment

Choose a reason for hiding this comment

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

I see nothing holding this PR back now.
The pawns appear when correctly called, as I've tested. The server will need to be re-built in order for these changes to take effect.

Glad we could figure this out!!

if (!sender.isEmpty() && tabSupervisor->getUserListsTab()) {
const int pixelSize = QFontInfo(cursor.charFormat().font()).pixelSize();
QMap<QString, UserListTWI *> buddyList = tabSupervisor->getUserListsTab()->getBuddyList()->getUsers();
cursor.insertImage(UserLevelPixmapGenerator::generatePixmap(pixelSize, userLevel,
buddyList.contains(sender)).toImage());
QMap<QString, UserListTWI *> userList = tabSupervisor->getUserListsTab()->getAllUsersList()->getUsers();
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 fine with this as is until we can come up with a better solution in the future. No need to withhold this PR for it.

levelString = "normal";

if (isBuddy)
levelString.append("_buddy");

Copy link
Member

Choose a reason for hiding this comment

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

Can remove this extra space

Copy link
Member

@Daenyth Daenyth left a comment

Choose a reason for hiding this comment

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

This looks reasonable.


int key = height * 10000 + (int) userLevel + (int) isBuddy;
QString key = QString::number(height * 10000 + (int)userLevel + (int)isBuddy) + privLevel;
Copy link
Member

Choose a reason for hiding this comment

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

If userLevel can be 2 or 1, then this collides because 2 + 0 == 1 + 1

Why not just height + ":" + userLevel + ":" + isBuddy or so?

Copy link
Contributor Author

@woogerboy21 woogerboy21 Dec 9, 2016

Choose a reason for hiding this comment

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

userLevel can be 0,1,2. Wouldnt the calculation be height * 1000 +0,1,2 + 0,1 + 3,4,7 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

I agree, this should be
QString key = QString::number(height * 10000) + ":" + (int)userLevel + ":" + (int)isBuddy + ":" + privLevel;

if (!email.isEmpty())
result.set_email(email.toStdString());

const QString clientid = query->value(9).toString();
const QString clientid = query->value(10).toString();
Copy link
Member

Choose a reason for hiding this comment

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

If there's a way in the future to get value from the column name of the query result we wouldn't have to keep bumping numbers like this. Like if you could query->value("clientId"). This might already be possible, I haven't checked the docs.

No change needed for now, maybe something to look at later

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good to me

@Daenyth
Copy link
Member

Daenyth commented Dec 9, 2016

To be clear, my 'approve' is on the general approach; we still need to fix the key collision

@ZeldaZach
Copy link
Member

Once the change asked for by Dae is added I'm ready to merge this in. I left a comment with what it should look like.

@ZeldaZach ZeldaZach dismissed ctrlaltca’s stale review December 9, 2016 03:28

Reviewed by two others

changed key calculation per request and removed blank space
@ZeldaZach ZeldaZach merged commit 960ecaa into Cockatrice:master Dec 9, 2016
@tooomm tooomm mentioned this pull request Jan 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
App - Cockatrice Tickets relating to the cockatrice application
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants