Skip to content

[lua] Add error message param to onreceive handler#5476

Merged
dacap merged 1 commit into
aseprite:mainfrom
martincapello:add-error-message-to-websocket-handler
Oct 22, 2025
Merged

[lua] Add error message param to onreceive handler#5476
dacap merged 1 commit into
aseprite:mainfrom
martincapello:add-error-message-to-websocket-handler

Conversation

@martincapello

Copy link
Copy Markdown
Member

Now script developer can define websocket's onreceive handlers with an additional parameter that holds error information when messages of type WebSocketMessageType.ERROR are received

NOTE: This PR was not requested explicitly, but I saw the opportunity to add this while trying to debug a script that used websockets.

Now script developer can define websocket's onreceive handlers with an
additional parameter that holds error information when messages of type
WebSocketMessageType.ERROR are received
@martincapello martincapello requested a review from dacap as a code owner October 20, 2025 18:41
@martincapello martincapello added the scripting Related to scripting API label Oct 20, 2025

@aseprite-bot aseprite-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

ws->setOnMessageCallback([L, ws, onreceiveRef](const ix::WebSocketMessagePtr& msg) {
int msgType = (msg->binary ? MESSAGE_TYPE_BINARY : static_cast<int>(msg->type));
std::string msgData = msg->str;
std::string msgError = msg->errorInfo.reason;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

warning: variable 'msgError' of type 'std::string' (aka 'basic_string') can be declared 'const' [misc-const-correctness]

Suggested change
std::string msgError = msg->errorInfo.reason;
std::string const msgError = msg->errorInfo.reason;

@dacap

dacap commented Oct 22, 2025

Copy link
Copy Markdown
Member

I'll merge it as it is.

It's a shame that the API design is not prepared for extensibility, i.e. just one "ev" argument with message type/data/error inside. I'll create a new issue to redesign the API in some way in the future.

@dacap dacap merged commit 85aae16 into aseprite:main Oct 22, 2025
12 checks passed
@martincapello

Copy link
Copy Markdown
Member Author

Yeah, that would be nice. It would break current scripts though.

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

Labels

scripting Related to scripting API

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants