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

Added Button to restore recently deleted beatmaps #1671

Merged
merged 18 commits into from Dec 21, 2017

Conversation

3 participants
@FreezyLemon
Member

FreezyLemon commented Dec 8, 2017

Closes #1215.
This now works reliably after the problem mentioned in that issue has been fixed.
However the BeatmapCarousel will not update when the button is clicked while having SongSelect open, though this should be fixed by #1657 .

@FreezyLemon FreezyLemon changed the title from Undelete button add to Added Button to restore recently deleted beatmaps Dec 8, 2017

@FreezyLemon

This comment has been minimized.

Show comment
Hide comment
@FreezyLemon

FreezyLemon Dec 8, 2017

Member

Update: There was some misunderstanding here. I didn't notice at first, but the map(set) actually does get added again (the carousel updates correctly), but it gets added at the bottom which confused me a bit since I have a lot of maps installed.
Maybe this needs to be changed to re-sort when adding new maps aswell?

Member

FreezyLemon commented Dec 8, 2017

Update: There was some misunderstanding here. I didn't notice at first, but the map(set) actually does get added again (the carousel updates correctly), but it gets added at the bottom which confused me a bit since I have a lot of maps installed.
Maybe this needs to be changed to re-sort when adding new maps aswell?

@Aergwyn

This comment has been minimized.

Show comment
Hide comment
@Aergwyn

Aergwyn Dec 8, 2017

Member

After looking into this why they appear at the end I noticed something.

qq

It does restore circles! too, which (I assume) shouldn't happen. Even if I'd love to play it.

Member

Aergwyn commented Dec 8, 2017

After looking into this why they appear at the end I noticed something.

qq

It does restore circles! too, which (I assume) shouldn't happen. Even if I'd love to play it.

Added check for "menu music beatmap hash" before undeleting so circle…
…s.osu doesn't get imported on Undelete. Also moved the const property to BeatmapManager.
@FreezyLemon

This comment has been minimized.

Show comment
Hide comment
@FreezyLemon

FreezyLemon Dec 8, 2017

Member

Fixed that oversight. The beatmap hash for the "menu music"'s beatmap is now inside BeatmapManager to centralise it a bit (and since I needed it twice I thought it would make more sense to put it there).

Member

FreezyLemon commented Dec 8, 2017

Fixed that oversight. The beatmap hash for the "menu music"'s beatmap is now inside BeatmapManager to centralise it a bit (and since I needed it twice I thought it would make more sense to put it there).

@peppy

This comment has been minimized.

Show comment
Hide comment
@peppy

peppy Dec 10, 2017

Member

Can't you just check the Protected flag and skip it? That's why it's there.

Member

peppy commented Dec 10, 2017

Can't you just check the Protected flag and skip it? That's why it's there.

@peppy

This comment has been minimized.

Show comment
Hide comment
@peppy

peppy Dec 18, 2017

Member

As this is a non-instant operation, I think a progress notification should be added similar to the other tasks associated with these buttons.

Member

peppy commented Dec 18, 2017

As this is a non-instant operation, I think a progress notification should be added similar to the other tasks associated with these buttons.

@FreezyLemon

This comment has been minimized.

Show comment
Hide comment
@FreezyLemon

FreezyLemon Dec 18, 2017

Member

It would be nice to have #1707 merged (or closed, for that matter) before this so I can add a custom CompletionText without re-PRing.

Member

FreezyLemon commented Dec 18, 2017

It would be nice to have #1707 merged (or closed, for that matter) before this so I can add a custom CompletionText without re-PRing.

@peppy

This comment has been minimized.

Show comment
Hide comment
@peppy

peppy Dec 21, 2017

Member

#1707 has been merged now – wanna update this to take advantage of it?

Member

peppy commented Dec 21, 2017

#1707 has been merged now – wanna update this to take advantage of it?

@peppy

This comment has been minimized.

Show comment
Hide comment
@peppy

peppy Dec 21, 2017

Member

@FreezyLemon make sure to pay attention to that change I made. quite important from a performance perspective (every Count() and Any() and foreach was hitting the database context to re-query in your previous implementation).

Member

peppy commented Dec 21, 2017

@FreezyLemon make sure to pay attention to that change I made. quite important from a performance perspective (every Count() and Any() and foreach was hitting the database context to re-query in your previous implementation).

@peppy

peppy approved these changes Dec 21, 2017

@peppy peppy merged commit 572e2d3 into ppy:master Dec 21, 2017

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@FreezyLemon FreezyLemon deleted the FreezyLemon:undelete-button-add branch Jan 7, 2018

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