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

Do not stack several automatic dialogs #1549

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ArturoManzoli
Copy link
Contributor

  • Implemented a dialog queue to handle potential modal stack-ups.

src/App.vue Show resolved Hide resolved
Comment on lines -2 to +4
<v-dialog v-model="internalShowDialog" :persistent="persistent" :width="maxWidth || 'auto'">
<v-dialog
v-model="internalShowDialog"
:persistent="persistent"
Copy link
Member

Choose a reason for hiding this comment

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

In this case are we coupling the interaction dialog to the queue system? It seems strange, since we use it for a lot of popups that are open explicitly by the user (and not automatically), like the about page, menus, error dialogs, etc.

It seems to me they should be completely uncoupled, and the queue system should live outside it and only for automatically-issued dialogs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The other dialogs will be unchanged and uncoupled from the queue. The ones that will be queued are only those that can potentially stack up, like the ones on the Cockpit startup.

It's also done like this on snackbar queue systems. Whenever a message can stackup, you queue it.

Copy link
Member

Choose a reason for hiding this comment

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

I mean that the queue system should be external to the InteractionDialog.vue component, otherwise we are putting queue logic on dialogs that should not be queue-aware.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In reality the InteractionDialog is uncoupled from the queue system.

The queue uses a logic based on dependency injection. If you check on App.vue, the components are loaded on startup and queued independently and the enqueueDialog function takes the component, its props and an id as parameter.

interfaceStore.enqueueDialog({ id: 'VehicleDiscoveryDialog', component: VehicleDiscoveryDialog, props: {}, })

For example, the system update dialog will only be queued if the function that checks for new versions triggers the dialog and will inject the component on the function to do so.
Actually it doesn't have to be a dialog. Could be virtually any other component we have.

Copy link
Member

@rafaellehmkuhl rafaellehmkuhl Jan 17, 2025

Choose a reason for hiding this comment

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

What I'm saying is that the interaction dialog should not be the one deciding on what happens with the log queue (interfaceStore.openNextDialogOnQueue()), but instead there should be an observer outside the interaction dialog checking if there's any interaction dialog open, and if not, open the next dialog in the queue.

The interaction dialog should be what is is, a dialog. It does not need to be aware of the queue. It's an unnecessary architecture coupling.

It's the same problem we had before, that you're fixing in this PR, about the interaction dialog being aware of the Tutorial functionality (it shouldn't).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But the InteractionDialog isn't aware of the queue. The only modification I made to it was the closing logic so it can handle the queue

@@ -0,0 +1,14 @@
export type DialogOnQueue = {
Copy link
Member

Choose a reason for hiding this comment

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

To not confuse it with typescript's interface, could we rename this file to ui.ts?

src/App.vue Show resolved Hide resolved
src/App.vue Show resolved Hide resolved
Comment on lines -2 to +4
<v-dialog v-model="internalShowDialog" :persistent="persistent" :width="maxWidth || 'auto'">
<v-dialog
v-model="internalShowDialog"
:persistent="persistent"
Copy link
Member

@rafaellehmkuhl rafaellehmkuhl Jan 17, 2025

Choose a reason for hiding this comment

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

What I'm saying is that the interaction dialog should not be the one deciding on what happens with the log queue (interfaceStore.openNextDialogOnQueue()), but instead there should be an observer outside the interaction dialog checking if there's any interaction dialog open, and if not, open the next dialog in the queue.

The interaction dialog should be what is is, a dialog. It does not need to be aware of the queue. It's an unnecessary architecture coupling.

It's the same problem we had before, that you're fixing in this PR, about the interaction dialog being aware of the Tutorial functionality (it shouldn't).

@ArturoManzoli ArturoManzoli force-pushed the do-not-stack-auto-dialogs branch from 88a2acc to d6147e2 Compare January 17, 2025 13:14
Signed-off-by: Arturo Manzoli <[email protected]>

App: Implement startup dialogs queue

Signed-off-by: Arturo Manzoli <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants