getActiveViewLocation() returns obsolete data in view's constructor

[mvysny]

trafficstars

Description of the bug

Calling UI.getCurrent().getActiveViewLocation() from view's constructor returns obsolete data - both the path is old, and most importantly, the query parameters are obsolete too.

From my point of view, the current/active view is the new one being constructed (or navigated to), however the function seems to return location of the previous view. The javadoc is really short and doesn't really mention when exactly the new value will be available.

Would be great to either modify the function to return the view being constructed, or modify javadoc to mention clearly and beyond any doubt that the function may return obsolete data.

Expected behavior

getActiveViewLocation() should return up-to-date data, including this view's path and query parameters.

Minimal reproducible example

@Route("")
public class MainView extends VerticalLayout {

    public MainView() {
        add(new Button("Click", e -> UI.getCurrent().navigate(SecondView.class, QueryParameters.of("foo", "bar"))));
    }
    @Route("second")
    public static class SecondView extends VerticalLayout {
        public SecondView() {
            final Location loc = UI.getCurrent().getActiveViewLocation();
            add(new Span("Path " + loc.getPath()));
            add(new Span("QP " + loc.getQueryParameters()));
        }
    }
}

Paste this class into skeleton starter and press the "Click" button. I'd expect SecondView to print Path second and up-to-date query parameters, however it prints an empty path and empty query parameters.

Versions

• Vaadin / Flow version: 24.3.6 / 24.3.6
• Java version: openjdk 21.0.2
• OS version: Ubuntu 23.10

[knoobie]

From a technical point of view; the user is still on the other view - no before or after view observers were called; therefore no navigation took place in the constructor.. just staiting the obvious.. don't use UI.getCurrent in the constructor nor build your views in the if you depend on navigation data 😬

[mcollovati]

I agree with Knoobie. The navigation is still ongoing during view creation. Furthermore, the current view itself could not be in the location chain, since it is currently in creation. Improving Javadoc of getActiveViewLocation() to better clarify what it returns based on navigation lifecycle makes sense

[mvysny]

I'd argue that the navigation still ongoing during view creation is an implementation detail, and intuitively I'd perceive the current view being constructed as the active one, but then again it can be argued that even semantically, the "previous view" (currently attached) is the active one until the replacement is constructed and attached.

Given that it's easy to reach this kind of confusion, I'd propose to make the javadoc rock-solid and explicitly mention that during the construction phase of the new view, the "old" one is perceived as active.

Subjectively, it's a good practice to fully construct the view in the constructor, rather than wait for the Before/AfterNavigationObserver stuff to get called and initialize the view lazily. @mcollovati is it possible to obtain the path+query parameters already in view's constructor please? If not, I'll open a feature request.

[mcollovati]

@mvysny I think the information is currently only available in the event itself during view construction. A potential workaround could be to have a custom Instantiator that overrides createRouteTarget(Class<T> routeTargetType, NavigationEvent event) and stores the event in a place reachable by the route constructor (e.g. a ThreadLocal).

Something like this (but better implemented :wink: )

    class MyInstantiator extends DefaultInstantiator {
        @Override
        public <T extends HasElement> T createRouteTarget(Class<T> routeTargetType, NavigationEvent event) {
            return OngoingNavigation.execute(event, () -> getOrCreate(routeTargetType));
        }
    }

    class OngoingNavigation {
        private static ThreadLocal<NavigationEvent> EVENT = new ThreadLocal<>();

        public static <T> T execute(NavigationEvent event, Supplier<T> action) {
            EVENT.set(event);
            try {
                return action.get();
            } finally {
                EVENT.remove();
            }
        }

        public static Location requestedLocation() {
            return EVENT.get().getLocation();
        }
    }

    @Route("second")
    public static class SecondView extends VerticalLayout {
        public SecondView() {
            final Location loc = OngoingNavigation.requestedLocation();
            add(new Span("Path " + loc.getPath()));
            add(new Span("QP " + loc.getQueryParameters()));
        }
    }

[mvysny]

Hey, good solution, thanks @mcollovati ! Yet I kinda feel nervous fiddling with instantiators, especially when SpringInstantiator comes into play... But this is a nice workaround indeed.