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

Introduce per message timestamps as a configurable option. #2184

Open
wants to merge 5 commits into
base: unstable
Choose a base branch
from

Conversation

ianmacd
Copy link

@ianmacd ianmacd commented Mar 2, 2022

Contributor checklist:

Description

This introduces per message timestamps as a configurable option. Enabling per message timestamps disables the display of date breaks between messages.

Confguration:

image

In action:

image

Tested on Linux and Windows 10.

@KeeJef
Copy link
Collaborator

KeeJef commented Mar 3, 2022

We should probably decide on just one of these options, either per message timestamp option or more frequent timestamp messages. I'd be in favor of just the per message timestamp option. I don't think this looks visually right either, its close to what Telegram is doing, but not sure i like how it looks visually.

@ianmacd
Copy link
Author

ianmacd commented Mar 3, 2022

We should probably decide on just one of these options, either per message timestamp option or more frequent timestamp messages. I'd be in favor of just the per message timestamp option. I don't think this looks visually right either, its close to what Telegram is doing, but not sure i like how it looks visually.

I was thinking the same thing: No need to have the timestamps both between messages and in the message bubbles.

I'm just playing with ideas here, and not all will make sense in combination with each other.

I changed the timestamp colour to color-last-seen-indicator-text earlier this evening, but didn't update the screenshot. The new colour visually separates the time from the body of the message and mutes it somewhat, the combined effect of which is quite pleasing, I feel:

image

@ianmacd ianmacd force-pushed the pr6 branch 2 times, most recently from d7f5ccb to 4a6fae3 Compare March 3, 2022 14:34
@ianmacd
Copy link
Author

ianmacd commented Mar 3, 2022

Modified to use a new definable colour, color-message-timestamp-text, because using the standard value of color-last-seen-indicator-text didn't provide enough contrast on outgoing messages.

@Bilb
Copy link
Collaborator

Bilb commented Mar 9, 2022

can you get a screenshot of what happens with this feature ON and a message without any text but just an attachment?

@ianmacd
Copy link
Author

ianmacd commented Mar 9, 2022

can you get a screenshot of what happens with this feature ON and a message without any text but just an attachment?

Ah yeah, nice catch.

This is from a note-to-self:

image

No timestamp.

@Bilb Bilb added the User Interface User interface issue label Mar 10, 2022
@ianmacd
Copy link
Author

ianmacd commented Mar 11, 2022

@blib I'm not sure how to fix the case of the timestamp being omitted from bodiless messages containing only an attachment.

This is an easy fix:

--- a/ts/components/conversation/composition/CompositionBox.tsx
+++ b/ts/components/conversation/composition/CompositionBox.tsx
@@ -841,7 +841,7 @@ class CompositionBoxInner extends React.Component<Props, State> {
       // this does not call call removeAllStagedAttachmentsInConvers
       const { attachments, previews } = await this.getFiles(linkPreview);
       this.props.sendMessage({
-        body: messagePlaintext,
+        body: messagePlaintext || '\0',
         attachments: attachments || [],
         quote: extractedQuotedMessageProps,
         preview: previews,

but because it creates a fake, non-printing body, unnecessary whitespace gets added to the message bubble.

@ianmacd
Copy link
Author

ianmacd commented Mar 11, 2022

I've added a commit (a232eea7e5f9ceb38b3fa788d37b38ff71cf7613) that ensures that attachment-only messages now get properly timestamped, and addresses the concern above by keeping the additional whitespace to the minimum possible with this approach.

image

@ianmacd ianmacd force-pushed the pr6 branch 4 times, most recently from 5ed578c to 00bc411 Compare March 11, 2022 20:12
@ianmacd
Copy link
Author

ianmacd commented Mar 11, 2022

Rebased on clearnet to make it independent of #2178, which has since been rejected, and to allow it to merge cleanly.

@ianmacd
Copy link
Author

ianmacd commented Mar 15, 2022

@Bilb OK, I think the latest commit to this makes the feature pretty good now.

With this latest change, the old-style date breaks are inserted between messages that span a date change.

This combination of punctuating messages with date break captions, plus putting a timestamp on every individual message seems ideal to me.

I hope you guys like the idea, too.

image

@Bilb Bilb added enhancement New feature or request Discussion Needs to be discussed labels Mar 28, 2022
@ianmacd ianmacd force-pushed the pr6 branch 5 times, most recently from c1b2ae0 to c94fb8b Compare May 26, 2022 11:40
Implementing timestamps at the MessageContent level ensures that all
messages receive a timestamp, regardless of whether they contain a body,
images or other types of attachment.
@ianmacd
Copy link
Author

ianmacd commented May 26, 2022

I have now rebased on HEAD and reimplemented this feature at a higher level, thereby obviating the issue of certain types of message not receiving a timestamp.

Is the feature still being considered for inclusion at this point?

The commits in this PR have been left intact to show the progression. Obviously, if this were to be included, I would squash everything down to a single commit.

@beantaco
Copy link

Looking at this screenshot, I personally like this design as it is.

However, are there any possibilities of showing the timestamp at the left or right of each message box, or would that have aesthetic or other issues? There's a fair amount of vacant space there, and having the timestamp in the lower right corner inside each message box does render the bottom of each message box void.

@ianmacd
Copy link
Author

ianmacd commented Sep 25, 2022

Looking at this screenshot, I personally like this design as it is.

However, are there any possibilities of showing the timestamp at the left or right of each message box, or would that have aesthetic or other issues? There's a fair amount of vacant space there, and having the timestamp in the lower right corner inside each message box does render the bottom of each message box void.

The code of this patch has changed somewhat over time, but perhaps hasn't been rebased here. The screenshots are certainly not up to date.

The current implementation wastes less space beneath the timestamp. Compare the messages above with this one:

image

There's now more space above the timestamp, but that can be reduced by the developers if they adopt the timestamp feature. They could also elect to offer the choice of where to place the timestamp.

Unfortunately, this PR is 6 months old now and still hasn't been merged. It remains open, however, so I find it hard to estimate its chance of inclusion at this point.

@Bilb Bilb changed the base branch from clearnet to unstable September 30, 2024 04:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Discussion Needs to be discussed enhancement New feature or request User Interface User interface issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants