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

Make import notifications fail when any imports fail #2245

Merged
merged 6 commits into from Mar 22, 2018

Conversation

3 participants
@peppy
Member

peppy commented Mar 19, 2018

No description provided.

@peppy peppy added the ux label Mar 19, 2018

@peppy peppy added this to the March 2018 milestone Mar 19, 2018

@@ -97,11 +98,11 @@ public void Import(params string[] paths)
try
{
notification.Text = $"Importing ({i} of {paths.Length})\n{Path.GetFileName(path)}";
notification.Text = $"Importing ({success} of {paths.Length})\n{Path.GetFileName(path)}";

This comment has been minimized.

@Aergwyn

Aergwyn Mar 19, 2018

Member

While here do we want to address the "Importing 0 of 123" for readability?

This comment has been minimized.

@peppy

peppy Mar 19, 2018

Member

yeah i'll change that too

@@ -331,7 +333,7 @@ private ArchiveReader getReaderFrom(string path)
{
if (ZipFile.IsZipFile(path))
return new ZipArchiveReader(Files.Storage.GetStream(path), Path.GetFileName(path));
return new LegacyFilesystemReader(path);
return new LegacyFilesystemReader(path);

This comment has been minimized.

@Aergwyn

Aergwyn Mar 19, 2018

Member

Nitpick: Sneaky whitespace

using (ArchiveReader reader = getReaderFrom(path))
imported.Add(Import(reader));
notification.Progress = (float)++i / paths.Length;
notification.Progress = (float)(current - 1) / paths.Length;

This comment has been minimized.

@smoogipoo

smoogipoo Mar 22, 2018

Contributor

Why current - 1? This means after importing the first beatmap, the progress is going to be 0. And if there is only one beatmap to import, the progress is always going to be 0?

This comment has been minimized.

@peppy

peppy Mar 22, 2018

Member

current is incremented above, right?

This comment has been minimized.

@smoogipoo

smoogipoo Mar 22, 2018

Contributor

Yeah but imagine if paths.Length == 1, then we'd get to this line with current = 1, so progress would be 0 / paths.Length = 0. Whereas previously it would do 1 / paths.Length.

This comment has been minimized.

@peppy

peppy Mar 22, 2018

Member

that is correct behaviour. unless we have more granular progress reporting, this is how progress bars are displayed.

This comment has been minimized.

@smoogipoo

smoogipoo Mar 22, 2018

Contributor

What, skipping one of the ticks completely? Since when? I'd much rather advocate for having a debounce from completing notifications so that the progress bar fills up completely before firing up a second notification.

This comment has been minimized.

@peppy

peppy Mar 22, 2018

Member

Is this visible to you? I couldn't see it in testing.

notification.Text = $"Deleting ({i} of {items.Count})";
notification.Progress = (float)++i / items.Count;
notification.Progress = (float)i / items.Count;
notification.Text = $"Deleting ({++i} of {items.Count})";

This comment has been minimized.

@smoogipoo

smoogipoo Mar 22, 2018

Contributor

Almost the same thing as mentioned above, progress would be 0 for the first map. How about the following sequence instead:

notification.Text = "Deleting {++i} of ...";
Delete(b);
notification.Progress = (float)i / items.Count;

This comment has been minimized.

@peppy

peppy Mar 22, 2018

Member

is this even visible?

Update progress with the current item, not the next item
In the case where there is no next item, the progress will not get updated, so we'll essentially skip one element from filling the progress bar further. In the future we may/will want to not hide the notification upon completion, so this will look better in such scenarios.

@peppy peppy merged commit 1a86a05 into ppy:master Mar 22, 2018

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@peppy peppy deleted the peppy:better-import-notice branch Jun 21, 2018

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