Closed Bug 1378823 Opened 3 years ago Closed 2 months ago

Implement tooltips for Matrix participants

Categories

(Chat Core :: Matrix, enhancement)

enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Instantbird 74

People

(Reporter: matrixisreal, Assigned: clokep)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

Assignee: nobody → pavankarthikboddeda
Attached patch tooltip.patch (obsolete) — Splinter Review
Turns out, there is much less showable info compared to IRc.

Currently, just 3 items:
Display NAme,
Presence Status Message
Last Activity
Flags: needinfo?(fred.wang)
Flags: needinfo?(clokep)
Attachment #8883973 - Flags: review?
Attachment #8883973 - Flags: feedback+
Comment on attachment 8883973 [details] [diff] [review]
tooltip.patch

Review of attachment 8883973 [details] [diff] [review]:
-----------------------------------------------------------------

Looks like a good start! We should probably also show the level? That seems to be included in riot.im.

::: chat/locales/en-US/matrix.properties
@@ +21,5 @@
> +#    from the "User" object of that user.
> +#    The human readable name of the user.
> +tooltip.displayName= Display name
> +tooltip.presenceStatusMsg= Status message
> +tooltip.lastActiveAgo= Last activity

Please remove the space before all of these.

::: chat/protocols/matrix/matrix.js
@@ +5,5 @@
>  var {classes: Cc, interfaces: Ci, results: Cr, utils: Cu} = Components;
>  
>  Cu.import("resource:///modules/imXPCOMUtils.jsm");
>  Cu.import("resource:///modules/jsProtoHelper.jsm");
> +Cu.import("resource:///modules/imServices.jsm");

Please keep these in alphabetical order!

@@ +248,5 @@
> +      return EmptyEnumerator;
> +
> +    // Convert timespan in milli-seconds into a human-readable form.
> +    let getNormalizedTime = function(aTime) {
> +      let valuesAndUnits = DownloadUtils.convertTimeUnits(aTime/1000);

Nit: spaces on either side of /.

@@ +261,5 @@
> +
> +    if (user.displayName)
> +      tooltipInfo.push(new TooltipInfo(_("tooltip.displayName"), user.displayName));
> +
> +    // We probably need to set this as Away Message?

This needs to be set the status properly: https://dxr.mozilla.org/comm-central/source/chat/protocols/irc/irc.js#1247-1260

Is this actually just a message? Is there a concept of away vs. idle?
Attachment #8883973 - Flags: review?
Attachment #8883973 - Flags: review-
Attachment #8883973 - Flags: feedback+
Flags: needinfo?(clokep)
Flags: needinfo?(fred.wang)
Depends on: 1394397
Assignee: pavankarthikboddeda → nobody
Blocks: 1572636
Summary: Add basic Tooltip info. → Implement tooltips for Matrix participants
Attached patch Patch v2Splinter Review

This un-bitrots attachment 8883973 [details] [diff] [review] and updates it to handle a bit more of the logic for tooltips (e.g. to properly handle icons).

I tested this by connecting to Matrix and hovering over some Matrix IDs.

This doesn't work in all situations for all Matrix participants, but it is a good start and I wanted to finish up this partially done work.

Assignee: nobody → clokep
Attachment #8883973 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #9122839 - Flags: review?(paul)
Comment on attachment 9122839 [details] [diff] [review]
Patch v2

Review of attachment 9122839 [details] [diff] [review]:
-----------------------------------------------------------------

The code additions seem fine as a good first step.  I applied the patch and could see the tooltips as expected.  Good to see progress towards full matrix support.
Attachment #9122839 - Flags: review?(paul) → review+

Pushed by paul@paulwmorris.com:
https://hg.mozilla.org/comm-central/rev/fdc16e6a8006
Implement tooltips for Matrix. r=pmorris

Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → Instandbird 74
You need to log in before you can comment on or make changes to this bug.