vaadin / quarkus

An extension to Quarkus to support Vaadin Flow
Apache License 2.0
28 stars 3 forks source link

@PageTitle not working on vaadin-quarkus #67

Open segersb opened 2 years ago

segersb commented 2 years ago

The @PageTitle annotation is looked up on the route target class by Vaadin

    private static Optional<PageTitle> lookForTitleInTarget(Component routeTarget) {
        return Optional.ofNullable(routeTarget.getClass().getAnnotation(PageTitle.class));
    }

However, when using Quarkus the route target is a proxy subclass. Since the @PageTitle annotation is not inherited, the annotation is not found.

I was able to fix the issue by overriding the Vaadin @PageTitle annotation and adding @Inherited

@Inherited
@Retention(RetentionPolicy.RUNTIME)
@Target({ElementType.TYPE})
public @interface PageTitle {
    String value();
}
caalador commented 2 years ago

I noted that running the project using java -jar target/quarkus-app/quarkus-run.jar the page title was set as expected, but not when running mvn quarkus:dev

mcollovati commented 2 years ago

It seems that in dev mode all beans are intercepted by Quarkus for monitoring, so this is why page title may work when running the production jar, but not in dev mode. Monitoring can be disabled in dev mode (https://quarkus.io/guides/cdi-reference#quarkus-arc_quarkus.arc.dev-mode.monitoring-enabled), but the problem may persist if the component is intercepted for other reasons.

Maybe instead of making @PageTitle inheritable, we can refactor AbstractNavigationStateRenderer.lookForTitleInTarget(...) (and similar cases) in Flow to make use of Vaadin's AnnotationReader.getAnnotationFor(...), that searches for the annotation on the whole class hierarchy.

MarcinVaadin commented 1 year ago

According to https://github.com/vaadin/flow/pull/2590:

Also there is AnnotationReader class which contains utility methods for reading annotation and it seems even read title methods.

Title should only be taken from the activated navigation target and parents should not be taken into account.

Somehow similar case but with HasDynamicTitle has been discussed in https://github.com/vaadin/flow/issues/13871

ErrorProne commented 1 year ago

Side note: Since this doesn't seem to be fixed any time soon, it should get a note in the "known Issues" section: https://vaadin.com/docs/latest/integrations/quarkus/#quarkus.vaadin.knownissues I've searched for quite a long time to get this working until I started to suspect a bug in integration, which I guess, is not the first thing most devs will assume.

mcollovati commented 1 year ago

@ErrorProne thanks for pointing out. This should definitely be documented until we can have it properly fixed. I created an issue (vaadin/docs#2550) in the docs repository