Change Message#edit and add editContent and editEmbed methods

[jyp17]

This PR addresses #822 and changes Message#edit to replace the entire message with new content or embeds. It also adds methods editContent and editEmbed to only update content or embeds without changing other parts of the message.

Currently, MessageEventImpl#editMessage uses Message#edit. Should editMessage use the new edit method where the entire message is replaced, or should it only call editContent or editEmbed?

I also ran into a bug where methods that call edit(String channelId, String messageId, String content, boolean updateContent, List<EmbedBuilder> embeds, boolean updateEmbed) in UncachedMessageUtil always change both the content and embeds even if the updateContent and updateEmbed arguments are false. Should I go ahead and fix this in this PR, or should I make this a separate issue?

[Saladoc]

I also ran into a bug where methods that call edit(String channelId, String messageId, String content, boolean updateContent, List embeds, boolean updateEmbed) in UncachedMessageUtil always change both the content and embeds even if the updateContent and updateEmbed arguments are false. Should I go ahead and fix this in this PR, or should I make this a separate issue?

Since it is closely related to the concern of changing message content, it's fine to make those changes in the same PR.

I've skimmed over the changes and noticed that while both message content and embeds are mentioned, the effects on attachments are not documented. Do those need to be considered as well or are they entirely separate from content/embeds?

[Vampire]

@KILLEliteMaste what's the reasoning to do this silently breaking change?
The edit methods behave fine and do exactly what the document they do.
With this change you will suddenly break all bots out there that rely on that behavior.
And they will not even notice as the signature stays identical, just the implementation and documentation suddenly changes.

[felldo]

@KILLEliteMaste what's the reasoning to do this silently breaking change? The edit methods behave fine and do exactly what the document they do. With this change you will suddenly break all bots out there that rely on that behavior. And they will not even notice as the signature stays identical, just the implementation and documentation suddenly changes.

It's been some time since I have created the issue so I can not 100% recall what I thought back then. iirc it was because of the naming of the methods. If we want to have co existing edit methods which can replace a message and update specific fields this is not possible without this breaking change I guess. Otherwise we would need to come up with other method names to replace the message and this wouldn't be that nice imo.
Yes this change is pretty hidden but that's also what a changelog is for to keep track of the changes. And I think it is highly recommended anyway for everyone to look at it if you look at the latest changes we have merged.

[Vampire]

that's also what a changelog is for to keep track of the changes. And I think it is highly recommended anyway for everyone to look at it if you look at the latest changes we have merged.

In an ideal world, yes.
Everyone reads cahnge logs and everyone knows all parts of the code of his project to know which points affect him.

But in reality mayb 5% of the people read changelogs and most of them don't know their code inside-out unless it is a very small project.

Such a silent change in documented behavior is imho one of the worst things you can do as a library.
It will silently break bots out there, changing their behavior without real chance to noice it up-front.

@Bastian what's your opinion on this?

[Saladoc]

We do have a heavily breaking release ahead, but I see your point Vampire. Under different circumstances I'd honestly not care for the breakage, but deleted embeds and message contents could not be restored and that would be on us.

If we can neither find better names for the methods nor make the changes appropriately visible, we can consider marking the methods as deprecated with an appropriate message and only adding the new methods, referring to them if people wish to keep the old behavior. Then we'd have to hold off on changing Message#edit until one or two more releases. That way only people who skipped multiple versions with tons of breaking changes would run into problems.

[Vampire]

Don't get me wrong, I don't have problems with breaking changes.
But silent breaking change that just silently change the behavior is what I have a problem with.
And making the method deprecated and still do that silent breaking change is still bad imho.

[Saladoc]

And making the method deprecated and still do that silent breaking change is still bad imho.

True. That's why I suggested deprecating the methods in preparation for the then-not-so-silent breaking change and delay the change, if we don't find a better. If the methods are deprecated for 1-3 releases (depending on how often they come) far more people will be made aware of the change ahead of time. That would be the best way to do our due diligence if we want that change and can't make it non-sneaky.

[jyp17]

Since there doesn't seem to be a consensus on what to do at the moment, would it be alright if I go ahead and make the bug fix I mentioned a separate pull request? So that it can be considered separately regardless of what is decided on later.

[Bastian]

@Bastian what's your opinion on this?

I do agree that a silent breaking change should be an absolute exception. This is not important enough to justify it.

I think, for now we should add editContent(...) and editEmbed() and deprecate edit(). I do not think we should re-introduce/change the edit() in a later release but just deprecate and eventually remove it. For more complex use-cases, the users can use the MessageUpdater class.

It might also be a good idea to allow the MessageUpdater to work for uncached messages, see #980

[felldo]

I am fine with that. Just thought it would be nice to have both behaviors (updating/replacing) a message without having to create a MessageBuilder. For that reason the issue was initially created.
If we are at it, should the naming continue to the be editX or do you think we should name it updateX if we a not going to follow the issues idea?

[jyp17]

For cases where edit changes both the content and embed, should there be a new editContentAndEmbed() method? Or only have users edit content and embeds separately?

[Bastian]

I wouldn't add a editContentAndEmbed() method. That's what I meant with

For more complex use-cases, the users can use the MessageUpdater class.

[jyp17]

I wouldn't add a editContentAndEmbed() method. That's what I meant with

For more complex use-cases, the users can use the MessageUpdater class.

Thank you for the clarification!
After deprecating the methods with the following Javadoc:

     * @see MessageUpdater#setContent(String)
     * @see MessageUpdater#setEmbeds(List)
     * @deprecated Use {@link MessageUpdater#setContent(String)} and {@link MessageUpdater#setEmbeds(List)} instead.

When generated, the Javadoc reads as:

Deprecated.
Use MessageBuilderBase.setContent(String) and MessageBuilderBase.setEmbeds(List) instead.

Should I handle the Javadoc differently, or is this alright?