search

snu-quiqcl / qiwis

QuIqcl Widget Integration Software
MIT License
5 stars 2 forks source link

[FEATURE] Generalize swift-call message protocol #139

Closed kangz12345 closed 1 year ago

kangz12345 commented 1 year ago

Feature you want to implement

The current implementation of swift-call message protocol requires handling case by case. Moreover, the message boxes introduced in #132 should be implemented for each action when we add more swift-calls in the future.

Therefore, I want to generalize this protocol.

How the feature is implemented

  1. Swift-call messages follow the format below: 
    {
"call": "destroyApp", # the method name of Swift to call
"args": {"name": "test-app"}, # arguments of the method, all as keyword arguments
}
  2. The swift-call handler will try to execute the given method call.
  3. If it fails with an exception, catch it and inform the caller app.

However, this has a limitation that some swift-calls' arguments cannot be converted to JSON strings. In these cases, I will provide a way to take JSON strings as one of the following:

  1. Define another method createAppFromJSON(name: str, info: str) that takes a JSON string and convert it to a dictionary and then to an AppInfo object. Then it will call the original createApp() with the AppInfo object.
  2. Allow the original createApp() method to take a string as well, i.e., the type hint for info becomes Union[AppInfo, str], and use isinstance(info, str).

The first option can clearly separate the method for public API and for swiftcall. The second option is convenient to implement. Your opinions are welcome!

BECATRUE commented 1 year ago

I think the first option is better because the original createApp() will not be changed and it's like an add-on. And how about changing the visibility of createApp() to private?

kangz12345 commented 1 year ago

Thank you for the opinion, I agree with that!

However, for the suggestion below:

And how about changing the visibility of createApp() to private?

I think we made it for a public API, and it is intended to be public. Even though I don't see any usage where it could be used directly as a method call, but I want to leave it public until some problems are found. Maybe createAppFromJSON() could be private.