Do not stack several automatic dialogs #1549

ArturoManzoli · 2025-01-07T21:03:44Z

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

src/App.vue

rafaellehmkuhl · 2025-01-13T13:33:21Z

src/components/InteractionDialog.vue

-  <v-dialog v-model="internalShowDialog" :persistent="persistent" :width="maxWidth || 'auto'">
+  <v-dialog
+    v-model="internalShowDialog"
+    :persistent="persistent"


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.

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.

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.

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.

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).

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

Discussed the details in a call. Main topic was to uncouple to facilitate maintenance, shrink the codebase and allow easy future changes (for new auto-instantiated dialogs).

src/views/ConfigurationGeneralView.vue

src/types/interface.ts

src/App.vue

rafaellehmkuhl · 2025-01-17T12:57:03Z

src/components/InteractionDialog.vue

-  <v-dialog v-model="internalShowDialog" :persistent="persistent" :width="maxWidth || 'auto'">
+  <v-dialog
+    v-model="internalShowDialog"
+    :persistent="persistent"


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 · 2025-01-27T15:19:15Z

On last patch:

Dialog queue now monitors the dom for its open elements by id, and once they cease to exist, they open the next one on the line, decoupling the enqueued elements from the queue logic.
Reverted changes made on components like UpdateNotification, VehicleDiscoveryDialog and InteractiveDialog;

rafaellehmkuhl · 2025-01-27T16:39:48Z

src/App.vue

-  <Tutorial :show-tutorial="interfaceStore.isTutorialVisible" />
  <VideoLibraryModal :open-modal="interfaceStore.isVideoLibraryVisible" />
-  <VehicleDiscoveryDialog v-model="showDiscoveryDialog" show-auto-search-option />
-  <UpdateNotification v-if="isElectron()" />
+  <Teleport to="body">
+    <Tutorial />
+  </Teleport>
+
+  <!-- Startup dialogs queue -->
+  <div v-if="activeDialog">
+    <component :is="activeDialog.component" v-bind="activeDialog.props" :id="activeDialog.id" />
+  </div>


Why is the tutorial treated differently? Aren't it a automatic dialog like all others?

rafaellehmkuhl · 2025-01-27T16:40:44Z

src/components/Tutorial.vue

@@ -403,6 +392,7 @@ const handleKeydown = (event: KeyboardEvent): void => {
 watch(
  () => interfaceStore.isTutorialVisible,
  (val) => {
+    console.log('🚀 ~ val:', val)


This was in an old PR, was asked to be removed, and is being added here again.

rafaellehmkuhl · 2025-01-27T16:51:00Z

src/components/UpdateNotification.vue

+    if (interfaceStore.activeDialog?.id === 'UpdateNotification' || interfaceStore.activeDialog === undefined) {
+      showUpdateDialog.value = true
+      return
+    }


This is still putting the instantiation logic inside the components, just with another name/approach, and there's no need for that.

This is solvable with a much easier approach where the one controlling the issue of automatic notifications (the App.vue file) is the one who instantiates the component, based on the queue logic (show tutorial, when it closes show the discovery dialog, when it closes show the user selection, etc). There's no need for any logic around the automation of the component mounting to be inside the component itself.

Every new dialog component still has to implement logic around an automatic issuing, which it should't need to be aware off. The dialog components should only care about their own content.

ArturoManzoli · 2025-02-17T20:19:54Z

@rafaellehmkuhl,
I have updated the approach to the dialog queue. Now there is a composable that controls the queue and it only needs the dialogs to have a v-model and emit its update to it on close.
Now it works fine on both Electron and Web versions.

I think it's a cleaner and more efficient way to solve the dialog stack issue Cockpit have.

I had to slightly modify the opening logic on UpdateNotification.vue, besides the regular props.modelValue and emit.
Since I didn't have the option of conditionally adding this dialog to queue, I added a closing on no update available. This way the component unclogged the queue and life went on normally again :)

Signed-off-by: Arturo Manzoli <arturomanzoli@gmail.com>

rafaellehmkuhl · 2025-02-18T12:04:51Z

src/App.vue

-  <Tutorial v-if="interfaceStore.isTutorialVisible" />
  <VideoLibraryModal v-if="interfaceStore.isVideoLibraryVisible" />
-  <VehicleDiscoveryDialog v-model="showDiscoveryDialog" show-auto-search-option />
-  <UpdateNotification v-if="isElectron()" />
+  <Tutorial :model-value="interfaceStore.isTutorialVisible" />
+  <div v-if="WrappedDialog">
+    <component :is="WrappedDialog" />
+  </div>


To better understand: shouldn't the Tutorial be used in the queue system, since it's opened automatically as well?

This is not for the queue. this for the manual opening on the General settings. There is also a similar implementation for the vehicle discovery dialog.
The other way to manually open those, is to create a single item queue for them when manually called.

So if we open those manually and they are triggered to be open automatically, there will be two instances of them in the screen, not?

@ArturoManzoli

Maybe yes, but I see them far from each other in the sense of UI and how the user will use either the auto pop up and the manual instance of it.

Would be a big problem if the user manage to intentionally open two instances of those dialogs?

If so, is better to also enqueue the manual opening.

I don't see a problem with this as long as they are not the same dialog.

If the user has a tutorial that was automatically open, it's fine if they open a vehicle discovery one manually, and this manual one gets on top of the other, but if the user has a vehicle discovery manually opened, an automatic one should not be opened over it.

Does it make sense?

rafaellehmkuhl · 2025-02-18T12:06:57Z

src/components/Tutorial.vue

-const showTutorial = ref(true)
+const props = defineProps<{
+  /**
+   *


Missing docs.

rafaellehmkuhl · 2025-02-18T12:07:07Z

src/components/UpdateNotification.vue

 import { app_version } from '@/libs/cosmos'
 import { isElectron } from '@/libs/utils'

+const props = defineProps<{
+  /**
+   *


Missing docs

rafaellehmkuhl · 2025-02-18T12:09:01Z

src/components/UpdateNotification.vue

+    if (props.modelValue && updateAvailable.value) {
+      showUpdateDialog.value = true
+    } else {
+      openSnackbar({ message: 'No updates available.', variant: 'success' })


This notification is wrong. If there's an update available but the modelValue props is false, it will enter here.

rafaellehmkuhl

There's apparently a bug in the logic.

A tutorial is always opening here, even if I click not to show it again.

Kapture.2025-02-19.at.09.24.39.mp4

ArturoManzoli · 2025-02-24T11:52:27Z

There's apparently a bug in the logic.

A tutorial is always opening here, even if I click not to show it again.

Yep, the don't show again button behaves correctly on step 1. On other steps it's indeed not saving on the storage.
Fixing that...

rafaellehmkuhl

The PR is reviving a bug I solved some PRs ago, where the Tutorial is affecting the UI even when not opened.

This is happening because the v-if was removed from the Tutorial component.

rafaellehmkuhl · 2025-02-25T11:40:04Z

src/components/Tutorial.vue

@@ -208,31 +219,27 @@ const handleStepChange = (newStep: number): void => {
      interfaceStore.componentToHighlight = 'menu-trigger'
      interfaceStore.currentSubMenuComponentName = null
      interfaceStore.currentSubMenuName = null
-      interfaceStore.userHasSeenTutorial = false


Why is that being removed?

It makes sure the Tutorial is reopened if the user clicked to change a step, which seems like the logical behavior.

Actually this variable is used on Cockpit's boot to decide if the tutorial will be auto opened or not. On step change, the tutorial does not close and doesn't need this variable set anyway.

It was removed because it was causing the tutorial to always show on boot if it was closed on a step different than step zero. This variable should only be modified by the Don't show again or Show on startup buttons.

Having those lines was causing the last issue you mentioned, that the tutorial was always coming back on boot. (last friday)

But if the user has interacted with the tutorial, it means they are still on it and should indeed be auto-opened.

The bug I mentioned was not on master, because once you click the "don't show again" button and restarts, it wouldn't open again. That was introduced here. It's not on master.

Behavior on master:

Kapture.2025-02-25.at.11.20.03.mp4

I know. Sorry, maybe I wasn't clear on my last message.
I wasn't talking about master, but about your message on this PR, from last friday, saying

"There's apparently a bug in the logic.
A tutorial is always opening here, even if I click not to show it again."

This was being caused by this lines that set the tutorial as not seen each time the user change a step. Even if you close the tutorial it will be set as unseen.

This variable should only be set by direct user action. Once he is ok with what the tutorial has to offer, he can click on Don't show again and the tutorial will not pop out again.
If the user is currently seeing the tutorial and presses F5, the tutorial will pop out again anyway.
When the user clicks on Don't show again, the tutorial window closes. There is no need to keep this variable as false on every step change.

Give the following situation:

User has seen the tutorial before (has-seen-tutorial === true)

User clicks to open the tutorial explicitly

User starts interacting with the tutorial (clicks next step)

User or system itself restarts

Does it make sense for the tutorial not to reopen, and in the exactly step the user was in before the restart?

I don't think it is such a feature that is worth that much discussion over, but my point in the end is: if this is working on master and the logic makes sense, why is it being changed here?

I agree that this takes time from more important things :)

The feature that returned into the same step after a reboot was added originally only on step 5, that was when the user was being presented to the vehicle address configuration. On this tutorial step the general configuration page was opened and he was invited to input its vehicle network location. After that, cockpit reboots automatically and it was important to put the user exactly on the same spot he was before the mandatory reboot. That was the only need for the 'tutorial resume' function.
After that, some weeks ago, the tutorial was refactored to include the changes needed to handle the tools submenus, and during that it was added this resume feature to all of the steps. But this messes up with the original intention and with a variable that wasn't used for that.
This was causing some misfires from that variable, so I decided to set it back to the use it had before the refactoring.

Nothing really special about that, this was all to explain why I decided to delete those lines. Now the component works fine with the new modifications added (v-model and emits).

But the logic of reopening if the user interacted with it makes sense, that's why it was changed on that PR.

rafaellehmkuhl · 2025-02-25T11:42:00Z

src/components/Tutorial.vue

@@ -332,7 +333,6 @@ const alwaysShowTutorialOnStartup = (): void => {

 const nextTutorialStep = (): void => {
  if (currentTutorialStep.value === steps.length) {
-    dontShowTutorialAgain()


Same here. It makes sure the Tutorial is not reopened if the user has seem it entirely.

Ok, this one makes sense :)

Signed-off-by: Arturo Manzoli <arturomanzoli@gmail.com>

ArturoManzoli force-pushed the do-not-stack-auto-dialogs branch from d02f53d to 83a3bd7 Compare January 7, 2025 21:05

ArturoManzoli requested a review from rafaellehmkuhl January 7, 2025 21:06

rafaellehmkuhl requested changes Jan 13, 2025

View reviewed changes

ArturoManzoli force-pushed the do-not-stack-auto-dialogs branch from 83a3bd7 to 310c843 Compare January 15, 2025 10:46

ArturoManzoli requested a review from rafaellehmkuhl January 15, 2025 10:47

rafaellehmkuhl reviewed Jan 15, 2025

View reviewed changes

src/views/ConfigurationGeneralView.vue Outdated Show resolved Hide resolved

ArturoManzoli force-pushed the do-not-stack-auto-dialogs branch from 310c843 to 88a2acc Compare January 17, 2025 11:52

ArturoManzoli requested a review from rafaellehmkuhl January 17, 2025 11:53

rafaellehmkuhl requested changes Jan 17, 2025

View reviewed changes

ArturoManzoli force-pushed the do-not-stack-auto-dialogs branch 2 times, most recently from d6147e2 to f6e9ea4 Compare January 17, 2025 15:49

ArturoManzoli requested a review from rafaellehmkuhl January 17, 2025 15:52

ArturoManzoli force-pushed the do-not-stack-auto-dialogs branch from f6e9ea4 to 38901db Compare January 27, 2025 15:15

ArturoManzoli force-pushed the do-not-stack-auto-dialogs branch 2 times, most recently from 1495c59 to c5cb8ce Compare January 27, 2025 15:59

rafaellehmkuhl reviewed Jan 27, 2025

View reviewed changes

rafaellehmkuhl requested changes Jan 27, 2025

View reviewed changes

ArturoManzoli force-pushed the do-not-stack-auto-dialogs branch from c5cb8ce to 2b70ec8 Compare January 27, 2025 17:19

ArturoManzoli marked this pull request as draft January 27, 2025 17:19

ArturoManzoli force-pushed the do-not-stack-auto-dialogs branch from 2b70ec8 to d0ea490 Compare January 27, 2025 18:32

ArturoManzoli force-pushed the do-not-stack-auto-dialogs branch 4 times, most recently from c725537 to f4753c6 Compare February 17, 2025 20:12

ArturoManzoli requested a review from rafaellehmkuhl February 17, 2025 20:20

ArturoManzoli force-pushed the do-not-stack-auto-dialogs branch from f4753c6 to e352580 Compare February 17, 2025 22:58

ArturoManzoli marked this pull request as ready for review February 17, 2025 23:34

Composables: Add dialog queue composable

7a381f4

Signed-off-by: Arturo Manzoli <arturomanzoli@gmail.com>

ArturoManzoli force-pushed the do-not-stack-auto-dialogs branch from e352580 to 0271bb7 Compare February 18, 2025 10:27

rafaellehmkuhl reviewed Feb 18, 2025

View reviewed changes

rafaellehmkuhl requested changes Feb 18, 2025

View reviewed changes

ArturoManzoli force-pushed the do-not-stack-auto-dialogs branch from 0271bb7 to 4a14b2b Compare February 18, 2025 12:52

ArturoManzoli requested a review from rafaellehmkuhl February 18, 2025 12:55

rafaellehmkuhl requested changes Feb 19, 2025

View reviewed changes

ArturoManzoli force-pushed the do-not-stack-auto-dialogs branch from 4a14b2b to 4b74a31 Compare February 24, 2025 12:45

ArturoManzoli requested a review from rafaellehmkuhl February 24, 2025 12:48

rafaellehmkuhl requested changes Feb 25, 2025

View reviewed changes

rafaellehmkuhl reviewed Feb 25, 2025

View reviewed changes

App: Startup dialogs: Implement changes to run under dialog queue

8eac571

Signed-off-by: Arturo Manzoli <arturomanzoli@gmail.com>

ArturoManzoli force-pushed the do-not-stack-auto-dialogs branch from 4b74a31 to 8eac571 Compare February 26, 2025 10:56

Do not stack several automatic dialogs #1549

Are you sure you want to change the base?

Do not stack several automatic dialogs #1549

Conversation

ArturoManzoli commented Jan 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rafaellehmkuhl Jan 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rafaellehmkuhl Jan 17, 2025 • edited Loading

Choose a reason for hiding this comment

ArturoManzoli commented Jan 27, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ArturoManzoli commented Feb 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rafaellehmkuhl Feb 18, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rafaellehmkuhl left a comment

Choose a reason for hiding this comment

ArturoManzoli commented Feb 24, 2025

rafaellehmkuhl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ArturoManzoli Feb 25, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ArturoManzoli Feb 25, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ArturoManzoli commented Jan 7, 2025 •

edited

Loading

rafaellehmkuhl Jan 17, 2025 •

edited

Loading

rafaellehmkuhl Jan 17, 2025 •

edited

Loading

ArturoManzoli commented Feb 17, 2025 •

edited

Loading

rafaellehmkuhl Feb 18, 2025 •

edited

Loading

ArturoManzoli Feb 25, 2025 •

edited

Loading

ArturoManzoli Feb 25, 2025 •

edited

Loading