From 8dc95dda634199db394384660cf6074c3fff425a Mon Sep 17 00:00:00 2001 From: relikd Date: Sat, 6 Jul 2019 13:27:00 +0200 Subject: [PATCH] Propagate 5xx server error to user + reload button. Closes #5 --- CHANGELOG.md | 7 +++ baRSS/AppHook.m | 1 + baRSS/Helper/FeedDownload.m | 28 ++++++++++-- baRSS/Helper/NSView+Ext.h | 2 +- baRSS/Helper/NSView+Ext.m | 6 ++- baRSS/Info.plist | 2 +- baRSS/Preferences/Feeds Tab/ModalFeedEdit.m | 29 ++++++++++-- .../Preferences/Feeds Tab/ModalFeedEditView.h | 1 + .../Preferences/Feeds Tab/ModalFeedEditView.m | 44 ++++++++++--------- .../Preferences/Feeds Tab/SettingsFeedsView.m | 2 +- 10 files changed, 88 insertions(+), 34 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0928d36..c80744d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,9 +6,16 @@ and this project does NOT adhere to [Semantic Versioning](https://semver.org/spe ## [Unreleased] +### Added +- Show users any 5xx server error response and extracted failure reason +- 5xx server errors have a reload button which will initiate a new download with the same URL +- Adding feed: Cmd+R will reload the same URL +- Settings, Feeds: Cmd+R will reload the data source + ### Fixed - Changed error message text when user cancels creation of new feed item - Comparing existing articles with nonexistent guid and link +- Adding feed: If URLs can't be resolved in the first run (5xx error), try a second time. E.g., 'Done' click (issue: #5) ### Changed - Interface builder files replaced with code equivalent diff --git a/baRSS/AppHook.m b/baRSS/AppHook.m index 7b7168c..154367b 100644 --- a/baRSS/AppHook.m +++ b/baRSS/AppHook.m @@ -187,6 +187,7 @@ static NSEventModifierFlags fnKeyFlags = NSEventModifierFlagShift | NSEventModif case 'a': if ([self sendAction:@selector(selectAll:) to:nil from:self]) return; break; case 'q': if ([self sendAction:@selector(performClose:) to:nil from:self]) return; break; case 'w': if ([self sendAction:@selector(performClose:) to:nil from:self]) return; break; + case 'r': if ([self sendAction:@selector(reloadData) to:nil from:self]) return; break; #pragma clang diagnostic push #pragma clang diagnostic ignored "-Wundeclared-selector" case 'z': if ([self sendAction:@selector(undo:) to:nil from:self]) return; break; diff --git a/baRSS/Helper/FeedDownload.m b/baRSS/Helper/FeedDownload.m index cabaae0..6951717 100644 --- a/baRSS/Helper/FeedDownload.m +++ b/baRSS/Helper/FeedDownload.m @@ -201,12 +201,32 @@ static BOOL _nextUpdateIsForced = NO; + (void)asyncRequest:(NSURLRequest*)request block:(nonnull void(^)(NSData * _Nullable data, NSError * _Nullable error, NSHTTPURLResponse *response))block { [[[self nonCachingSession] dataTaskWithRequest:request completionHandler:^(NSData * _Nullable data, NSURLResponse * _Nullable response, NSError * _Nullable error) { NSHTTPURLResponse* httpResponse = (NSHTTPURLResponse*)response; - if (error || [httpResponse statusCode] == 304) + NSInteger status = [httpResponse statusCode]; + if (error || status == 304) { // 304 Not Modified data = nil; + } else if (status >= 500 && status < 600) { // 5xx Server Error + NSString *reason = [NSString stringWithFormat:NSLocalizedString(@"Server HTTP error %ld.\n––––\n%@", nil), + status, [self extractReadableHTML:data]]; + error = [NSError errorWithDomain:NSURLErrorDomain code:NSURLErrorBadServerResponse userInfo:@{NSLocalizedDescriptionKey: reason}]; + data = nil; + } block(data, error, httpResponse); // if status == 304, data & error nil }] resume]; } +/// Helper method to extract readable text from HTML ++ (NSString*)extractReadableHTML:(NSData*)data { + NSString *str = [[NSString alloc] initWithData:data encoding:NSUTF8StringEncoding]; + // replace all with (presumably) non-used character + str = [[NSRegularExpression regularExpressionWithPattern:@"<[^>]*>\\s*" options:kNilOptions error:nil] + stringByReplacingMatchesInString:str options:kNilOptions range:NSMakeRange(0, str.length) withTemplate:@"◊"]; + // then replace multiple occurences of that character with a single new line + str = [[NSRegularExpression regularExpressionWithPattern:@"◊+" options:kNilOptions error:nil] + stringByReplacingMatchesInString:str options:kNilOptions range:NSMakeRange(0, str.length) withTemplate:@"\n"]; + // finally trim whitespace at start and end + return [str stringByTrimmingCharactersInSet: NSCharacterSet.whitespaceAndNewlineCharacterSet]; +} + #pragma mark - Download RSS Feed - @@ -266,9 +286,9 @@ static BOOL _nextUpdateIsForced = NO; dispatch_sync(dispatch_get_main_queue(), ^{ // sync! (thread is already in background) chosenURL = askUser(parsedMeta); }); - if (!chosenURL || chosenURL.length == 0) { - // User canceled operation, show appropriate error message - *err = [NSError errorWithDomain:NSURLErrorDomain code:NSURLErrorCancelled userInfo:@{NSLocalizedDescriptionKey: NSLocalizedString(@"Operation canceled.", nil)}]; + if (!chosenURL || chosenURL.length == 0) { // User canceled operation, show appropriate error message + NSString *reason = NSLocalizedString(@"Operation canceled.", nil); + *err = [NSError errorWithDomain:NSURLErrorDomain code:NSURLErrorCancelled userInfo:@{NSLocalizedDescriptionKey: reason}]; return NO; } [self parseFeedRequest:[self newRequestURL:chosenURL] xmlBlock:nil feedBlock:block]; diff --git a/baRSS/Helper/NSView+Ext.h b/baRSS/Helper/NSView+Ext.h index fa7f9ef..c150548 100644 --- a/baRSS/Helper/NSView+Ext.h +++ b/baRSS/Helper/NSView+Ext.h @@ -56,7 +56,7 @@ NS_INLINE CGFloat YFromTop(NSView *view) { return NSHeight(view.superview.frame) // UI: Buttons + (NSButton*)button:(NSString*)text; + (NSButton*)buttonImageSquare:(nonnull NSImageName)name; -+ (NSButton*)buttonIcon:(NSImage*)img size:(CGFloat)size; ++ (NSButton*)buttonIcon:(nonnull NSImageName)name size:(CGFloat)size; + (NSButton*)inlineButton:(NSString*)text; + (NSPopUpButton*)popupButton:(CGFloat)w; // UI: Others diff --git a/baRSS/Helper/NSView+Ext.m b/baRSS/Helper/NSView+Ext.m index c3eb017..80f72b6 100644 --- a/baRSS/Helper/NSView+Ext.m +++ b/baRSS/Helper/NSView+Ext.m @@ -87,11 +87,11 @@ } /// Create pure image button with no border. -+ (NSButton*)buttonIcon:(NSImage*)img size:(CGFloat)size { ++ (NSButton*)buttonIcon:(nonnull NSImageName)name size:(CGFloat)size { NSButton *btn = [[NSButton alloc] initWithFrame: NSMakeRect(0, 0, size, size)]; btn.bezelStyle = NSBezelStyleRounded; btn.bordered = NO; - btn.image = img; + btn.image = [NSImage imageNamed:name]; return btn; } @@ -271,6 +271,8 @@ self.toolTip = tt; if (self.accessibilityLabel.length == 0) self.accessibilityLabel = tt; + else + self.accessibilityValueDescription = tt; return self; } diff --git a/baRSS/Info.plist b/baRSS/Info.plist index 9b2584d..bef2b2d 100644 --- a/baRSS/Info.plist +++ b/baRSS/Info.plist @@ -32,7 +32,7 @@ CFBundleVersion - 7539 + 7702 LSMinimumSystemVersion $(MACOSX_DEPLOYMENT_TARGET) LSUIElement diff --git a/baRSS/Preferences/Feeds Tab/ModalFeedEdit.m b/baRSS/Preferences/Feeds Tab/ModalFeedEdit.m index 590b708..4b7e258 100644 --- a/baRSS/Preferences/Feeds Tab/ModalFeedEdit.m +++ b/baRSS/Preferences/Feeds Tab/ModalFeedEdit.m @@ -146,6 +146,7 @@ self.httpEtag = nil; self.httpDate = nil; self.faviconURL = nil; + self.previousURL = self.view.url.stringValue; } /** @@ -179,8 +180,11 @@ @return Either URL string or @c nil if user canceled the selection. */ - (NSString*)letUserChooseXmlUrlFromList:(NSArray *)list { - if (list.count == 1) // nothing to choose + if (list.count == 1) { // nothing to choose + // Feeds like https://news.ycombinator.com/ return 503 if URLs are requested too rapidly + //CFRunLoopRunInMode(kCFRunLoopDefaultMode, 1.0, false); // Non-blocking sleep (1s) return list.firstObject.link; + } NSMenu *menu = [[NSMenu alloc] initWithTitle:NSLocalizedString(@"Choose feed menu", nil)]; menu.autoenablesItems = NO; for (RSHTMLMetadataFeedLink *fl in list) { @@ -203,11 +207,19 @@ - (void)postDownload:(NSString*)responseURL { if (self.modalSheet.didCloseAndCancel) return; + + BOOL hasError = (self.feedError != nil); // 1. Stop spinner animation for name field. (keep spinner for URL running until favicon downloaded) [self.view.spinnerName stopAnimation:nil]; // 2. If URL was redirected, replace original text field value with new one. (e.g., https redirect) if (responseURL.length > 0 && ![responseURL isEqualToString:self.previousURL]) { - self.previousURL = responseURL; + if (!hasError) { + // If the url has changed and there is an error: + // This probably means the feed URL was resolved, but the successive download returned 5xx error. + // Presumably to prevent site crawlers accessing many pages in quick succession. (delay of 1s does help) + // By not setting previousURL, a second hit on the 'Done' button will retry the resolved URL again. + self.previousURL = responseURL; + } self.view.url.stringValue = responseURL; } // 3. Copy parsed feed title to text field. (only if user hasn't set anything else yet) @@ -218,7 +230,6 @@ // TODO: user preference to automatically select refresh interval (selection: None,min,max,avg,median) [self statsForDownloadObject]; // 4. Continue with favicon download (or finish with error) - BOOL hasError = (self.feedError != nil); self.view.favicon.hidden = hasError; self.view.warningButton.hidden = !hasError; if (hasError) { @@ -305,7 +316,6 @@ - (void)controlTextDidEndEditing:(NSNotification *)obj { if (obj.object == self.view.url) { if (![self.previousURL isEqualToString:self.view.url.stringValue]) { - self.previousURL = self.view.url.stringValue; [self downloadRSS]; } } @@ -316,15 +326,26 @@ if (!self.feedError) return; + // show reload button if server is temporarily offline (any 5xx server error) + BOOL serverError = (self.feedError.domain == NSURLErrorDomain && self.feedError.code == NSURLErrorBadServerResponse); + self.view.warningReload.hidden = !serverError; + + // set error description as text self.view.warningText.objectValue = self.feedError.localizedDescription; NSSize newSize = self.view.warningText.fittingSize; // width is limited by the textfield's preferred width newSize.width += 2 * self.view.warningText.frame.origin.x; // the padding newSize.height += 2 * self.view.warningText.frame.origin.y; + // apply fitting size and display self.view.warningPopover.contentSize = newSize; [self.view.warningPopover showRelativeToRect:sender.bounds ofView:sender preferredEdge:NSRectEdgeMinY]; } +/// Either hit by Cmd+R or reload button inside warning popover error description +- (void)reloadData { + [self downloadRSS]; +} + @end diff --git a/baRSS/Preferences/Feeds Tab/ModalFeedEditView.h b/baRSS/Preferences/Feeds Tab/ModalFeedEditView.h index bbd58bc..29520b4 100644 --- a/baRSS/Preferences/Feeds Tab/ModalFeedEditView.h +++ b/baRSS/Preferences/Feeds Tab/ModalFeedEditView.h @@ -38,6 +38,7 @@ @property (weak) IBOutlet NSButton *warningButton; @property NSPopover *warningPopover; @property (weak) IBOutlet NSTextField *warningText; +@property (weak) IBOutlet NSButton *warningReload; - (instancetype)initWithController:(ModalFeedEdit*)controller NS_DESIGNATED_INITIALIZER; - (instancetype)initWithFrame:(NSRect)frameRect NS_UNAVAILABLE; diff --git a/baRSS/Preferences/Feeds Tab/ModalFeedEditView.m b/baRSS/Preferences/Feeds Tab/ModalFeedEditView.m index bf87bd9..7b69d86 100644 --- a/baRSS/Preferences/Feeds Tab/ModalFeedEditView.m +++ b/baRSS/Preferences/Feeds Tab/ModalFeedEditView.m @@ -47,10 +47,7 @@ self.url = [[[NSView inputField:@"https://example.org/feed.rss" width:0] placeIn:self x:x yTop:0] sizeToRight:PAD_S + 18]; self.spinnerURL = [[NSView activitySpinner] placeIn:self xRight:1 yTop:2.5]; self.favicon = [[[NSView imageView:nil size:18] tooltip:NSLocalizedString(@"Favicon", nil)] placeIn:self xRight:0 yTop:1.5]; - NSTextField *errorDesc = [self warningPopoverContentView]; - self.warningPopover = [self warningPopoverControllerWith:errorDesc]; - self.warningText = errorDesc; // after added to parent view, otherwise will be released immediatelly (weak ivar) - self.warningButton = [[[[NSView buttonIcon:[NSImage imageNamed:NSImageNameCaution] size:18] action:@selector(didClickWarningButton:) target:nil] // up the responder chain + self.warningButton = [[[[NSView buttonIcon:NSImageNameCaution size:18] action:@selector(didClickWarningButton:) target:nil] // up the responder chain tooltip:NSLocalizedString(@"Click here to show failure reason", nil)] placeIn:self xRight:0 yTop:1.5]; // 2. row @@ -61,34 +58,39 @@ self.refreshUnit = [[NSView popupButton:120] placeIn:self x:NSMaxX(self.refreshNum.frame) + PAD_M yTop:2*rowHeight]; // initial state + self.url.accessibilityLabel = lbls[0]; + self.name.accessibilityLabel = lbls[1]; + self.refreshNum.accessibilityLabel = NSLocalizedString(@"Refresh interval", nil); self.url.delegate = controller; self.warningButton.hidden = YES; self.refreshNum.formatter = [StrictUIntFormatter new]; // see below ... - //[self.warningButton.cell setHighlightsBy:(error ? NSContentsCellMask : NSNoCellMask)]; + [self prepareWarningPopover]; return self; } -/// User visible error description text (after click on warning button) -- (NSTextField*)warningPopoverContentView { - NSTextField *txt = [[[NSView label:@""] selectable] sizableWidthAndHeight]; - [txt setFrameSize: NSMakeSize(300, 100)]; - txt.lineBreakMode = NSLineBreakByWordWrapping; - txt.maximumNumberOfLines = 7; - return txt; -} - -/// Prepare popover controller to display download errors -- (NSPopover*)warningPopoverControllerWith:(NSTextField*)content { +/// Prepare popover controller to display errors during download +- (void)prepareWarningPopover { NSPopover *pop = [[NSPopover alloc] init]; pop.behavior = NSPopoverBehaviorTransient; pop.contentViewController = [[NSViewController alloc] init]; - pop.contentViewController.view = [[NSView alloc] initWithFrame:content.frame]; - [pop.contentViewController.view addSubview:content]; + NSView *content = [[NSView alloc] initWithFrame:NSMakeRect(0, 0, 300, 100)]; + pop.contentViewController.view = content; - content.frame = NSInsetRect(content.frame, 4, 2); - content.preferredMaxLayoutWidth = NSWidth(content.frame); - return pop; + // User visible error description text (after click on warning button) + NSTextField *txt = [[[NSView label:@""] selectable] sizableWidthAndHeight]; + txt.frame = NSInsetRect(content.frame, 4, 2); + txt.preferredMaxLayoutWidth = NSWidth(txt.frame); + txt.lineBreakMode = NSLineBreakByWordWrapping; + txt.maximumNumberOfLines = 7; + [content addSubview:txt]; + + self.warningPopover = pop; + self.warningText = txt; + // Reload button is only visible on 5xx server error (right of ––––) + self.warningReload = [[[[NSView buttonIcon:NSImageNameRefreshTemplate size:16] placeIn:content x:35 yTop:21] + tooltip:NSLocalizedString(@"Retry download (Cmd+R)", nil)] + action:@selector(reloadData) target:nil]; // up the responder chain } @end diff --git a/baRSS/Preferences/Feeds Tab/SettingsFeedsView.m b/baRSS/Preferences/Feeds Tab/SettingsFeedsView.m index a88bdae..3b7e850 100644 --- a/baRSS/Preferences/Feeds Tab/SettingsFeedsView.m +++ b/baRSS/Preferences/Feeds Tab/SettingsFeedsView.m @@ -134,7 +134,7 @@ */ - (CGFloat)generateButtons { NSButton *add = [[NSView buttonImageSquare:NSImageNameAddTemplate] tooltip:NSLocalizedString(@"Add new item", nil)]; - NSButton *del = [[NSView buttonImageSquare:NSImageNameRemoveTemplate] tooltip:NSLocalizedString(@"Delete selected item(s)", nil)]; + NSButton *del = [[NSView buttonImageSquare:NSImageNameRemoveTemplate] tooltip:NSLocalizedString(@"Delete selected items", nil)]; NSButton *share = [[NSView buttonImageSquare:NSImageNameShareTemplate] tooltip:NSLocalizedString(@"Import or export data", nil)]; [self button:add copyActions:3 to:5];