Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Un-generify Popup class #3651

Merged
merged 1 commit into from
Nov 21, 2019
Merged

Conversation

stejbac
Copy link
Contributor

@stejbac stejbac commented Nov 21, 2019

Remove the type parameter T from Popup<T> extends Overlay<Popup>, as it appears to have never been used or set anywhere in the code. (This mainly involves replacing a lot of new Popup<> occurrences, which was done entirely automatically by the IDE.)

I guess the original purpose of the type parameter was to allow different kinds of popup, all extending the same class, e.g. Foo extends Popup<Foo> extends Overlay<Foo>. However, due to the fluent interface of Overlay, this wouldn't be entirely type safe as long as Popup has a public constructor and standalone instances of it can be created. For example,

Foo foo = new Popup<Foo>().headline("foo");

would typecheck fine but throw a ClassCastException at runtime, when it tries to assign the underived Popup instance to foo. (In more complex cases this could cause heap pollution.)

If different popup subclasses are later required, it would probably be better to define an abstract base class but keep all the concrete subclasses non-generic, e.g. BasePopup<T extends BasePopup<T>> extends Overlay<T>, similar to the TabbedOverlay class.

Remove the type parameter from Popup<T>, as it appears to have never
been used or set anywhere in the code. (This mainly involves replacing a
lot of "new Popup<>" occurrences.)
Copy link
Contributor

@ripcurlx ripcurlx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK - Now I'm officially <> blind after reviewing this PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants