snu-quiqcl / qiwis

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

[FEATURE] Creation and destruction API of App and Bus #66

Closed BECATRUE closed 1 year ago

BECATRUE commented 1 year ago

Feature you want to implement

Our goal is the below:

How the feature is implemented

Swift was implemented in swift.swift module.

Original structure

class Swift(...):
  def __init__(...):
    self._init_bus(...)    # initialize global buses
    self._init_app(...)    # initialize apps
    self._show_frame(...)  # show frames of apps

Expected structure

class Swift(...):
  def __init__(...):
    self.load(...)  # initialize buses and apps, and show frames

  def createBus(...):
    pass

  def createApp(...):
    pass

  def destroyBus(...):
    pass

  def destroyApp(...):
    pass

Documentations

load(appInfos: dict, busInfos: dict)

createBus(name: str)

createApp(name: str, pos: str, show: bool, ...)

destroyBus(name: str)

destroyApp(name: str)

kangz12345 commented 1 year ago

I edited it, so please ask me if there is something you don't understand.

BECATRUE commented 1 year ago

I edited it, so please ask me if there is something you don't understand.

It looks better!

kangz12345 commented 1 year ago

Great, you may start!

BECATRUE commented 1 year ago

Oh, I have a question!

I think the argument busNames in load should be changed to a dict busInfos, since it may have a param, i.e. timeout. How do you think about it?

@kangz12345

kangz12345 commented 1 year ago

Right, I agree with you!

BECATRUE commented 1 year ago

I have another question!😂

We need to save setup_env dict as a field of Swift because we may use it in createBus() and createApp(). Then, isn't the params of load() i.e. appInfo and busInfo neccessary? We can access them through self.setup_env directly.

@kangz12345

kangz12345 commented 1 year ago

I would like to make createBus() and createApp() more stateless, i.e., they just create a bus or an app based on the given arguments (name, path, module, show, pos, etc.).

I don't think setup_env should be saved, because it is a setup environment information, which is only necessary for setup. Only the current status, which includes the list (or dict, etc.) of apps and buses, should be tracked, I think.

Therefore, what I want for this new API is supporting a way of addition or removal of apps and buses in swift. Of course, this shouldn't allowed to apps directly (they might use it in a bad manner). However, for this issue, it only provides the API via methods Swift, and the apps do not have the Swift instance, hence cannot call the API.

Later, we would make a special messaging API for the apps (let's talk about the details later..), so the apps can request swift to create or to destroy an app or a bus. The authorization would then happen, for example, swift pops up a dialog for the confirmation from the user.

kangz12345 commented 1 year ago

Please ask me further if there is something you don't understand or ambiguous!

@BECATRUE

BECATRUE commented 1 year ago

I would like to make createBus() and createApp() more stateless, i.e., they just create a bus or an app based on the given arguments (name, path, module, show, pos, etc.).

I don't think setup_env should be saved, because it is a setup environment information, which is only necessary for setup. Only the current status, which includes the list (or dict, etc.) of apps and buses, should be tracked, I think.

Therefore, what I want for this new API is supporting a way of addition or removal of apps and buses in swift. Of course, this shouldn't allowed to apps directly (they might use it in a bad manner). However, for this issue, it only provides the API via methods Swift, and the apps do not have the Swift instance, hence cannot call the API.

Later, we would make a special messaging API for the apps (let's talk about the details later..), so the apps can request swift to create or to destroy an app or a bus. The authorization would then happen, for example, swift pops up a dialog for the confirmation from the user.

Oh, I see. I misunderstood that you intended offering only show and pos to createApp.

Then, I won't save setup_env.

BECATRUE commented 1 year ago

I have a problem..

In createApp, there are many arguments (e.g. path, module). Thus, it causes Pylint warning.

swift\swift.py:100:4: R0913: Too many arguments (9/5) (too-many-arguments)
swift\swift.py:100:4: R0914: Too many local variables (16/15) (too-many-locals)

Thus, how about changing the arguments into a dict?

@kangz12345

BECATRUE commented 1 year ago

I have a problem..

In createApp, there are many arguments (e.g. path, module). Thus, it causes Pylint warning.

swift\swift.py:100:4: R0913: Too many arguments (9/5) (too-many-arguments)
swift\swift.py:100:4: R0914: Too many local variables (16/15) (too-many-locals)

Thus, how about changing the arguments into a dict?

@kangz12345

Or using **kwargs.

kangz12345 commented 1 year ago

I didn't know such lint rules 🤣 In that case, let's use namedtuple or dataclasses or anything else you think good enough, since we want to force the info structure.

For example, define AppInfo and BusInfo in swift.swift module as a namedtuple or a dataclass:

info = AppInfo.from_dict(info_dict) # just an example; not necessarily this way
self.createApp(info)

In addition, I think dataclasses would be better since it is easier to define helpful methods inside. Please let me know if you have opinions about this.

You can make another PR for this if you want. How do you think?

BECATRUE commented 1 year ago

Originally, I tried to use a dict! How about making another issue about dataclasses?

Before dataclasses, I want to request PR.

kangz12345 commented 1 year ago

Oh, very good idea. Please make a PR now, and let's make the dataclasses in the next PR.

If you want to make it, you can do it. If you want to develop the next step (regarding to #62), I will do this (dataclass).

BECATRUE commented 1 year ago

Oh, very good idea. Please make a PR now, and let's make the dataclasses in the next PR.

If you want to make it, you can do it. If you want to develop the next step (regarding to #62), I will do this (dataclass).

Cool! Please make dataclasses after this issue.