Skip to content

Add the possibility of default selection to MultiItemSelector #468

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

Closed
wants to merge 5 commits into from
Closed

Conversation

Drevsh
Copy link
Contributor

@Drevsh Drevsh commented Jul 18, 2022

I hope I did everything correctly. (It's my first time committing to an open source project)

I think there might be a problem regarding default selection in the SingleItemSelector.
You can now set multiple items as selected in the SingleItemSelector and the DefaultSingleItemSelectorContext will return one at random.

		@Override
		public Optional<I> getResultItem() {
			if (getResultItems() == null) {
				return Optional.empty();
			}
			return getResultItems().stream().findFirst();
		}

Or that is at least what I think will happen. Maybe this needs to be handled differently in SingleItemSelector/MultiItemSelector

Copy link
Contributor

@jvalkeal jvalkeal left a comment

Choose a reason for hiding this comment

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

Thx, it looks something how I'd done the quick hack to add this feature without rewriting shared parent class with single/multi. It probably gives some bullets for a user to shoot own leg if this is used with single selector but I think we can address that later with additional changes or documentation.

I left some comments to address. Feel free to add yourself as author to files if you want. I think we can sneak this in into 2.1.0.

@Drevsh Drevsh requested a review from jvalkeal July 19, 2022 18:58
@Drevsh
Copy link
Contributor Author

Drevsh commented Jul 19, 2022

Hey @jvalkeal I hope I got everything.

@jvalkeal jvalkeal added this to the 2.1.0 milestone Jul 20, 2022
@jvalkeal
Copy link
Contributor

Thx a lot, merged per 2862cc8

@jvalkeal jvalkeal closed this Jul 20, 2022
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