From feb5e86aacd791fbeb0dc23d5423d34eeb49fe37 Mon Sep 17 00:00:00 2001 From: Jay Harris Date: Fri, 12 Jan 2024 13:18:22 +1300 Subject: [PATCH] [Uplift 1.62] [Brave News]: Improve offline handling (#21545) [Brave News]: Better offline handling & UX tweaks (#21507) --- browser/ui/webui/brave_webui_source.cc | 2 + .../components/default/braveNews/FeedV2.tsx | 21 +-------- .../browser/brave_news_controller.cc | 18 +++++++- .../browser/brave_news_controller.h | 15 ++++-- .../brave_news/browser/feed_v2_builder.cc | 36 ++++++++++++--- .../brave_news/browser/feed_v2_builder.h | 2 + .../browser/publishers_controller.cc | 6 +++ .../brave_news/browser/resources/Feed.tsx | 26 +++++------ .../browser/resources/FeedNavigation.tsx | 8 ++-- .../brave_news/browser/resources/FeedPage.tsx | 2 +- .../browser/resources/feed/LoadingCard.tsx | 1 - .../browser/resources/feed/NotConnected.tsx | 46 +++++++++++++++++++ .../browser/resources/shared/useFeedV2.ts | 5 ++ components/brave_news/common/brave_news.mojom | 9 ++++ components/resources/brave_news_strings.grdp | 6 +++ 15 files changed, 155 insertions(+), 48 deletions(-) create mode 100644 components/brave_news/browser/resources/feed/NotConnected.tsx diff --git a/browser/ui/webui/brave_webui_source.cc b/browser/ui/webui/brave_webui_source.cc index 84781d343158..0b22235c3bc3 100644 --- a/browser/ui/webui/brave_webui_source.cc +++ b/browser/ui/webui/brave_webui_source.cc @@ -244,6 +244,8 @@ void CustomizeWebUIHTMLSource(content::WebUI* web_ui, { "braveNewsSourcesRecommendation", IDS_BRAVE_NEWS_SOURCES_RECOMMENDATION}, { "braveNewsNoArticlesTitle", IDS_BRAVE_NEWS_NO_ARTICLES_TITLE}, { "braveNewsNoArticlesMessage", IDS_BRAVE_NEWS_NO_ARTICLES_MESSAGE}, + {"braveNewsOfflineTitle", IDS_BRAVE_NEWS_OFFLINE_TITLE}, + {"braveNewsOfflineMessage", IDS_BRAVE_NEWS_OFFLINE_MESSAGE}, { "braveNewsCustomizeFeed", IDS_BRAVE_NEWS_CUSTOMIZE_FEED}, { "braveNewsRefreshFeed", IDS_BRAVE_NEWS_REFRESH_FEED}, { "braveNewsOpenArticlesIn", IDS_BRAVE_NEWS_OPEN_ARTICLES_IN}, diff --git a/components/brave_new_tab_ui/components/default/braveNews/FeedV2.tsx b/components/brave_new_tab_ui/components/default/braveNews/FeedV2.tsx index fc6ca4194b0d..405192205b63 100644 --- a/components/brave_new_tab_ui/components/default/braveNews/FeedV2.tsx +++ b/components/brave_new_tab_ui/components/default/braveNews/FeedV2.tsx @@ -14,7 +14,6 @@ import FeedNavigation from '../../../../brave_news/browser/resources/FeedNavigat import NewsButton from '../../../../brave_news/browser/resources/NewsButton' import Variables from '../../../../brave_news/browser/resources/Variables' import { useBraveNews } from '../../../../brave_news/browser/resources/shared/Context' -import { isPublisherEnabled } from '../../../../brave_news/browser/resources/shared/api' import { CLASSNAME_PAGE_STUCK } from '../page' const Root = styled(Variables)` @@ -73,23 +72,7 @@ const LoadNewContentButton = styled(NewsButton)` ` export default function FeedV2() { - const { feedV2, setCustomizePage, refreshFeedV2, feedV2UpdatesAvailable, publishers, channels } = useBraveNews() - - // We don't want to decide whether we have subscriptions until the publishers - // and channels have loaded. - const loaded = React.useMemo(() => !!Object.values(publishers).length && !!Object.values(channels).length, [publishers, channels]) - - // This is a bit of an interesting |useMemo| - we only want it to be updated - // when the feed changes so as to not break the case where: - // 1. The user has no feeds (we show the NoFeeds card) - // 2. The user subscribes to a feed (we should still show the NoFeeds card, - // not the "Empty Feed") - // To achieve this, |hasSubscriptions| is only updated when the feed changes, - // or the opt-in status is changed. - const hasSubscriptions = React.useMemo(() => !loaded - || Object.values(publishers).some(isPublisherEnabled) - || Object.values(channels).some(c => c.subscribedLocales.length), [feedV2, loaded]) - + const { feedV2, setCustomizePage, refreshFeedV2, feedV2UpdatesAvailable } = useBraveNews() const ref = React.useRef() // Note: Whenever the feed is updated, if we're viewing the feed, scroll to @@ -111,7 +94,7 @@ export default function FeedV2() { {feedV2UpdatesAvailable && {getLocale('braveNewsNewContentAvailable')} } - + diff --git a/components/brave_news/browser/brave_news_controller.cc b/components/brave_news/browser/brave_news_controller.cc index fb80dcab2de4..2bc8be70e891 100644 --- a/components/brave_news/browser/brave_news_controller.cc +++ b/components/brave_news/browser/brave_news_controller.cc @@ -48,6 +48,7 @@ #include "components/prefs/scoped_user_pref_update.h" #include "mojo/public/cpp/bindings/pending_remote.h" #include "mojo/public/cpp/bindings/remote_set.h" +#include "net/base/network_change_notifier.h" #include "services/network/public/cpp/shared_url_loader_factory.h" #include "services/network/public/cpp/simple_url_loader.h" #include "third_party/abseil-cpp/absl/types/optional.h" @@ -119,6 +120,8 @@ BraveNewsController::BraveNewsController( publishers_observation_(this), weak_ptr_factory_(this) { DCHECK(prefs_); + net::NetworkChangeNotifier::AddNetworkChangeObserver(this); + // Set up preference listeners pref_change_registrar_.Init(prefs_); pref_change_registrar_.Add( @@ -142,7 +145,9 @@ BraveNewsController::BraveNewsController( ConditionallyStartOrStopTimer(); } -BraveNewsController::~BraveNewsController() = default; +BraveNewsController::~BraveNewsController() { + net::NetworkChangeNotifier::RemoveNetworkChangeObserver(this); +} void BraveNewsController::Bind( mojo::PendingReceiver receiver) { @@ -840,4 +845,15 @@ void BraveNewsController::OnPublishersUpdated( } } +void BraveNewsController::OnNetworkChanged( + net::NetworkChangeNotifier::ConnectionType type) { + if (!GetIsEnabled(prefs_)) { + return; + } + + // Ensure publishers are fetched (this won't do anything if they are). This + // handles the case where Brave News is started with no network. + publishers_controller_.GetOrFetchPublishers(base::DoNothing()); +} + } // namespace brave_news diff --git a/components/brave_news/browser/brave_news_controller.h b/components/brave_news/browser/brave_news_controller.h index 6a9a009149dd..f8987051e650 100644 --- a/components/brave_news/browser/brave_news_controller.h +++ b/components/brave_news/browser/brave_news_controller.h @@ -16,6 +16,7 @@ #include "base/memory/scoped_refptr.h" #include "base/scoped_observation.h" #include "base/task/cancelable_task_tracker.h" +#include "base/time/time.h" #include "base/timer/timer.h" #include "brave/components/api_request_helper/api_request_helper.h" #include "brave/components/brave_news/browser/channels_controller.h" @@ -37,6 +38,8 @@ #include "mojo/public/cpp/bindings/receiver_set.h" #include "mojo/public/cpp/bindings/remote.h" #include "mojo/public/cpp/bindings/remote_set.h" +#include "net/base/network_change_notifier.h" +#include "services/network/public/cpp/network_connection_tracker.h" #include "services/network/public/cpp/shared_url_loader_factory.h" class PrefRegistrySimple; @@ -62,9 +65,11 @@ bool GetIsEnabled(PrefService* prefs); // Orchestrates FeedController and PublishersController for data, as well as // owning prefs data. // Controls remote feed update logic via Timer and prefs values. -class BraveNewsController : public KeyedService, - public mojom::BraveNewsController, - public PublishersController::Observer { +class BraveNewsController + : public KeyedService, + public mojom::BraveNewsController, + public PublishersController::Observer, + public net::NetworkChangeNotifier::NetworkChangeObserver { public: static void RegisterProfilePrefs(PrefRegistrySimple* registry); @@ -151,6 +156,10 @@ class BraveNewsController : public KeyedService, // PublishersController::Observer: void OnPublishersUpdated(brave_news::PublishersController*) override; + // net::NetworkChangeNotifier::NetworkChangeObserver: + void OnNetworkChanged( + net::NetworkChangeNotifier::ConnectionType type) override; + private: void OnOptInChange(); void ConditionallyStartOrStopTimer(); diff --git a/components/brave_news/browser/feed_v2_builder.cc b/components/brave_news/browser/feed_v2_builder.cc index c3b3c7275ab6..0050f866a90b 100644 --- a/components/brave_news/browser/feed_v2_builder.cc +++ b/components/brave_news/browser/feed_v2_builder.cc @@ -73,13 +73,18 @@ using ContentGroup = std::pair; constexpr char kAllContentGroup[] = "all"; constexpr float kSampleContentGroupAllRatio = 0.2f; -std::string GetFeedHash(const Channels& channels, - const Publishers& publishers, - const ETags& etags) { +// Returns a tuple of the feed hash and the number of subscribed publishers. +std::tuple GetFeedHashAndSubscribedCount( + const Channels& channels, + const Publishers& publishers, + const ETags& etags) { std::vector hash_items; + size_t subscribed_count = 0; + for (const auto& [channel_id, channel] : channels) { if (!channel->subscribed_locales.empty()) { hash_items.push_back(channel_id); + subscribed_count++; } } @@ -87,6 +92,7 @@ std::string GetFeedHash(const Channels& channels, if (publisher->user_enabled_status == mojom::UserEnabled::ENABLED || publisher->type == mojom::PublisherType::DIRECT_SOURCE) { hash_items.push_back(id); + subscribed_count++; } // Disabling a publisher should also change the hash, as it will affect what @@ -107,7 +113,7 @@ std::string GetFeedHash(const Channels& channels, hash = base::NumberToString(hasher(hash + hash_item)); } - return hash; + return {hash, subscribed_count}; } // Gets all relevant signals for an article. @@ -904,7 +910,8 @@ void FeedV2Builder::RecheckFeedHash() { const auto& publishers = publishers_controller_->GetLastPublishers(); auto channels = channels_controller_->GetChannelsFromPublishers(publishers, &*prefs_); - hash_ = GetFeedHash(channels, publishers, feed_etags_); + std::tie(hash_, subscribed_count_) = + GetFeedHashAndSubscribedCount(channels, publishers, feed_etags_); for (const auto& listener : listeners_) { listener->OnUpdateAvailable(hash_); } @@ -1057,7 +1064,8 @@ void FeedV2Builder::NotifyUpdateCompleted() { const auto& publishers = publishers_controller_->GetLastPublishers(); auto channels = channels_controller_->GetChannelsFromPublishers(publishers, &*prefs_); - hash_ = GetFeedHash(channels, publishers, feed_etags_); + std::tie(hash_, subscribed_count_) = + GetFeedHashAndSubscribedCount(channels, publishers, feed_etags_); // Fire all the pending callbacks. for (auto& callback : current_update_->callbacks) { @@ -1099,6 +1107,16 @@ void FeedV2Builder::GenerateFeed( feed->type = std::move(type); feed->source_hash = builder->hash_; + if (feed->items.empty()) { + if (builder->raw_feed_items_.size() == 0) { + feed->error = mojom::FeedV2Error::ConnectionError; + } else if (builder->subscribed_count_ == 0) { + feed->error = mojom::FeedV2Error::NoFeeds; + } else { + feed->error = mojom::FeedV2Error::NoArticles; + } + } + std::move(callback).Run(feed->Clone()); }, weak_ptr_factory_.GetWeakPtr(), std::move(type), @@ -1185,6 +1203,12 @@ mojom::FeedV2Ptr FeedV2Builder::GenerateAllFeed() { } } + // If we aren't subscribed to anything, or we failed to fetch any articles + // from the internet, don't try and generate a feed. + if (eligible_content_groups.size() == 0 || raw_feed_items_.size() == 0) { + return feed; + } + // Step 1: Generate a block // https://docs.google.com/document/d/1bSVHunwmcHwyQTpa3ab4KRbGbgNQ3ym_GHvONnrBypg/edit#heading=h.rkq699fwps0 std::vector initial_block = diff --git a/components/brave_news/browser/feed_v2_builder.h b/components/brave_news/browser/feed_v2_builder.h index f824866459e2..4c0b52029d0c 100644 --- a/components/brave_news/browser/feed_v2_builder.h +++ b/components/brave_news/browser/feed_v2_builder.h @@ -6,6 +6,7 @@ #ifndef BRAVE_COMPONENTS_BRAVE_NEWS_BROWSER_FEED_V2_BUILDER_H_ #define BRAVE_COMPONENTS_BRAVE_NEWS_BROWSER_FEED_V2_BUILDER_H_ +#include #include #include #include @@ -155,6 +156,7 @@ class FeedV2Builder : public PublishersController::Observer { Signals signals_; std::vector suggested_publisher_ids_; TopicsResult topics_; + size_t subscribed_count_ = 0; absl::optional current_update_; absl::optional next_update_; diff --git a/components/brave_news/browser/publishers_controller.cc b/components/brave_news/browser/publishers_controller.cc index b9aec87f7a68..ae7673c4a6d9 100644 --- a/components/brave_news/browser/publishers_controller.cc +++ b/components/brave_news/browser/publishers_controller.cc @@ -201,7 +201,13 @@ void PublishersController::EnsurePublishersIsUpdating() { // TODO(petemill): handle bad status or response absl::optional publisher_list = ParseCombinedPublisherList(api_request_result.value_body()); + + // Update failed, we'll just reuse whatever publishers we had before. if (!publisher_list) { + controller->on_current_update_complete_->Signal(); + controller->is_update_in_progress_ = false; + controller->on_current_update_complete_ = + std::make_unique(); return; } diff --git a/components/brave_news/browser/resources/Feed.tsx b/components/brave_news/browser/resources/Feed.tsx index 9645714eb637..a4a93471babc 100644 --- a/components/brave_news/browser/resources/Feed.tsx +++ b/components/brave_news/browser/resources/Feed.tsx @@ -4,7 +4,7 @@ // You can obtain one at https://mozilla.org/MPL/2.0/. import { spacing } from "@brave/leo/tokens/css"; -import { FeedItemV2, FeedV2 } from "gen/brave/components/brave_news/common/brave_news.mojom.m"; +import { FeedItemV2, FeedV2, FeedV2Error } from "gen/brave/components/brave_news/common/brave_news.mojom.m"; import * as React from 'react'; import styled from "styled-components"; import Advert from "./feed/Ad"; @@ -17,6 +17,7 @@ import LoadingCard from "./feed/LoadingCard"; import NoArticles from "./feed/NoArticles"; import NoFeeds from "./feed/NoFeeds"; import { getHistoryValue, setHistoryState } from "./shared/history"; +import NotConnected from "./feed/NotConnected"; // Restoring scroll position is complicated - we have two available strategies: // 1. Scroll to the same position - as long as the window hasn't been resized, @@ -45,7 +46,6 @@ const FeedContainer = styled.div` interface Props { feed: FeedV2 | undefined; - hasSubscriptions: boolean; } const getKey = (feedItem: FeedItemV2, index: number): React.Key => { @@ -76,7 +76,13 @@ const saveScrollPos = (itemId: React.Key) => () => { }) } -export default function Component({ feed, hasSubscriptions }: Props) { +const errors = { + [FeedV2Error.ConnectionError]: , + [FeedV2Error.NoArticles]: , + [FeedV2Error.NoFeeds]: +} + +export default function Component({ feed }: Props) { const [cardCount, setCardCount] = React.useState(getHistoryValue(HISTORY_CARD_COUNT, PAGE_SIZE)); // Store the number of cards we've loaded in history - otherwise when we @@ -145,18 +151,12 @@ export default function Component({ feed, hasSubscriptions }: Props) { }) }, [cardCount, feed?.items]) - // An empty feed may still have ads in it, so we need to filter them out to - // determine if there are no articles. - const noArticles = React.useMemo(() => !feed?.items.filter(i => !i.advert).length, [feed]) - return {feed - ? noArticles - ? hasSubscriptions ? : - : <> - {cards} - - + ? errors[feed.error!] ?? <> + {cards} + + : } } diff --git a/components/brave_news/browser/resources/FeedNavigation.tsx b/components/brave_news/browser/resources/FeedNavigation.tsx index fba9f5d71eab..8620f11c95af 100644 --- a/components/brave_news/browser/resources/FeedNavigation.tsx +++ b/components/brave_news/browser/resources/FeedNavigation.tsx @@ -150,7 +150,7 @@ export default function Sidebar() { {getLocale('braveNewsMyFeedHeading')} -
+ {!!subscribedChannels.length &&
{Marker} {getLocale('braveNewsChannelsHeader')} @@ -166,8 +166,8 @@ export default function Sidebar() { ? getLocale('braveNewsShowLess') : getLocale('braveNewsShowAll')} } -
-
+
} + {!!subscribedPublisherIds.length &&
{Marker} {getLocale('braveNewsPublishersHeading')} @@ -183,6 +183,6 @@ export default function Sidebar() { ? getLocale('braveNewsShowLess') : getLocale('braveNewsShowAll')} } -
+
} } diff --git a/components/brave_news/browser/resources/FeedPage.tsx b/components/brave_news/browser/resources/FeedPage.tsx index 5d50e6e22e30..0a4881eeb779 100644 --- a/components/brave_news/browser/resources/FeedPage.tsx +++ b/components/brave_news/browser/resources/FeedPage.tsx @@ -23,6 +23,6 @@ export default function FeedPage() { const { feedV2 } = useBraveNews() return

The Feed ({feedV2?.items.length} items. Truncated at {truncate})

- + } diff --git a/components/brave_news/browser/resources/feed/LoadingCard.tsx b/components/brave_news/browser/resources/feed/LoadingCard.tsx index 272c75b77ede..e4ec6a8db2bb 100644 --- a/components/brave_news/browser/resources/feed/LoadingCard.tsx +++ b/components/brave_news/browser/resources/feed/LoadingCard.tsx @@ -15,7 +15,6 @@ const Container = styled(Card)` const Spinner = styled(ProgressRing)` --leo-progressring-color: var(--bn-glass-25); - backdrop-filter: blur(64px); ` export default function LoadingCard() { diff --git a/components/brave_news/browser/resources/feed/NotConnected.tsx b/components/brave_news/browser/resources/feed/NotConnected.tsx new file mode 100644 index 000000000000..883ded2ae87b --- /dev/null +++ b/components/brave_news/browser/resources/feed/NotConnected.tsx @@ -0,0 +1,46 @@ +// Copyright (c) 2024 The Brave Authors. All rights reserved. +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this file, +// You can obtain one at https://mozilla.org/MPL/2.0/. + +import Flex from '$web-common/Flex'; +import { spacing } from '@brave/leo/tokens/css'; +import * as React from 'react'; +import styled from 'styled-components'; +import NewsButton from '../NewsButton'; +import { useBraveNews } from '../shared/Context'; +import { Title } from './Card'; +import { getLocale } from '$web-common/locale'; + +const Container = styled(Flex)` + text-align: center; + + padding: ${spacing['3Xl']}; + gap: ${spacing.m}; + color: var(--bn-glass-70); + + & > svg { + margin-bottom: ${spacing.xl}; + fill: none; + } +` + +export default function NotConnected() { + const { refreshFeedV2 } = useBraveNews() + return + + + + {getLocale('braveNewsOfflineTitle')} +
+ {getLocale('braveNewsOfflineMessage')} +
+ + {getLocale('braveNewsRefreshFeed')} + + +} diff --git a/components/brave_news/browser/resources/shared/useFeedV2.ts b/components/brave_news/browser/resources/shared/useFeedV2.ts index a0fe471acf4f..6d0c6555f5f6 100644 --- a/components/brave_news/browser/resources/shared/useFeedV2.ts +++ b/components/brave_news/browser/resources/shared/useFeedV2.ts @@ -75,6 +75,11 @@ const maybeLoadFeed = (view?: FeedView) => { return undefined } + // Don't load errored feeds. + if (feed.error !== undefined) { + return undefined + } + // If the feed doesn't match what we stored, don't return it. return !view || feedTypeToFeedView(feed.type) === view ? feed diff --git a/components/brave_news/common/brave_news.mojom b/components/brave_news/common/brave_news.mojom index 06d934b674f9..6bd4748f204e 100644 --- a/components/brave_news/common/brave_news.mojom +++ b/components/brave_news/common/brave_news.mojom @@ -208,11 +208,20 @@ union FeedV2Type { FeedV2ChannelType channel; }; +enum FeedV2Error { + NoArticles, + NoFeeds, + ConnectionError, +}; + struct FeedV2 { mojo_base.mojom.Time construct_time; string source_hash; FeedV2Type? type; array items; + + // If |error| is set items will be empty. + FeedV2Error? error; }; struct DisplayAd { diff --git a/components/resources/brave_news_strings.grdp b/components/resources/brave_news_strings.grdp index 5768225b4460..fedd12a8dab3 100644 --- a/components/resources/brave_news_strings.grdp +++ b/components/resources/brave_news_strings.grdp @@ -219,6 +219,12 @@ There don't seem to be any articles in this feed at this moment. Try coming back later or refreshing the feed. + + You appear to be offline + + + Check your internet connection and refresh + Customize