Added Window.PreCloseListener that can be used to prevent window close

[jreznot]

Window.PreCloseListener is triggered only if users try to close window
from client side. If PreCloseListener sets preventClose flag into event
object then window close is not performed.

---

This change is 

[jreznot]

Usually, this event is needed when we want to show "Unsaved changes" dialog to users and prevent window close.

[jreznot]

It would be great to see this changes in the upcoming 8.4 release

[elmot]

The idea on the change is pretty good, but I prefer to have it more in line with existing API. Please have a look at
ViewChangeListener.beforeViewChange() and rewrite the code similar way.

---

Review status: 0 of 5 files reviewed at latest revision, 9 unresolved discussions.

---

server/src/main/java/com/vaadin/ui/Window.java, line 46 at r1 (raw file):

import java.io.Serializable;
import java.lang.reflect.Method;
import java.util.*;

Please expand to individual imports. We do not use asterisk imports in Framework.

---

server/src/main/java/com/vaadin/ui/Window.java, line 243 at r1 (raw file):

    }

    protected void closeFromClient() {

Missing javadoc and empty @since. They are mandatory for protected and public classes and members

---

server/src/main/java/com/vaadin/ui/Window.java, line 744 at r1 (raw file):

     * {@link UI#removeWindow(Window)} call.
     *
     * @since 8.3

@since should be empty. they are set to a particular version when released.

---

server/src/main/java/com/vaadin/ui/Window.java, line 786 at r1 (raw file):

     * </p>
     *
     * @since 8.3

@since should be empty.

---

server/src/main/java/com/vaadin/ui/Window.java, line 810 at r1 (raw file):

     *
     * @param listener
     *            the PreCloseListener to add, not null

According to our code conventions, the class should be WindowBeforeCloseListener. And the same for the event, all the methods etc.

---

server/src/main/java/com/vaadin/ui/Window.java, line 811 at r1 (raw file):

     * @param listener
     *            the PreCloseListener to add, not null
     * @since 8.3

@since should be empty.

---

server/src/test/java/com/vaadin/tests/server/components/WindowTest.java, line 6 at r1 (raw file):

import java.util.Map;

import com.vaadin.ui.Window.*;

We do not use asterisk imports. For tests the rule isn't that strict but anyway.

---

uitest/src/test/java/com/vaadin/tests/components/window/WindowPreCloseListenerTest.java, line 13 at r1 (raw file):

import static org.junit.Assert.assertTrue;

public class WindowPreCloseListenerTest extends MultiBrowserTest {

SingleBrowserTest is enough, I think

---

uitest/src/test/java/com/vaadin/tests/components/window/WindowPreCloseListenerTest.java, line 15 at r1 (raw file):

public class WindowPreCloseListenerTest extends MultiBrowserTest {

    @Override

Please add @Before in sake of readability

---

Comments from Reviewable

[jreznot]

I've redesigned API and tried to fix all the review comments

[elmot]

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

---

server/src/main/java/com/vaadin/ui/Window.java, line 271 at r4 (raw file):

     * @return true if the window close should be allowed, false to silently
     *         block the close operation
     */

missing @since

---

Comments from Reviewable

[jreznot]

Reviewed 4 of 9 files at r3, 2 of 3 files at r4, 1 of 1 files at r5.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

server/src/main/java/com/vaadin/ui/Window.java, line 271 at r4 (raw file):

Previously, elmot (Ilia Motornyi) wrote…

missing @since

Done.

---

Comments from Reviewable

[jreznot]

Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

server/src/main/java/com/vaadin/ui/Window.java, line 243 at r1 (raw file):

Previously, elmot (Ilia Motornyi) wrote…

Missing javadoc and empty @since. They are mandatory for protected and public classes and members

Done.

---

server/src/main/java/com/vaadin/ui/Window.java, line 744 at r1 (raw file):

Previously, elmot (Ilia Motornyi) wrote…

@since should be empty. they are set to a particular version when released.

Done.

---

server/src/main/java/com/vaadin/ui/Window.java, line 786 at r1 (raw file):

Previously, elmot (Ilia Motornyi) wrote…

@since should be empty.

Done.

---

server/src/main/java/com/vaadin/ui/Window.java, line 810 at r1 (raw file):

Previously, elmot (Ilia Motornyi) wrote…

According to our code conventions, the class should be WindowBeforeCloseListener. And the same for the event, all the methods etc.

Done.

---

server/src/main/java/com/vaadin/ui/Window.java, line 811 at r1 (raw file):

Previously, elmot (Ilia Motornyi) wrote…

@since should be empty.

Done.

---

server/src/test/java/com/vaadin/tests/server/components/WindowTest.java, line 6 at r1 (raw file):

Previously, elmot (Ilia Motornyi) wrote…

We do not use asterisk imports. For tests the rule isn't that strict but anyway.

Done.

---

uitest/src/test/java/com/vaadin/tests/components/window/WindowPreCloseListenerTest.java, line 13 at r1 (raw file):

Previously, elmot (Ilia Motornyi) wrote…

SingleBrowserTest is enough, I think

Done.

---

uitest/src/test/java/com/vaadin/tests/components/window/WindowPreCloseListenerTest.java, line 15 at r1 (raw file):

Previously, elmot (Ilia Motornyi) wrote…

Please add @Before in sake of readability

Done.

---

Comments from Reviewable

[jreznot]

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

---

server/src/main/java/com/vaadin/ui/Window.java, line 46 at r1 (raw file):

Previously, elmot (Ilia Motornyi) wrote…

Please expand to individual imports. We do not use asterisk imports in Framework.

Done.

---

Comments from Reviewable

[jreznot]

I totally missed where to fix review points (
Just marked all the points as Done.

[jreznot]

Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

server/src/main/java/com/vaadin/ui/Window.java, line 271 at r4 (raw file):

Previously, jreznot (Yuriy Artamonov) wrote…

Done.

Done

---

Comments from Reviewable

[jreznot]

LGTM

---

Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

Comments from Reviewable

[mehdi-vaadin]

Please format your code. It can be done by this command mvn process-sources. Also, a test should be added for the case a confirm dialog is shown before closing the windows for the user to make their decision. After all, the purpose of this PR is to show the user a message for unsaved changes.

Reviewable status: all files reviewed, 18 unresolved discussions (waiting on @elmot and @jreznot)

---

server/src/main/java/com/vaadin/ui/Window.java, line 116 at r5 (raw file):

private LinkedHashSet<WindowBeforeCloseListener> beforeCloseListeners = null;

This is not needed and it's better to use addListener from AbstractComponent like one of the previous comments. I can't see why it has been changed. So, it should be removed and the related code should be changed accordingly.

---

server/src/main/java/com/vaadin/ui/Window.java, line 257 at r5 (raw file):

     * Fires an event before window close.
     * <p>

This comment is not necessary and should be removed. Actually, it's deducted from the method name.

---

server/src/main/java/com/vaadin/ui/Window.java, line 268 at r5 (raw file):

not null

So, we need a Objects.requireNonNull in the code. And, the rest of the comment about event is extra.

---

server/src/main/java/com/vaadin/ui/Window.java, line 275 at r5 (raw file):

        // a copy of the listener list is needed to avoid
        // ConcurrentModificationException as a listener can add/remove
        // listeners

Since beforeCloseListeners should be removed, getListeners method should be called to get the list of listeners. So, no need to make a copy of the list.

---

server/src/main/java/com/vaadin/ui/Window.java, line 800 at r5 (raw file):

Quoted 5 lines of code…

        /**
         * Gets the Window.
         *
         * @return the window.
         */

Unnecessary comment.

---

server/src/main/java/com/vaadin/ui/Window.java, line 816 at r5 (raw file):

Quoted 6 lines of code…

     * <p>
     * Implementation of the listener may cancel window close using
     * {@link WindowBeforeCloseEvent#setClosePrevented(boolean)} method call.
     * </p>
     *
     * @since

Not relevant any more. Should be removed.

---

server/src/main/java/com/vaadin/ui/Window.java, line 858 at r5 (raw file):

Quoted 14 lines of code…

        if  (beforeCloseListeners == null) {
            beforeCloseListeners = new LinkedHashSet<>();
        }
 
        beforeCloseListeners.add(listener);
 
        return () -> {
            if (beforeCloseListeners != null) {
                beforeCloseListeners.remove(listener);
                if (beforeCloseListeners.isEmpty()) {
                    beforeCloseListeners = null;
                }
            }
        };

addListener method should be used instead.

---

server/src/main/java/com/vaadin/ui/Window.java, line 1686 at r5 (raw file):

Quoted 23 lines of code…

    @Override
    public Collection<?> getListeners(Class<?> eventType) {
        if (WindowBeforeCloseEvent.class.isAssignableFrom(eventType)) {
            if (beforeCloseListeners == null) {
                return Collections.EMPTY_LIST;
            } else {
                return Collections
                        .unmodifiableCollection(beforeCloseListeners);
            }
        } else if (ConnectorEvent.class.isAssignableFrom(eventType)
                || Component.Event.class.isAssignableFrom(eventType)) {
            if (beforeCloseListeners == null) {
                return super.getListeners(eventType);
            } else {
                Set<Object> listeners = new LinkedHashSet<>();
                listeners.addAll(beforeCloseListeners);
                listeners.addAll(super.getListeners(eventType));
                return listeners;
            }
        }
 
        return super.getListeners(eventType);
    }

should be removed.

---

uitest/src/main/java/com/vaadin/tests/components/window/WindowBeforeCloseListener.java, line 9 at r5 (raw file):

WindowBeforeCloseListener

Let's use a different name from the actual listener being tested here e.g. WindowWithBeforeCloseListener.

---

uitest/src/test/java/com/vaadin/tests/components/window/WindowBeforeCloseListenerTest.java, line 27 at r5 (raw file):

"Unexpected log contents,"

A more clear message is needed here e.g. "Unexpected log contents, the CloseListener is not called."

---

uitest/src/test/java/com/vaadin/tests/components/window/WindowBeforeCloseListenerTest.java, line 28 at r5 (raw file):

        $(WindowElement.class).$(ButtonElement.class).first().click();
        assertEquals("Unexpected log contents,",
                "1. Window 'Sub-window' closed", getLogRow(0));

What about adding assertFalse("The window is not closed after closgin by server-side code.", $(WindowElement.class).exists()); here?

---

uitest/src/test/java/com/vaadin/tests/components/window/WindowBeforeCloseListenerTest.java, line 36 at r5 (raw file):

assertTrue

Please add a message here e.g. "The window is closed although it shouldn't be after returning false in WindowBeforeCloseListener".

---

uitest/src/test/java/com/vaadin/tests/components/window/WindowBeforeCloseListenerTest.java, line 45 at r5 (raw file):

        assertLogText();
        assertTrue($(WindowElement.class).exists());

The same message here too.

---

uitest/src/test/java/com/vaadin/tests/components/window/WindowBeforeCloseListenerTest.java, line 47 at r5 (raw file):

        assertTrue($(WindowElement.class).exists());
    }

We also need to test the case when the listener returns true.

---

uitest/src/test/java/com/vaadin/tests/components/window/WindowBeforeCloseListenerTest.java, line 52 at r5 (raw file):

assertLogText

Rename to a more proper name e.g. assertPreventedWindowClosingLogText.

---

uitest/src/test/java/com/vaadin/tests/components/window/WindowBeforeCloseListenerTest.java, line 53 at r5 (raw file):

"Unexpected log contents,"

A more clear message is needed here e.g. "Unexpected log contents, the WindowBeforeCloseListener is not called."

---

uitest/src/test/java/com/vaadin/tests/components/window/WindowBeforeCloseListenerTest.java, line 54 at r5 (raw file):

assertTrue($(WindowElement.class).exists());

Maybe it would be a good idea to move assertTrue($(WindowElement.class).exists()); here.

[jreznot]

It seems that there is no real need in the community for this feature except our case. Thanks for the comments, I'm closing this.