Skip to content

Commit

Permalink
fix PTN dropdown focus bugs
Browse files Browse the repository at this point in the history
They were clearing out fields when they shouldn't, and not updating when
they should.

Hugely, this also includes a PTN test that does not use the UI test
framework, but rather a normal unit test framework. This is much faster,
and hopefully more reliable!
  • Loading branch information
yshavit authored Nov 14, 2022
2 parents b2f1943 + d317b05 commit d6f4f5d
Show file tree
Hide file tree
Showing 7 changed files with 310 additions and 90 deletions.
12 changes: 12 additions & 0 deletions whatdid.xcodeproj/project.pbxproj
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,11 @@
6E012216A8475ABF72DEBC69 /* EntriesTreeControllerTest.swift in Sources */ = {isa = PBXBuildFile; fileRef = 6E012ED45DEC1A299BFA43AB /* EntriesTreeControllerTest.swift */; };
6E012342D0F39A9653C28569 /* ExtraAssertions.swift in Sources */ = {isa = PBXBuildFile; fileRef = 6E0129247B26E0835305551D /* ExtraAssertions.swift */; };
6E0128278A73B6631B9459E6 /* Sorting.swift in Sources */ = {isa = PBXBuildFile; fileRef = 6E012C854B86EF8C65210EBE /* Sorting.swift */; };
6E01289DF626AB0D2507E523 /* ControllerTestBase.swift in Sources */ = {isa = PBXBuildFile; fileRef = 6E01235C79DD985D5113CB89 /* ControllerTestBase.swift */; };
6E012A1B7806208A1E8E8FFE /* NSTableView+Helpers.swift in Sources */ = {isa = PBXBuildFile; fileRef = 6E012ED1C001C59F49F03A0F /* NSTableView+Helpers.swift */; };
6E012A5B746EC256131FAD0A /* PtnViewControllerTest.swift in Sources */ = {isa = PBXBuildFile; fileRef = 6E01231E7D1758169D1CCC4D /* PtnViewControllerTest.swift */; };
6E012B1EE5BB0F46B7799088 /* PushableString.swift in Sources */ = {isa = PBXBuildFile; fileRef = 6E01214AC5B51926451AA2BC /* PushableString.swift */; };
6E012DDCB930C9662AA534E4 /* NSEventHelpers.swift in Sources */ = {isa = PBXBuildFile; fileRef = 6E0128A0FF062E560EA67CD9 /* NSEventHelpers.swift */; };
6E012EEB6AED2650C8DD05AB /* EntriesTreeDataSource.swift in Sources */ = {isa = PBXBuildFile; fileRef = 6E012CA6525EE76D8540F1AA /* EntriesTreeDataSource.swift */; };
7E0562F9261AB07800F6CDD7 /* screenshot-entries.txt in Resources */ = {isa = PBXBuildFile; fileRef = 7E0562F8261AB07800F6CDD7 /* screenshot-entries.txt */; };
7E07C3B824D121770007FD23 /* DayEndReportController.swift in Sources */ = {isa = PBXBuildFile; fileRef = 7E07C3B624D121770007FD23 /* DayEndReportController.swift */; };
Expand Down Expand Up @@ -191,6 +194,9 @@

/* Begin PBXFileReference section */
6E01214AC5B51926451AA2BC /* PushableString.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = PushableString.swift; sourceTree = "<group>"; };
6E01231E7D1758169D1CCC4D /* PtnViewControllerTest.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = PtnViewControllerTest.swift; sourceTree = "<group>"; };
6E01235C79DD985D5113CB89 /* ControllerTestBase.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = ControllerTestBase.swift; sourceTree = "<group>"; };
6E0128A0FF062E560EA67CD9 /* NSEventHelpers.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = NSEventHelpers.swift; sourceTree = "<group>"; };
6E0129247B26E0835305551D /* ExtraAssertions.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = ExtraAssertions.swift; sourceTree = "<group>"; };
6E012C854B86EF8C65210EBE /* Sorting.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = Sorting.swift; sourceTree = "<group>"; };
6E012CA6525EE76D8540F1AA /* EntriesTreeDataSource.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = EntriesTreeDataSource.swift; sourceTree = "<group>"; };
Expand Down Expand Up @@ -383,6 +389,7 @@
isa = PBXGroup;
children = (
6E0129247B26E0835305551D /* ExtraAssertions.swift */,
6E0128A0FF062E560EA67CD9 /* NSEventHelpers.swift */,
);
path = test_helpers;
sourceTree = "<group>";
Expand Down Expand Up @@ -481,6 +488,8 @@
children = (
7E210AF6284C3CA60050AD31 /* PrefsViewControllerTest.swift */,
6E012D78358839E319024F1D /* LargeReportController */,
6E01235C79DD985D5113CB89 /* ControllerTestBase.swift */,
6E01231E7D1758169D1CCC4D /* PtnViewControllerTest.swift */,
);
path = controllers;
sourceTree = "<group>";
Expand Down Expand Up @@ -1098,6 +1107,9 @@
7E63051E2846C71700565880 /* SegmentedTimelineViewTest.swift in Sources */,
6E012342D0F39A9653C28569 /* ExtraAssertions.swift in Sources */,
6E012216A8475ABF72DEBC69 /* EntriesTreeControllerTest.swift in Sources */,
6E01289DF626AB0D2507E523 /* ControllerTestBase.swift in Sources */,
6E012A5B746EC256131FAD0A /* PtnViewControllerTest.swift in Sources */,
6E012DDCB930C9662AA534E4 /* NSEventHelpers.swift in Sources */,
);
runOnlyForDeploymentPostprocessing = 0;
};
Expand Down
32 changes: 21 additions & 11 deletions whatdid/controllers/PtnViewController.swift
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,16 @@ class PtnViewController: NSViewController {

var scheduler: Scheduler = DefaultScheduler.instance

private var modelOverride: Model?

private var model: Model {
modelOverride ?? AppDelegate.instance.model
}

func override(model: Model) {
modelOverride = model
}

override func viewDidLoad() {
super.viewDidLoad()
projectField.placeholderString = "project"
Expand All @@ -43,10 +53,10 @@ class PtnViewController: NSViewController {
}
}
projectField.optionsLookup = {
AppDelegate.instance.model.listProjects()
self.model.listProjects()
}
taskField.optionsLookup = {
AppDelegate.instance.model.listTasks(project: self.projectField.stringValue)
self.model.listTasks(project: self.projectField.stringValue)
}
projectField.onTextChange = {
self.taskField.stringValue = ""
Expand All @@ -58,8 +68,8 @@ class PtnViewController: NSViewController {

projectTaskFinder.onOpen = {
var options = [ProjectAndTask]()
for project in AppDelegate.instance.model.listProjects() {
for task in AppDelegate.instance.model.listTasks(project: project) {
for project in self.model.listProjects() {
for task in self.model.listTasks(project: project) {
options.append(ProjectAndTask(project: project, task: task))
}
}
Expand All @@ -68,7 +78,6 @@ class PtnViewController: NSViewController {
task: self.taskField.stringValue,
notes: self.noteField.stringValue)
return (saveState, options)

}
projectTaskFinder.previewSelect = {pt in
self.projectField.stringValue = pt.project
Expand Down Expand Up @@ -250,7 +259,7 @@ class PtnViewController: NSViewController {
}

private func updateHeaderText() {
let lastEntryDate = AppDelegate.instance.model.lastEntryDate
let lastEntryDate = model.lastEntryDate
headerText.stringValue = headerText.placeholderString!.replacingBracketedPlaceholders(with: [
"TIME": TimeUtil.formatSuccinctly(date: lastEntryDate),
"DURATION": TimeUtil.daysHoursMinutes(for: scheduler.timeInterval(since: lastEntryDate))
Expand Down Expand Up @@ -302,7 +311,7 @@ class PtnViewController: NSViewController {
let confirm = ConfirmViewController()
let showConfirmation = confirm.prepareToAttach(to: window)
let duration = TimeUtil.daysHoursMinutes(
for: scheduler.timeInterval(since: AppDelegate.instance.model.lastEntryDate))
for: scheduler.timeInterval(since: model.lastEntryDate))
confirm.header = "Skip this session?"
confirm.detail = """
If you skip this session, the last \(duration) will not be recorded.
Expand All @@ -323,7 +332,7 @@ class PtnViewController: NSViewController {
}

private func skipSession() {
AppDelegate.instance.model.setLastEntryDateToNow()
model.setLastEntryDateToNow()
self.closeWindowAsync()
}

Expand Down Expand Up @@ -393,8 +402,9 @@ class PtnViewController: NSViewController {
private func grabFocusEvenIfHasSheet() {
perform(#selector(grabFocusNow), with: nil, afterDelay: TimeInterval.zero, inModes: [RunLoop.Mode.common])
}

@objc private func grabFocusNow() {

/// Only intended for unit tests. In prod code, prefer `grabFocus`.
@objc func grabFocusNow() {
var firstResponder = noteField
if projectField.stringValue.isEmpty {
firstResponder = projectField
Expand Down Expand Up @@ -430,7 +440,7 @@ class PtnViewController: NSViewController {
allowEntry = false
}
if allowEntry {
AppDelegate.instance.model.addEntryNow(
model.addEntryNow(
project: project,
task: task,
notes: notes,
Expand Down
29 changes: 21 additions & 8 deletions whatdid/views/TextFieldWithPopup.swift
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,10 @@ class TextFieldWithPopup: WhatdidTextField, NSTextViewDelegate, NSTextFieldDeleg
}
}
}

var popupIsOpen: Bool {
popupManager.isOpen
}

/// Called after the rest of initialization is complete. Put whatever you want here.
func finishInit() {
Expand Down Expand Up @@ -231,10 +235,11 @@ class TextFieldWithPopup: WhatdidTextField, NSTextViewDelegate, NSTextFieldDeleg

override func textDidChange(_ notification: Notification) {
super.textDidChange(notification)
textDidChange()
updatePopupAutocomplete(dueToTextChange: true)
}

private func textDidChange() {
private func updatePopupAutocomplete(dueToTextChange: Bool) {
var notifyTextChange = dueToTextChange
let maybeAutocomplete = popupManager.contents?.onTextChanged(to: stringValue)
// If the selection is at the tail of the string, fill in the autocomplete.
if shouldAutocompleteOnTextChange, let autoComplete = maybeAutocomplete, autoComplete.starts(with: stringValue) {
Expand All @@ -243,17 +248,23 @@ class TextFieldWithPopup: WhatdidTextField, NSTextViewDelegate, NSTextFieldDeleg
let autocompleTail = String(autoComplete.dropFirst(stringValue.count))
let stringCountBeforeAutocomplete = stringValue.count
stringValue += autocompleTail
notifyTextChange = true
currentEditor()!.selectedRange = NSRange(location: stringCountBeforeAutocomplete, length: charsToAutocomplete)
}
}
onTextChange()
if notifyTextChange {
onTextChange()
}
}

override func textDidEndEditing(_ notification: Notification) {
if popupManager.isOpen, let selected = popupManager.contents?.selectedText {
// This occurs when the user hit "enter" after arrowing down to the text, instead of
// clicking on it with the cursor.
stringValue = selected
if selected != stringValue {
stringValue = selected
onTextChange()
}
}
super.textDidEndEditing(notification)
popupManager.close()
Expand Down Expand Up @@ -286,7 +297,7 @@ class TextFieldWithPopup: WhatdidTextField, NSTextViewDelegate, NSTextFieldDeleg
popupManager.show(
minWidth: frame.width,
atTopLeft: originWithinScreen)
textDidChange()
updatePopupAutocomplete(dueToTextChange: false)
}

private class NoKeyButton: NSButton {
Expand Down Expand Up @@ -439,7 +450,7 @@ fileprivate class PopupManager: NSObject, NSWindowDelegate, TextFieldWithPopupCa
// let the popup handle it.
if contents.asView.bounds.contains(locationInContents) {
if let result = contents.handleClick(at: locationInContents) {
parent.stringValue = result // Don't call setText! It will trigger onTextChange(), which double-notifies
setText(to: result)
/// `let _ =`: this returns `false` if the action/target aren't set. But we don't care, it's still handled.
let _ = parent.sendAction(parent.action, to: parent.target)
close()
Expand Down Expand Up @@ -481,8 +492,10 @@ fileprivate class PopupManager: NSObject, NSWindowDelegate, TextFieldWithPopupCa
}

func setText(to string: String) {
parent.stringValue = string
parent.onTextChange()
if string != parent.stringValue {
parent.stringValue = string
parent.onTextChange()
}
}

func scroll(to bounds: NSRect, within: NSView) {
Expand Down
84 changes: 84 additions & 0 deletions whatdidTests/controllers/ControllerTestBase.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
// whatdidTests?

import XCTest
@testable import whatdid

class ControllerTestBase<T: NSViewController>: XCTestCase {

private(set) var thisMorning: Date!
private var model: Model!

override func setUpWithError() throws {
thisMorning = TimeUtil.dateForTime(.previous, hh: 9, mm: 00)
}

func loadModel(withData dataLoader: (DataBuilder) -> Void) -> Model {
let uniqueName = name.replacingOccurrences(of: "\\W", with: "", options: .regularExpression)
model = Model(modelName: uniqueName, clearAllEntriesOnStartup: true)
// fetch the (empty set of) entries, to force the model's lazy loading. Otherwise, the unit test's adding of entries can
// race with the controller's fetching of them, such that they both try to clear out the same set of old files (and
// whoever gets there second, fails due to those files not being there.)
let _ = model.listEntries(from: thisMorning, to: DefaultScheduler.instance.now)

let dataBuilder = DataBuilder(using: model, startingAt: thisMorning)
dataLoader(dataBuilder)

// Wait until we have as many entries as our DataBuilder expects
let timeoutAt = Date().addingTimeInterval(3)
while model.listEntries(from: Date.distantPast, to: Date.distantFuture).count < dataBuilder.expected {
usleep(50000)
XCTAssertLessThan(Date(), timeoutAt)
}
return model
}

func createController(withData dataLoader: (DataBuilder) -> Void) -> T {
let _ = loadModel(withData: dataLoader)
if let nib = NSNib(nibNamed: nibName, bundle: Bundle(for: T.self)) {
var topLevelObjects: NSArray? = NSArray()
nib.instantiate(withOwner: nil, topLevelObjects: &topLevelObjects)
if let controller = topLevelObjects?.compactMap({$0 as? T}).first {
controller.viewDidLoad()
load(model: model, into: controller)
return controller
} else {
XCTFail("couldn't find EntriesTreeController in nib")
}
} else {
XCTFail("couldn't load nib")
}
fatalError()
}

func load(model: Model, into controller: T) {
XCTFail("must override this method!")
}

var nibName: String {
T.className().replacingOccurrences(of: "^.*\\.", with: "", options: .regularExpression)
}

class DataBuilder {
private let model: Model
private let startingAt: Date
private var lastEventOffset = TimeInterval(0)
private(set) var expected = 0
/// An offset that's applied to all events (after you set this; it's not retroactive).
var eventOffset = TimeInterval(0)

init(using model: Model, startingAt: Date) {
self.model = model
self.startingAt = startingAt
}

func add(project: String, task: String, note: String, withDuration minutes: Int) {
let thisTaskDuration = TimeInterval(minutes * 60)
let from = startingAt.addingTimeInterval(lastEventOffset + eventOffset)
let to = from.addingTimeInterval(thisTaskDuration)
let entry = FlatEntry(from: from, to: to, project: project, task: task, notes: note)
model.add(entry, andThen: {})
lastEventOffset += thisTaskDuration
expected += 1
}
}
}
Loading

0 comments on commit d6f4f5d

Please sign in to comment.