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

Catalyst Release Notes Support #1482

Closed
Vortec4800 opened this issue Oct 7, 2019 · 37 comments · Fixed by #2315
Closed

Catalyst Release Notes Support #1482

Vortec4800 opened this issue Oct 7, 2019 · 37 comments · Fixed by #2315
Milestone

Comments

@Vortec4800
Copy link

Can I use Sparkle to auto-update apps (distributed outside of the Mac App Store of course) that are built using Catalyst? Is there anything I need to do special to make it all work?

@kornelski
Copy link
Member

I haven't tried Catalyst. If it's compatible with Cocoa and can use frameworks, it should be able to use Sparkle.

@samdeane
Copy link
Contributor

Sparkle builds against the normal macOS sdk, so I'm pretty sure you can't use it directly from a Catalyst app.

You can probably use it by bridging to macOS via a plugin, and having the plugin link against Sparkle and create the SUUpdater object.

I'm not sure how well this will work, but I'm planning to try it. Will let you know... :)

@samdeane
Copy link
Contributor

samdeane commented Feb 26, 2020

Update: I think the approach I described above should work, but I've currently hit an issue.

My bridging plugin links to Sparkle, embeds the framework, and owns an SPUStandardUpdaterController object. It sets itself to be the updaterDelegate.

The various SU keys are in the main application's Info.plist, as usual.

Checking for updating appears to be working fine.

However, currently, when it finds an update, I'm getting a crash in [SPUStandardUserDriver setUpFocusForActiveUpdateAlert], calling [self.activeUpdateAlert window].

It seems that the system is actually crashing inside WebView, trying to access the alert's window:

Stack

As yet, I haven't figured out what's going on here.

@samdeane
Copy link
Contributor

I did wonder if this was something to do with WebView being deprecated on 10.15, and the system log is showing

2020-02-26 14:29:30.695271+0000 Action Status[14298:327670] -[WebView initWithCoder:]: unrecognized selector sent to instance 0x7b180006e1c0

which looks suspiciously like WebView itself isn't there.

However, I've tried swapping out WebView for WKWebView, and I'm still getting a crash in a similar place, so I don't think it's that.

@samdeane
Copy link
Contributor

This crash seems to be related to using WebKit from the bridging plugin, and not specifically anything to do with Sparkle. I suspect that it may be caused by WebKit getting confused about whether or not it's running under AppKit or Catalyst.

Unfortunately WebKit is used by the default driver (via SUUpdateAlert), so it may be necessary to write an alternative driver.

@kornelski
Copy link
Member

Does it help if you hide release notes?

@samdeane
Copy link
Contributor

So, I didn't try the hiding of release notes, but I suspect that it wouldn't help.

The WebView would still be in the xib, and just opening a xib-based window containing a WebView from the AppKit side of things is enough to crash.

However, I have got it working: https://twitter.com/samdeane/status/1233081407947395074

The approach I took required a bit of effort, but it allows me to present a custom Sparkle UI from the Catalyst side of things (as it happens I'm using SwiftUI for this, though it's not required).

To do this I had to create a custom Sparkle user driver (implementing the SPUUserDriver protocol), and have it forward all communication over to the Catalyst application.

The details are probably too much to go into here, but I will write it up as a blog post and drop a link here when I'm done.

@samdeane
Copy link
Contributor

I've not yet had time to write that blog post, but I have managed to generalise the code a bit and extract it into an example which you can find here.

@samdeane
Copy link
Contributor

samdeane commented Mar 5, 2020

FWIW I updated generalised my code a bit more and made a fairly self-contained solution.

You just link to the framework from your Catalyst app, and it deals with embedding Sparkle and loading the plugin.

@tfonfara
Copy link

tfonfara commented Dec 2, 2020

@kornelski I've also tested a bit, unfortunately hiding the release notes doesn't help as @samdeane already expected because the nib still contains the class.
But if I just remove the class from the xib everything works fine (without release notes but that's okay). Would it be possible to not instantiate the webview from the interface file but adding it from code with a "class exists" check?

@tfonfara
Copy link

tfonfara commented Dec 2, 2020

Another idea: I just noticed this marker:
WEBKIT_CLASS_DEPRECATED_MAC(10_3, 10_14, "No longer supported; please adopt WKWebView.")
If WKWebView would be supported this issue would be solved automatically 😊

@kornelski
Copy link
Member

WKWebView requires macOS 10.10+. I've only very recently raised minimum macOS version for Sparkle to 10.9.

@tfonfara
Copy link

tfonfara commented Dec 3, 2020

Got it, what about a if ([WebView class]) { ... } or if (NSClassFromString(@"WebView")) { ... } check? 🤔 Like

if ([WebView class]) {
    WebView webView = [[[WebView] alloc] initWithFrame: [containerView bounds]];
    // set autoresizingMasks
    [containerView addSubview: webView];
} else {
    // set showsReleaseNotes to false to prevent crashes
}

@kornelski
Copy link
Member

I know some apps use release notes to warn that an update is not free, so hiding release notes could be an unpleasant surprise.

@kornelski
Copy link
Member

Maybe it would be fine to switch Sparkle 2.x only?

@tfonfara
Copy link

tfonfara commented Dec 3, 2020

I could try, can you push the 2.x to cocoapods please (maybe as 2.x-beta)? I've just checked the repo but only the 1.x is available there 🤔

@kornelski
Copy link
Member

I don't use Cocoapods. I could release 2.0-beta if you create a podspec.

@tfonfara
Copy link

tfonfara commented Dec 3, 2020

What about the podspec that is in the project already?

zorgiepoo added a commit that referenced this issue Dec 31, 2020
We fallback to legacy WebView in two cases:

* On systems prior to macOS 10.11 (it may be possible to support WKWebView in 10.10, but due to an 10.11+ only API I didn't bother putting in the effort)
* For apps using the downloader XPC service which are most likely sandboxed apps without an outgoing network entitlement. WKWebView doesn't function in that case (FB6993802 - feedback-assistant/reports#1)

Additional notes:

* Added an abstraction for using WebView or WKWebView
* Web view is created and inserted programmatically rather than from the nib which has previously caused issues (and is not allowed for WKWebView prior to 10.12 due to some NSCoding bug).
* We need to rely on WKWebView private API for drawing transparent background (FB7539179 - feedback-assistant/reports#81). Additional notes in the code.
* We always inject a custom stylesheet for dark mode colors and default font family/size (for WKWebView).
* This should also improve the Catalyst support story (#1482)
kornelski pushed a commit that referenced this issue Jan 3, 2021
We fallback to legacy WebView in two cases:

* On systems prior to macOS 10.11 (it may be possible to support WKWebView in 10.10, but due to an 10.11+ only API I didn't bother putting in the effort)
* For apps using the downloader XPC service which are most likely sandboxed apps without an outgoing network entitlement. WKWebView doesn't function in that case (FB6993802 - feedback-assistant/reports#1)

Additional notes:

* Added an abstraction for using WebView or WKWebView
* Web view is created and inserted programmatically rather than from the nib which has previously caused issues (and is not allowed for WKWebView prior to 10.12 due to some NSCoding bug).
* We need to rely on WKWebView private API for drawing transparent background (FB7539179 - feedback-assistant/reports#81). Additional notes in the code.
* We always inject a custom stylesheet for dark mode colors and default font family/size (for WKWebView).
* This should also improve the Catalyst support story (#1482)
@zorgiepoo
Copy link
Member

@samdeane @tfonfara The story here should be somewhat better after #1704 landed (and #1709 lands for 1.x). Notably WKWebView is preferred, and the web views aren't instantiated in the nibs now. In theory WKWebView should "just work", in practice I haven't tried it out.

@vincode-io
Copy link
Contributor

@zorgiepoo I gave it a try and there are still WKWebView problems. I guess because WKWebView is different between UIKit and AppKit? Trying to hide the release notes didn't seem to help.

2021-01-05 13:17:55.536020-0600 Zavala[45635:8477758] -[WKWebView _setSuperview:]: unrecognized selector sent to instance 0x7f8440079400
2021-01-05 13:17:55.539372-0600 Zavala[45635:8477758] [General] An uncaught exception was raised
2021-01-05 13:17:55.539455-0600 Zavala[45635:8477758] [General] -[WKWebView _setSuperview:]: unrecognized selector sent to instance 0x7f8440079400
2021-01-05 13:17:55.539572-0600 Zavala[45635:8477758] [General] (
	0   CoreFoundation                      0x00007fff204b36af __exceptionPreprocess + 242
	1   libobjc.A.dylib                     0x00007fff201eb3c9 objc_exception_throw + 48
	2   CoreFoundation                      0x00007fff20535c85 -[NSObject(NSObject) __retain_OA] + 0
	3   UIKitCore                           0x00007fff45f381e9 -[UIResponder doesNotRecognizeSelector:] + 292
	4   CoreFoundation                      0x00007fff2041b07d ___forwarding___ + 1467
	5   CoreFoundation                      0x00007fff2041aa38 _CF_forwarding_prep_0 + 120
	6   AppKit                              0x00007fff22c71cc2 -[NSView addSubview:] + 174
	7   Sparkle                             0x000000010a0e8228 -[SUUpdateAlert windowDidLoad] + 169
	8   AppKit                              0x00007fff22e29b4a -[NSWindowController _windowDidLoad] + 570
	9   AppKit                              0x00007fff22e257b2 -[NSWindowController window] + 110
	10  Sparkle                             0x000000010a0c8999 -[SPUStandardUserDriver setUpFocusForActiveUpdateAlert] + 58
	11  Sparkle                             0x000000010a0c8d5e -[SPUStandardUserDriver showUpdateFoundWithAlertHandler:] + 299
	12  Sparkle                             0x000000010a0c8e4b -[SPUStandardUserDriver showUpdateFoundWithAppcastItem:userInitiated:reply:] + 171
	13  Sparkle                             0x000000010a0cd0e3 -[SPUUIBasedUpdateDriver basicDriverDidFindUpdateWithAppcastItem:] + 375
	14  Sparkle                             0x000000010a0bd5b5 -[SPUCoreBasedUpdateDriver basicDriverDidFindUpdateWithAppcastItem:] + 356
	15  Sparkle                             0x000000010a0bc46b -[SPUBasicUpdateDriver didFindValidUpdateWithAppcastItem:] + 454
	16  Sparkle                             0x000000010a0d7d55 -[SUAppcastDriver appcastDidFinishLoading:includesSkippedUpdates:] + 976
	17  Sparkle                             0x000000010a0d7972 __96-[SUAppcastDriver loadAppcastFromURL:userAgent:httpHeaders:inBackground:includesSkippedUpdates:]_block_invoke + 110
	18  Sparkle                             0x000000010a0d59b5 __62-[SUAppcast fetchAppcastFromURL:inBackground:completionBlock:]_block_invoke + 281
	19  libdispatch.dylib                   0x0000000107ecbe78 _dispatch_call_block_and_release + 12
	20  libdispatch.dylib                   0x0000000107ecd0b0 _dispatch_client_callout + 8
	21  libdispatch.dylib                   0x0000000107edd260 _dispatch_main_queue_callback_4CF + 1107
	22  CoreFoundation                      0x00007fff20476970 __CFRUNLOOP_IS_SERVICING_THE_MAIN_DISPATCH_QUEUE__ + 9
	23  CoreFoundation                      0x00007fff20438852 __CFRunLoopRun + 2731
	24  CoreFoundation                      0x00007fff204376ce CFRunLoopRunSpecific + 563
	25  HIToolbox                           0x00007fff28755eb0 RunCurrentEventLoopInMode + 292
	26  HIToolbox                           0x00007fff28755cac ReceiveNextEventCommon + 709
	27  HIToolbox                           0x00007fff287559cf _BlockUntilNextEventMatchingListInModeWithFilter + 64
	28  AppKit                              0x00007fff22c4fde9 _DPSNextEvent + 883
	29  AppKit                              0x00007fff22c4e5af -[NSApplication(NSEvent) _nextEventMatchingEventMask:untilDate:inMode:dequeue:] + 1366
	30  AppKit                              0x00007fff22c40b0a -[NSApplication run] + 586
	31  AppKit                              0x00007fff22c14df2 NSApplicationMain + 816
	32  AppKit                              0x00007fff22f0b80a _NSApplicationMainWithInfoDictionary + 16
	33  UIKitMacHelper                      0x00007fff3459dffe UINSApplicationMain + 1418
	34  UIKitCore                           0x00007fff4525be90 UIApplicationMain + 144
	35  libswiftUIKit.dylib                 0x00007fff58049d42 $s5UIKit17UIApplicationMainys5Int32VAD_SpySpys4Int8VGGSgSSSgAJtF + 98
	36  Zavala                              0x00000001077ea25a $sSo21UIApplicationDelegateP5UIKitE4mainyyFZ + 122
	37  Zavala                              0x00000001077ea1ce $s6Zavala11AppDelegateC5$mainyyFZ + 46
	38  Zavala                              0x00000001077ea2a9 main + 41
	39  libdyld.dylib                       0x00007fff2035c621 start + 1
)

@vincode-io
Copy link
Contributor

I did get this working by making one small change shown in #1713. If you make that change, disable showing release notes, and use a bundle to load Sparkle, it works. This wouldn't have been possible without #1704, so many thanks for that change.

Showing release notes in a Catalyst app with Sparkle is something I suspect isn't possible. The UIKit WKWebView vs. the AppKit WKWebView are probably too different.

At least this gets those of us doing Catalyst apps a way to distribute test builds, since there still isn't TestFlight for the Mac yet.

@tfonfara
Copy link

tfonfara commented Jan 6, 2021

Actually I figured out that even without any Sparkle code at all, if run this:

let view = NSView(frame: NSRect(x: 0, y: 0, width: 100, height: 100))
view.addSubview(WKWebView(frame: NSRect(x: 0, y: 0, width: 100, height: 100), configuration: WKWebViewConfiguration()))

within my AppKit bundle, it crashes with same error.

I've created a related bug report at feedbackassistant.apple.com.

@zorgiepoo
Copy link
Member

zorgiepoo commented Jan 6, 2021

Yeah I guess WKWebView isn't a NSView in this case which sounds quite odd to me.. but I'm not so familiar with Catalyst.

Thanks for #1713 which is a good addition and for filing the Feedback report.

And kudos for trying the custom user driver / UI approach here.

@tfonfara
Copy link

tfonfara commented Jan 6, 2021

I've adopted the changes of @vincode-io for the 1.x version in #1715

@tfonfara
Copy link

FYI, I just checked the status of my feedback report, it is set to: Resolution: Investigation complete - Works as currently designed...

@zorgiepoo
Copy link
Member

zorgiepoo commented Jan 17, 2021

It then seems like if you want to use a web view to show release notes in a Catalyst app, you need to show the web view on the iOS side of your app, which would entail a custom user driver (2.x). Hiding release notes is probably simpler for majority.

@zorgiepoo
Copy link
Member

Catalyst is more documented on https://sparkle-project.org/documentation/programmatic-setup/ now.

@WeirdHat
Copy link

WeirdHat commented Sep 7, 2022

Hello - I have run into this problem, and hiding release notes doesn't seem like a satisfactory solution, nor does reinventing the wheel by replacing the whole window with a custom UI.

How feasible would it be to display release notes with something other than a WKWebView? I believe NSAttributedString can be created from HTML, and displayed in an NSTextView?

Failing that, can anybody who's already solved this with a custom UI share their code?

@zorgiepoo
Copy link
Member

zorgiepoo commented Sep 8, 2022

sparkle-cli has a custom UI (or lack of one) that creates an NSAttributedString from the HTML and prints it to the console. So if you create a custom UI, it should be possible.

@zorgiepoo
Copy link
Member

zorgiepoo commented Sep 8, 2022

If you want Sparkle to automatically use a NSTextView for Catalyst apps then maybe that is possible as well (maybe would involve a new SUWebView implementation). I don't know if it will look good enough to "ship" though and someone would have to contribute to it.

@WeirdHat
Copy link

WeirdHat commented Sep 8, 2022

Okay, so I think I got it working well enough for my own purposes, with an SUWebView implementation that does this:

- (instancetype)initWithColorStyleSheetLocation:(NSURL *)colorStyleSheetLocation fontFamily:(NSString *)fontFamily fontPointSize:(int)fontPointSize javaScriptEnabled:(BOOL)javaScriptEnabled
{
    self = [super init];
    if (self != nil) {
        _scrollView = [[NSScrollView alloc] initWithFrame: NSZeroRect];
        _textView = [[NSTextView alloc] initWithFrame: NSZeroRect];
        _scrollView.documentView = _textView;
    }
    return self;
}

- (NSView *)view
{
    return self.scrollView;
}

- (void)loadHTMLString:(NSString *)htmlString baseURL:(NSURL * _Nullable)baseURL completionHandler:(void (^)(NSError * _Nullable))completionHandler
{
    self.completionHandler = [completionHandler copy];
    
    NSData* data = [htmlString dataUsingEncoding:NSUTF8StringEncoding];
    NSAttributedString *attributedString = [[NSAttributedString alloc] initWithHTML:data documentAttributes:nil];
    [[self.textView textStorage] setAttributedString:attributedString];
    
    NSSize contentSize = [_scrollView contentSize];
    [_textView setFrame:NSMakeRect(0, 0, contentSize.width, contentSize.height)];
    [_textView setMinSize:NSMakeSize(0.0, contentSize.height)];
    [_textView setMaxSize:NSMakeSize(FLT_MAX, FLT_MAX)];
    [_textView setVerticallyResizable:YES];
    [_textView setHorizontallyResizable:NO];
    [_textView setAutoresizingMask:NSViewWidthSizable];
    [_textView setTextContainerInset:NSMakeSize(8, 8)];
    _textView.editable = NO;
    
    [_scrollView setHasVerticalScroller:YES];
    [_scrollView setHasHorizontalScroller:NO];
    
    if (self.completionHandler != nil) {
        self.completionHandler(nil);
        self.completionHandler = nil;
    }
}

Like you said, not good enough to ship, but hopefully this is a helpful start if anybody wants/needs to take it further.

@zorgiepoo
Copy link
Member

Nice. How does it look though or what’s lacking?

@WeirdHat
Copy link

WeirdHat commented Sep 9, 2022

Looks basically fine, except it's not changing the text color for dark mode (that might be an easy fix though)

Screen Shot 2022-09-08 at 9 35 46 PM

But some things it lacks that I don't need personally:

  • Doesn't support the styling that the webview does.
  • Doesn't support javascript.
  • I haven't tested what would happen if you clicked on a link in it.
  • Doesn't check whether it's running in a Catalyst app or not - I'm not sure it's actually possible to know that, since it has to be run from a plugin that's separate from the Catalyst part of the app, so it might have to be a manual option.

@zorgiepoo
Copy link
Member

Doesn't support the styling that the webview does.

The styling from the attributed string will be as much as we can get here.

Doesn't support javascript.

Yeah this is expected (Sparkle also disables javascript by default generally; most apps don't use it).

Doesn't check whether it's running in a Catalyst app or not - I'm not sure it's actually possible to know that, since it has to be run from a plugin that's separate from the Catalyst part of the app, so it might have to be a manual option.

I think you can use -[NSProcessInfo macCatalystApp]

Link clicking and dark mode should be possible or at least TextEdit can do it. I think this would be nice over no release notes.

@zorgiepoo
Copy link
Member

Dark mode should be usesAdaptiveColorMappingForDarkAppearance property.

Link clicking might need to override this delegate method https://developer.apple.com/documentation/appkit/nstextviewdelegate/1449527-textview?language=objc - for web views we do have some "safe link" handling and only allow certain url schemes.

@zorgiepoo
Copy link
Member

I looked into this more and tried to do a more complete implementation based on @WeirdHat 's snippet however it cannot be shipped.

The NSAttributedString HTML reader APIs use WebKit underneath. If you pass any HTML options like I tried to do, a Catalyst app will crash with the stacktrace pointing to WebKit usage. If you don't pass any options, it will "work" (for now) but I consider this brittle and easily breakable in the future.

To use NSAttributedString+HTML, I think there needs to be an out-of-process (XPC) implementation. If you want to support a non-HTML path, this could technically be safe and do-able today though (but I consider this somewhat incomplete).

@zorgiepoo zorgiepoo changed the title Catalyst App Support Catalyst Release Notes Support Sep 18, 2022
@zorgiepoo zorgiepoo reopened this Sep 18, 2022
@zorgiepoo zorgiepoo added this to the 2.4 milestone Sep 18, 2022
@zorgiepoo
Copy link
Member

zorgiepoo commented Feb 12, 2023

I added support for a plain text view for release notes in #2315

After thinking about it, I don't like the idea of converting HTML into a NSAttributedString. I don't want it to (potentially) go through WebKit and it also didn't always give the result I'd expect if I recall correctly.

I think markdown support in the future would make a better alternative #2319

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 a pull request may close this issue.

7 participants