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

Add message type for correction #685

Merged
merged 1 commit into from
Jan 15, 2018
Merged

Add message type for correction #685

merged 1 commit into from
Jan 15, 2018

Conversation

Diadlo
Copy link

@Diadlo Diadlo commented Jan 14, 2018

This change is Reviewable

@iphydf iphydf added this to the v0.2.0 milestone Jan 15, 2018
@Diadlo
Copy link
Author

Diadlo commented Jan 15, 2018

Does somebody know, why Messager.h defines its own message types instead of incude and using defines in tox.h?

@iphydf
Copy link
Member

iphydf commented Jan 15, 2018

:lgtm_strong:


Reviewed 6 of 6 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@iphydf iphydf merged commit e16d389 into TokTok:master Jan 15, 2018
@Diadlo Diadlo deleted the correction branch January 15, 2018 11:36
@nurupo
Copy link
Member

nurupo commented Jan 18, 2018

This is for correcting the last message, right? Why not utilize the message_id to be able to correct any message?

@sudden6
Copy link

sudden6 commented Jan 18, 2018

Would it make sense? when allowing changes to any message, one could sneakily change the start of conversations.

@zoff99
Copy link

zoff99 commented Jan 18, 2018

hashtag: persistent longterm message IDs
we will need those soon for other things aswell

@zoff99
Copy link

zoff99 commented Jan 18, 2018

@Diadlo how is "last message" defined? messages have no guarenteed order of arrival (and sometimes arrive out of order)

so the sender thinks he deletes or corrects one message, while in fact another message on the receiver side is deleted/corrected

@Diadlo
Copy link
Author

Diadlo commented Jan 18, 2018

@zoff99 Same with writing normal messages. When you write message you are not sure, that friend already receive previous. Correction works the same way

ADDED: Last message := last received message

@Diadlo
Copy link
Author

Diadlo commented Jan 18, 2018

@nurupo I'm not too familiar with toxcore. So current implementation is easies way to create at least some way to edit messages. I will be happy if someone can implement message editing within ~session or something like this
@sudden6 I don't think, that "one could sneakily change the start of conversations" should be real reason to avoid implementation of this feature. Many other IM have this feature and it's very useful

@zoff99
Copy link

zoff99 commented Jan 18, 2018

#548
now with 0.2.0 we have the chance to implement long term message IDs
but only once. otherwise we need to wait for 0.3.0 again

@Diadlo
Copy link
Author

Diadlo commented Jan 18, 2018

I'm not sure, that long term message ID is needed now

@zoff99
Copy link

zoff99 commented Jan 18, 2018

the need has been there for a long time:
messages arrive out of order
messages are sent 2 or 3 times (with faux offline messaging)
messages receipts never arrive

@GrayHatter
Copy link

This is a mistake... it makes the code base harder to maintain (why are you using an enum inside of a define?). Requires clients to implement code/features in a similar manner, while also not providing any specifics as to how. Adds an additional level of required complexity for clients who need to prevent malicious use. Allows message history from two different clients to go out of sync, in an unreasonable way.

my general reaction to this pull http://i.imgur.com/iouyret.gif

@nurupo
Copy link
Member

nurupo commented Feb 2, 2018

I'm not a big fan of this change either. Especially after zoff99 pointed out that the messages might not arrive in the right order, so you might correct the wrong message.

@Diadlo
Copy link
Author

Diadlo commented Feb 2, 2018

So, maybe revert this change then?

@Diadlo
Copy link
Author

Diadlo commented Feb 2, 2018

why are you using an enum inside of a define?

I did it by analogy with the previous value: PACKET_ID_ACTION

Requires clients to implement code/features in a similar manner, while also not providing any specifics as to how

Sorry, maybe I skip something, but where described how implement action message?

Allows message history from two different clients to go out of sync, in an unreasonable way.

Hm... AFAIK the history of two clients has never been synchronized

@iphydf
Copy link
Member

iphydf commented Feb 2, 2018

Let's revert it for now. @Diadlo can you send a PR? We can keep the idea in mind and revisit it in the upcoming redesign.

@GrayHatter
Copy link

I'm not a big fan of this change either. Especially after zoff99 pointed out that the messages might not arrive in the right order, so you might correct the wrong message.

Messages WILL arrive in the order given to toxcore, or not at all. Zoff99 is incorrect when he said that.

why are you using an enum inside of a define?

I did it by analogy with the previous value: PACKET_ID_ACTION

Requires clients to implement code/features in a similar manner,>>while also not providing any specifics as to how

Sorry, maybe I skip something, but where described how implement
action message?

It says, in tox.h (or used to) "similar to an action message in IRC" or
something to that effect.

Also, you don't really need to do anything specific with an action
message, treating exactly the same as a standard message is perfectly
acceptable. Would you make the same assertion for a message correction?

What if I receive a message correction as the first message? How should
my client handle that?

Allows message history from two different clients to go out of sync,
in an unreasonable way.

Hm... AFAIK the history of two clients has never been synchronized

It's ALWAYS been synchronized. Now, if client A replaces the message
with the correction, and client B appends the correction. The two
histories will diverge. Additionally, what's to stop me from ONLY
editing the last message over and over. One client will have dozens of
messages, the other will have 1.

@GrayHatter
Copy link

GrayHatter commented Feb 2, 2018 via email

@GrayHatter
Copy link

GrayHatter commented Feb 2, 2018 via email

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

Successfully merging this pull request may close these issues.

6 participants