Refactoring popups to use the same window class

[StanleyGoldman]

No description provided.

[shana]

Fields go on top. See resharper layout file for ordering

[StanleyGoldman]

👍

[shana]

A getter that sets values is confusing, and ActiveSubview/ActivePopupView isn't really clear about what is what. If we switch views by setting the enum value, then probably best to set the view there immediately.

[StanleyGoldman]

👍

[shana]

The casing of this file needs fixing, btw, probably a good time to do it.

[StanleyGoldman]

The private const fields?

[StanleyGoldman]

👍

[shana]

You want to call InitializeWindow, not Initialize here. Check ApplicationManager.cs for how it does that for the other window. Also probably cast to BaseWindow here, or add a IWindow for it so we don't have to cast.

[StanleyGoldman]

I was looking at it a bit more, I was returning IView out of copy-pasting older code, since there is now only one type of popup being PopupWindow, I figured the easiest thing to do is return PopupWindow from the Open method.

[shana]

So the thing with the GetWindow method is that it'll return an existing window if there's one already. That means that we need to decide whether we need to reset the window and how. If there's another thing waiting for the close callback. we need to make sure it gets closure on it, probably by having the window shut itself down with onClose(false) and then resetting all the data and state.

This Open method should probably do all the work, including calling InitializeWindow (which is currently being called in Launch above. The PopupWindow class could probably use a Reset method to cancel any callbacks and clear all data before anything else, which it can probably do in it's InitializeWindow implementation.

InitializeWindow should probably also be called before calling show on the window, right now it's the other way around.

[StanleyGoldman]

The Open and InitializeWindow stuff was no problem...

I made RaiseOnClose virtual and calling a method to clear the OnClose handler In the PopupWindow override.

Unity/src/UnityExtension/Assets/Editor/GitHub.Unity/UI/PopupWindow.cs

Lines 86 to 90 in d4121b2

	protected override void RaiseOnClose(bool result)
	{
	base.RaiseOnClose(result);
	ClearOnClose();
	}

[shana]

The override is just calling ClearOnClose to clear the callback, and that method is defined here and should always be called after OnClose is invoked, so shouldn't we just call it right here instead of making this a virtual and requiring all subclasses to remember to call a magic method?

[shana]

TL;DR: ClearOnClose should be called directly from RaiseOnClose so that subclasses don't accidentally forget to do it. That means this method doesn't need to be virtual.

[StanleyGoldman]

Only a PopupWindow should need to ClearOnClose clearing the OnClose handler is generally new functionality that BaseWindow has never had/needed.

If i had a choice about the matter, I wouldn't have put ClearOnClose in BaseWindow. Even though the event is public you just can't set an event to null in a subclass so i was forced to define ClearOnClose there.