Remove RxSwift in favor of combine #671
Conversation
allenhumphreys
commented
May 28, 2023
•
allenhumphreys
commented
- App startup is a flickering mess
- Check for performance optimizations on the streams especially with regard to realm
- Refine the UI binding extensions dealing with Error == Never and the default values and such. Also, add ones to drive an @published UI property
- TouchBar/Now Playing test?
- CloudKit Code
|
This is way smaller than I thought it would be 😅 We'll definitely wait until after next week's keynote before merging this, since we already have a lengthy PR (#669) for this year's update, but I'm definitely in favor of doing it. |
allenhumphreys
force-pushed
the
ah/rx-swift-removal
branch
16 times, most recently
from
aa15e8b to
fb578c8
Compare
allenhumphreys
force-pushed
the
ah/rx-swift-removal
branch
from
fb578c8 to
bd1ef21
Compare
|
@insidegui I think this should probably get merged in so you can start building on it too. I don't think it's perfect, but there's some good fixes in here. I cannot test Touch Bar or CloudKit stuff, so there's a few places that are #if 'd away that may need to be migrated to Combine! |
| if !hasPerformedInitialListUpdate { | ||
| // Filters only need configured once, the other stuff in | ||
| // here might only need to happen once as well | ||
| self.searchCoordinator.configureFilters() |
There was a problem hiding this comment.
This was causing filters to be reset after the content sync, I put a gate around it for now, but the long term solution is different than this.
There was a problem hiding this comment.
OMG thanks, this has been driving me nuts!
There was a problem hiding this comment.
The original reason it's always called is because it uses values from Storage. But the 1st setup and the update need to be separated so that they can take any user modifications into account after the first load. By blocking this, we're potentially missing an update to filtering capabilities, but that's a small thing for now!
|
@allenhumphreys I don't have Touch Bar hardware either, but there's a simulator in Xcode. Window > Touch Bar > Show Touch Bar. Could you test with that and let me know? I'll check out the CloudKit stuff. |
allenhumphreys
changed the title
allenhumphreys
force-pushed
the
ah/rx-swift-removal
branch
from
554b2f9 to
45af680
Compare
| latestInfo = info | ||
| if info.taskState == .suspended { | ||
| // We can get progress updates that were from while the task was suspending | ||
| return DownloadStatus.paused(info) |
There was a problem hiding this comment.
@insidegui This fixes the bug where the pause/resume button gets into the wrong state which has been there since I implemented it like 4 years ago!
@insidegui It's still working as expected, I think the icons will get an update when you finish your branch, so remember to check the Touch Bar buttons in that branch! |
|
I wonder if we should even keep Touch Bar support in. It’s not trivial to support and Apple has made it quite clear that it’s a dead feature. |
|
If the now playing stuff gives the basic controls in the touchbar... seems like that could make sense. On the other hand there are a lot of touchbar macs out there. |
