FEAT(client): Add "View Description" context action to channels #6526
Conversation
|
Hi, thanks for working on this :) I have not reviewed your changes yet, but since this is adding and/or modifying existing UI please take a look at this document/checklist https://github.com/mumble-voip/mumble/blob/master/docs/dev/Accessibility.md |
Hartmnt
added
client
ui
feature-request
|
The UI change is just a new context menu item on channels which works the same way as the existing "View Comment" menu action on users, so that should be fine. I haven't set up translations for the new text yet. |
dexgs
force-pushed
the
master
branch
3 times, most recently
from
73c4763 to
2d6974a
Compare
|
I've used a function pointer rather than the SLOT macro and updated the translation file to contain the new UI text. |
To update the translation files, please call the |
|
Okay, PR is no longer a draft. I think it's ready to merge unless there are any more corrections. |
|
@dexgs No, not from your point of view :) We struggle a little with time and CI stuff which has delayed some things. But this PR is still on the TODO list. |
|
@dexgs Please rebase this against |
Previously, channel descriptions could only be viewed as a pop-up which does not allow text selection/copying and doesn't allow you to scroll through descriptions to large to fit on your screen. User comments already support this use case via the "View Comment" context menu action on users which opens their comment in a separate window. This commit adds a "View Description" context menu action to channels which works the same way as the "View Comment" action.
done |
|
|
||
| if (!c) | ||
| return; | ||
|
|
There was a problem hiding this comment.
| if (!c) | |
| return; | |
| if (!c) { | |
| return; | |
| } | |
| } else if (c->iId == static_cast< unsigned int >(~iChannelDescription)) { | ||
| iChannelDescription = -1; |
There was a problem hiding this comment.
The whole casting here looks somewhat fishy. It is probably working as expected (and I realize you replicated the same mechanism as for the user comment).
I think this might theoretically break when reaching like 2 billion channels, which would be fine. But have considered edge cases here? root has channel id 0 which would be ~0 == -1 when considering signed integers.
time passes
Haha, ok guess what. This indeed breaks for root. I just tried it. If you set the description of the root channel, a message will popup every time the content is received from the server. (e.g. when reconnecting).
Sadly as of posting this I have no solid idea how to fix this.
There was a problem hiding this comment.
The best thing I can think of without refactoring receiving descriptions/comments from the server is to add an extra field to UserModel which is an enum indicating what the received description is for (popup vs. "view description" with the flexibility to support more cases in the future)
If this is acceptable, I can try implementing it soon
There was a problem hiding this comment.
the problem doesn't exist in ClientUser since users never get session id 0, but using the negation of the IDs to differentiate between popup and the "view description/comment" option is hacky in both cases
There was a problem hiding this comment.
The extra field as an enum sounds good to me.
There was a problem hiding this comment.
I am all for switching to a "proper" implementation instead of negating ids. I do not think anything will break if you remove the existing negation logic.
| if (!c->qbaDescHash.isEmpty() && c->qsDesc.isEmpty()) { | ||
| c->qsDesc = QString::fromUtf8(Global::get().db->blob(c->qbaDescHash)); | ||
| if (c->qsDesc.isEmpty()) { | ||
| pmModel->iChannelDescription = ~static_cast< int >(c->iId); |
There was a problem hiding this comment.
Just for the record, I think here the same issue with the root channel applies that Hartmnt has found elsewhere already.
Previously, channel descriptions could only be viewed as a pop-up which does not allow text selection/copying and doesn't allow you to scroll through descriptions to large to fit on your screen.
User comments already support this use case via the "View Comment" context menu action on users which opens their comment in a separate window.
This commit adds a "View Description" context menu action to channels which works the same way as the "View Comment" action.
Fixes #6525
Checks