Skip to content

Commit

Permalink
[Uplift 1.62] [Brave News]: Improve offline handling (#21545)
Browse files Browse the repository at this point in the history
[Brave News]: Better offline handling & UX tweaks (#21507)
  • Loading branch information
fallaciousreasoning authored Jan 12, 2024
1 parent d68e351 commit feb5e86
Show file tree
Hide file tree
Showing 15 changed files with 155 additions and 48 deletions.
2 changes: 2 additions & 0 deletions browser/ui/webui/brave_webui_source.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)`
Expand Down Expand Up @@ -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<HTMLDivElement>()

// Note: Whenever the feed is updated, if we're viewing the feed, scroll to
Expand All @@ -111,7 +94,7 @@ export default function FeedV2() {
{feedV2UpdatesAvailable && <LoadNewContentButton onClick={refreshFeedV2}>
{getLocale('braveNewsNewContentAvailable')}
</LoadNewContentButton>}
<Feed feed={feedV2} hasSubscriptions={hasSubscriptions} />
<Feed feed={feedV2} />
</Flex>

<ButtonsContainer>
Expand Down
18 changes: 17 additions & 1 deletion components/brave_news/browser/brave_news_controller.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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(
Expand All @@ -142,7 +145,9 @@ BraveNewsController::BraveNewsController(
ConditionallyStartOrStopTimer();
}

BraveNewsController::~BraveNewsController() = default;
BraveNewsController::~BraveNewsController() {
net::NetworkChangeNotifier::RemoveNetworkChangeObserver(this);
}

void BraveNewsController::Bind(
mojo::PendingReceiver<mojom::BraveNewsController> receiver) {
Expand Down Expand Up @@ -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
15 changes: 12 additions & 3 deletions components/brave_news/browser/brave_news_controller.h
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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;
Expand All @@ -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);

Expand Down Expand Up @@ -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();
Expand Down
36 changes: 30 additions & 6 deletions components/brave_news/browser/feed_v2_builder.cc
Original file line number Diff line number Diff line change
Expand Up @@ -73,20 +73,26 @@ using ContentGroup = std::pair<std::string, bool>;
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<std::string, size_t> GetFeedHashAndSubscribedCount(
const Channels& channels,
const Publishers& publishers,
const ETags& etags) {
std::vector<std::string> 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++;
}
}

for (const auto& [id, publisher] : publishers) {
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
Expand All @@ -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.
Expand Down Expand Up @@ -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_);
}
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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),
Expand Down Expand Up @@ -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<mojom::FeedItemV2Ptr> initial_block =
Expand Down
2 changes: 2 additions & 0 deletions components/brave_news/browser/feed_v2_builder.h
Original file line number Diff line number Diff line change
Expand Up @@ -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 <cstddef>
#include <memory>
#include <string>
#include <vector>
Expand Down Expand Up @@ -155,6 +156,7 @@ class FeedV2Builder : public PublishersController::Observer {
Signals signals_;
std::vector<std::string> suggested_publisher_ids_;
TopicsResult topics_;
size_t subscribed_count_ = 0;

absl::optional<UpdateRequest> current_update_;
absl::optional<UpdateRequest> next_update_;
Expand Down
6 changes: 6 additions & 0 deletions components/brave_news/browser/publishers_controller.cc
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,13 @@ void PublishersController::EnsurePublishersIsUpdating() {
// TODO(petemill): handle bad status or response
absl::optional<Publishers> 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<base::OneShotEvent>();
return;
}

Expand Down
26 changes: 13 additions & 13 deletions components/brave_news/browser/resources/Feed.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand All @@ -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,
Expand Down Expand Up @@ -45,7 +46,6 @@ const FeedContainer = styled.div`

interface Props {
feed: FeedV2 | undefined;
hasSubscriptions: boolean;
}

const getKey = (feedItem: FeedItemV2, index: number): React.Key => {
Expand Down Expand Up @@ -76,7 +76,13 @@ const saveScrollPos = (itemId: React.Key) => () => {
})
}

export default function Component({ feed, hasSubscriptions }: Props) {
const errors = {
[FeedV2Error.ConnectionError]: <NotConnected/>,
[FeedV2Error.NoArticles]: <NoArticles />,
[FeedV2Error.NoFeeds]: <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
Expand Down Expand Up @@ -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 <FeedContainer className={NEWS_FEED_CLASS}>
{feed
? noArticles
? hasSubscriptions ? <NoArticles /> : <NoFeeds />
: <>
{cards}
<CaughtUp />
</>
? errors[feed.error!] ?? <>
{cards}
<CaughtUp />
</>
: <LoadingCard />}
</FeedContainer>
}
8 changes: 4 additions & 4 deletions components/brave_news/browser/resources/FeedNavigation.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ export default function Sidebar() {
<Heading>{getLocale('braveNewsMyFeedHeading')}</Heading>
<Item id='all' name={getLocale('braveNewsForYouFeed')} />
<Item id='following' name={getLocale('braveNewsFollowingFeed')} />
<Section open>
{!!subscribedChannels.length && <Section open>
<summary>
{Marker}
{getLocale('braveNewsChannelsHeader')}
Expand All @@ -166,8 +166,8 @@ export default function Sidebar() {
? getLocale('braveNewsShowLess')
: getLocale('braveNewsShowAll')}
</CustomButton>}
</Section>
<Section open>
</Section>}
{!!subscribedPublisherIds.length && <Section open>
<summary>
{Marker}
{getLocale('braveNewsPublishersHeading')}
Expand All @@ -183,6 +183,6 @@ export default function Sidebar() {
? getLocale('braveNewsShowLess')
: getLocale('braveNewsShowAll')}
</CustomButton>}
</Section>
</Section>}
</Container>
}
2 changes: 1 addition & 1 deletion components/brave_news/browser/resources/FeedPage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,6 @@ export default function FeedPage() {
const { feedV2 } = useBraveNews()
return <Container>
<h2>The Feed ({feedV2?.items.length} items. Truncated at {truncate})</h2>
<Feed feed={feedV2} hasSubscriptions />
<Feed feed={feedV2} />
</Container>
}
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down
Loading

0 comments on commit feb5e86

Please sign in to comment.