Implement dynamic route titles

[torok-peter]

Plus

• Add a missing @Test annotation
• Add a comment to explain an unclear code part

Fixes #2397

---

This change is 

[denis-anisimov]

Reviewed 5 of 8 files at r1, 3 of 3 files at r2.
Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

---

flow-server/src/main/java/com/vaadin/router/NavigationStateRenderer.java, line 251 at r2 (raw file):

if (routeTarget instanceof HasDynamicTitle) {
title = ((HasDynamicTitle) routeTarget).getTitle();
} else {

I think the requirement in javadoc of HasDynamicTitle should be forced here:
throw a runtime exception if routerTarget implements HasDynamicTitle and has HasTitle annotation.

---

flow-server/src/main/java/com/vaadin/router/NavigationStateRenderer.java, line 254 at r2 (raw file):

lookForTitleInParents(routeLayoutTypes)

Isn't HasTitle annotation inherited ?
In this case getAnnotation method should return annotation from some parent class even if it's not directly in the class itself.
Please revisit.

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

---

flow-server/src/main/java/com/vaadin/server/startup/RouteRegistryInitializer.java, line 72 at r2 (raw file):

route.isAnnotationPresent(Title.class)

It might me that another method should be used for inherited annotations.

By the way what about situation: class has HasTitle and its parent has HasDynamicTitle(especially if the annotation is inherited).

---

Comments from Reviewable

[caalador]

Review status: 8 of 9 files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.

---

flow-documentation/routing/tutorial-routing-page-titles.asciidoc, line 41 at r3 (raw file):

Implementing the interface `HasDynamicTitle` allows us to change the title
from Java at runtime:
[source,java]

all source parts need to also be added to a java class. in this case to com.vaadin.flow.tutorial.routing.RoutingViewTitles

---

flow-server/src/main/java/com/vaadin/router/NavigationStateRenderer.java, line 254 at r2 (raw file):

Previously, denis-anisimov (Denis) wrote…

lookForTitleInParents(routeLayoutTypes)

Isn't HasTitle annotation inherited ?
In this case getAnnotation method should return annotation from some parent class even if it's not directly in the class itself.
Please revisit.

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.

When section titles is implemented then the title for the activated navigation target is sent to all parent layouts so that they may update or replace the title.

---

flow-server/src/main/java/com/vaadin/server/startup/RouteRegistryInitializer.java, line 72 at r2 (raw file):

Previously, denis-anisimov (Denis) wrote…

route.isAnnotationPresent(Title.class)

It might me that another method should be used for inherited annotations.

By the way what about situation: class has HasTitle and its parent has HasDynamicTitle(especially if the annotation is inherited).

As replied above, parent layout title information should not be considered in any way for this as it's only a restriction for the route target.

---

Comments from Reviewable

[denis-anisimov]

Reviewed 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.

---

flow-server/src/main/java/com/vaadin/router/NavigationStateRenderer.java, line 254 at r2 (raw file):

Previously, caalador wrote…

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

When section titles is implemented then the title for the activated navigation target is sent to all parent layouts so that they may update or replace the title.

Oh, I misread the code, my bad.
I thought it introspect the Java hierarchy .
But yeah, it introspects the Component hierarchy.
Then my comments has no sense.

---

Comments from Reviewable

[torok-peter]

Review status: all files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.

---

flow-server/src/main/java/com/vaadin/router/NavigationStateRenderer.java, line 251 at r2 (raw file):

Previously, denis-anisimov (Denis) wrote…

if (routeTarget instanceof HasDynamicTitle) {
title = ((HasDynamicTitle) routeTarget).getTitle();
} else {

I think the requirement in javadoc of HasDynamicTitle should be forced here:
throw a runtime exception if routerTarget implements HasDynamicTitle and has HasTitle annotation.

It is already enforced at startup time, in RouteRegistryInitializer.checkForConflictingAnnotations().

---

flow-server/src/main/java/com/vaadin/server/startup/RouteRegistryInitializer.java, line 72 at r2 (raw file):

Previously, caalador wrote…

As replied above, parent layout title information should not be considered in any way for this as it's only a restriction for the route target.

Hmmm... good point!

Added a test case for it.

---

Comments from Reviewable

[torok-peter]

Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

---

flow-documentation/routing/tutorial-routing-page-titles.asciidoc, line 41 at r3 (raw file):

Previously, caalador wrote…

all source parts need to also be added to a java class. in this case to com.vaadin.flow.tutorial.routing.RoutingViewTitles

Done.

---

Comments from Reviewable

[caalador]

Reviewed 2 of 3 files at r4.
Review status: 9 of 10 files reviewed at latest revision, 2 unresolved discussions.

---

flow-server/src/test/java/com/vaadin/router/RouterTest.java, line 467 at r4 (raw file):

    @Test
    public void page_title_set_from_annotation_in_parent()

This should not set the title. Only title from the routeTarget is to be used.

---

Comments from Reviewable

[torok-peter]

Review status: 9 of 10 files reviewed at latest revision, 2 unresolved discussions.

---

flow-server/src/test/java/com/vaadin/router/RouterTest.java, line 467 at r4 (raw file):

Previously, caalador wrote…

This should not set the title. Only title from the routeTarget is to be used.

Done.

---

Comments from Reviewable

[caalador]

Reviewed 1 of 3 files at r4, 3 of 3 files at r5.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[caalador]

Code needs fixes after merging master.

[caalador]

Reviewed 1 of 1 files at r6, 2 of 2 files at r8.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable