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

Populate HandleInput #1242

Merged
merged 10 commits into from Dec 11, 2017
Merged

Populate HandleInput #1242

merged 10 commits into from Dec 11, 2017

Conversation

UselessToucan
Copy link
Contributor

resolves #1235

@UselessToucan
Copy link
Contributor Author

UselessToucan commented Dec 9, 2017

It looks like I can't add reviewers, but I still should summon @default0 somehow.

private readonly ConcurrentDictionary<Type, bool> cachedValues = new ConcurrentDictionary<Type, bool>();

private readonly string[] inputMethods = {
"OnHover",

This comment was marked as off-topic.

Copy link
Contributor

@default0 default0 left a comment

Choose a reason for hiding this comment

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

The code to actually do the checking looks solid enough. I just kind of disagree that it has to be a publicly exposed instantiable class that is used via DI, instead of an internal static helper that is maybe even a nested class inside Drawable (though, I don't particularly care if it is or isn't).

Also notable that I only looked at the code and did not do any testing, but want to expressly mention that someone should put in the time to check all components where the override was removed to ensure that they retained their old behaviour.


namespace osu.Framework.Caching
{
public class HandleInputCache

This comment was marked as off-topic.

This comment was marked as off-topic.

@@ -1830,10 +1836,31 @@ protected virtual void OnFocusLost(InputState state)
/// is propagated up the scene graph to the next eligible Drawable.</returns>
protected virtual bool OnMouseMove(InputState state) => false;

private HandleInputCache handleInputCache;

public string[] InputMethods => new[]

This comment was marked as off-topic.

@peppy
Copy link
Sponsor Member

peppy commented Dec 10, 2017

I like this because it takes out the manual overhead of remembering to add HandleInput, but still allows the override to exist for dynamic scenarios.

smoogipoo
smoogipoo previously approved these changes Dec 11, 2017
Copy link
Contributor

@smoogipoo smoogipoo left a comment

Choose a reason for hiding this comment

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

I like this.

@peppy
Copy link
Sponsor Member

peppy commented Dec 11, 2017

Let's adjust the HandleInput xmldoc before merging this.

Copy link
Sponsor Member

@peppy peppy left a comment

Choose a reason for hiding this comment

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

docs plz

Rather than doing one dictionary lookup for every drawable every frame.
@smoogipoo
Copy link
Contributor

@peppy plz approve.

@peppy peppy merged commit 0e4e653 into ppy:master Dec 11, 2017
@UselessToucan UselessToucan deleted the HandleInput branch December 11, 2017 19:58
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.

Populate HandleInput based on existence of overridden input-handling methods
4 participants