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

Editable entities #94

Merged
merged 3 commits into from Feb 17, 2018

Conversation

Projects
None yet
6 participants
@SuperTux88
Copy link
Member

commented Jan 23, 2018

I started with this some time ago (and already discussed parts of it with @annando over a year ago), but didn't have the time to finish it until now.

This adds basic edit features to the protocol:

  • I added an edited_at property to all entities with a text property to make them editable in the future. It's an optional property, so when it's missing, it's the first unedited original and it's only present when it's actually edited content.

    This is backward compatible with current diaspora versions, because receiving a second "duplicate" of the same post (same guid) would just be ignored. But others can handle the edit.

    It's up to the implementation if they want to store each version (and display diffs in the frontend) or just overwrite it and keep the latest version. Both is possible with this protocol spec.

  • I also added an edited_at property to the profile entity, which is already editable. But with this property we can make sure that we don't overwrite newer information by receiving an older message (for example when it was stuck and later retried).

  • And last I added it to events and event_participations too, mainly because of the same reason as for the profile. People need to be able to change their participation state, but we need to make sure to always keep the latest.

    Same for the event itself: maybe some important information needs to be changed in the description, and we need to make sure to always have the latest. It's again up to the implementation if they want to keep the history for the description or only the latest version.

Not all properties are editable (for example you can't change the guid or the author), I marked that for editable entities in the documentation.

Related to: diaspora/diaspora#1762

SuperTux88 added some commits Sep 5, 2017

Add edited_at property to the profile message
This is needed so we can make sure to not overwrite newer data by older
one, for example when there are two edits close together and the newer
arrives faster.
@jhass

This comment has been minimized.

Copy link
Member

commented Jan 23, 2018

So, I'm not sure we do want this backward compatible actually. Perhaps not even to the point that anything still works with an incompatible implementation. Having one side send edits and the other side silently ignoring them seems like a major UX fuckup to me, wouldn't it be very very confusing?

@SuperTux88

This comment has been minimized.

Copy link
Member Author

commented Jan 23, 2018

Maybe "backward compatible" was the wrong word, but it's without the need of inviting something completely new. But how do you want to prevent that somebody ignores your message? Even when you invite something completely new, but when one system sends it to another system that doesn't understand it yet (maybe it's not implemented yet, or it's an old version and not updated yet), there is nothing other than that you can do than fail/ignore the message, but the sender already did the action.

Yeah, doing something that isn't supported by everybody isn't great, that's why we try to wait until some servers support it until we add the frontend (for examply only adding the account migration/import after other servers already can process the migrate message), but there will always be servers which don't support it (and silently ignore it).

We already have these "major UX fuckups" today:

  • diaspora can send likes for posts, but socialhome silently ignores them
  • friendica can send likes for comments, but diaspora ignores them (well, actually we even save and relay them, but don't display them)
  • diaspora can send polls with a post, but friendica silently ignores them
  • diaspora can send private posts, but socialhome silently ignores them

So we have this problem all over the place, but I have no idea how you want to solve this?

@annando

This comment has been minimized.

Copy link

commented Jan 23, 2018

Friendica even can send unlikes that are ignored (but relayed) by Diaspora ;-)

I don't see a huge problem by sending this either.

@jhass

This comment has been minimized.

Copy link
Member

commented Jan 23, 2018

Well, all of your examples are missing content. This would introduce wrong/inconsistent content. Breaking backwards compatibility could be an incentive to for the ecosystem to upgrade quicker. This doesn't have to be a single step, like before we could introduce forward compatibility early and stop sending backward compatible messages only a major release later.

@SuperTux88

This comment has been minimized.

Copy link
Member Author

commented Jan 23, 2018

Friendica even can send unlikes that are ignored (but relayed) by Diaspora ;-)

Yes, there are maybe more things that work differently, but that was just what came to my mind in two minutes ;)

Breaking backwards compatibility could be an incentive to for the ecosystem to upgrade quicker.

This only adds a unnecessary breaking change for everybody (means more stress for other devs and podmins), and I think it wouldn't make a big difference in how consistent this feature works. Sure, we could add a editable_status_message, but this doesn't guarantee that others really implement the edit feature, or if they just alias status_message to editable_status_message and still ignore the edit.

I also think that socialhome already supports editing, but they just resend the current status_message again, which then is rejected at diaspora (because it's a duplicate). So we already have the situation you mentioned, where edit's aren't consistent.

And even if everybody would implement this correctly, it wouldn't guarantee that everybody who has the original post would receive the edit (it's the same as nowadays not everybody who has the post receives comments for it). I have other ideas to fix that problem, but that's not related to the edit feature.

So I think this only makes the whole thing overly complicated, but doesn't add a benefit. People are used that things sometimes aren't consistent for different reasons (because we are a federated system). I'm working on making this better, but adding a breaking change where it isn't needed would make the whole situation even worse, and I don't like that. Some people wouldn't receive the original post then at all, instead of just not receiving the edit (which maybe not even happens, because the original post doesn't contain a typo, so no edit needed).

But with this little change we would have a clean specified way to send edits with a timestamp, and everybody who does it the same way, can profit from it (that's why I want to have it in the documentation, so everybody can do it the same way). It doesn't make the current situation worse (because we are already inconsistent, even with edits, see socialhome), but it allows to make the future better, because we can be more consistent then.

When implementing this at diaspora, we can discuss there, if we want to wait a major release until we allow the users to edit their post, after we implemented the backend/frontend for receiving and displaying edits (after we added forward compatibility). But that discussion doesn't belong here and we don't need to break the protocol for everybody else if only we want to wait.

@jhass

This comment has been minimized.

Copy link
Member

commented Jan 23, 2018

Alright, just wanted to point it out. I think the "others will and already do mess it up" is a strawman but this is largely opinion anyway.

@SuperTux88

This comment has been minimized.

Copy link
Member Author

commented Jan 23, 2018

Alright, just wanted to point it out.

Yes, thanks for that. And I thought about it, but I just couldn't find another solution than break it (to make sure nobody can receive a post that's maybe edited in the future), and that just doesn't sound like a good solution to me. And the problem will solve itself with time, the more pods update.

@Zauberstuhl

This comment has been minimized.

Copy link

commented Jan 26, 2018

I don't think its necessary to store that kind of extra information in the protocol itself.
I would use the already existing created_at field (the edited entity was just created).

In my opinion all further logic should be handled by the application server.
If the application server knows the guid already its an update.

I think that is exactly how @annando does it in friendica right now.

Just wanted to throw my hat in the ring,
Lukas

@SuperTux88

This comment has been minimized.

Copy link
Member Author

commented Jan 26, 2018

When reusing the created_at we would lose the original created time. When using fetching you're only able to fetch the latest state, so you need both timestamps (created_at for when the post was created, and edited_at to know that the post was edited and when). Also reusing created_at feels unclean, because it's not always the "created timestamp" anymore, it's "created or edited timestamp" which isn't clear by reading the name.

So I think the additional property is necessary. Also the profile doesn't have a created_at property already, so we can't reuse it there.

@annando

This comment has been minimized.

Copy link

commented Jan 26, 2018

At Friendica we do have two fields: "published" and "updated". The first one is the creation date, the second one the date it was updated. We always send both fields and only show that it is updated when there is a difference between these two fields.

@cmrd-senya

This comment has been minimized.

Copy link
Member

commented Jan 26, 2018

So basically we have two types of editable entities. First type contain uneditable created_at field and the second type doesn't contain created_at field. To the first type belong status_messages, photos, comments and PM and to the second type belong event, event participation and profile.

These two types imply a little bit different logic. For entities with created_at field the created_at defines the original submission time and missing of the edited_at fields indicates if it is an edit or not. For entities withouth created_at field there is basically no such thing as "original time".

Should we keep the mandatory created_at for edit entities?

If we already know the original entity there is no much reason to send created_at with edit because it doesn't change.

But what do we do if we receive the edit when we didn't have the original entity in the first place? Do we fetch the original post explicitly? If so then we don't need created_at in the edit because we get it with post fetching. If we don't fetch the original post, then we could keep created_at for displaying the original submission time, but that would mean that the users will see the original submission time but not the original text. Also, if for some reason the edit contains different created_at than actual post had then the recepient post would trust it and store a different value, while we consider created_at uneditable, but basically allow having storing different values in some edge cases. For me it looks a bit asymmetrical that we keep sending original created_at field in the edits while not sending original text or something else with the edits. Why do we prefer to send some of the original data in the edits over another? It also is not needed for identification because we have author & GUID pair for unique identification.

So mostly created_at in edits feels reduntant to me. Having one timestamp is enough. If we have only one timestamp per message than it would have the same logic across all the entities, including status messages, profiles and events.

Then. How do we uniquely identify revisions? Do we want to support functionality of fetching an arbitrary revisions of an entity? Or it is enough to support of fetching of the first and the last versions only?

If we want to fetch an arbitrary revision, then we need a unique identifier of a revision. Currently the only thing that identifies a revision is a edited_at timestamp which can't be really unique. As an edge case it is possible that some pod sends a few edits at the same second. It could be because some automatic script does very fast updates or because server corrected it's own time via NTP. How do we decide then which edit is the last one?

Also how do we fetch a specific edit then? Do we use a route that includes the timestamp? Can't we then fetch different edits from the same second?

Also you introduce a concept of an editable field in the protocol. What if instead of having uneditable created_at field we could just make a entity_timestamp field which is editable? So for original posts it would mean creation time and for edits it would mean edit time. In this respect timestamp would have the same meaning as text -- it's just an ordinary editable property of a entity. If we don't give special meaning to a timestamp as an indicator of an edition then we won't have problems in the listed edge cases.

Another approach I think of and I like it more, is not to include the absolute edit time to each entity, but rather include the number of seconds passed since the last (or the first?) revision of the entity. This way we could avoid handling weird situations where the edit is older than the original post for some reason. In the case when we don't have mandatory created_at in each editable entity we would depend on having at least the original version fetched but that's another point to consider.

And speaking about fetching. If we want to allow fetching of every revision, then we could make use of keeping an incremental revision number. Say that you have revisions 1,2,3 of the post and you suddenly receive revision number 5. Without having revision numbers in the entities we couldn't even know that there was a revision 4, because we only see dates. And even if we knew the specific date of some revision we would request a revision by its timestamp which feels clumsy. So if we want to have a possibility of fetching any revision then we should consider assigning incremental numbers for them.

That's nothing I see as complete proposals, but rather things to consider as potentially better approaches.

Also concerning the implementation, I would definetely support having an Editable module to encapsulate editable behavior (like we have a Relayable module for relayables).

@cmrd-senya

This comment has been minimized.

Copy link
Member

commented Jan 26, 2018

Okay, here I structured my thoughts a little bit better. This is more like a proposal :)

  • Introduce incremental revision numbers for every editable entity.
  • Keep created_at for original posts. We could also introduce created_at for profiles which would mean the user registration date. To identify edit time we use "seconds after created_at" value. The same goes for event.
  • But for event_participation the logic should be closer to poll participations or likes, so if we don't have timestamps there, we don't need them for event_participation. As for likes we could simply override the previous value. (Like is kinda editable now as well, because you can change the state).

So basically with my proposals we divide editable entities to two types: "wiki" entities and "overridable" entities. "wiki" entities have revision numbers, created_at in the first revision and "seconds since first revision" in next revisions. "Overridable" entities don't have neither timestamps nor revision numbres, they just silently replace old version with a new one (as for likes now).

For "wiki" entities the original version is one that:

  • doesn't have revision number
  • includes created_at

and the edit

  • has revision number
  • doesn't contain created_at
  • contains number of seconds since the original version

For "overridable" entities the format of edit is just the same as of the original entity.

@SuperTux88

This comment has been minimized.

Copy link
Member Author

commented Jan 26, 2018

Here is a tl;dr of why I did it the way I did:

  • I want it to be as easy as possible for others to add it to their software.
    • As little changes as possible
  • I want this to be an optional feature:
    • We from diaspora don't even know if we want this, while others already use it. That's also why I don't want to add breaking changes for it, because otherwise everybody else needs to wait for the slowest (probably us), or we would break complete communication between the projects.
    • New projects should have an easy start and add feature by feature and shouldn't need to add everything until they can start talk to others (like socialhome currently doesn't support all features)
  • I don't want to force others to save revisions
    • I don't know if existing implementations already do this, but we from diaspora probably want to save and show revisions, but I don't want to force others to do the same

And now a bit more verbose as response to your proposal:

For entities withouth created_at field there is basically no such thing as "original time".

They don't need it, but we need edited_at to have an order and detect the latest edit.

Should we keep the mandatory created_at for edit entities?

Yes, because when fetching them we need it.

If we already know the original entity there is no much reason to send created_at with edit because it doesn't change.

Yes, but the redundant created_at doesn't hurt if we already know it, but when we don't know the original, we need it and when it's fetched we need it.

Do we fetch the original post explicitly?

You can't fetch the original post, because the fetch specification doesn't support revisions. Also when a project just overwrites the original and doesn't keep revisions, the original doesn't exist anymore.

Why do we prefer to send some of the original data in the edits over another?

Some properties aren't allowed to change, but I want to always send a complete message. Yes, some information is redundant, but it doesn't matter, and I don't want to rely on that others already know it.

Also allowing missing mandatory properties in the case of an edit would make the validation much more complicated, and I want to keep it as simple as possible.

Then. How do we uniquely identify revisions? Do we want to support functionality of fetching an arbitrary revisions of an entity? Or it is enough to support of fetching of the first and the last versions only?

Se above: I don't want to force others to store revisions, so you can only fetch the last version.

It could be because some automatic script does very fast updates

If a script does multiple edits to the same entity in the same second I just don't care (about the fact that edits are messed up in this case). We probably need to do something against spam in this case, but I don't care about scripts. And humans probably need at least a second to edit a post or comment.

because server corrected it's own time via NTP

That's probably really an edge case. But:

but rather include the number of seconds passed since the last (or the first?) revision of the entity.

In the NTP edge case, the time difference would end at 0 seconds if NTP sets the time to the exact same second as the original post.

Also this could easily break, or been implemented wrong. When you miss the last edit, and we define it as seconds since the last edit, you would calculate the wrong timestamp. And if we define it as seconds since the original, but somebody implements it wrong and uses the last edit, we would again have the wrong timestamp. While just having the timestamp is clear and easy, no calculation needed.

This way we could avoid handling weird situations where the edit is older than the original post for some reason.

This should never happen, and if it happens, I don't care. The current protocol relies on the fact that the servers have the correct time. If timestamps for created_at are wrong, then even the original post would be added at the wrong place in the stream or in a comment thread. And I don't care about that neither. If the time is wrong on a server, things break (things would even break pod-local).

What if instead of having uneditable created_at field we could just make a entity_timestamp field which is editable?

This would be a breaking change, and as said, I don't want to break if, if it isn't really needed. Also I think the implementation would be more complicated when you have one field with different meanings.

Introduce incremental revision numbers for every editable entity.

I didn't respond to other revision number related proposals, because I don't want to enforce to store every revision. So every project implementing the protocol can decide if they want to store the old revisions (like on discourse) or just overwrite them and only display an "edited" note (like here on github).

Allowing revisions to be fetchable would also need changes in the fetch specification, which would make make the implementation for every project more complex.

We could use revision numbers instead of the edited_at, which would be enough for sorting, but I think the timestamp is much more useful (you can display a timestamp, when you only display a "edited" not), works great for sorting and we don't need to keep track of a counter for example when editing profiles (where we at diaspora also don't keep the old revisions).

We could also introduce created_at for profiles which would mean the user registration date.

This is currently private information and we didn't even store this for very old profiles.

But for event_participation the logic should be closer to poll participations or likes, so if we don't have timestamps there, we don't need them for event_participation.

No, we need the timestamp, to prevent that we end up processing an old message, which then would end in the wrong state being stored. You can't edit poll participations or likes (you can delete likes, and preventing processing a like message for an already deleted like is also on my todo-list, but this would need another solution, in our code, not the protocol).

Also concerning the implementation, I would definetely support having an Editable module to encapsulate editable behavior (like we have a Relayable module for relayables).

Sure, that makes sense, I'll change that.

@cmrd-senya

This comment has been minimized.

Copy link
Member

commented Jan 26, 2018

Okay, I see. We don't want to support revisions on the protocol level. Looks good to me then.

You can't edit poll participations or likes (you can delete likes, and preventing processing a like message for an already deleted like is also on my todo-list, but this would need another solution, in our code, not the protocol).

What if you send a like and then send a dislike for the same post. Isn't it an edit? Do we just ignore it then?

Also since we add editing capabilities to other entities, why then can't we add editing for poll participations and likes then? For poll participations that would make sense to change the poll answer and for likes it would mean changing from like to dislike or vice-a-versa. Having edited_at would then indicate the latest version. Just as for event_participation.

@SuperTux88

This comment has been minimized.

Copy link
Member Author

commented Jan 26, 2018

What if you send a like and then send a dislike for the same post. Isn't it an edit? Do we just ignore it then?

Diaspora currently doesn't display dislikes, but would receive and relay them. But that wouldn't be an edit (because it has a different GUID) and when you send both, I think the second would fail, because we have a unique index at the likes table preventing this. But when we want to support that, I wouldn't count that as an edit. They are two different entities, so we would need to decide if you can like and dislike the same post (but I think that's out of scope here).

Also since we add editing capabilities to other entities, why then can't we add editing for poll participations ... For poll participations that would make sense to change the poll answer

I didn't add anything about polls in this PR (I even marked them as not editable for now in the status_message entity. I think polls need more thinking about it. For example:

  • Do we want to allow to edit polls?
    • From my point of view it should only be possible to add more answers, but not delete existing answers
    • and changing the text for an existing answer could be dangerous (and difficult to display old revisions of the text without overloading the UI, so I don't know if we should support editing for poll_answers
  • Do we want to support closing a poll, so now answers are accepted after that?

But since we are the only one supporting polls (AFAIK?), I think there is no hurry here. So we can discuss the poll related changes another time.

... and likes then?

Likes aren't editable, you can delete them and recreate them, but that would be a new entity since it has a new GUID (even dislikes would be a different entity).

@cmrd-senya

This comment has been minimized.

Copy link
Member

commented Jan 26, 2018

Well, what I basically meant is whether we want to allow changing a poll participation from one existing poll answer to another existing poll answer without allowing editing anything else. But in fact we can do it the same way as with likes: retract the original poll participation and push another poll participation as a different entity.

I still don't feel there are any design reasons in making event_participation editable while having likes and poll participations only "replaceble" (retraction, new entity push). For me it feels that if one of the approaches is better than another for some reason then we should stick to it everywhere. But still, I'm okay with whatever works.

@SuperTux88

This comment has been minimized.

Copy link
Member Author

commented Jan 26, 2018

Well, what I basically meant is whether we want to allow changing a poll participation from one existing poll answer to another existing poll answer without allowing editing anything else. But in fact we can do it the same way as with likes: retract the original poll participation and push another poll participation as a different entity.

We currently don't allow deletion of poll_participations (at least not from the user). But when we decide we want to make poll participations deletable and changeable, I probably would make it the same way as I did it here for event_participations (with making them editable).

But for now I just didn't change anything related to polls here (including poll participations), and I think we can discuss all the poll stuff in another discussion, because I think there are maybe more questions or more things we can do (for example maybe the author wants to decide if the people are allowed to change their vote after they created it?). But I just don't want to start this discussion here, so I want to keep the polls as they currently are for the moment.

About likes:

For me likes aren't editable, the only thing you can do after you created a like is you can delete it. When doing a dislike this isn't a edit to me, this is a new entity, and in theory you can do both at the same time (but the diaspora DB doesn't allow this currently). But since diaspora doesn't allow dislikes we can maybe ask @annando how friendica handles dislikes and if it is possible to have a like and a dislike at the same time or if friendica changes the boolean for the entity with the same guid (if so, we should maybe make likes editable too, but I'm not sure about it)?

@jaywink

This comment has been minimized.

Copy link

commented Jan 26, 2018

I "vote" for the original suggestion by Benjamin because I think it makes sense to have the dates separate, but also because this is the in line with Activitypub, thus being able to handle both in the same way.

👍 and "finally" 😁

@SuperTux88 SuperTux88 merged commit 63f1cbd into diaspora:develop Feb 17, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
pronto/rubocop Coast is clear!

SuperTux88 added a commit that referenced this pull request Feb 17, 2018

@SuperTux88

This comment has been minimized.

Copy link
Member Author

commented Feb 17, 2018

Since there weren't any objections remaining and there was no discussion since more than 3 weeks, I merged this. Thanks for all the feedback.

@SuperTux88 SuperTux88 deleted the SuperTux88:editable-entities branch Feb 17, 2018

@SuperTux88

This comment has been minimized.

Copy link
Member Author

commented Feb 18, 2018

Also concerning the implementation, I would definetely support having an Editable module to encapsulate editable behavior (like we have a Relayable module for relayables).

Sure, that makes sense, I'll change that.

@cmrd-senya SHIT, I completely forgot to do this before merging :( Sorry (3 weeks are a long time)

But now that I remembered it again and thought about it (if I should do it in a follow up commit), I'm not sure if this is really useful. There is absolutely no "editable behavior" (like in Relayable or Signable), it's only a property. And I think it would make everything unnecessary more complex. We don't have a "guid module", "author module", or "text module" either, but these properties are also used in multiple entities. So I think I'm against having modules for properties, only because they are used in multiple entities, because I think it's clearer to keep the entities simple and keep the properties directly in the entities.

Do you really think a module is needed here?

@ghost

This comment has been minimized.

Copy link

commented Feb 18, 2018

Since nobody answered that question... friendica and hubzilla are activitystream based networks and likes and dislikes are activities which have no direct relationship with each other. Many people like and dislike the same post/comment which indicates "ambivalence". We didn't see any need to prevent or block somebody from having mixed feelings. Both activities can be reversed by deleting the underlying activity.

@annando annando referenced this pull request Feb 21, 2018

Open

Diaspora: Protocol enhancements #4486

0 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.