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.

Sign up for GitHub

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

Jump to bottom

[Improvement] Async bottle loading #574

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

[Improvement] Async bottle loading #574

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
fbdf086
Protect access to programs list with a lock
vcsoares Oct 29, 2023
0aedc68
Don't reload bottle programs list unless necessary
vcsoares Oct 29, 2023
bf0e8af
Add loading indicator for programs list
vcsoares Oct 29, 2023
ee792f7
Avoid reloading bottle programs list in Start Menu unless necessary
vcsoares Oct 29, 2023
bcdae7f
Add loading indicator in Start Menu programs grid
vcsoares Oct 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 16 additions & 5 deletions Whisky/Models/Bottle.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,14 +92,19 @@ extension Bottle {
let programFilesx86 = url
.appending(path: "drive_c")
.appending(path: "Program Files (x86)")
programs.removeAll()

lock.withLock {
programs.removeAll()
}

let enumerator64 = FileManager.default.enumerator(at: programFiles,
includingPropertiesForKeys: [.isExecutableKey],
options: [.skipsHiddenFiles])
while let url = enumerator64?.nextObject() as? URL {
if !url.hasDirectoryPath && url.pathExtension == "exe" {
programs.append(Program(name: url.lastPathComponent, url: url, bottle: self))
lock.withLock {
programs.append(Program(name: url.lastPathComponent, url: url, bottle: self))
}
}
}

Expand All @@ -108,14 +113,20 @@ extension Bottle {
options: [.skipsHiddenFiles])
while let url = enumerator32?.nextObject() as? URL {
if !url.hasDirectoryPath && url.pathExtension == "exe" {
programs.append(Program(name: url.lastPathComponent, url: url, bottle: self))
lock.withLock {
programs.append(Program(name: url.lastPathComponent, url: url, bottle: self))
}
}
}

// Apply blocklist
programs = programs.filter { !settings.blocklist.contains($0.url) }
lock.withLock {
programs = programs.filter {
!settings.blocklist.contains($0.url)
}
programs.sort { $0.name.lowercased() < $1.name.lowercased() }
}

programs.sort { $0.name.lowercased() < $1.name.lowercased() }
return programs
}

Expand Down
72 changes: 49 additions & 23 deletions Whisky/Views/Bottle Views/BottleView.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,23 +34,38 @@ struct BottleView: View {
// This just provides a way to trigger a refresh
@State var loadStartMenu: Bool = false
@State var showWinetricksSheet: Bool = false
@State private var isLoadingInstalledPrograms: Bool = false

private let gridLayout = [GridItem(.adaptive(minimum: 100, maximum: .infinity))]

var body: some View {
NavigationStack(path: $path) {
ScrollView {
if pins.count > 0 {
LazyVGrid(columns: gridLayout, alignment: .center) {
ForEach(pins, id: \.url) { pin in
PinnedProgramView(bottle: bottle,
pin: pin,
loadStartMenu: $loadStartMenu,
path: $path)
ZStack {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this ZStack is needed. Why block the user from accessing loaded pins until they've all loaded?

Copy link
Author

Choose a reason for hiding this comment

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

Hmmm, I see what you mean - yeah, maybe a less fancy but less obstructive way of displaying the indicator would be better indeed. I'll change that, thanks for the feedback :)

if pins.count > 0 {
LazyVGrid(columns: gridLayout, alignment: .center) {
ForEach(pins, id: \.url) { pin in
PinnedProgramView(bottle: bottle,
pin: pin,
loadStartMenu: $loadStartMenu,
path: $path)
}
}
.padding()
}

if isLoadingInstalledPrograms {
HStack {
Spacer()
ProgressView()
.padding()
.background(Material.regular)
.clipShape(RoundedRectangle(cornerSize: .init(width: 16, height: 16)))
Spacer()
}
}
.padding()
}

Form {
NavigationLink(value: BottleStage.programs) {
HStack {
Expand Down Expand Up @@ -121,6 +136,7 @@ struct BottleView: View {
}
}
.disabled(programLoading)

if programLoading {
Spacer()
.frame(width: 10)
Expand All @@ -140,7 +156,7 @@ struct BottleView: View {
case .config:
ConfigView(bottle: $bottle)
case .programs:
ProgramsView(bottle: bottle,
ProgramsView(bottle: $bottle,
reloadStartMenu: $loadStartMenu,
path: $path)
}
Expand All @@ -159,22 +175,32 @@ struct BottleView: View {
}

func updateStartMenu() {
bottle.programs = bottle.updateInstalledPrograms()
let startMenuPrograms = bottle.getStartMenuPrograms()
for startMenuProgram in startMenuPrograms {
for program in bottle.programs where
// For some godforsaken reason "foo/bar" != "foo/Bar" so...
program.url.path().caseInsensitiveCompare(startMenuProgram.url.path()) == .orderedSame {
program.pinned = true
if !bottle.settings.pins.contains(where: { $0.url == program.url }) {
bottle.settings.pins.append(PinnedProgram(name: program.name
.replacingOccurrences(of: ".exe", with: ""),
url: program.url))
guard !isLoadingInstalledPrograms else { return }

isLoadingInstalledPrograms = true

DispatchQueue(label: "whisky.lock.queue").async {
if bottle.programs.isEmpty {
bottle.updateInstalledPrograms()
}

let startMenuPrograms = bottle.getStartMenuPrograms()
for startMenuProgram in startMenuPrograms {
for program in bottle.programs where
// For some godforsaken reason "foo/bar" != "foo/Bar" so...
program.url.path().caseInsensitiveCompare(startMenuProgram.url.path()) == .orderedSame {
program.pinned = true
if !bottle.settings.pins.contains(where: { $0.url == program.url }) {
bottle.settings.pins.append(PinnedProgram(name: program.name
.replacingOccurrences(of: ".exe", with: ""),
url: program.url))
}
}
}
}

pins = bottle.settings.pins
pins = bottle.settings.pins
isLoadingInstalledPrograms = false
}
}
}

Expand Down Expand Up @@ -289,7 +315,7 @@ struct PinnedProgramView: View {
}
.onAppear {
name = pin.name
Task.detached {
DispatchQueue(label: "whisky.lock.queue").async {
program = bottle.programs.first(where: { $0.url == pin.url })
if let program {
if let peFile = program.peFile {
Expand Down
63 changes: 50 additions & 13 deletions Whisky/Views/Bottle Views/ProgramsView.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import SwiftUI
import WhiskyKit

struct ProgramsView: View {
let bottle: Bottle
@Binding var bottle: Bottle
@State var programs: [Program] = []
@State var blocklist: [URL] = []
@State private var selectedPrograms = Set<Program>()
Expand All @@ -30,12 +30,13 @@ struct ProgramsView: View {
@State var resortPrograms: Bool = false
@State var isExpanded: Bool = true
@State var isBlocklistExpanded: Bool = false
@State var isLoadingPrograms: Bool = false
@Binding var reloadStartMenu: Bool
@Binding var path: NavigationPath

var body: some View {
Form {
Section("program.title", isExpanded: $isExpanded) {
Section(isExpanded: $isExpanded) {
List($programs, id: \.self, selection: $selectedPrograms) { $program in
ProgramItemView(program: program,
resortPrograms: $resortPrograms,
Expand All @@ -47,7 +48,18 @@ struct ProgramsView: View {
resortPrograms.toggle()
}
}
} header: {
HStack {
Text("program.title")

if isLoadingPrograms {
ProgressView()
.scaleEffect(0.5)
.padding(.vertical, -16)
}
}
}

Section("program.blocklist", isExpanded: $isBlocklistExpanded) {
List($blocklist, id: \.self, selection: $selectedBlockitems) { $blockedUrl in
BlocklistItemView(blockedUrl: blockedUrl,
Expand All @@ -68,26 +80,46 @@ struct ProgramsView: View {
.animation(.easeInOut(duration: 0.2), value: isBlocklistExpanded)
.navigationTitle("tab.programs")
.onAppear {
programs = bottle.updateInstalledPrograms()
blocklist = bottle.settings.blocklist
sortPrograms()
loadPrograms()
}
.onChange(of: resortPrograms) {
sortPrograms()
reloadStartMenu.toggle()
}
}

func loadPrograms() {
if bottle.programs.isEmpty {
updatePrograms()
return
}

programs = bottle.programs
blocklist = bottle.settings.blocklist
sortPrograms()
}

private func updatePrograms() {
isLoadingPrograms = true

DispatchQueue(label: "whisky.lock.queue").async {
programs = bottle.updateInstalledPrograms()
blocklist = bottle.settings.blocklist
sortPrograms()
isLoadingPrograms = false
}
}

func sortPrograms() {
var favourites = programs.filter { $0.pinned }
var nonFavourites = programs.filter { !$0.pinned }
favourites = favourites.sorted { $0.name < $1.name }
nonFavourites = nonFavourites.sorted { $0.name < $1.name }
programs.removeAll()
programs.append(contentsOf: favourites)
programs.append(contentsOf: nonFavourites)
private func sortPrograms() {
DispatchQueue(label: "whisky.lock.queue").async {
Copy link
Member

Choose a reason for hiding this comment

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

In general, we don't use GCD in Whisky, using structured concurrency would be preferable.

Copy link
Author

Choose a reason for hiding this comment

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

I opted for using a DispatchQueue in this specific scenario to ensure bottle loading operations get queued up in the same thread. Structured concurrency would not have this guarantee (at least as far as I know), and in general I avoided going down the route of rewriting Bottle to be an Actor.

Copy link
Member

Choose a reason for hiding this comment

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

Actor based approach might be preferred here.

var favourites = programs.filter { $0.pinned }
var nonFavourites = programs.filter { !$0.pinned }
favourites = favourites.sorted { $0.name < $1.name }
nonFavourites = nonFavourites.sorted { $0.name < $1.name }
programs.removeAll()
programs.append(contentsOf: favourites)
programs.append(contentsOf: nonFavourites)
}
}
}

Expand All @@ -113,8 +145,11 @@ struct ProgramItemView: View {
.buttonStyle(.plain)
.foregroundColor(isPinned ? .accentColor : .secondary)
.opacity(isPinned ? 1 : showButtons ? 1 : 0)

Text(program.name)

Spacer()

if showButtons {
if let peFile = program.peFile,
let archString = peFile.architecture.toString() {
Expand All @@ -126,6 +161,7 @@ struct ProgramItemView: View {
.stroke(.secondary)
)
}

Button {
path.append(program)
} label: {
Expand All @@ -134,6 +170,7 @@ struct ProgramItemView: View {
.buttonStyle(.plain)
.foregroundStyle(.secondary)
.help("program.config")

Button {
Task {
await program.run()
Expand Down
2 changes: 2 additions & 0 deletions WhiskyKit/Sources/WhiskyKit/Whisky/Bottle.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,8 @@ public class Bottle: Hashable, Identifiable {
self.inFlight = inFlight
self.isActive = isActive
}

public let lock = NSLock()
}

extension Array where Element == Bottle {
Expand Down