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

Feature/display message state #33

Merged
merged 4 commits into from
Jul 30, 2021
Merged

Conversation

ericthegrey
Copy link
Collaborator

@ericthegrey ericthegrey commented Jul 12, 2021

This PR adds more information to each message:

  • Whether the message was 'secure' or not (in which cas an open padlock is shown)
  • Whether the message was sent (one checkmark), delivered (two) or read (three)
  • For group messages, how many delivery and how many read receipts

I also added code to display failed messages and in-progress messages but I've not seen them in any backup.

This PR is over the bugfix/rid_not_found branch so that should be committed first. I think with this code, we're now showing messages as meaningfully as the native client.

In addition, the version is now logged on startup to help troubleshooting.

Copy link
Owner

@GjjvdBurg GjjvdBurg left a comment

Choose a reason for hiding this comment

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

Thanks for this PR @ericthegrey! I think it's nice to show the checkmarks and match the Signal UI, but I'm not sure about the display count on the group messages. I find it slightly distracting and I'm not sure how accurate it is (I have one group with 6 people including myself, and there's a message with a count of 7). What would you think about showing this info on hover, or making it optional using a command line flag?

@GjjvdBurg
Copy link
Owner

As an aside, I noticed that the time and checkmarks on my messages are left-aligned instead of right-aligned as it is in the app. I can take a look at that once we sort this PR out.

@ericthegrey
Copy link
Collaborator Author

I find it slightly distracting and I'm not sure how accurate it is (I have one group with 6 people including myself, and there's a message with a count of 7).

I suppose this comes from some people having multiple linked devices, but I'm not sure. Well, since the usefulness is limited, I will remove it. At some point we can have a hover of all group members who have been sent/been delivered/read the message.

What would you think about showing this info on hover, or making it optional using a command line flag?

@GjjvdBurg
Copy link
Owner

Okay, thanks! This looks good to me so can be merged when you're ready 👍

Just out of curiosity I was playing around with the double checkmarks, and I found that there is a letter-spacing html property that we could use to reduce the spacing between them. It could work like this:

diff --git a/signal2html/templates/thread.html b/signal2html/templates/thread.html
index 375e3d5..3dfada1 100644
--- a/signal2html/templates/thread.html
+++ b/signal2html/templates/thread.html
@@ -34,7 +34,7 @@
   {% elif state == "DISPLAY_TYPE_FAILED" %}
     ⚠{# Warning sign #}
   {% elif state == "DISPLAY_TYPE_DELIVERED" %}
-    ✓✓{# Double checkmark #}
+    <span class="double-checkmarks">&#x2713;&#x2713;</span>{# Double checkmark #}
   {% elif state == "DISPLAY_TYPE_READ" %}
     &#x2713;&#x2713;&#x2713;{# Triple checkmark #}
   {% endif%}
@@ -144,6 +144,10 @@
         width: 400px;
       }
 
+      .double-checkmarks {
+        letter-spacing: -0.3em;
+      }
+
       .msg-img-container input[type=checkbox] {
         display: none;
       }

Would that be a nice addition you think? Feel free to merge this PR as you see fit!

@ericthegrey
Copy link
Collaborator Author

Just out of curiosity I was playing around with the double checkmarks, and I found that there is a letter-spacing html property that we could use to reduce the spacing between them. It could work like this:

[...]

Would that be a nice addition you think? Feel free to merge this PR as you see fit!

Hi @GjjvdBurg !

I tried it on Firefox and the result appears to chop the checkmarks:

image-multi-checkmarks

So I added some margins as well. (You'd think adding it on the right side should suffice, but no, left side is decisive.)

@ericthegrey ericthegrey merged commit 208714b into master Jul 30, 2021
@GjjvdBurg
Copy link
Owner

Thanks for merging this btw! Nice fix on the double checkmarks, I think it looks great now :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants