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

Save and quit before navigating home from marker app (OT-884) #1147

Merged
merged 7 commits into from
Jun 5, 2024

Conversation

darrellcolehill
Copy link
Collaborator

@darrellcolehill darrellcolehill commented May 28, 2024

This change is Reviewable

Copy link
Collaborator

@AnonymousWalker AnonymousWalker left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @darrellcolehill and @jsarabia)


jvm/workbookapp/src/main/kotlin/org/wycliffeassociates/otter/jvm/workbookapp/ui/NavigationMediator.kt line 104 at r1 (raw file):

                val homePage = find<HomePage2>()
                fire(NavigationRequestEvent(homePage))
                NavigationRequestEvent(homePage)

what's this line for?

Copy link
Collaborator Author

@darrellcolehill darrellcolehill left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @AnonymousWalker and @jsarabia)


jvm/workbookapp/src/main/kotlin/org/wycliffeassociates/otter/jvm/workbookapp/ui/NavigationMediator.kt line 104 at r1 (raw file):

Previously, AnonymousWalker (Tony) wrote…

what's this line for?

Thanks for catching that! I am not sure why that is there.

Copy link
Collaborator

@jsarabia jsarabia left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @AnonymousWalker)

Copy link
Collaborator

@AnonymousWalker AnonymousWalker left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 3 files reviewed, 2 unresolved discussions (waiting on @darrellcolehill)


jvm/markerapp/src/main/kotlin/org/wycliffeassociates/otter/jvm/markerapp/app/view/MarkerView.kt line 72 at r2 (raw file):

        }

        subscribe<NavigationRequestBlockedEvent> {

Would it be possible to use the existing PluginCloseRequestEvent approach? I believe marker app is also a "plugin" and we have solved a similar problem using that event.

Copy link
Collaborator Author

@darrellcolehill darrellcolehill left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 3 files reviewed, 2 unresolved discussions (waiting on @AnonymousWalker)


jvm/markerapp/src/main/kotlin/org/wycliffeassociates/otter/jvm/markerapp/app/view/MarkerView.kt line 72 at r2 (raw file):

Previously, AnonymousWalker (Tony) wrote…

Would it be possible to use the existing PluginCloseRequestEvent approach? I believe marker app is also a "plugin" and we have solved a similar problem using that event.

The issue I was having with that was with coordinating the narration view model since it is still docked. Are you thinking of firing the PluginClosRequestEvent on home button click when the marker app is open?

Copy link
Collaborator Author

@darrellcolehill darrellcolehill left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 3 files reviewed, 3 unresolved discussions (waiting on @AnonymousWalker)


jvm/workbookapp/src/main/kotlin/org/wycliffeassociates/otter/jvm/workbookapp/ui/narration/NarrationViewModel.kt line 182 at r3 (raw file):

                }
            } else if (pluginOpenedProperty.value) {
                navigator.navigateHomeOnPluginClosed = true

@AnonymousWalker This could be modified by doing the following:

  1. Move navigateHomeOnPluginClosed from the navigator, to NarrationViewModel
  2. Allow PluginClosedEvent to take in an optional parameter named something like "naviateHome" that has a default value of false
  3. Have the navigator's subscription to PluginClosedEvent navigate home when navigateHome == true
  4. Pass the value of navigateHome when firing PluginClosedEvent in onChapterReturnFromPlugin

I am not crazy about adding another Boolean in the NarrationViewModel, but the above steps seem more in line with what we discussed earlier.

Copy link
Collaborator

@AnonymousWalker AnonymousWalker left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 3 files reviewed, 4 unresolved discussions (waiting on @darrellcolehill and @jsarabia)


jvm/workbookapp/src/main/kotlin/org/wycliffeassociates/otter/jvm/workbookapp/ui/narration/NarrationViewModel.kt line 186 at r4 (raw file):

        subscribe<NavigationRequestEvent>() {
            if (it.view == find<HomePage2>() && pluginOpenedProperty.value) {

could we do it.view is HomePage2 instead?

Copy link
Collaborator

@jsarabia jsarabia left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: 0 of 3 files reviewed, 4 unresolved discussions (waiting on @AnonymousWalker and @darrellcolehill)

@darrellcolehill darrellcolehill merged commit 973d14f into dev Jun 5, 2024
9 of 10 checks passed
@darrellcolehill darrellcolehill deleted the dh-home-navigation-from-marker-app-bug-OT-884 branch June 5, 2024 18:38
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.

None yet

3 participants