Add highlight capabilities to RouterLink

[heruan]

Fixes #482 while tempting to leave an open door to support highlighting on other components, e.g. Tab.

Introduces two interfaces:

• HighlightCondition<T> extends SerializableBiPredicate<T, AfterNavigationEvent>
• HighlightAction<T> extends SerializableBiConsumer<T, Boolean>

with some predefined logic in HighlightConditions and HighlightActions.

Then enables RouterLink implementing AfterNavigationObserver with getter/setters for both (with reasonable defaults).

---

This change is 

[Legioth]

Reviewed 2 of 6 files at r1, 3 of 3 files at r2.
Review status: 5 of 6 files reviewed at latest revision, 14 unresolved discussions.

---

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

     *            the target of the highlight action
     * @param highlight
     *            true if the target should be highlighted

If this can be null, then it should be documented what that means.
If this cannot be null, then it should be boolean instead of Boolean.

Please also document what false means, i.e to explicitly clear any previously set highlight.

---

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

    public static <C extends HasStyle> HighlightAction<C> toggleClassName(
            String className) {
        return (link, highligh) -> {

1. link is a quite specific name in code that uses generics to not be tied to only RouterLink.
2. highligh -> highlight

These remarks also apply to lots of similar code in this file.

---

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

            String className) {
        return (link, highligh) -> {
            if (highligh) {

This can be shortened to a oneliner: link.getClassNames().set(className, highlight);

Also applies for toggleTheme.

---

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

    /**
     * Highlight if the navigation path is the same of the target

"same as"

---

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

    /**
     * Gets the {@link HighlightCondition} of this component.
     *

Add @see for the setter which should have a more thorough description of the feature.

---

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

     * Gets the {@link HighlightCondition} of this component.
     *
     * @return the highlight condition

May this be null?

---

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

    /**
     * Sets the {@link HighlightCondition} of this component.

Javadoc should describe what the default setting is. In general, the javadoc could also be a little more elaborative.

---

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

    public void setHighlightCondition(
            HighlightCondition<RouterLink> highlightCondition) {
        this.highlightCondition = highlightCondition;

Should probably throw if null

---

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

    /**
     * Gets the {@link HighlightAction} of this component.
     *

Add @see for the setter which should have a more thorough description of the feature.

---

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

     * Gets the {@link HighlightAction} of this component.
     *
     * @return the highlight action

May this be null?

---

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

    /**
     * Sets the {@link HighlightAction} of this component.

Javadoc should describe what the default setting is. In general, the javadoc could also be a little more elaborative.

---

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

    public void setHighlightAction(
            HighlightAction<RouterLink> highlightAction) {
        this.highlightAction = highlightAction;

This is kind of an edge case, but I think this should explicitly invoke the old action with false to give it a chance to remove any currently active highlight.

---

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

    public void setHighlightAction(
            HighlightAction<RouterLink> highlightAction) {
        this.highlightAction = highlightAction;

Should probably throw if null

---

flow-server/src/test/java/com/vaadin/flow/router/RouterLinkTest.java, line 345 at r2 (raw file):

    @Test
    public void testRouterLinkDefaultHighlightCondition()

It's quite hard to see what these methods test since most of the code is only about setting things up. I'd suggest extracting some helpers to make the actual "business logic" stand out.

---

Comments from Reviewable

[heruan]

Review status: 5 of 6 files reviewed at latest revision, 14 unresolved discussions.

---

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

Previously, Legioth (Leif Åstrand) wrote…

If this can be null, then it should be documented what that means.
If this cannot be null, then it should be boolean instead of Boolean.

Please also document what false means, i.e to explicitly clear any previously set highlight.

Done.

---

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

Previously, Legioth (Leif Åstrand) wrote…

1. link is a quite specific name in code that uses generics to not be tied to only RouterLink.
2. highligh -> highlight

These remarks also apply to lots of similar code in this file.

Done.

---

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

Previously, Legioth (Leif Åstrand) wrote…

This can be shortened to a oneliner: link.getClassNames().set(className, highlight);

Also applies for toggleTheme.

Done.

---

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

Previously, Legioth (Leif Åstrand) wrote…

"same as"

Done.

---

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

Previously, Legioth (Leif Åstrand) wrote…

Add @see for the setter which should have a more thorough description of the feature.

Done.

---

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

Previously, Legioth (Leif Åstrand) wrote…

May this be null?

Done.

---

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

Previously, Legioth (Leif Åstrand) wrote…

Javadoc should describe what the default setting is. In general, the javadoc could also be a little more elaborative.

Done.

---

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

Previously, Legioth (Leif Åstrand) wrote…

Should probably throw if null

Done.

---

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

Previously, Legioth (Leif Åstrand) wrote…

Add @see for the setter which should have a more thorough description of the feature.

Done.

---

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

Previously, Legioth (Leif Åstrand) wrote…

May this be null?

Done.

---

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

Previously, Legioth (Leif Åstrand) wrote…

Javadoc should describe what the default setting is. In general, the javadoc could also be a little more elaborative.

Done.

---

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

Previously, Legioth (Leif Åstrand) wrote…

This is kind of an edge case, but I think this should explicitly invoke the old action with false to give it a chance to remove any currently active highlight.

Done.

---

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

Previously, Legioth (Leif Åstrand) wrote…

Should probably throw if null

Done.

---

flow-server/src/test/java/com/vaadin/flow/router/RouterLinkTest.java, line 345 at r2 (raw file):

Previously, Legioth (Leif Åstrand) wrote…

It's quite hard to see what these methods test since most of the code is only about setting things up. I'd suggest extracting some helpers to make the actual "business logic" stand out.

Done.

---

Comments from Reviewable

[Legioth]

Reviewed 6 of 6 files at r3.
Review status: 5 of 6 files reviewed at latest revision, 8 unresolved discussions.

---

flow-server/src/main/java/com/vaadin/flow/router/HighlightCondition.java, line 37 at r3 (raw file):

     * @param event
     *            the navigation event
     */

@return

---

flow-server/src/main/java/com/vaadin/flow/router/HighlightCondition.java, line 38 at r3 (raw file):

     *            the navigation event
     */
    boolean test(T t, AfterNavigationEvent event);

Minor nit: This method name could be clarified now when it's no longer inherited from BiPredicate. E.g. shouldHighlight.

---

flow-server/src/main/java/com/vaadin/flow/router/RouterLink.java, line 447 at r3 (raw file):

    /**
     * Sets the {@link HighlightCondition} of this link, which determines if the

Javadoc could also provide a link to the factory class for built-in conditions.

---

flow-server/src/main/java/com/vaadin/flow/router/RouterLink.java, line 449 at r3 (raw file):

     * Sets the {@link HighlightCondition} of this link, which determines if the
     * link should be highlighted when a {@link AfterNavigationEvent} occurs.
     *

<p>

---

flow-server/src/main/java/com/vaadin/flow/router/RouterLink.java, line 465 at r3 (raw file):

    }

    /**

Javadoc could also provide a link to the factory class for built-in actions.

---

flow-server/src/main/java/com/vaadin/flow/router/RouterLink.java, line 482 at r3 (raw file):

     * Sets the {@link HighlightAction} of this link, which will be performed
     * with the evaluation of this link's {@link HighlightCondition}.
     *

<p>

---

flow-server/src/test/java/com/vaadin/flow/router/RouterLinkTest.java, line 434 at r3 (raw file):

        triggerNavigationEvent(router, link, "foo/bar");
        Assert.assertTrue(link.hasClassName("highlight"));

Could also test that no attribute has been added even though the condition is triggered.

---

flow-server/src/test/java/com/vaadin/flow/router/RouterLinkTest.java, line 437 at r3 (raw file):

        triggerNavigationEvent(router, link, "bar");
        Assert.assertFalse(link.getElement().hasAttribute("highlight"));

Shouldn't this check hasClassName?

---

Comments from Reviewable

[heruan]

Review status: 5 of 6 files reviewed at latest revision, 8 unresolved discussions.

---

flow-server/src/main/java/com/vaadin/flow/router/HighlightCondition.java, line 37 at r3 (raw file):

Previously, Legioth (Leif Åstrand) wrote…

@return

Done.

---

flow-server/src/main/java/com/vaadin/flow/router/HighlightCondition.java, line 38 at r3 (raw file):

Previously, Legioth (Leif Åstrand) wrote…

Minor nit: This method name could be clarified now when it's no longer inherited from BiPredicate. E.g. shouldHighlight.

Done.

---

flow-server/src/main/java/com/vaadin/flow/router/RouterLink.java, line 447 at r3 (raw file):

Previously, Legioth (Leif Åstrand) wrote…

Javadoc could also provide a link to the factory class for built-in conditions.

Done.

---

flow-server/src/main/java/com/vaadin/flow/router/RouterLink.java, line 449 at r3 (raw file):

Previously, Legioth (Leif Åstrand) wrote…

<p>

Done.

---

flow-server/src/main/java/com/vaadin/flow/router/RouterLink.java, line 465 at r3 (raw file):

Previously, Legioth (Leif Åstrand) wrote…

Javadoc could also provide a link to the factory class for built-in actions.

Done.

---

flow-server/src/main/java/com/vaadin/flow/router/RouterLink.java, line 482 at r3 (raw file):

Previously, Legioth (Leif Åstrand) wrote…

<p>

Done.

---

flow-server/src/test/java/com/vaadin/flow/router/RouterLinkTest.java, line 434 at r3 (raw file):

Previously, Legioth (Leif Åstrand) wrote…

Could also test that no attribute has been added even though the condition is triggered.

Is that necessary? It's like testing the setter itself, as the action is simply assigned to the field. I'd rather test the clearing of the old action when a new one is set.

---

flow-server/src/test/java/com/vaadin/flow/router/RouterLinkTest.java, line 437 at r3 (raw file):

Previously, Legioth (Leif Åstrand) wrote…

Shouldn't this check hasClassName?

Done.

---

Comments from Reviewable

[Legioth]

Reviewed 4 of 4 files at r4.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

a discussion (no related file):
I'm happy, but leaving final approval in the hands of the implementation team. (Reject me from this discussion when you're happy)

---

Comments from Reviewable

[caalador]

Reviewed 2 of 6 files at r3, 4 of 4 files at r4.
Review status: all files reviewed at latest revision, 4 unresolved discussions.

---

flow-server/src/main/java/com/vaadin/flow/router/RouterLink.java, line 461 at r4 (raw file):

    public void setHighlightCondition(
            HighlightCondition<RouterLink> highlightCondition) {
        assert highlightCondition != null;

I would expect that we actually throw here as seldom do developers run with assertions enabled.
So it might be good to just useObject.requireNonNull(highlightCondition, "HighlightCondition may not be null.")

---

flow-server/src/main/java/com/vaadin/flow/router/RouterLink.java, line 496 at r4 (raw file):

    public void setHighlightAction(
            HighlightAction<RouterLink> highlightAction) {
        assert highlightAction != null;

Here we should probably also use Object::requireNonNull

---

flow-server/src/main/java/com/vaadin/flow/router/RouterLink.java, line 501 at r4 (raw file):

        this.highlightAction = highlightAction;
    }

Should there also be a shorthand for setHighlight(HighlightAction<RouterLink> action, HighlightCondition<RouterLink> condition)?

---

Comments from Reviewable

[heruan]

Review status: all files reviewed at latest revision, 4 unresolved discussions.

---

flow-server/src/main/java/com/vaadin/flow/router/RouterLink.java, line 461 at r4 (raw file):

Previously, caalador wrote…

I would expect that we actually throw here as seldom do developers run with assertions enabled.
So it might be good to just useObject.requireNonNull(highlightCondition, "HighlightCondition may not be null.")

I'm not much used to assert so I "mimicked" assertions in other methods. Should I replace assert with Objects::requireNonNull or just add that after the assertion?

---

flow-server/src/main/java/com/vaadin/flow/router/RouterLink.java, line 501 at r4 (raw file):

Previously, caalador wrote…

Should there also be a shorthand for setHighlight(HighlightAction<RouterLink> action, HighlightCondition<RouterLink> condition)?

I'd stick with the setters only, the shorthand with two functional interfaces as arguments might quickly become hard to read.

---

Comments from Reviewable

[caalador]

Review status: all files reviewed at latest revision, 4 unresolved discussions.

---

flow-server/src/main/java/com/vaadin/flow/router/RouterLink.java, line 461 at r4 (raw file):

Previously, heruan (Giovanni Lovato) wrote…

I'm not much used to assert so I "mimicked" assertions in other methods. Should I replace assert with Objects::requireNonNull or just add that after the assertion?

You can just replace the assert so that a null value throws.
Without a message it's fine, but with a message it's easier to understand why.

---

flow-server/src/main/java/com/vaadin/flow/router/RouterLink.java, line 501 at r4 (raw file):

Previously, heruan (Giovanni Lovato) wrote…

I'd stick with the setters only, the shorthand with two functional interfaces as arguments might quickly become hard to read.

Well I would see it used with predefined action and condition, but it can be added later also.

---

Comments from Reviewable

[heruan]

Review status: all files reviewed at latest revision, 4 unresolved discussions.

---

flow-server/src/main/java/com/vaadin/flow/router/RouterLink.java, line 461 at r4 (raw file):

Previously, caalador wrote…

You can just replace the assert so that a null value throws.
Without a message it's fine, but with a message it's easier to understand why.

Done.

---

flow-server/src/main/java/com/vaadin/flow/router/RouterLink.java, line 496 at r4 (raw file):

Previously, caalador wrote…

Here we should probably also use Object::requireNonNull

Done.

---

Comments from Reviewable

[vaadin-bot]

SonarQube analysis reported 32 issues

• 15 major
• 17 minor

Top 10 extra issues

Note: The following issues were found on lines that were not modified in the pull request. Because these issues can't be reported as line comments, they are summarized here:

1. RouterLink.java#L67: Remove this call from a constructor to the overridable "getElement" method.
2. RouterLink.java#L93: Remove this call from a constructor to the overridable "setText" method.
3. RouterLink.java#L94: Remove this call from a constructor to the overridable "setRoute" method.
4. RouterLink.java#L115: Remove this call from a constructor to the overridable "setText" method.
5. RouterLink.java#L116: Remove this call from a constructor to the overridable "setRoute" method.
6. RouterLink.java#L131: Remove this call from a constructor to the overridable "setText" method.
7. RouterLink.java#L134: Remove this call from a constructor to the overridable "setRoute" method.
8. RouterLink.java#L143: Remove this call from a constructor to the overridable "setRoute" method.
9. RouterLink.java#L166: Remove this call from a constructor to the overridable "setText" method.
10. RouterLink.java#L167: Remove this call from a constructor to the overridable "setRoute" method.

[caalador]

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

---

Comments from Reviewable