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 ability to set trade offer message #3262

Closed
wants to merge 1 commit into from

Conversation

dm1tz
Copy link
Contributor

@dm1tz dm1tz commented Aug 2, 2024

Checklist

  • I read and understood the Contributing Guidelines.
  • This is not a duplicate of an existing merge request.
  • I believe this falls into the scope of the project and should be part of the built-in functionality.
  • My code follows the code style of this project.
  • I have added tests to cover my changes, wherever they are necessary.
  • All new and existing tests pass.

Changes

New functionality

Adds the ability to set a custom message when sending an offer.

Additional info

This is particularly useful for third-party plugins that implement custom trading logic, as a message can convey useful information and/or help users differentiate offers created by ASF from those created by plugins.

@Abrynos
Copy link
Member

Abrynos commented Aug 2, 2024

I would prefer this to be IN ADDITION to the information already contained and not instead of it.

@Abrynos
Copy link
Member

Abrynos commented Aug 2, 2024

Now that I think of it, I personally also would like for ASF to include information about the calling assembly (in case it differs from ASF and official plugins).

@nolddor
Copy link
Contributor

nolddor commented Aug 2, 2024

What if provided message is larger than the max. chars (128) a trade offer message can handle?

  • Does it truncate the message with no error / warnings? <-- This is how web trade window works at least.
  • Does it fail to send and we get an clear error message?

I'm still unsure if it is safe to allow 3rd party plugins to override the message,.. would be better to append I guess.

@dm1tz
Copy link
Contributor Author

dm1tz commented Aug 2, 2024

What if provided message is larger than the max. chars (128) a trade offer message can handle?

  • Does it truncate the message with no error / warnings? <-- This is how web trade window works at least.
  • Does it fail to send and we get an clear error message?

Good point, we could preemptively truncate the message if it exceeds the limit.

I'm still unsure if it is safe to allow 3rd party plugins to override the message,.. would be better to append I guess.

Could you please elaborate? I'd argue that overriding the standard message is a more flexible approach, and given that the default message (e.g. Sent by ArchiSteamFarm-modded/6.0.4.4) is already 37 characters, it would consume space for a custom one.

Besides, nothing is stopping you from re-implementing SendTradeOffer and directly overriding the original message in a plugin.

@@ -628,7 +628,8 @@ public sealed class ArchiWebHandler : IDisposable {
{ "partner", steamID.ToString(CultureInfo.InvariantCulture) },
{ "serverid", "1" },
{ "trade_offer_create_params", !string.IsNullOrEmpty(token) ? new JsonObject { { "trade_offer_access_token", token } }.ToJsonText() : "" },
{ "tradeoffermessage", $"Sent by {SharedInfo.PublicIdentifier}/{SharedInfo.Version}" }
// Message length is limited to 128 characters.
{ "tradeoffermessage", !string.IsNullOrEmpty(message) ? (message.Length > 128 ? string.Concat(message.AsSpan(0, 125), "...") : message) : $"Sent by {SharedInfo.PublicIdentifier}/{SharedInfo.Version}" }
Copy link
Member

Choose a reason for hiding this comment

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

Aside all the other stuff: 128 should be a const and the substring length should be derived from it. Furthermore, there is a continuation character containing 3 dots (reducing the amount of chars we use). Please take a look at SteamChatMessage.cs

@JustArchiNET JustArchiNET deleted a comment from Wilonyyy Aug 3, 2024
@JustArchi JustArchi closed this in d3dbfc5 Aug 3, 2024
@JustArchi
Copy link
Member

JustArchi commented Aug 3, 2024

I don't see any added value other than abuse potential in replacing the message, not like I care much about it, but trade offer message isn't a place for putting arbitrary info.

We can totally append extra informaton though, so with current code I've committed above you have approx 80 characters for adding anything you deem important, which should be enough for an identifier, link, or even some kind of hash or summary.

Should be sufficient for any legit use cases this PR aimed for, while not disrupting ASF's way of doing things by being transparent in everything it's doing.

@dm1tz dm1tz deleted the trade-message branch August 3, 2024 14:31
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants