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

Fix input settings not displaying in visual test browser #20842

Merged
merged 2 commits into from
Oct 28, 2022

Conversation

frenzibyte
Copy link
Member

Noticeable with TestSceneSettingsPanel, as the input settings aren't created due to the specifications residing in OsuGameDesktop.

@peppy
Copy link
Sponsor Member

peppy commented Oct 21, 2022

Is the goal to have this usable for future/upcoming tests?

@frenzibyte
Copy link
Member Author

Not particularly, this was originally reported by a ruleset dev (@LumpBloom7) who came across it while testing their ruleset, and thought it's wrong the way it's currently done.

@bdach
Copy link
Collaborator

bdach commented Oct 21, 2022

Not a fan of this change at all if I'm honest. The changes make the code read objectively worse as far as I'm concerned. Putting the input handlers behind a flag in OsuGameBase defeats the purpose of the method they're in being overridable at all.

Any reason not to put this sort of stuff directly in OsuTestBrowser? At least that's confined to test contexts and I don't have to look at it ever again or have to worry about it potentially influencing mobile platforms.

@frenzibyte
Copy link
Member Author

frenzibyte commented Oct 21, 2022

I did consider this approach at first, but my main concern is that now we're duplicating code in two places and we're likely to forget adding new settings to the tests one as opposed to the game-side one. Also looking at this again, input settings will still not show up for visual test browser running on mobile platforms so this may need some reconsideration. I'll have a think through it, potentially coupling it with static method to avoid duplication.

@bdach
Copy link
Collaborator

bdach commented Oct 21, 2022

I don't think forgetting to add an input handler to the visual test browser is that serious of an issue. It can be added whenever it's spotted missing. Hell, it took this long for anyone to notice or bring up that the input settings aren't there at all.

I say copy across and don't overcomplicate this.

@frenzibyte
Copy link
Member Author

Well, I would prefer adding a static shared factory method at least than keeping it prune to error (which could end up wasting someone's time after realising that they can't find the input section they're looking for after deploying visual tests to their mobile).

@frenzibyte
Copy link
Member Author

Never mind, mobile projects are just too complicated to even get it working through direct override. Replacing the platform conditional with override on both classes wasn't a simple change, to the point I'm not even sure if it's still preferred over conditional (f1872a7).

@peppy
Copy link
Sponsor Member

peppy commented Oct 24, 2022

I guess this change is okay, but definitely needs an inline comment.

Also need @smoogipoo's opinion on this direction.

@peppy peppy requested a review from smoogipoo October 24, 2022 06:43
@smoogipoo
Copy link
Contributor

It's fine.

@peppy peppy self-requested a review October 28, 2022 08:19
@peppy peppy merged commit 92167af into ppy:master Oct 28, 2022
@frenzibyte frenzibyte deleted the fix-input-sections-in-tests branch November 4, 2022 00:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants