Add support for moving multiple users/channels

[austinliou]

The ability to move multiple users/channels at once has been a requested feature for several years now (e.g. #1829). This is a simple 5-line change that extends the drag-and-drop movement by allowing multiple selections (using ctrl, shift, or mouse drag).

This will NOT do other bulk actions (mute, PM, kick/ban, etc), because I think that's a whole different can of worms based on how controls are implemented.

I've tested this in private builds for personal use for about a year now, and the basic functionality works fine, but there may be some undefined or unwanted behaviors:

• Right-click context menu after a multi-select will clear selections
• Log will still show one message per user/channel moved, which can kind of clutter up the log window
• Multiple channel movement will show a prompt for every channel in selection (which can get annoying), and will not show channel name in prompt ("Are you sure you want to drag this channel?")
• User and Channel context menus do not support multiple selections. In fact, I think they reflect the user/channel most recently added to selection (or most recently serialized to QDataStream)
• Technically allows both users and channels to be selected and moved together, but who knows what spooky things might happen because of that?

I'm not particularly well versed in QT, so if there's something that I've broken or implemented poorly with this change, please let me know.

This is not meant to be a comprehensive feature addition, but hopefully it's a start and will better allow channel operators to better deal with large groups of users.

Fixes #1829

[Tarun80]

This would be so very helpful. Thanks for submitting this, I hope it can be added. :)

[Natenom]

Technically allows both users and channels to be selected and moved together, but who knows what spooky things might happen because of that?

When you drag more than one channel at once and one has users in it, they all land in the channel you dragged all into, not in the subchannel they were in.

Maybe you could move the channels first and then the users?

[austinliou]

@Natenom
I think that would happen if you also selected the users in the channel, in addition to the channel itself.
It might make more sense to consider this change as simply doing the following behaviors:

• Allow multiple selections
• For each element in your selection, perform a drag-move to the target channel, as if you had moved that element by itself.

In that sense, if you shift-select channel Bravo AND its users, and you dragged them to channel Alpha, the users would not remain in channel Bravo but would be directly in channel Alpha. So to preserve the channel hierarchy, you'd have to move ONLY channel Bravo into channel Alpha without its users selected.

I agree it might not seem intuitive, but like I said: undefined behaviors :)

[mkrautz]

From the code changes alone, it'd be easy to merge this as-is.

But I worry that the behavior when dragging channels is too inconsistent.

It'd be nice if there were a way to only allow multiple selections of users, as I think that is what most people would use the feature for...

[mkrautz]

Here's the whitespace insensitive diff, BTW:

austinliou@445eaf1?w=1

[austinliou]

@mkrautz Thanks, much more readable, lol.

As I understand it, because users and channels are effectively the same UI element, they can't be completely decoupled from each other. Although, I think it might be possible to ignore channel selections in a multiselection by playing around with the isChannel logic. I'll play around with this later and see if I can't limit this to only user selections.

[austinliou]

As requested, austinliou@791dfe7 will only move users when more than one item is selected, and channels will be ignored.

This will still allow the actual selection of channels (probably would require a designer UI change otherwise), but any selected channels will not be moved. Individual channels can still be moved with single-select.

I'm assuming that User::uiSession (uint) and Channel::iId (int) are the same width in bytes for now, but I think it would be easy to fix if the datatypes ever change.

Still no guarantees regarding the User/Channel context-menu behavior!

[Natenom]

Compiling worked the last time but now it fails with:
UserModel.cpp: In member function ‘virtual bool UserModel::dropMimeData(const QMimeData*, Qt::DropAction, int, int, const QModelIndex&)’:

UserModel.cpp:1375:34: error: comparison between signed and unsigned integer expressions [-Werror=sign-compare] bool isMultiSelect = qba.size() > (sizeof(bool) + sizeof( ((ClientUser*)0)->uiSession) );

[hacst]

We now have warnings as errors enabled in trunk. That's the reason it fails to build now. A simple static_cast will fix that.

Btw. while I guess the sizeof pointer magic is valid and harmless (only types) I would much prefer sizeof(ClientUser::uiSession) there ;)

[austinliou]

I'll fix this later tonight. QByteArray::size() returns int, whereas sizeof returns size_t (unsigned).
Did I miss a commit where -Werror=sign-compare was enabled?

Also, I believe sizeof(ClientUser::uiSession) will fail to compile in all but C++11, because the operand is invalid (even though the operand isn't evaluated). Hence, the workaround! You could also do sizeof(ClientUser().uiSession) if the default constructor exists I think, if that looks cleaner to you.

[hacst]

@austinliou Interesting. You are totally correct about the sizeof. I just couldn't remember that ever not working. I blame gcc (and probably vc) for not being standards compliant enough ;) Your code uses a common idiom (I just didn't know about) so it's fine.

[mkrautz]

Sorry for letting this sit. I have not yet tried to patch this into my client.

I am a bit wary of the unintended side effects of the change, but I will try it out...

[mkrautz]

I wonder if we could tweak the selection logic such that you cannot multi-select items that you cannot move together.

Do you think it's possible?

[Natenom]

When focusing the chat bar while having multiple clients selected the selection should be changed to the last user only.

[austinliou]

@mkrautz It might be possible, but I'm not as familiar with the codebase or QT as you. However, my intuition is that maybe UserView could be subclassed to decouple users and channels, at least in UI. I'll play around with it later.

@Natenom Does "last" mean "nearest the bottom" or "most recently selected and added"? What sorts of behavior should be impacted? For example, I'm assuming that this would change which user is targeted by the User context menu in both the menu bar and right click.

[Natenom]

I mean the "most recently selected".

[austinliou]

@mkrautz I compromised and only tweaked the logic such that it will deselect any channels in a multiselection. The reasoning being, if the user creates a selection that has both channels and users, it would seem unintuitive if the entire selection is just cleared out (or unresponsive) because mixed selections are not allowed. But this means you can still do something like Ctrl-A to select all visible users on the server.

@Natenom I think this behavior is not so trivial to implement, because the "most recently selected" user it falls back to may not necessarily be the most intuitive or expected one. I will think about this a bit more, and get back to you.

[mkrautz]

Awesome work, I will try it out later today.

[mkrautz]

@austinliou

Can you explain the shift-deselect issue you describe in the latest commit message?

Also, is it possible if we could have an email for you in the commit messages? (I can amend the email in for you when landing, don't worry). If you write it here, or email me with it, I can put it in there.

Other than this, I think this is now Good Enough to land. It works really well. Thanks! :-)

[austinliou]

@mkrautz

Thanks, I will send you an email.

To explain more clearly: when you shift-select only users, they will become selected and deselected as expected. When your shift-selection includes a channel, you can no longer shift-click to deselect users.

For example, suppose you have 3 channels: C1, C2, and C3. Only channel C2 is populated with 10 users (U1 to U10).

Normal behavior

1. Click U5 -> User 5 is selected
2. Shift-click U8 -> Users 5 to 8 are selected.
3. Shift-click U3 -> Users 6 to 8 are deselected, and users 3 to 5 are selected.

Unexpected behavior:

1. Click U5 -> User 5 is selected
2. Shift-click C2 -> Users 5 to 10 are selected
3. Shift-click U3 -> Users 3 to 10 are selected, but we expect only users 3 to 5 to be selected.

I don't think it's a critical issue, especially since you can still deselect with ctrl-click. For any other weird UI-specific issues (e.g. if a channel manages to sneak into your selection somehow), austinliou@791dfe7 should still ignore channels when actually trying to drag and drop. I think I was able to sneak a channel into the selection once with ctrl-a, but I haven't tried to reproduce since.

If this issue becomes more than a slight inconvenience, I can do some deeper digging to try to fix it, but for now I'm looking into implementing the correct chatbar focus behavior.

[mkrautz]

It seems to me that the chat bar focus thing works as expected. It did when I tested it. The most recently selected user was the target. But if you target yourself, you target the channel.

[austinliou]

Agreed. I think the undefined behavior is: what happens when you select multiple users by shift-selecting or ending your selection on a channel?

Currently, the chatbar focus will remain on whatever you last clicked or focused, i.e., a channel, even if the channel doesn't become part of the selection.

I think the most intuitive behavior would be:

• If the user ctrl-clicks on a channel to add to the selection, ignore it and re-focus on the previously focused user.
• If the user shift-clicks on a channel to add users to the selection, "snap" backwards and focus the closest user just added to the selection.

This might be tricky to implement because indexes in selections aren't guaranteed to be ordered, and have relative rather than absolute row numbers.

Although, this might all become irrelevant if the chatbar focus instead allowed you to PM multiple users at once :)

[Tarun80]

This is absolutely amazing and would be beneficial in many ways. Thank you for this, @austinliou!

[Tarun80]

Any word on this being implemented?

[austinliou]

@Tarun80 Sorry, I've been busy and have not touched this in a while, and I have not made any progress in implementing chatbar focus behavior (from #1871 (comment)).

That said, the basic multiselect (for drag-and-drop purposes) still works as expected. So I think it's a question of whether or not the chatbar focus behavior should be additionally defined and implemented before merging? I'm open to suggestions.

[Tarun80]

If the log issue for moving multiple users was not resolved perhaps a log message like, "Users <name1, name 2, name 3, etc.> moved to by <Admin/User>."

Chatbar focus either Selected Users or Target Channel (if in one channel). Selected Users would be ideal, in my opinion. An option in the Mumble settings for UI might work well too?

[Krzmbrzl]

What's the status of this PR? From the comments it seems as if there had been some things that weren't clear as to how they should be handled...