tinypilot icon indicating copy to clipboard operation
tinypilot copied to clipboard

Unify initialization procedure of dialogs

Open jotaen4tinypilot opened this issue 10 months ago • 0 comments

Originated in https://github.com/tiny-pilot/tinypilot/issues/1770.

The initialization process is basically the same in all our dialogs:

  1. We trigger one operation to initialize the dialog.
    • We often (but don’t always) have a method called .initialize() for this, which is implemented in the respective dialog. In some cases, the method is called differently, but that can be seen as accidental naming inconsistency.
  2. We call .show()

It makes sense for these two things to occur in the stated order, to avoid temporary view state artifacts to flicker when the user opens the dialog.

Considering the amount of dialogs we have, and that they all essentially work in the same way, we could consolidate the flow, and encode a unified initialization process in the code.

Potential Solution

We use an event-based flow:

  • show() could dispatch two custom events into the dialog component:
    • dialog-requested: the dialog may listen to that event, to carry out it’s general init procedure (if applicable)
    • dialog-shown: the dialog may listen to that event and do final adjustments, e.g. for setting focus (if applicable)

I believe the work items for that would be these:

  • Dispatch dialog-requested and dialog-shown events from <overlay-panel> component
    • We may want to declare these events in events.js
  • Set up respective event listeners in all dialogs (if applicable), and remove the initialize() et al calls from app.js
    • Attaching the dialog-shown listener in <paste-dialog> would resolve this issue.
  • Document initialization flow, either in <overlay-panel> component, or in events.js
  • Take care of edge cases (though after briefly scanning the code, there only appears to be one):

jotaen4tinypilot avatar Apr 01 '24 19:04 jotaen4tinypilot