/osu

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

Hide menu cursor when taking screenshots by default #2397

Merged
merged 17 commits into from Apr 15, 2018

Conversation

Reviewers

@peppy peppy

Assignees
No one assigned
Labels
Milestone
  April 2018
4 participants
@smoogipoo @peppy @UselessToucan @Frontear
@smoogipoo
Contributor

smoogipoo commented Apr 13, 2018 •
edited by peppy

Alternative to / closes #2309

Prereqs:

Over #2309:

  • Doesn't fade out the cursor using the cursor's standard Show/Hide, possibly leading to problems if something else calls Show/Hide continuously themselves.
  • Instantaneously hides the cursor for the exact amount of frames required to capture the screenshot (4), instantaneously shows it again afterwards, all through IsPresent.
  • Doesn't crash when taking screenshots in quick succession.
  • Makes TakeScreenShotAsync truly async - it was previously blocking update thread, even if only for a "short" amount of time.
  • Don't lead to the possibly of the cursor still being shown if taking screenshots in laggy scenarios.
@Frontear

This comment has been minimized.

Sign in to view
Contributor

Frontear commented on osu.Game/Graphics/ScreenshotManager.cs in dd5cc59 Mar 25, 2018

Would it be better to use BindableBool here instead?

This comment has been minimized.

Sign in to view
Contributor

UselessToucan replied Mar 25, 2018

Yes. Will use BindableBool.

This comment has been minimized.

Sign in to view
Contributor

UselessToucan replied Mar 27, 2018

I checked BindableBool usages in ReSharper. It is not used for config related stuff anywhere. So it doesn't look like I need to do this.

@smoogipoo smoogipoo added proposal settings labels Apr 13, 2018

@smoogipoo smoogipoo added this to the April 2018 milestone Apr 13, 2018

@smoogipoo smoogipoo requested a review from peppy Apr 13, 2018

osu.Game/Graphics/Cursor/MenuCursor.cs Outdated
@@ -18,6 +18,9 @@ namespace osu.Game.Graphics.Cursor
{
public class MenuCursor : CursorContainer
{
public bool ShowCursor = true;

This comment has been minimized.

Sign in to view
Copy link
@peppy

peppy Apr 13, 2018

Member

no xmldoc, also don't like this. what's stopping something else from setting this for its own usage, and having them conflict?
osu.Game/Graphics/Cursor/MenuCursor.cs Outdated
protected override Drawable CreateCursor() => new Cursor();
private Bindable<bool> cursorRotate;
private bool dragging;
private bool startRotation;
private ScreenshotManager screenshotManager;

This comment has been minimized.

Sign in to view
Copy link
@peppy

peppy Apr 15, 2018

Member

no need for this?

@peppy peppy changed the title from Add option to hide menu cursor in screenshots to Hide menu cursor when taking screenshots by default Apr 15, 2018

@peppy

peppy approved these changes Apr 15, 2018
View changes

@peppy peppy merged commit 0490c52 into ppy:master Apr 15, 2018
1 check passed

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@smoogipoo smoogipoo deleted the smoogipoo:instant-hide-screenshot branch Jun 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment