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.

Sign up for GitHub

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

Add error indicators to feed list #457

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Add error indicators to feed list #457

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
81b1f5b
Add error indicators to feed list
xuhcc Jun 2, 2019
9e0f166
Store timestamp of the last successful fetch
xuhcc Jun 2, 2019
5b4f385
Indicate errors with colored feed title
xuhcc Jun 3, 2019
5d9d752
Show tooltip on feeds with error status
xuhcc Jun 3, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions _locales/en/messages.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,9 @@
"previewTitle": {
"message": "$1 [Feed Preview - Brief]"
},
"feedError_tooltip": {
"message": "Failed to fetch feed"
},

// Buttons
"subscribeButton_knownFeed_label": {
Expand Down
11 changes: 10 additions & 1 deletion modules/database.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -428,6 +428,8 @@ export let Database = {
dateModified: 0,
lastUpdated: 0,
oldestEntryDate: 0,
lastFetched: 0,
error: false,
};
console.log('Creating node', newFeed);
this._feeds.push(newFeed);
Expand Down Expand Up @@ -553,6 +555,11 @@ export let Database = {
modified = parseDateValue(parsedFeed.updated);
}
if(!entries.length || (modified && modified <= feed.dateModified)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This block will likely be modified / removed in the near future as quite some feeds have incorrect dateModified, but for now looks correct.

await this.modifyFeed({
feedID: feed.feedID,
lastFetched: now,
error: false,
});
return {entries: [], newEntries: []};
}
let newEntries = await this._pushEntries({feed, entries});
Expand All @@ -563,8 +570,10 @@ export let Database = {
subtitle: parsedFeed.subtitle ? parsedFeed.subtitle.text : '',
oldestEntryDate: Math.min(entries.map(e => e.date)) || feed.oldestEntryDate,
language: parsedFeed.language,
lastUpdated: Date.now(),
lastUpdated: now,
dateModified: modified,
lastFetched: now,
error: false,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May be a good idea to also store a timestamp of the last successful fetch as "this feed failed to update last time" and "this feed could not update for two months" are different situations.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

});
await this.modifyFeed(feedUpdates);
await this.expireEntries(feed);
Expand Down
2 changes: 2 additions & 0 deletions modules/updater.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,8 @@ export let FeedUpdater = {
entryCount += newEntries.length;
this.updatedFeeds.set(feedID, entryCount);
}
} else if (!feed.error) {
this.db.modifyFeed({feedID: feedID, error: true});
}

//Do we need to refresh the favicon?
Expand Down
4 changes: 4 additions & 0 deletions skin/feedlist.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,10 @@ tree-folder-header > .title {
font-weight: bold !important;
}

tree-item.error > .title {
color: #B23628 !important;
}

.unread-count {
min-width: 2em !important;
margin-left: auto !important;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having this attribute for both items causes them to be spaced in a weird way if both are displayed.
Screenshot_20190603_005325

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to apply different styles, but none was good enough. So I removed the circle and painted feed titles in red instead.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can also replace feed icon with https://github.com/brief-rss/brief/blob/master/icons/error.png. feedlist.css contains some css rules for that, probably left from the pre-webext days.

Expand Down
1 change: 0 additions & 1 deletion ui/brief.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,6 @@ async function init() {
},
});
// TODO: should update FeedView and feed view title too(?) on title change
// TODO: loading/error indicators

Commands.applyStyle();

Expand Down
9 changes: 7 additions & 2 deletions ui/feedlist.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,8 @@ TreeView.prototype = {
if(loading !== undefined)
element.dataset.loading = loading;
if(error !== undefined)
element.dataset.error = error;
element.classList.toggle('error', error);
element.title = error ? browser.i18n.getMessage('feedError_tooltip') : '';
if(collapsed !== undefined)
element.classList.toggle('collapsed', collapsed);
if(children !== undefined)
Expand Down Expand Up @@ -666,7 +667,11 @@ export let FeedList = {
};

let unreadCount = await Database.query(query).count();
this.tree.updateElement(aFeed.feedID, {title: aFeed.title || aFeed.feedURL, unreadCount});
this.tree.updateElement(aFeed.feedID, {
title: aFeed.title || aFeed.feedURL,
unreadCount,
error: aFeed.error,
});
},

_faviconUrl: function FeedList__faviconUrl(aFeed) {
Expand Down