Integrate Voyager #87

Oleg-Melnik · 2024-12-16T08:13:10Z

This PR integrates the Voyager into the application.

From now on, Chords provides two levels of navigation enabled by the Voyager multiplatform navigation library. The first level is screen-level navigation, which allows selecting a screen to be displayed across the entire main window. Currently, two screens can be displayed: SignInScreen for user authentication and the MainScreen, which is shown when a user is logged into the system. There is no public API available to display a custom screen at the moment. This may change in the future as relevant use-cases appear.

The second level — for selecting an AppView to be displayed on the MainScreen.

The ApplicationUI, which is a public API for end user, has been updated with the following changes:

Added select(View) and currentView methods to manage application views.
The methods show(Screen) and closeCurrentScreen have been removed since DomainImportWizard has become a dialog and there are no more use-cases to support Screen API.

Currently the ApplicationUI looks like the following:

/**
 * A top-level API that concerns the application's UI.
 *
 * @param appWindow The main application's window.
 */
public class ApplicationUI
internal constructor(private val appWindow: AppWindow) {

    /**
     * Selects the given [appView] to be displayed on the [MainScreen].
     *
     * @param appView The view to be shown.
     */
    public fun select(appView: AppView) {
        appWindow.select(appView)
    }

    /**
     * Returns the currently selected [AppView] on the [MainScreen].
     */
    public val currentView: AppView get() = appWindow.currentView

    /**
     * Displays the given [Dialog] instance.
     *
     * When the modal window is shown, no other components from other screens
     * will be interactable, focusing user interaction on the modal content.
     *
     * It is designed to be used only as an internal low-level API, for dialog
     * implementation to be able to display themselves. In regular application
     * code though, the [Dialog]'s API should be used instead. For example,
     * when needed to display a specific dialog `SomeDialog` in the application,
     * the proper way to do this is like this:
     *
     * ```
     *    SomeDialog.open()
     *
     *    // or like this if dialog' properties need to be specified as well:
     *
     *    SomeDialog.open {
     *        prop1 = prop1Value
     *        prop2 = prop2Value
     *      ...
     *    }
     *
     *    // or if you already have a dialog instance and need to display it:
     *
     *    val dialog: SomeDialog = ...
     *    dialog.open()
     * ```
     *
     * @param dialog The [Dialog] instance, which needs to be displayed.
     *
     * @see Dialog
     * @see DialogSetup
     * @see closeDialog
     */
    internal fun openDialog(dialog: Dialog) {
        appWindow.openDialog(dialog)
    }

    /**
     * Closes the specified dialog if it doesn't have any nested
     * dialogs displayed.
     *
     * Note that this method ignores any data that might have been entered
     * in it.
     *
     * On a par with [openDialog], this is a part of an internal API for
     * [Dialog]s to be able to control their display lifecycle.
     *
     * @param dialog The dialog that needs to be closed.
     * @see openDialog
     */
    internal fun closeDialog(dialog: Dialog) {
        appWindow.closeDialog(dialog)
    }
}

# Conflicts: # dependencies.md # pom.xml # version.gradle.kts

armiol

@Oleg-Melnik 6/14 reviewed so far. Please see my comments. Also, as soon as this change is massive in terms of API changes, please expand the PR description so that "before" and "after" are clearer for readers.

armiol · 2024-12-23T12:46:50Z

buildSrc/src/main/kotlin/io/spine/internal/dependency/Voyager.kt

+object Voyager {
+    private const val version = "1.0.0"
+    private const val group = "cafe.adriel.voyager"
+    private const val baseName = "voyager"


Just name is enough, given this is a private const.

core/build.gradle.kts

core/src/main/kotlin/io/spine/chords/core/appshell/AppView.kt

armiol · 2024-12-23T12:49:36Z

core/src/main/kotlin/io/spine/chords/core/appshell/AppWindow.kt

@@ -98,6 +96,12 @@ public class AppWindow(
     */
    private var bottomDialog by mutableStateOf<Dialog?>(null)

+    /**
+     * An instance of the screens [Navigator] that will be initialized during


"screens"? Plural?

Please review.

armiol · 2024-12-23T12:50:20Z

core/src/main/kotlin/io/spine/chords/core/appshell/AppWindow.kt

-        }
+    internal fun showScreen(
+        screen: Screen,
+        keepCurrentScreenInHistory: Boolean = true


keepInHistory?

This parameter is removed at all.

armiol · 2024-12-23T12:51:22Z

core/src/main/kotlin/io/spine/chords/core/appshell/AppWindow.kt

-     * display is requested while another screen is already displayed.
+     * @param screen The screen to be shown.
+     * @param keepCurrentScreenInHistory Specifies whether to save the currently
+     * visible screen in the history or not.


"or not" is redundant when used in conjunction with whether. Unless you really-really want to put emphasis on this "or not", which I don't think is the case.

armiol · 2024-12-23T12:51:48Z

core/src/main/kotlin/io/spine/chords/core/appshell/AppWindow.kt

-     * @throws IllegalStateException
-     *          to indicate the illegal state when another modal screen
-     *          is already displayed.
+     * @throws IllegalStateException To indicate the illegal state when some


Please explain why this case is an illegal one.

armiol · 2024-12-23T12:52:24Z

core/src/main/kotlin/io/spine/chords/core/appshell/AppWindow.kt

-            "Cannot display the modal screen when a dialog is displayed."
+            "Cannot display the screen when a dialog is displayed."
+        }
+        checkScreenNavigatorIsInitialized()


Let's shorten the name of this method.

armiol · 2024-12-23T12:53:18Z

core/src/main/kotlin/io/spine/chords/core/appshell/AppWindow.kt

+    internal fun closeCurrentScreen() {
+        checkScreenNavigatorIsInitialized()
+        check(screenNavigator!!.size > 1) {
+            "Cannot close the current screen because this is a bottom-most " +


If possible, let's add something that would allow the devs to identify the currently displayed screen once this exception is raised.

armiol · 2024-12-23T12:56:14Z

core/src/main/kotlin/io/spine/chords/core/appshell/Application.kt

+ *
+ * @param appWindow The main application's window.
+ */
+public class Screens


Why isn't this a part of AppWindow API?

The same question applies to Views below.

The Screens and Views classes have been removed.

dpikhulya

@Oleg-Melnik Please see my thoughts to consider.

dpikhulya · 2024-12-23T17:43:26Z

core/src/main/kotlin/io/spine/chords/core/appshell/AppWindow.kt

+     * An instance of the screens [Navigator] that will be initialized during
+     * the rendering of the main window.
+     */
+    private var screenNavigator: Navigator? = null


I'd consider just making it a lateinit non-nullable property because IIUC we expect it to be set upon the very first app window's rendering, and are apparently using it with !! ever since anyway (accompanied with defensive check...Initialized calls).

dpikhulya · 2024-12-23T22:06:43Z

core/src/main/kotlin/io/spine/chords/core/appshell/Application.kt

+    /**
+     * Displays the given [screen].
+     *
+     * This screen will be rendered using the entire area
+     * of the application window. No other components
+     * from other screens will be visible or interactable.
+     *
+     * @param screen The screen to be shown.
+     * @param keepCurrentScreenInHistory Specifies whether to save the currently
+     * visible screen in the navigation history or not. Default value is `true`.
+     */
+    public fun show(screen: Screen, keepCurrentScreenInHistory: Boolean = true) {
+        appWindow.showScreen(screen, keepCurrentScreenInHistory)
+    }
+
+    /**
+     * Closes the currently visible screen.
+     *
+     * The currently visible screen won't be saved to the navigation history and
+     * the top-most screen in the history will be displayed.
+     */
+    public fun closeCurrent() {
+        appWindow.closeCurrentScreen()
+    }


Regarding the screens management API in general:

Regarding documentation.
As a newcomer, I feel I'm missing information on the model of screen management mechanics in general. I've partially figured it out by checking out the implementation, but here are things where I had and still partially have some doubts, which might IMO be useful to document:

Does the Application (or its constituents) track some predefined/custom-supplied list of screens itself, and this is an API for switching between them (similar to how we deal with views)? Or this is just an API that covers the navigation functionality only, while leaving the actual definition of the list of screens, their lifecycle/storage/etc. to be on behalf of the actual application's implementation?

Are screens a one-level notion or they are hierarchical (I know they were one-level and we discussed them to be like this, but I got a little confused by the method pair show/close)? This might either be a lack of documentation, or/and the need for correcting an API (I'm metioning it below separately in context of API).

Since we now have both screens and views as part of public API, I believe it would be useful to document both of these notions and their relationship with each other explicitly as well in some general place.

How exactly does history work in context of having multiple screens and views? IMO it would be useful to describe this in some general form. In particular, I can image having such questions:

What is the very first screen that is displayed? Can there be no screens displayed at all?

What if I invoke "show" for the same screen more than once (in a row, or interlaced with showing other screens), without invoking "close" for it? Does it mean pulling that screen from the middle of the deck of screens and putting it on top again? Will the first "show" call for the same screen still remain in history in this case though?

How does views history relate to screens history (if there's history for views at all)?

History support in general, the form of API.

Overall, to be honest, my preference regarding the history API would be to refrain from introducing it (at least in a public form) until we have some actual clearly defined needs that we practically have to address. Guessing potential scenarios leads to an opinionated, and possibly excessively complex API that we'll have to deal with, but do we really have a clear picture that we'll need history support in a desktop app?

The thing is, if we don't need history, we could probably keep the API as simple as one property like currentScreen!

Some potentially possible history-involving scenarios could possibly require alternative forms of APIs, for example, the "keepInHistory" flag to actually be a part of the screen instead, or, possibly, having some app-wide "trackingHistory" flag, which would be turned on/off globally, etc. In other words, we introduce a solution, but what are the problems? And are we sure it's the best/most convenient/adequate form of a solution for those problems?

These are just my thoughts to consider. We can discuss this if needed or you can decide whatever would be appropriate at this point.

The Screens API will be removed once ImportDomainWizard becomes a dialog.

dpikhulya · 2024-12-23T22:16:31Z

core/src/main/kotlin/io/spine/chords/core/appshell/Application.kt

+    public fun select(appView: AppView) {
+        appWindow.selectView(appView)
+    }
+
+    /**
+     * Returns the currently selected view.
+     */
+    public fun current(): AppView = appWindow.currentView()


Depending on anticipated scenarios that we aim to support, I'd also consider a possibility to replace both of these methods with a single mutable property like currentView. Just a thought to ponder.

dpikhulya · 2024-12-23T23:22:36Z

core/src/main/kotlin/io/spine/chords/core/appshell/SignInScreen.kt

+    @Composable
+    override fun Content() {
+        content {
+            onSuccessAuthentication.invoke()


Since onSuccessAuthentication is not nullable, I'd rather use just onSuccessAuthentication() for conciseness.

dpikhulya · 2024-12-23T23:26:36Z

core/src/main/kotlin/io/spine/chords/core/appshell/SignInScreen.kt

+/**
+ * Represents the Sign-In screen in the application.
+ */
+public class SignInScreen(


Do I understand correctly that so far it's only used internally in the library (instantiated to initialize AppWindow.signInScreen), and it's not meant to be overridden or used outside of the library? If so, can we make it internal?

Actually, if it's really a specifics of implementing AppWindow, I'd also consider declaring it inside of AppWindow.kt, since having it in a separate file, especially the one in the root package, provokes thoughts on its reusability.

armiol

@Oleg-Melnik see my comments.

My primary concern is explaining to our end-users that difference that we are trying to establish between "views" and "screens". We mix the interfaces and words in our code right now so that it becomes difficult to comprehend.

armiol · 2024-12-24T16:05:36Z

core/src/main/kotlin/io/spine/chords/core/appshell/AppView.kt

@@ -56,6 +57,10 @@ import io.spine.chords.core.Component
 * It is equally possible to declare [AppView] subclasses as regular classes
 * if needed.
 *
+ * The [AppView] implements the [Screen] interface, enabling the use of


Here, it's the same question regarding exposing Voyager API. If AppView is public, and it acts as a Voyager's Screen, then end-users will need Voyager in their classpath. Hence, it must be api.

Please correct me if I am wrong.

armiol · 2024-12-24T16:06:06Z

core/src/main/kotlin/io/spine/chords/core/appshell/AppWindow.kt

-        check(currentScreen == mainScreen) {
-            "Another modal screen is visible already."
-        }
+    internal fun showScreen(screen: Screen) {


Can we shorten it to show(screen: Screen)?

armiol · 2024-12-24T16:07:17Z

core/src/main/kotlin/io/spine/chords/core/appshell/AppWindow.kt

@@ -196,3 +221,18 @@ public class AppWindow(
        }
    }
 }
+
+/**
+ * Represents the Sign-In screen in the application.


Let's simplify without this pretentious "Represents":

A sign-in screen of the application.

armiol · 2024-12-24T16:10:05Z

core/src/main/kotlin/io/spine/chords/core/appshell/Application.kt

    }

+    /**
+     * Returns the currently selected view.


There should be a place somewhere in the code docs where we explain the difference between a view and a screen. Or telling that this is the same thing (which it is not, as far as we discussed).

Current API of AppView and the API of this type are confusing: they have both "show..." methods working with Screens and currentView returning AppView (which is ALSO as Screen).

armiol · 2024-12-24T16:11:51Z

pom.xml

+  <dependency>
+    <groupId>cafe.adriel.voyager</groupId>
+    <artifactId>voyager-screenmodel</artifactId>
+    <version>1.0.0</version>


They have 1.0.1, which I believe is a bugfix release.

# Conflicts: # dependencies.md

armiol

@Oleg-Melnik LGTM in general. However, please see my comments.

armiol · 2024-12-30T13:38:39Z

core/build.gradle.kts

    implementation(compose.desktop.currentOs)
    implementation(Material3.Desktop.lib)
+    api(Voyager.navigator)


Let's move this to be the first item. It's more logical, because api-level dependencies are transitive.

armiol · 2024-12-30T13:39:27Z

core/src/main/kotlin/io/spine/chords/core/appshell/AppView.kt

@@ -56,6 +57,10 @@ import io.spine.chords.core.Component
 * It is equally possible to declare [AppView] subclasses as regular classes
 * if needed.
 *
+ * Implements the [Screen] interface, enabling the use of navigation
+ * functionality provided by the Voyager multiplatform navigation library.
+ * See [AppWindow] on how to select some [AppView] to be displayed.


It would be good to provide the link to some particular method, not just to a type.

armiol · 2024-12-30T13:45:28Z

core/src/main/kotlin/io/spine/chords/core/appshell/AppWindow.kt

 *
- * @param signInScreenContent A content for the sign-in screen.
- * @param views The list of application's views.
+ * Provides two levels of navigation enabled by the Voyager multiplatform


Let's simplify:

* Powered by Voyager, provides two levels of navigation: * * 1.The screen-level navigation. Allows selecting a screen * to fill the entire main window. * * Currently, there is no public API available to display a custom screen. * Instead, two screens are predefined: [SignInScreen] and [MainScreen]. * Please refer to their description for more detail. * In the future, more screen implementations may be created. * * 2. The view-level navigation. Allows selecting an [AppView] to be displayed * on the `MainScreen`. * Please refer to [ApplicationUI] <TODO: can we point to the method here?> * for details.

dpikhulya

@Oleg-Melnik LGTM with just a small idea to consider.

dpikhulya · 2024-12-30T17:13:22Z

core/src/main/kotlin/io/spine/chords/core/appshell/Application.kt

+    public fun select(appView: AppView) {
+        appWindow.select(appView)
    }

    /**
-     * Closes the currently visible modal screen.
-     *
-     * @throws IllegalStateException
-     *         to indicate the illegal state when no modal screen to close.
+     * Returns the currently selected [AppView] on the [MainScreen].
     */
-    public fun closeCurrentModalScreen() {
-        appWindow.closeCurrentModalScreen()
-    }
+    public val currentView: AppView get() = appWindow.currentView


@Oleg-Melnik I think you've considered this, and there probably were some arguments for having the select() method separately, but, just in case, I'd like to pinpoint an alternative that might seem more natural if no changes to select are planned in the future:

we could actually replace select with turning currentView into a mutable property and having the respective functionality in its setter.

Oleg-Melnik added 3 commits December 16, 2024 09:09

Add Voyager dependency.

15a7a41

Use Navigator to show app screens and views.

119c167

Bump version -> 2.0.0-SNAPSHOT.55.

cdde412

Oleg-Melnik self-assigned this Dec 16, 2024

Oleg-Melnik added 10 commits December 16, 2024 09:52

Use Navigator for app views.

6c7c44a

Update docs.

a5f2d23

Update docs.

d26a92a

Merge remote-tracking branch 'origin/master' into voyager_screens

372843d

# Conflicts: # dependencies.md # pom.xml # version.gradle.kts

Bump version —> 2.0.0-SNAPSHOT.57

fee606e

Implement Views API.

8e6ea77

Update version of Spine-base dependency.

99639d7

Update version of Spine-base dependency.

d155ff8

Update docs.

61c4155

Clean up code.

74a5e58

Oleg-Melnik marked this pull request as ready for review December 23, 2024 12:42

Oleg-Melnik requested review from dpikhulya and armiol December 23, 2024 12:42

armiol requested changes Dec 23, 2024

View reviewed changes

Oleg-Melnik added 6 commits December 23, 2024 14:56

Refactor constants in Voyager dependency.

dc6987c

Update docs.

4ea0fc3

Update docs.

6aedf5b

Clean up code.

3a7ae3b

Clean up code.

40bfca5

Update docs.

7d01f59

dpikhulya reviewed Dec 23, 2024

View reviewed changes

Oleg-Melnik added 5 commits December 24, 2024 14:03

Use lateinit for Navigator variable.

f0e07df

Declare SignInScreen inside AppWindow.

2603a73

Refactor Screens API.

86e6111

Refactor Views API.

b4aa1b9

Refactor View and Screen API.

083c6ae

Oleg-Melnik requested review from dpikhulya and armiol December 24, 2024 16:02

armiol requested changes Dec 24, 2024

View reviewed changes

Oleg-Melnik added 13 commits December 26, 2024 15:01

Sort out dependencies.

0a8d57a

Update Voyager version.

ee3bd5c

Merge remote-tracking branch 'origin/master' into voyager_screens

f8c97ea

# Conflicts: # dependencies.md

Bump version —> 2.0.0-SNAPSHOT.58

ecea4cf

Update docs.

3ade5da

Update docs.

8538c1b

Refactor Screen and View API.

0061e1c

Update docs.

e71e2c2

Remove Screen API.

7eee2f0

Clean up code.

acc7571

Clean up code.

5bef49a

Update docs.

b9fa642

Update version of Spine-validation dependency.

3234331

Oleg-Melnik requested a review from armiol December 30, 2024 12:39

armiol approved these changes Dec 30, 2024

View reviewed changes

Oleg-Melnik added 3 commits December 30, 2024 15:16

Sort out dependencies.

b125462

Update docs.

0c4d024

Update docs.

412cd3a

dpikhulya approved these changes Dec 30, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrate Voyager #87

Integrate Voyager #87

Oleg-Melnik commented Dec 16, 2024 •

edited

Loading

armiol left a comment

armiol Dec 23, 2024

armiol Dec 23, 2024

armiol Dec 23, 2024

Oleg-Melnik Dec 24, 2024

armiol Dec 23, 2024

armiol Dec 23, 2024

armiol Dec 23, 2024

armiol Dec 23, 2024

armiol Dec 23, 2024

Oleg-Melnik Dec 24, 2024

dpikhulya left a comment

dpikhulya Dec 23, 2024

dpikhulya Dec 23, 2024 •

edited

Loading

Oleg-Melnik Dec 24, 2024

dpikhulya Dec 23, 2024

dpikhulya Dec 23, 2024

dpikhulya Dec 23, 2024

armiol left a comment

armiol Dec 24, 2024

armiol Dec 24, 2024

armiol Dec 24, 2024

armiol Dec 24, 2024

armiol Dec 24, 2024

armiol left a comment

armiol Dec 30, 2024

armiol Dec 30, 2024

armiol Dec 30, 2024

dpikhulya left a comment

dpikhulya Dec 30, 2024

Integrate Voyager #87

Are you sure you want to change the base?

Integrate Voyager #87

Conversation

Oleg-Melnik commented Dec 16, 2024 • edited Loading

armiol left a 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

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

dpikhulya left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpikhulya Dec 23, 2024 • 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

armiol left a 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

armiol left a 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

dpikhulya left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Oleg-Melnik commented Dec 16, 2024 •

edited

Loading

dpikhulya Dec 23, 2024 •

edited

Loading