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.

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 ViewController open #544

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

tomaszpieczykolan
Copy link
Contributor

This PR makes the ViewController class open, so that it can be overriden.

@tomaszpieczykolan
Copy link
Contributor Author

This is work in progress. Before submitting for review, I would like to audit the whole ViewController class and write some more unit tests for it.

@tomaszpieczykolan tomaszpieczykolan force-pushed the open_viewcontroller branch 3 times, most recently from 36588b9 to 90b3e9f Compare May 18, 2021 20:29
Tests/UICoreTests/ViewControllerTests.swift Show resolved Hide resolved
Tests/UICoreTests/ViewControllerTests.swift Outdated Show resolved Hide resolved
func testTitleGetterAndSetter() {
let sut = ViewController()

// XCTAssertNil(sut.title) // This is currently failing, but the initial value of `title` should be `nil`
Copy link
Owner

Choose a reason for hiding this comment

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

Oh interesting, this sounds like you've found a bug!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can this bug be fixed outside the scope of this PR, and can we mark this discussion as resolved?

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah, that seems reasonable. Please write the test with the correct expectation; I'll add a utility to help mark the test as expected to fail.

Tests/UICoreTests/ViewControllerTests.swift Outdated Show resolved Hide resolved
Tests/UICoreTests/ViewControllerTests.swift Outdated Show resolved Hide resolved
Tests/UICoreTests/ViewControllerTests.swift Show resolved Hide resolved
Tests/UICoreTests/ViewControllerTests.swift Outdated Show resolved Hide resolved
Tests/UICoreTests/ViewControllerTests.swift Outdated Show resolved Hide resolved
@tomaszpieczykolan tomaszpieczykolan force-pushed the open_viewcontroller branch 2 times, most recently from 356c6f2 to eaf7550 Compare May 22, 2021 17:21
@compnerd
Copy link
Owner

@tomaszpieczykolan would you mind rebasing the change? I think that the new features that I've enabled will be nice to take advantage of.

@codecov
Copy link

codecov bot commented May 29, 2021

Codecov Report

Merging #544 (6b9c15d) into main (fda786e) will increase coverage by 1.19%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #544      +/-   ##
==========================================
+ Coverage   13.79%   14.98%   +1.19%     
==========================================
  Files         111      111              
  Lines        4443     4443              
==========================================
+ Hits          613      666      +53     
+ Misses       3830     3777      -53     
Impacted Files Coverage Δ
...s/SwiftWin32/View Controllers/ViewController.swift 46.75% <ø> (+46.75%) ⬆️
...n32/Views and Controls/DirectionalEdgeInsets.swift 43.75% <0.00%> (+12.50%) ⬆️
...in32/App and Environment/ApplicationDelegate.swift 17.54% <0.00%> (+17.54%) ⬆️
...ces/SwiftWin32/Views and Controls/EdgeInsets.swift 33.33% <0.00%> (+33.33%) ⬆️

@egorzhdan
Copy link
Contributor

The ability to subclass ViewController & override buildMenu(with: MenuBuilder) is really helpful for menus, it would be great to merge this PR if there is no concern left.

@compnerd
Copy link
Owner

compnerd commented Jul 3, 2021

I that that when the class is being marked open, we need to ensure that the members are properly marked as open as well, which is not the case yet. Furthermore, @tomaszpieczykolan has this marked as a draft still. I definitely am waiting on this change as I have some changes sitting around that require this to be open.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants