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

Already on GitHub? Sign in to your account

UserList Improvements #148

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
2 participants
Contributor

Zuko commented Mar 1, 2013

-- This is my latest and last attempt with this.

Changes:

  • Better GUI
  • Added "Search Box"
  • Added Multi-Select
  • Added "Last Seen"
  • If server does not support "last seen function" empty columns are hidden

Known bugs:

  • Date and Time are not in UTC
  • Deleting ~4000 "inactive accounts" can take a while (but this is not my fault)
  • Windows title should be changed to "User List (1337 items)"

More important, my programming skills are not that good as I wanted, so if something is wrong in this patch - simply fix it, commit and move forward - if not leave it and use old ugly User List for another two years.

Preview:
User_List_-2948-_Registered_Users_2013-03-02_08-53-24
User_List_-2948-_Registered_Users_2013-03-02_08-53-40

Owner

Kissaki commented Mar 1, 2013

Awesome!

Is the “search box” a search or a filter?

I don’t quite know how to take your closing paragraph. Are you not willing to implement suggestions, and we should implement them ourselves?

Contributor

Zuko commented Mar 1, 2013

It's a "filter", it just hides rows.
And yes, I did as much as I could, if you need to change something, please do it.

Owner

Kissaki commented Mar 1, 2013

Ok, thank you very much. :)

Owner

Kissaki commented Mar 22, 2013

This PR has previously been submitted as a patch in the patch tracker:
https://sourceforge.net/p/mumble/patches/335/

The last (unreplied, unsolved) comment from slicer states some issues that should be verified here as well, to make sure they were addressed:

This patch modifies the wording and/or whitespace of the license. It cannot be accepted as is.

While I'm not sure, I think tr() can specialcase zero of a plural form, so you don't need the logic at all. Please test this?

Adding 'days ago' to every single entry seems a bit excessive, especially since they are all identical. Either make the column simply be "Inactive Days" (or some shorter formulation) or do the full days/weeks/months/years analysis (which means custom sort order to maintain the relationships).

Dates need to be ISO format and in UTC.

This will break on servers that have third-party authentication. Either modify Server::getRegisteredUsers to return what you need, or at the very least base your returned data on the output from Server::getRegisteredUsers.

Channel IDs are not unsigned, please update your proto definiton.

If the server does not support the updated info, the columns should be removed. Since the data is either always there, or never there, simply check whether or not any of the received entries have last on fields.

I'm not sure where the UserInfo struct belongs, but I don't think Net.h is the right place. It's probably better to put it in User.h (and add a comment to it). Also, please use QDateTime for the storage of a in-memory date instead of a string.

I understand that 'last channel' is what you use on your server, and that a highly hierarchical structure with a high correlation between social group association and channel structure makes this handy. However, this is not universally true for all servers, and hence will feel rather strange on a server that does not use strong channel hierarchies, but instead uses e.g. strong ACL group hierarchies. Please see if there is a method of differentiation that is slightly less instance-specific.

Owner

Kissaki commented Mar 22, 2013

For reference: Currently, on 1.2.4, the dialog looks like this:
Registered Users_2013-03-22_20-28-43

Contributor

Zuko commented Mar 22, 2013

I tried to fix all the things mentioned by slicer.
I didn't know how how to "fix" dates to "UTC" and " third-party authentication", the rest is "fixed/corrected" (I hope :) )

Contributor

Zuko commented Jun 10, 2013

ehm, any progress? :)

Owner

Kissaki commented Jun 23, 2013

Thank you @bendem for looking at the code and providing some feedback!

Based on this pull request I created a pr-userlist branch, and fixed the indentation issues, and changed the string.

Owner

Kissaki commented Jun 23, 2013

When I launch compiled murmur and mumble I somehow don’t see the two new columns.

@Kissaki Kissaki commented on the diff Jun 23, 2013

src/murmur/ServerDB.cpp
+ SQLPREP("SELECT `user_id`, `name`, `lastchannel`, `last_active` FROM `%1users` WHERE `server_id` = ?");
+ query.addBindValue(iServerNum);
+ SQLEXEC();
+
+ while (query.next()) {
+ UserInfo userinfo;
+ userinfo.user_id = query.value(0).toInt();
+ userinfo.name = query.value(1).toString();
+ userinfo.last_channel = query.value(2).toInt();
+ userinfo.last_active = query.value(3).toString();
+
+ m << userinfo;
+ }
+
+ return m;
+}
@Kissaki

Kissaki Jun 23, 2013

Owner

The implementation of getRegisteredUsersEx() is currently missing the signal for RPCs (dbus, ice, rpc). See getRegisteredUsers() emit getRegisteredUsersSig(filter, m);.

Owner

Kissaki commented Jun 23, 2013

Going over slicers comments:

This patch modifies the wording and/or whitespace of the license. It cannot be accepted as is.

This has been fixed in this PR. No license text is touched.

Adding 'days ago' to every single entry seems a bit excessive, especially since they are all identical. Either make the column simply be "Inactive Days" (or some shorter formulation) or do the full days/weeks/months/years analysis (which means custom sort order to maintain the relationships).

From the screenshot, it seems to have been fixed for this PR. I can not verify currently as my build does not show these columns when there are registrations.

Dates need to be ISO format and in UTC.

I don’t see what this refers to!? I do not see any dates, just time-spans (“days”).

This will break on servers that have third-party authentication. Either modify Server::getRegisteredUsers to return what you need, or at the very least base your returned data on the output from Server::getRegisteredUsers.

The missing RPC thing I mentioned in an earlier comment.

Channel IDs are not unsigned, please update your proto definiton.

Not sure about this. In the Mumble.proto I only see uint32s for channel ids. So why would it be different here? Did this change?

While I'm not sure, I think tr() can specialcase zero of a plural form, so you don't need the logic at all. Please test this?

Probably refers to setWindowTitle(tr("User List - %n - Registered Users", "", n));, which was not fixed.
https://github.com/mumble-voip/mumble/pull/148/files#L2R46

If the server does not support the updated info, the columns should be removed. Since the data is either always there, or never there, simply check whether or not any of the received entries have last on fields.

Could not verify.

I'm not sure where the UserInfo struct belongs, but I don't think Net.h is the right place. It's probably better to put it in User.h (and add a comment to it). Also, please use QDateTime for the storage of a in-memory date instead of a string.

Was moved to User.h.
Comment should be changed to a more general one.
Dates are still strings, which should be changed.

I understand that 'last channel' is what you use on your server, and that a highly hierarchical structure with a high correlation between social group association and channel structure makes this handy. However, this is not universally true for all servers, and hence will feel rather strange on a server that does not use strong channel hierarchies, but instead uses e.g. strong ACL group hierarchies. Please see if there is a method of differentiation that is slightly less instance-specific.

Should be evaluated and discussed still.

Contributor

Zuko commented Jun 23, 2013

Don't look at old slicers comment, it's not valid after 2 years.

Owner

Kissaki commented Jun 23, 2013

As I pointed out it is.
You can’t say it’s invalid without evaluating it.

Contributor

Zuko commented Jun 23, 2013

nvm then, do it your way. I just want to see this in official release. (soon) btw. dunno why it's not working on your build.

Owner

Kissaki commented Jun 23, 2013

I noticed an nmake did not generate the Mumble.pb.h from the proto file. Removing it it was created with the new fields.
But apparently that did not fix it yet. Issue for another day.

@ghost ghost assigned Kissaki Jun 28, 2013

@Kissaki Kissaki commented on the diff Jun 28, 2013

src/mumble/UserEdit.cpp
+void UserEdit::refreshUserList(int inactive) {
+ qtwUserList->clear();
+ QMapIterator<int, UserInfo> i(qmUsers);
+
+ while (i.hasNext()) {
+ i.next();
+ UserEditListItem *ueli = new UserEditListItem(i.key());
+ ueli->setText(0, i.value().name);
+
+ QString last_active;
+ QDateTime qdtLastActive;
+ int last_seen;
+ if (!i.value().last_active.isEmpty()) {
+ qdtLastActive = QDateTime::fromString(i.value().last_active, QLatin1String("yyyy-MM-dd hh:mm:ss"));
+ if (!qdtLastActive.isValid())
+ qdtLastActive = QDateTime::fromString(i.value().last_active, QLatin1String("yyyy-MM-ddThh:mm:ss"));
@Kissaki

Kissaki Jun 28, 2013

Owner

Why do we have two parse-formats here?
Shouldn't the format we save to into the DB always be the same?

@Zuko

Zuko Jun 28, 2013

Contributor

Don't ask me ;)
It's different on Win / Linux, check yourself.

@Kissaki

Kissaki Jun 28, 2013

Owner

I switched to the appropriate Qt QDateTime and QDateFormats Qt::ISO<...>, which works on windows as well - where sqlitebrowser indeed shows me the date without a T.
As it works in this off-case which Qt does not explicitly mention/document (as it's not valid ISO) it should work in any case.

Owner

Kissaki commented Jun 28, 2013

The time displayed as a tooltip is in UTC and ISO.
Still not sure this is what slicer meant.

I implemented the RPC callback.

The plural form has been added to the window title - untested.
reference

Fixed datetime to be displayed as local time - after being transmitted as UTC.

I noticed one can edit the inactive days and last channel columns. This should be disabled.
Looking at QTreeWidget, it seems I can not disable columns or cells.
See Stackoverflow: Making only one column of a QTreeWidgetItem editable

So the remaining issues are:

  • Handle datetime as QDateTime in UserInfo

And the nonblockers:

  • Columns two and three are editable, which they should not be (effectively do not set/save anything).
  • Test the plural form in the title
  • Discuss what slicer meant with channel id should not be unsigned
  • Discuss channel hierarchy display

Kissaki added a commit that referenced this pull request Jun 28, 2013

Merge branch 'pr-userlist'
Integrate feature branch for filtered and improved registration list.
This merges pull request #148.
Owner

Kissaki commented Jun 28, 2013

Fixed: * Handle datetime as QDateTime in UserInfo

The feature branch with these changes has been merged into master.

Points open for discussion, which should be handled in further and individual issue tickets:

  • Columns two and three are editable, which they should not be (effectively do not set/save anything).
  • Test the plural form in the title
  • Discuss what slicer meant with channel id should not be unsigned
  • Discuss channel hierarchy display

Again, thank you very much Zuko for your work!

@Kissaki Kissaki closed this Jun 28, 2013

Contributor

Zuko commented Jun 29, 2013

thanks, it's working ;D

2013-06-29_15-39-12

Contributor

Zuko commented Jun 29, 2013

"* Discuss channel hierarchy display"

There is nothing to discuss right now, slicer had doubts about this: (red rectangle) but I removed this functionality.

2013-06-29_16-02-18

Kissaki, thanks for your work ;)

Owner

Kissaki commented Jun 29, 2013

Ah, ok. Thank you for clarifying.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment